Add --gitVerifySignaturesMode (fluxcd#2704)

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

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

    * "first-parent" uses the behavior described in issue fluxcd#2704, 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, "if set, the signature of commits will be verified before Flux applies them")
+		gitVerifySignaturesMode = fs.String("git-verify-signatures-mode", "all", fmt.Sprintf("if git-verify-signatures is set, which strategy to use for signature verification (one of {$s}", strings.Join([]string{"all", "first-parent"}, ",")))
 
 		// 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")
@@ -657,6 +658,7 @@ func main() {
 		"email", *gitEmail,
 		"signing-key", *gitSigningKey,
 		"verify-signatures", *gitVerifySignatures,
+		"verify-signatures-mode", *gitVerifySignaturesMode,
 		"sync-tag", *gitSyncTag,
 		"state", *syncState,
 		"readonly", *gitReadonly,
@@ -722,12 +724,13 @@ func main() {
 		ManifestGenerationEnabled: *manifestGeneration,
 		GitSecretEnabled:          *gitSecret,
 		LoopVars: &daemon.LoopVars{
-			SyncInterval:        *syncInterval,
-			SyncTimeout:         *syncTimeout,
-			SyncState:           syncProvider,
-			AutomationInterval:  *automationInterval,
-			GitTimeout:          *gitTimeout,
-			GitVerifySignatures: *gitVerifySignatures,
+			SyncInterval:            *syncInterval,
+			SyncTimeout:             *syncTimeout,
+			SyncState:               syncProvider,
+			AutomationInterval:      *automationInterval,
+			GitTimeout:              *gitTimeout,
+			GitVerifySignatures:     *gitVerifySignatures,
+			GitVerifySignaturesMode: *gitVerifySignaturesMode,
 		},
 	}
 

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
@@ -42,6 +43,7 @@ require (
 	github.com/fluxcd/helm-operator v1.0.0-rc6
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-kit/kit v0.9.0
+	github.com/go-stack/stack v1.8.0 // indirect
 	github.com/gogo/googleapis v1.3.1 // indirect
 	github.com/gogo/protobuf v1.3.1
 	github.com/gogo/status v1.1.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.GitVerifySignatures && err != nil {
 				return err
 			}
 			result, err = update(ctx, jobID, working, logger)
@@ -375,7 +375,7 @@ func (d *Daemon) sync() jobFunc {
 		}
 		if d.GitVerifySignatures {
 			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 string) (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 == "first-parent"
+
 	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 string) 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/loop.go

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

pkg/daemon/sync.go

@@ -139,10 +139,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

@@ -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")
@@ -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")
@@ -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")

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...)
 	}

pkg/git/operations_test.go

@@ -192,7 +192,53 @@ func TestOnelinelog_NoGitpath(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	commits, err := onelinelog(context.Background(), newDir, "HEAD~2..HEAD", nil)
+	commits, err := onelinelog(context.Background(), newDir, "HEAD~2..HEAD", nil, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(commits) != 2 {
+		t.Fatal(err)
+	}
+}
+
+func TestOnelinelog_NoGitpath_Merged(t *testing.T) {
+	newDir, cleanup := testfiles.TempDir(t)
+	defer cleanup()
+
+	branch := "tmp"
+	subdirs := []string{"dev", "prod"}
+	err := createRepo(newDir, subdirs)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = execCommand("git", "-C", newDir, "checkout", "-b", branch); err != nil {
+		t.Fatal(err)
+	}
+	if err = updateDirAndCommit(newDir, "dev", testfiles.FilesUpdated); err != nil {
+		t.Fatal(err)
+	}
+	if err = updateDirAndCommit(newDir, "prod", testfiles.FilesUpdated); err != nil {
+		t.Fatal(err)
+	}
+	if err = execCommand("git", "-C", newDir, "checkout", "master"); err != nil {
+		t.Fatal(err)
+	}
+	if err = execCommand("git", "-C", newDir, "merge", "--no-ff", branch); err != nil {
+		t.Fatal(err)
+	}
+
+	commits, err := onelinelog(context.Background(), newDir, "HEAD", nil, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(commits) != 4 {
+		t.Fatal(err)
+	}
+
+	commits, err = onelinelog(context.Background(), newDir, "HEAD", nil, true)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -220,7 +266,7 @@ func TestOnelinelog_WithGitpath(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	commits, err := onelinelog(context.Background(), newDir, "HEAD~2..HEAD", []string{"dev"})
+	commits, err := onelinelog(context.Background(), newDir, "HEAD~2..HEAD", []string{"dev"}, false)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -230,6 +276,57 @@ func TestOnelinelog_WithGitpath(t *testing.T) {
 	}
 }
 
+func TestOnelinelog_WithGitpath_Merged(t *testing.T) {
+	newDir, cleanup := testfiles.TempDir(t)
+	defer cleanup()
+
+	branch := "tmp"
+	subdirs := []string{"dev", "prod"}
+	err := createRepo(newDir, subdirs)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = execCommand("git", "-C", newDir, "checkout", "-b", branch); err != nil {
+		t.Fatal(err)
+	}
+	if err = updateDirAndCommit(newDir, "dev", testfiles.FilesUpdated); err != nil {
+		t.Fatal(err)
+	}
+	if err = updateDirAndCommit(newDir, "dev", testfiles.FilesUpdated); err != nil {
+		t.Fatal(err)
+	}
+	if err = updateDirAndCommit(newDir, "prod", testfiles.FilesUpdated); err != nil {
+		t.Fatal(err)
+	}
+	if err = execCommand("git", "-C", newDir, "checkout", "master"); err != nil {
+		t.Fatal(err)
+	}
+	if err = execCommand("git", "-C", newDir, "merge", "--no-ff", branch); err != nil {
+		t.Fatal(err)
+	}
+
+	// show the 2 update commits as well as init commit
+	commits, err := onelinelog(context.Background(), newDir, "HEAD", []string{"prod"}, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(commits) != 3 {
+		t.Fatal(err)
+	}
+
+	// show the merge commit as well as init commit
+	commits, err = onelinelog(context.Background(), newDir, "HEAD", []string{"prod"}, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(commits) != 2 {
+		t.Fatal(err)
+	}
+}
+
 func TestCheckPush(t *testing.T) {
 	upstreamDir, upstreamCleanup := testfiles.TempDir(t)
 	defer upstreamCleanup()