Repository: palantir/policy-bot
Pull request #285 (2021-04-16 16:13:30)

Add support for signature verification predicates
This PR introduces support for policy-bot to verify the presence of git commit signatures in a pull request.

- New predicate `has_valid_signatures`, this predicate should be used to test if commits are signed and their signatures valid.
- New predicate `has_valid_signatures_by`, commits are required to have valid signatures and additionally the signatures must be authenticated as one of the valid users/team/org members listed.
- New predicate `has_valid_signatures_by_key`, commits are required to have valid signatures and additionally the signatures must be attributed to one of the key_ids listed.

```yaml
approval_rules:

- name: commits are signed
  if:
    has_valid_signatures: true

- name: commits are signed by excavator
  if:
    has_valid_signatures_by:
      users: ["svc-excavator-bot"]
      
- name: commits are signed by known excavator key
  if:
    has_valid_signatures_by_keys:
      key_ids: ["3AA5C34371567BD2"]

```



Patch:
diff --git a/README.md b/README.md
index c65251b..50b93bc 100644
--- a/README.md
+++ b/README.md
@@ -244,6 +244,25 @@ if:
     not_matches:
       - "^(docs|style|chore): (\\w| )+$"
 
+  # "has_valid_signatures" is satisfied if the commits in the pull request
+  # all have git commit signatures that have been verified by GitHub
+  has_valid_signatures: true
+
+  # "has_valid_signatures_by" is satisfied if the commits in the pull request
+  # all have git commit signatures that have been verified by GitHub, and
+  # the authenticated signatures are attributed to a user in the users list 
+  # or belong to a user in any of the listed organizations or teams.
+  has_valid_signatures_by:
+    users: ["user1", "user2", ...]
+    organizations: ["org1", "org2", ...]
+    teams: ["org1/team1", "org2/team2", ...]
+
+  # "has_valid_signatures_by_keys" is satisfied if the commits in the pull request
+  # all have git commit signatures that have been verified by GitHub, and
+  # the authenticated signatures are attributed to a GPG key with an ID in the list.
+  has_valid_signatures_by_keys:
+    key_ids: ["3AA5C34371567BD2"]
+
 # "options" specifies a set of restrictions on approvals. If the block does not
 # exist, the default values are used.
 options:
diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go
index 7c24d6d..45a21d6 100644
--- a/policy/predicate/predicates.go
+++ b/policy/predicate/predicates.go
@@ -33,6 +33,10 @@ type Predicates struct {
 	HasLabels *HasLabels `yaml:"has_labels"`
 
 	Title *Title `yaml:"title"`
+
+	HasValidSignatures       *HasValidSignatures       `yaml:"has_valid_signatures"`
+	HasValidSignaturesBy     *HasValidSignaturesBy     `yaml:"has_valid_signatures_by"`
+	HasValidSignaturesByKeys *HasValidSignaturesByKeys `yaml:"has_valid_signatures_by_keys"`
 }
 
 func (p *Predicates) Predicates() []Predicate {
@@ -81,5 +85,17 @@ func (p *Predicates) Predicates() []Predicate {
 		ps = append(ps, Predicate(p.Title))
 	}
 
+	if p.HasValidSignatures != nil {
+		ps = append(ps, Predicate(p.HasValidSignatures))
+	}
+
+	if p.HasValidSignaturesBy != nil {
+		ps = append(ps, Predicate(p.HasValidSignaturesBy))
+	}
+
+	if p.HasValidSignaturesByKeys != nil {
+		ps = append(ps, Predicate(p.HasValidSignaturesByKeys))
+	}
+
 	return ps
 }
diff --git a/policy/predicate/signature.go b/policy/predicate/signature.go
new file mode 100644
index 0000000..90cd205
--- /dev/null
+++ b/policy/predicate/signature.go
@@ -0,0 +1,153 @@
+// Copyright 2021 Palantir Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package predicate
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/pkg/errors"
+
+	"github.com/palantir/policy-bot/policy/common"
+	"github.com/palantir/policy-bot/pull"
+)
+
+type HasValidSignatures bool
+
+var _ Predicate = HasValidSignatures(false)
+
+func (pred HasValidSignatures) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) {
+	commits, err := prctx.Commits()
+	if err != nil {
+		return false, "", errors.Wrap(err, "failed to get commits")
+	}
+
+	for _, c := range commits {
+		valid, desc := hasValidSignature(ctx, c)
+		if !valid {
+			if pred {
+				return false, desc, nil
+			}
+			return true, "", nil
+		}
+	}
+
+	if pred {
+		return true, "", nil
+	}
+	return false, "All commits are signed and have valid signatures", nil
+}
+
+func (pred HasValidSignatures) Trigger() common.Trigger {
+	return common.TriggerCommit
+}
+
+type HasValidSignaturesBy struct {
+	common.Actors `yaml:",inline"`
+}
+
+var _ Predicate = &HasValidSignaturesBy{}
+
+func (pred *HasValidSignaturesBy) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) {
+	commits, err := prctx.Commits()
+	if err != nil {
+		return false, "", errors.Wrap(err, "failed to get commits")
+	}
+
+	signers := make(map[string]struct{})
+
+	for _, c := range commits {
+		valid, desc := hasValidSignature(ctx, c)
+		if !valid {
+			return false, desc, nil
+		}
+		signers[c.Signature.Signer] = struct{}{}
+	}
+
+	for signer := range signers {
+		member, err := pred.IsActor(ctx, prctx, signer)
+		if err != nil {
+			return false, "", err
+		}
+		if !member {
+			return false, fmt.Sprintf("Contributor %q does not meet the required membership conditions for signing", signer), nil
+		}
+	}
+
+	return true, "", nil
+}
+
+func (pred *HasValidSignaturesBy) Trigger() common.Trigger {
+	return common.TriggerCommit
+}
+
+type HasValidSignaturesByKeys struct {
+	KeyIDs []string `yaml:"key_ids"`
+}
+
+var _ Predicate = &HasValidSignaturesByKeys{}
+
+func (pred *HasValidSignaturesByKeys) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) {
+	commits, err := prctx.Commits()
+	if err != nil {
+		return false, "", errors.Wrap(err, "failed to get commits")
+	}
+
+	keys := make(map[string]struct{})
+
+	for _, c := range commits {
+		valid, desc := hasValidSignature(ctx, c)
+		if !valid {
+			return false, desc, nil
+		}
+		// Only GPG signatures are valid for this predicate
+		switch c.Signature.Type {
+		case pull.SignatureGpg:
+			keys[c.Signature.KeyID] = struct{}{}
+		default:
+			return false, fmt.Sprintf("Commit %.10s signature is not a GPG signature", c.SHA), nil
+		}
+	}
+
+	for key := range keys {
+		isValidKey := false
+		for _, acceptedKey := range pred.KeyIDs {
+			if key == acceptedKey {
+				isValidKey = true
+				break
+			}
+		}
+		if !isValidKey {
+			return false, fmt.Sprintf("Key %q does not meet the required key conditions for signing", key), nil
+		}
+	}
+
+	return true, "", nil
+}
+
+func (pred *HasValidSignaturesByKeys) Trigger() common.Trigger {
+	return common.TriggerCommit
+}
+
+func hasValidSignature(ctx context.Context, commit *pull.Commit) (bool, string) {
+	if commit.Signature == nil {
+		return false, fmt.Sprintf("Commit %.10s has no signature", commit.SHA)
+	}
+	if !commit.Signature.IsValid {
+		reason := commit.Signature.State
+		return false, fmt.Sprintf("Commit %.10s has an invalid signature due to %s", commit.SHA, reason)
+	}
+	return true, ""
+}
diff --git a/pull/context.go b/pull/context.go
index 824de20..cd991bc 100644
--- a/pull/context.go
+++ b/pull/context.go
@@ -148,6 +148,10 @@ type Commit struct {
 	// PushedAt is the timestamp when the commit was pushed. It is nil if that
 	// information is not available for this commit.
 	PushedAt *time.Time
+
+	// Signature is the signature and details that was extracted from the commit.
+	// It is nil if the commit has no signature
+	Signature *Signature
 }
 
 // Users returns the login names of the users associated with this commit.
@@ -200,3 +204,11 @@ type Reviewer struct {
 	Name    string
 	Removed bool
 }
+
+type Signature struct {
+	Type    SignatureType
+	IsValid bool
+	KeyID   string
+	Signer  string
+	State   string
+}
diff --git a/pull/github.go b/pull/github.go
index a126db7..603de9c 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -860,6 +860,7 @@ type v4Commit struct {
 			OID string
 		}
 	} `graphql:"parents(first: 3)"`
+	Signature *v4GitSignature
 }
 
 func (c *v4Commit) ToCommit() *Commit {
@@ -868,6 +869,11 @@ func (c *v4Commit) ToCommit() *Commit {
 		parents = append(parents, p.OID)
 	}
 
+	var signature *Signature
+	if c.Signature != nil {
+		signature = c.Signature.ToSignature()
+	}
+
 	return &Commit{
 		SHA:             c.OID,
 		Parents:         parents,
@@ -875,6 +881,7 @@ func (c *v4Commit) ToCommit() *Commit {
 		Author:          c.Author.GetV3Login(),
 		Committer:       c.Committer.GetV3Login(),
 		PushedAt:        c.PushedDate,
+		Signature:       signature,
 	}
 }
 
@@ -933,3 +940,59 @@ func isNotFound(err error) bool {
 	}
 	return false
 }
+
+type SignatureType string
+
+const (
+	SignatureGpg   SignatureType = "GpgSignature"
+	SignatureSmime SignatureType = "SmimeSignature"
+)
+
+type v4GitSignature struct {
+	Type  string           `graphql:"__typename"`
+	GPG   v4GpgSignature   `graphql:"... on GpgSignature"`
+	SMIME v4SmimeSignature `graphql:"... on SmimeSignature"`
+}
+
+func (s *v4GitSignature) ToSignature() *Signature {
+	switch SignatureType(s.Type) {
+	case SignatureGpg:
+		return &Signature{
+			IsValid: s.GPG.IsValid,
+			KeyID:   s.GPG.KeyID,
+			Signer:  s.GPG.Signer.GetV3Login(),
+			State:   s.GPG.State,
+			Type:    SignatureGpg,
+		}
+	case SignatureSmime:
+		return &Signature{
+			IsValid: s.SMIME.IsValid,
+			Signer:  s.SMIME.Signer.GetV3Login(),
+			State:   s.SMIME.State,
+			Type:    SignatureSmime,
+		}
+	default:
+		return nil
+	}
+}
+
+type v4SmimeSignature struct {
+	Email             string
+	IsValid           bool
+	Payload           string
+	Signature         string
+	Signer            *v4Actor
+	State             string
+	WasSignedByGitHub bool
+}
+
+type v4GpgSignature struct {
+	Email             string
+	IsValid           bool
+	KeyID             string
+	Payload           string
+	Signature         string
+	Signer            *v4Actor
+	State             string
+	WasSignedByGitHub bool
+}


Test patch:
diff --git a/policy/predicate/signature_test.go b/policy/predicate/signature_test.go
new file mode 100644
index 0000000..30dab8d
--- /dev/null
+++ b/policy/predicate/signature_test.go
@@ -0,0 +1,286 @@
+// Copyright 2021 Palantir Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package predicate
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/palantir/policy-bot/policy/common"
+	"github.com/palantir/policy-bot/pull"
+	"github.com/palantir/policy-bot/pull/pulltest"
+)
+
+func TestHasValidSignatures(t *testing.T) {
+	pTrue := HasValidSignatures(true)
+	pFalse := HasValidSignatures(false)
+
+	testCases := []SignatureTestCase{
+		{
+			"ValidSignature",
+			true,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "mhaypenny",
+						Committer: "mhaypenny",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: true,
+							Signer:  "ttest",
+							State:   "VALID",
+							KeyID:   "3AA5C34371567BD2",
+						},
+					},
+				},
+			},
+		},
+		{
+			"InvalidSignature",
+			false,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "mhaypenny",
+						Committer: "mhaypenny",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: false,
+							Signer:  "ttest",
+							State:   "INVALID",
+							KeyID:   "3AA5C34371567BD2",
+						},
+					},
+				},
+			},
+		},
+		{
+			"NoSignature",
+			false,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "mhaypenny",
+						Committer: "mhaypenny",
+						Signature: nil,
+					},
+				},
+			},
+		},
+	}
+
+	runSignatureTests(t, pTrue, testCases)
+
+	// Invert the expected outcomes and test against the false predicate
+	for idx := range testCases {
+		testCases[idx].Expected = !testCases[idx].Expected
+	}
+	runSignatureTests(t, pFalse, testCases)
+}
+
+func TestHasValidSignaturesBy(t *testing.T) {
+	p := &HasValidSignaturesBy{
+		common.Actors{
+			Teams:         []string{"testorg/team"},
+			Users:         []string{"mhaypenny"},
+			Organizations: []string{"testorg"},
+		},
+	}
+
+	runSignatureTests(t, p, []SignatureTestCase{
+		{
+			"ValidSignatureByUser",
+			true,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "mhaypenny",
+						Committer: "mhaypenny",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: true,
+							Signer:  "mhaypenny",
+							State:   "VALID",
+							KeyID:   "3AA5C34371567BD2",
+						},
+					},
+				},
+			},
+		},
+		{
+			"ValidSignatureButNotUser",
+			false,
+			&pulltest.Context{
+				AuthorValue: "badcommitter",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "badcommitter",
+						Committer: "badcommitter",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: true,
+							Signer:  "badcommitter",
+							State:   "VALID",
+							KeyID:   "3AD5C34671567BC3",
+						},
+					},
+				},
+			},
+		},
+		{
+			"ValidSignatureByTeamMember",
+			true,
+			&pulltest.Context{
+				AuthorValue: "ttest",
+				TeamMemberships: map[string][]string{
+					"ttest": {
+						"testorg/team",
+					},
+				},
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "ttest",
+						Committer: "ttest",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: true,
+							Signer:  "ttest",
+							State:   "VALID",
+							KeyID:   "3AA5C34371567BD2",
+						},
+					},
+				},
+			},
+		},
+		{
+			"NoSignature",
+			false,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "mhaypenny",
+						Committer: "mhaypenny",
+						Signature: nil,
+					},
+				},
+			},
+		},
+	})
+}
+
+func TestHasValidSignaturesByKeys(t *testing.T) {
+	p := &HasValidSignaturesByKeys{
+		KeyIDs: []string{"3AA5C34371567BD2"},
+	}
+
+	runSignatureTests(t, p, []SignatureTestCase{
+		{
+			"ValidSignatureByValidKey",
+			true,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "ttest",
+						Committer: "ttest",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: true,
+							Signer:  "mhaypenny",
+							State:   "VALID",
+							KeyID:   "3AA5C34371567BD2",
+						},
+					},
+				},
+			},
+		},
+		{
+			"ValidSignatureByInvalidKey",
+			false,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "ttest",
+						Committer: "ttest",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: true,
+							Signer:  "mhaypenny",
+							State:   "VALID",
+							KeyID:   "3AB5C35371567CE7",
+						},
+					},
+				},
+			},
+		},
+		{
+			"InvalidSignatureByInvalidKey",
+			false,
+			&pulltest.Context{
+				AuthorValue: "mhaypenny",
+				CommitsValue: []*pull.Commit{
+					{
+						SHA:       "abcdef123456789",
+						Author:    "ttest",
+						Committer: "ttest",
+						Signature: &pull.Signature{
+							Type:    pull.SignatureGpg,
+							IsValid: false,
+							Signer:  "mhaypenny",
+							State:   "BAD_EMAIL",
+							KeyID:   "3AB5C35371567CE7",
+						},
+					},
+				},
+			},
+		},
+	})
+}
+
+type SignatureTestCase struct {
+	Name     string
+	Expected bool
+	Context  pull.Context
+}
+
+func runSignatureTests(t *testing.T, p Predicate, cases []SignatureTestCase) {
+	ctx := context.Background()
+
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			ok, _, err := p.Evaluate(ctx, tc.Context)
+			if assert.NoError(t, err, "evaluation failed") {
+				assert.Equal(t, tc.Expected, ok, "predicate was not correct")
+			}
+		})
+	}
+}
diff --git a/pull/github_test.go b/pull/github_test.go
index a50da3a..dd3e492 100644
--- a/pull/github_test.go
+++ b/pull/github_test.go
@@ -114,17 +114,25 @@ func TestCommits(t *testing.T) {
 	assert.Equal(t, "mhaypenny", commits[0].Author)
 	assert.Equal(t, "mhaypenny", commits[0].Committer)
 	assert.Equal(t, newTime(expectedTime), commits[0].PushedAt)
+	assert.Nil(t, commits[0].Signature)
 
 	assert.Equal(t, "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9", commits[1].SHA)
 	assert.Equal(t, "mhaypenny", commits[1].Author)
 	assert.Equal(t, "mhaypenny", commits[1].Committer)
 	assert.Equal(t, newTime(expectedTime), commits[1].PushedAt)
+	assert.Nil(t, commits[1].Signature)
 
 	assert.Equal(t, "e05fcae367230ee709313dd2720da527d178ce43", commits[2].SHA)
 	assert.Equal(t, "ttest", commits[2].Author)
 	assert.Equal(t, "mhaypenny", commits[2].Committer)
 	assert.Equal(t, newTime(expectedTime.Add(48*time.Hour)), commits[2].PushedAt)
 
+	// verify that the signature was handled correctly
+	assert.NotNil(t, commits[2].Signature)
+	assert.Equal(t, "3AA5C34371567BD2", commits[2].Signature.KeyID)
+	assert.Equal(t, "mhaypenny", commits[2].Signature.Signer)
+	assert.True(t, commits[2].Signature.IsValid)
+
 	// verify that the commit list is cached
 	commits, err = ctx.Commits()
 	require.NoError(t, err)
diff --git a/pull/testdata/responses/pull_commits.yml b/pull/testdata/responses/pull_commits.yml
index 731a43e..34e1dd2 100644
--- a/pull/testdata/responses/pull_commits.yml
+++ b/pull/testdata/responses/pull_commits.yml
@@ -96,6 +96,20 @@
                           "oid": "1fc89f1cedf8e3f3ce516ab75b5952295c8ea5e9"
                         }
                       ]
+                    },
+                    "signature": {
+                      "__typename": "GpgSignature",
+                      "email": "mhaypenny@example.com",
+                      "isValid": true,
+                      "keyId": "3AA5C34371567BD2",
+                      "payload": "tree 2075e4ceb83e3054e2bfba591e4325a8dd35af96\nparent 1fceb875cfcddb56ea683683784c020e0ea0693c\nauthor ttest \u003cttest@example.com\u003e 1618750938 +0000\ncommitter mhaypenny \u003cmhaypenny@example.com\u003e 1618750938 +0000\n\nExample commit",
+                      "signature": "-----BEGIN PGP SIGNATURE-----\n\nwl4EABMIABAFAmB8LdoJEA5EZb3lUxDmAADqTQD9EvRNEmdpX13Lo6YfHsty0NaN\nKI6CebzIyFJjVfmPjjQBAKPBz3VYyWEJCM2KF/GcQ0F3dIcYM4XVYXkvoL3zqxlr\n=7g9t\n-----END PGP SIGNATURE-----\n",
+                      "signer": {
+                        "__typename": "User",
+                        "login": "mhaypenny"
+                      },
+                      "state": "VALID",
+                      "wasSignedByGitHub": false
                     }
                   }
                 }