Support multiple git verification strategies

Add a new flag that puts the `--gitVerifySignatures` behavior into one
of two modes:

    * "all" (default) uses the existing `--gitVerifySignatures` behavior

    * "first-parent" uses an alternative behavior, wherein only each
      first-parent of each commit from the tip of the flux branch are
      considered when verifying GPG signatures

cmd/fluxd/main.go

@@ -134,9 +134,10 @@ func main() {
 		gitTimeout      = fs.Duration("git-timeout", 20*time.Second, "duration after which git operations time out")
 
 		// GPG commit signing
-		gitImportGPG        = fs.StringSlice("git-gpg-key-import", []string{}, "keys at the paths given will be imported for use of signing and verifying commits")
-		gitSigningKey       = fs.String("git-signing-key", "", "if set, commits Flux makes will be signed with this GPG key")
-		gitVerifySignatures = fs.Bool("git-verify-signatures", false, "if set, the signature of commits will be verified before Flux applies them")
+		gitImportGPG               = fs.StringSlice("git-gpg-key-import", []string{}, "keys at the paths given will be imported for use of signing and verifying commits")
+		gitSigningKey              = fs.String("git-signing-key", "", "if set, commits Flux makes will be signed with this GPG key")
+		gitVerifySignatures        = fs.Bool("git-verify-signatures", false, "(deprecated) sets --git-verify-signatures-mode=all when set")
+		gitVerifySignaturesModeStr = fs.String("git-verify-signatures-mode", fluxsync.VerifySignaturesModeDefault, fmt.Sprintf("if git-verify-signatures is set, which strategy to use for signature verification (one of %s)", strings.Join([]string{fluxsync.VerifySignaturesModeNone, fluxsync.VerifySignaturesModeAll, fluxsync.VerifySignaturesModeFirstParent}, ",")))
 
 		// syncing
 		syncInterval = fs.Duration("sync-interval", 5*time.Minute, "apply config in git to cluster at least this often, even if there are no new commits")
@@ -202,6 +203,7 @@ func main() {
 	fs.MarkDeprecated("k8s-namespace-whitelist", "changed to --k8s-allow-namespace, use that instead")
 	fs.MarkDeprecated("registry-poll-interval", "changed to --automation-interval, use that instead")
 	fs.MarkDeprecated("k8s-in-cluster", "no longer used")
+	fs.MarkDeprecated("git-verify-signatures", "changed to --git-verify-signatures-mode, use that instead")
 
 	var kubeConfig *string
 	{
@@ -308,6 +310,19 @@ func main() {
 		*automationInterval = *registryPollInterval
 	}
 
+	// Maintain backwards compatibility with the --git-verify-signatures
+	// flag, but only if the --git-verify-signature-mode is not set to a
+	// custom (non default) value.
+	var gitVerifySignaturesMode fluxsync.VerifySignaturesMode
+	if *gitVerifySignatures && !fs.Changed("git-verify-signatures-mode") {
+		gitVerifySignaturesMode = fluxsync.VerifySignaturesModeAll
+	} else {
+		if gitVerifySignaturesMode, err = fluxsync.ToVerifySignaturesMode(*gitVerifySignaturesModeStr); err != nil {
+			logger.Log("error", err.Error())
+			os.Exit(1)
+		}
+	}
+
 	// Sort out values for the git tag and notes ref. There are
 	// running deployments that assume the defaults as given, so don't
 	// mess with those unless explicitly told.
@@ -345,7 +360,7 @@ func main() {
 
 	// Import GPG keys, if we've been told where to look for them
 	for _, p := range *gitImportGPG {
-		keyfiles, err := gpg.ImportKeys(p, *gitVerifySignatures)
+		keyfiles, err := gpg.ImportKeys(p, gitVerifySignaturesMode != fluxsync.VerifySignaturesModeNone)
 		if err != nil {
 			logger.Log("error", fmt.Sprintf("failed to import GPG key(s) from %s", p), "err", err.Error())
 		}
@@ -655,7 +670,7 @@ func main() {
 		"user", *gitUser,
 		"email", *gitEmail,
 		"signing-key", *gitSigningKey,
-		"verify-signatures", *gitVerifySignatures,
+		"verify-signatures-mode", gitVerifySignaturesMode,
 		"sync-tag", *gitSyncTag,
 		"state", *syncState,
 		"readonly", *gitReadonly,
@@ -694,7 +709,7 @@ func main() {
 			repo,
 			*gitSyncTag,
 			*gitSigningKey,
-			*gitVerifySignatures,
+			gitVerifySignaturesMode,
 			gitConfig,
 		)
 		if err != nil {
@@ -721,13 +736,13 @@ func main() {
 		ManifestGenerationEnabled: *manifestGeneration,
 		GitSecretEnabled:          *gitSecret,
 		LoopVars: &daemon.LoopVars{
-			SyncInterval:        *syncInterval,
-			SyncTimeout:         *syncTimeout,
-			SyncState:           syncProvider,
-			AutomationInterval:  *automationInterval,
-			GitTimeout:          *gitTimeout,
-			GitVerifySignatures: *gitVerifySignatures,
-			ImageScanDisabled:   *registryDisableScanning,
+			SyncInterval:            *syncInterval,
+			SyncTimeout:             *syncTimeout,
+			SyncState:               syncProvider,
+			AutomationInterval:      *automationInterval,
+			GitTimeout:              *gitTimeout,
+			GitVerifySignaturesMode: gitVerifySignaturesMode,
+			ImageScanDisabled:       *registryDisableScanning,
 		},
 	}
 

docs/references/git-gpg.md

@@ -240,3 +240,32 @@ b. Sign the sync tag by yourself, with a key that is imported, to point
    $ git tag --force --local-user=<key id> -a -m "Sync pointer" <tag name> <revision>
    $ git push --force origin <tag name>
    ```
+
+### Choosing a `--git-verify-signatures-mode`
+
+#### `"none"` (default)
+
+By default, Flux skips GPG verification of all commits.
+
+#### `"all"`
+
+This is the regular verification behavior, consistent with the original
+`--gitVerifySignatures` flag. It will perform GPG verification on every commit
+between the tip of the Flux branch and the Flux sync tag, including all parents.
+If your `master` branch contains only signed commits ([a flow which GitHub
+supports][github-required-gpg]), then this flow ought to work.
+
+#### `"first-parent"`
+
+However, there are some arguments for more limited signing behaviors, e.g. [this
+parable][gpg-dontsign-horror] and [this thread][gpg-dontsign-linus]). In
+particular, it can be useful to allow unsigned commits into `master`, and to
+point Flux at a `release` branch containing signed merges from `master`. A merge
+commit has two parents: the previous commit "in the branch," as well as the last
+commit in the merged branch. In this scenario, use the `"first-parent"` mode --
+only the merge commits "in the branch" should be GPG-verified, since the commits
+from master have no signature.
+
+[github-required-gpg]:https://help.github.com/en/github/administering-a-repository/about-required-commit-signing
+[gpg-dontsign-horror]:https://mikegerwitz.com/2012/05/a-git-horror-story-repository-integrity-with-signed-commits
+[gpg-dontsign-linus]:http://git.661346.n2.nabble.com/GPG-signing-for-git-commit-tp2582986p2583316.html

go.mod

@@ -29,6 +29,7 @@ replace github.com/fluxcd/flux/pkg/install => ./pkg/install
 
 require (
 	github.com/Jeffail/gabs v1.4.0
+	github.com/Masterminds/semver v1.5.0 // indirect
 	github.com/Masterminds/semver/v3 v3.0.3
 	github.com/Masterminds/sprig v2.22.0+incompatible // indirect
 	github.com/VividCortex/gohistogram v1.0.0 // indirect

go.sum

@@ -63,6 +63,8 @@ github.com/Masterminds/goutils v1.1.0 h1:zukEsf/1JZwCMgHiK3GZftabmxiCw4apj3a28RP
 github.com/Masterminds/goutils v1.1.0/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU=
 github.com/Masterminds/semver v1.4.2 h1:WBLTQ37jOCzSLtXNdoo8bNM8876KhNqOKvrlGITgsTc=
 github.com/Masterminds/semver v1.4.2/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y=
+github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww=
+github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y=
 github.com/Masterminds/semver/v3 v3.0.3 h1:znjIyLfpXEDQjOIEWh+ehwpTU14UzUPub3c3sm36u14=
 github.com/Masterminds/semver/v3 v3.0.3/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs=
 github.com/Masterminds/sprig v2.22.0+incompatible h1:z4yfnGrZ7netVz+0EDJ0Wi+5VZCSYp4Z0m2dk6cEM60=

pkg/daemon/daemon.go

@@ -249,7 +249,7 @@ func (d *Daemon) makeJobFromUpdate(update updateFunc) jobFunc {
 		var result job.Result
 		err := d.WithWorkingClone(ctx, func(working *git.Checkout) error {
 			var err error
-			if err = verifyWorkingRepo(ctx, d.Repo, working, d.SyncState); d.GitVerifySignatures && err != nil {
+			if err = verifyWorkingRepo(ctx, d.Repo, working, d.SyncState, d.GitVerifySignaturesMode); d.GitVerifySignaturesMode != sync.VerifySignaturesModeNone && err != nil {
 				return err
 			}
 			result, err = update(ctx, jobID, working, logger)
@@ -373,9 +373,9 @@ func (d *Daemon) sync() jobFunc {
 		if err != nil {
 			return result, err
 		}
-		if d.GitVerifySignatures {
+		if d.GitVerifySignaturesMode != sync.VerifySignaturesModeNone {
 			var latestValidRev string
-			if latestValidRev, _, err = latestValidRevision(ctx, d.Repo, d.SyncState); err != nil {
+			if latestValidRev, _, err = latestValidRevision(ctx, d.Repo, d.SyncState, d.GitVerifySignaturesMode); err != nil {
 				return result, err
 			} else if head != latestValidRev {
 				result.Revision = latestValidRev
@@ -566,7 +566,7 @@ func (d *Daemon) JobStatus(ctx context.Context, jobID job.ID) (job.Status, error
 	if err != nil {
 		return status, errors.Wrap(err, "enumerating commit notes")
 	}
-	commits, err := d.Repo.CommitsBefore(ctx, "HEAD", d.GitConfig.Paths...)
+	commits, err := d.Repo.CommitsBefore(ctx, "HEAD", false, d.GitConfig.Paths...)
 	if err != nil {
 		return status, errors.Wrap(err, "checking revisions for status")
 	}
@@ -602,7 +602,7 @@ func (d *Daemon) SyncStatus(ctx context.Context, commitRef string) ([]string, er
 		return nil, err
 	}
 
-	commits, err := d.Repo.CommitsBetween(ctx, syncMarkerRevision, commitRef, d.GitConfig.Paths...)
+	commits, err := d.Repo.CommitsBetween(ctx, syncMarkerRevision, commitRef, false, d.GitConfig.Paths...)
 	if err != nil {
 		return nil, err
 	}
@@ -880,7 +880,7 @@ func policyEventTypes(u resource.PolicyUpdate) []string {
 // In case the signature of the tag can not be verified, or it points
 // towards a revision we can not get a commit range for, it returns an
 // error.
-func latestValidRevision(ctx context.Context, repo *git.Repo, syncState sync.State) (string, git.Commit, error) {
+func latestValidRevision(ctx context.Context, repo *git.Repo, syncState sync.State, gitVerifySignaturesMode sync.VerifySignaturesMode) (string, git.Commit, error) {
 	var invalidCommit = git.Commit{}
 	newRevision, err := repo.BranchHead(ctx)
 	if err != nil {
@@ -893,15 +893,17 @@ func latestValidRevision(ctx context.Context, repo *git.Repo, syncState sync.Sta
 		return "", invalidCommit, err
 	}
 
+	var gitFirstParent = gitVerifySignaturesMode == sync.VerifySignaturesModeFirstParent
+
 	var commits []git.Commit
 	if tagRevision == "" {
-		commits, err = repo.CommitsBefore(ctx, newRevision)
+		commits, err = repo.CommitsBefore(ctx, newRevision, gitFirstParent)
 	} else {
 		// Assure the commit _at_ the high water mark is a signed and valid commit
 		if err = repo.VerifyCommit(ctx, tagRevision); err != nil {
 			return "", invalidCommit, errors.Wrap(err, "failed to verify signature of last sync'ed revision")
 		}
-		commits, err = repo.CommitsBetween(ctx, tagRevision, newRevision)
+		commits, err = repo.CommitsBetween(ctx, tagRevision, newRevision, gitFirstParent)
 	}
 
 	if err != nil {
@@ -925,8 +927,8 @@ func latestValidRevision(ctx context.Context, repo *git.Repo, syncState sync.Sta
 }
 
 // verifyWorkingRepo checks that a working clone is safe to be used for a write operation
-func verifyWorkingRepo(ctx context.Context, repo *git.Repo, working *git.Checkout, syncState sync.State) error {
-	if latestVerifiedRev, _, err := latestValidRevision(ctx, repo, syncState); err != nil {
+func verifyWorkingRepo(ctx context.Context, repo *git.Repo, working *git.Checkout, syncState sync.State, gitVerifySignaturesMode sync.VerifySignaturesMode) error {
+	if latestVerifiedRev, _, err := latestValidRevision(ctx, repo, syncState, gitVerifySignaturesMode); err != nil {
 		return err
 	} else if headRev, err := working.HeadRevision(ctx); err != nil {
 		return err

pkg/daemon/daemon_test.go

@@ -727,7 +727,7 @@ func mockDaemon(t *testing.T) (*Daemon, func(), func(), *mock.Mock, *mockEventWr
 
 	manifests := kubernetes.NewManifests(kubernetes.ConstNamespacer("default"), log.NewLogfmtLogger(os.Stdout))
 
-	gitSync, _ := fluxsync.NewGitTagSyncProvider(repo, syncTag, "", false, params)
+	gitSync, _ := fluxsync.NewGitTagSyncProvider(repo, syncTag, "", fluxsync.VerifySignaturesModeNone, params)
 
 	// Finally, the daemon
 	d := &Daemon{
@@ -741,7 +741,7 @@ func mockDaemon(t *testing.T) (*Daemon, func(), func(), *mock.Mock, *mockEventWr
 		JobStatusCache: &job.StatusCache{Size: 100},
 		EventWriter:    events,
 		Logger:         logger,
-		LoopVars:       &LoopVars{SyncTimeout: timeout, GitTimeout: timeout, SyncState: gitSync},
+		LoopVars:       &LoopVars{SyncTimeout: timeout, GitTimeout: timeout, SyncState: gitSync, GitVerifySignaturesMode: fluxsync.VerifySignaturesModeNone},
 	}
 
 	start := func() {

pkg/daemon/loop.go

@@ -14,13 +14,13 @@ import (
 )
 
 type LoopVars struct {
-	SyncInterval        time.Duration
-	SyncTimeout         time.Duration
-	AutomationInterval  time.Duration
-	GitTimeout          time.Duration
-	GitVerifySignatures bool
-	SyncState           fluxsync.State
-	ImageScanDisabled   bool
+	SyncInterval            time.Duration
+	SyncTimeout             time.Duration
+	AutomationInterval      time.Duration
+	GitTimeout              time.Duration
+	GitVerifySignaturesMode fluxsync.VerifySignaturesMode
+	SyncState               fluxsync.State
+	ImageScanDisabled       bool
 
 	initOnce               sync.Once
 	syncSoon               chan struct{}
@@ -115,8 +115,8 @@ func (d *Daemon) Loop(stop chan struct{}, wg *sync.WaitGroup, logger log.Logger)
 			var err error
 
 			ctx, cancel := context.WithTimeout(context.Background(), d.GitTimeout)
-			if d.GitVerifySignatures {
-				newSyncHead, invalidCommit, err = latestValidRevision(ctx, d.Repo, d.SyncState)
+			if d.GitVerifySignaturesMode != fluxsync.VerifySignaturesModeNone {
+				newSyncHead, invalidCommit, err = latestValidRevision(ctx, d.Repo, d.SyncState, d.GitVerifySignaturesMode)
 			} else {
 				newSyncHead, err = d.Repo.BranchHead(ctx)
 			}

pkg/daemon/sync.go

@@ -141,10 +141,10 @@ func getChangeSet(ctx context.Context, state revisionRatchet, headRev string, re
 
 	ctx, cancel := context.WithTimeout(ctx, timeout)
 	if c.oldTagRev != "" {
-		c.commits, err = repo.CommitsBetween(ctx, c.oldTagRev, c.newTagRev, paths...)
+		c.commits, err = repo.CommitsBetween(ctx, c.oldTagRev, c.newTagRev, false, paths...)
 	} else {
 		c.initialSync = true
-		c.commits, err = repo.CommitsBefore(ctx, c.newTagRev, paths...)
+		c.commits, err = repo.CommitsBefore(ctx, c.newTagRev, false, paths...)
 	}
 	cancel()
 

pkg/daemon/sync_test.go

@@ -155,7 +155,7 @@ func TestPullAndSync_InitialSync(t *testing.T) {
 	}
 
 	syncTag := "sync"
-	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", false, d.GitConfig)
+	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", fluxsync.VerifySignaturesModeNone, d.GitConfig)
 	syncState := &lastKnownSyncState{logger: d.Logger, state: gitSync}
 
 	if err := d.Sync(ctx, time.Now().UTC(), head, syncState); err != nil {
@@ -188,7 +188,7 @@ func TestPullAndSync_InitialSync(t *testing.T) {
 	// It creates the tag at HEAD
 	if err := d.Repo.Refresh(context.Background()); err != nil {
 		t.Errorf("pulling sync tag: %v", err)
-	} else if revs, err := d.Repo.CommitsBefore(context.Background(), syncTag); err != nil {
+	} else if revs, err := d.Repo.CommitsBefore(context.Background(), syncTag, false); err != nil {
 		t.Errorf("finding revisions before sync tag: %v", err)
 	} else if len(revs) <= 0 {
 		t.Errorf("Found no revisions before the sync tag")
@@ -243,7 +243,7 @@ func TestDoSync_NoNewCommits(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", false, d.GitConfig)
+	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", fluxsync.VerifySignaturesModeNone, d.GitConfig)
 	syncState := &lastKnownSyncState{logger: d.Logger, state: gitSync}
 
 	if err := d.Sync(ctx, time.Now().UTC(), head, syncState); err != nil {
@@ -266,12 +266,12 @@ func TestDoSync_NoNewCommits(t *testing.T) {
 	}
 
 	// It doesn't move the tag
-	oldRevs, err := d.Repo.CommitsBefore(ctx, syncTag)
+	oldRevs, err := d.Repo.CommitsBefore(ctx, syncTag, false)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	if revs, err := d.Repo.CommitsBefore(ctx, syncTag); err != nil {
+	if revs, err := d.Repo.CommitsBefore(ctx, syncTag, false); err != nil {
 		t.Errorf("finding revisions before sync tag: %v", err)
 	} else if !reflect.DeepEqual(revs, oldRevs) {
 		t.Errorf("Should have kept the sync tag at HEAD")
@@ -362,7 +362,7 @@ func TestDoSync_WithNewCommit(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", false, d.GitConfig)
+	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", fluxsync.VerifySignaturesModeNone, d.GitConfig)
 	syncState := &lastKnownSyncState{logger: d.Logger, state: gitSync}
 
 	if err := d.Sync(ctx, time.Now().UTC(), head, syncState); err != nil {
@@ -397,7 +397,7 @@ func TestDoSync_WithNewCommit(t *testing.T) {
 	defer cancel()
 	if err := d.Repo.Refresh(ctx); err != nil {
 		t.Errorf("pulling sync tag: %v", err)
-	} else if revs, err := d.Repo.CommitsBetween(ctx, oldRevision, syncTag); err != nil {
+	} else if revs, err := d.Repo.CommitsBetween(ctx, oldRevision, syncTag, false); err != nil {
 		t.Errorf("finding revisions before sync tag: %v", err)
 	} else if len(revs) <= 0 {
 		t.Errorf("Should have moved sync tag forward")
@@ -426,7 +426,7 @@ func TestDoSync_WithErrors(t *testing.T) {
 	}
 
 	syncTag := "sync"
-	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", false, d.GitConfig)
+	gitSync, _ := fluxsync.NewGitTagSyncProvider(d.Repo, syncTag, "", fluxsync.VerifySignaturesModeNone, d.GitConfig)
 	syncState := &lastKnownSyncState{logger: d.Logger, state: gitSync}
 
 	if err := d.Sync(ctx, time.Now().UTC(), head, syncState); err != nil {

pkg/git/gittest/repo_test.go

@@ -49,7 +49,7 @@ func TestCommit(t *testing.T) {
 		t.Error(err)
 	}
 
-	commits, err := repo.CommitsBefore(ctx, "HEAD")
+	commits, err := repo.CommitsBefore(ctx, "HEAD", false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -105,7 +105,7 @@ func TestSignedCommit(t *testing.T) {
 		t.Error(err)
 	}
 
-	commits, err := repo.CommitsBefore(ctx, "HEAD")
+	commits, err := repo.CommitsBefore(ctx, "HEAD", false)
 
 	if err != nil {
 		t.Fatal(err)

pkg/git/operations.go

@@ -251,10 +251,16 @@ func refRevision(ctx context.Context, workingDir, ref string) (string, error) {
 }
 
 // Return the revisions and one-line log commit messages
-func onelinelog(ctx context.Context, workingDir, refspec string, subdirs []string) ([]Commit, error) {
+func onelinelog(ctx context.Context, workingDir, refspec string, subdirs []string, firstParent bool) ([]Commit, error) {
 	out := &bytes.Buffer{}
-	args := []string{"log", "--pretty=format:%GK|%G?|%H|%s", refspec}
-	args = append(args, "--")
+	args := []string{"log", "--pretty=format:%GK|%G?|%H|%s"}
+
+	if firstParent {
+		args = append(args, "--first-parent")
+	}
+
+	args = append(args, refspec, "--")
+
 	if len(subdirs) > 0 {
 		args = append(args, subdirs...)
 	}