Add support for signature verification predicates #285

IainSteers · 2021-04-16T16:13:30Z

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.

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"]

policy/predicate/signature.go

pull/context.go

pull/github.go

README.md

bluekeyes

I'm interested to see how these predicates end up being used. I can see uses as disapproval conditions and in combination with other conditions, but I'm curious if they will be used on their own. Maybe commit signatures could replace the forgeable (without additional pre-receive hooks) author and committer checks to some extent?

bluekeyes · 2021-04-19T23:49:26Z

README.md

+  # "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 any of the listed organizations or teams.


Suggested change

# or belong to any of the listed organizations or teams.

# or belong to a user in any of the listed organizations or teams.

README.md

bluekeyes · 2021-04-19T23:54:19Z

policy/predicate/signature.go

+	if !pred {
+		return true, "", nil
+	}


See my note above about what it means when this predicate is false. If we do keep the current implementation, let's do this check before getting the commits.

policy/predicate/signature.go

bluekeyes · 2021-04-20T04:05:19Z

pull/context.go

+type Signature interface {
+	GetEmail() string
+	GetIsValid() bool
+	GetPayload() string
+	GetSignature() string
+	GetSigner() string
+	GetState() string
+	GetSignedByGitHub() bool
+}


I'm not sold on the use of an interface to represent the different signature types here, especially since the only difference is the addition of the ID field in the GPG case. It stands out in comparison to the rest of the pull.Context return types, which are all simple concrete structs.

Using a pointer to a concrete Signature in Commit also makes it clearer that the value may not be present.

What do you think about using a concrete type with an optional KeyID field and maybe a Type field (similar to the Type field on Reviewer, especially since you already defined a type type for use with GraphQL)? Since you already have functions to convert the GitHub API types into the pull.Context types, I don't think it would change the code much.

That's fair. I mostly went with the interface out of habit rather than a particular need.

You're right it didn't change the code much and arguably given then types involved it feels neater now.

bluekeyes · 2021-04-20T04:08:35Z

pull/github.go

+const (
+	SignatureGpg     SignatureType = "GpgSignature"
+	SignatureSmime   SignatureType = "SmimeSignature"
+	SignatureUnknown SignatureType = "UnknownSignature"


Is this type used anywhere?

No, I thought it might be necessary at one point but forgot to remove it. Done

bluekeyes

One non-blocking comment on minimizing unused fields, but otherwise this looks good to me.

bluekeyes · 2021-05-03T18:10:54Z

pull/context.go

+type Signature struct {
+	Type              SignatureType
+	Email             string
+	IsValid           bool
+	KeyID             string
+	Payload           string
+	Signature         string
+	Signer            string
+	State             string
+	WasSignedByGitHub bool
+}


Historically, I've tried to only expose fields that were actually used and then add additional fields as needed. It looks like WasSignedByGitHub, Email, Payload, Signature, and maybe others are not used. What do you think about removing those for now?

Co-authored-by: Andrew Svoboda <svoboda.andrew@gmail.com>

asvoboda reviewed Apr 16, 2021

View reviewed changes

policy/predicate/signature.go Show resolved Hide resolved

policy/predicate/signature.go Outdated Show resolved Hide resolved

pull/context.go Outdated Show resolved Hide resolved

pull/github.go Show resolved Hide resolved

IainSteers force-pushed the isteers/signature-predicates branch 3 times, most recently from 9ad431e to a5a6357 Compare April 19, 2021 07:10

IainSteers marked this pull request as ready for review April 19, 2021 07:13

asvoboda reviewed Apr 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

bluekeyes reviewed Apr 20, 2021

View reviewed changes

bluekeyes approved these changes May 3, 2021

View reviewed changes

IainSteers and others added 11 commits May 6, 2021 16:54

Introduce signature objects in graphql response

0b3c056

Add predicates for signature verification of commits

b7eb265

Update README with signature predicates

8ec0ae6

Make signature an interface, extract using inline fragments

2619776

Add key-id based predicate

7f939ce

Update readme with keys example

dc27d82

Fix typo in example

362fb0d

Co-authored-by: Andrew Svoboda <svoboda.andrew@gmail.com>

Move to concrete signature type and pointers

b159c27

Use boolean value of predicate to fork

b218013

Test the false predicate path

a5d1c14

README nits

aff3922

IainSteers force-pushed the isteers/signature-predicates branch from db40ddc to 46f36b5 Compare May 6, 2021 16:02

Don't expose unused sig fields for now

4dee23c

IainSteers force-pushed the isteers/signature-predicates branch from 46f36b5 to 4dee23c Compare May 6, 2021 16:10

IainSteers merged commit 762711a into develop May 6, 2021

IainSteers deleted the isteers/signature-predicates branch May 6, 2021 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for signature verification predicates #285

Add support for signature verification predicates #285

IainSteers commented Apr 16, 2021 •

edited

bluekeyes left a comment

bluekeyes Apr 19, 2021

bluekeyes Apr 19, 2021

bluekeyes Apr 20, 2021

IainSteers Apr 30, 2021

bluekeyes Apr 20, 2021

IainSteers Apr 30, 2021

bluekeyes left a comment

bluekeyes May 3, 2021

	# or belong to any of the listed organizations or teams.
	# or belong to a user in any of the listed organizations or teams.

Add support for signature verification predicates #285

Add support for signature verification predicates #285

Conversation

IainSteers commented Apr 16, 2021 • edited

bluekeyes left a comment

Choose a reason for hiding this comment

bluekeyes Apr 19, 2021

Choose a reason for hiding this comment

bluekeyes Apr 19, 2021

Choose a reason for hiding this comment

bluekeyes Apr 20, 2021

Choose a reason for hiding this comment

IainSteers Apr 30, 2021

Choose a reason for hiding this comment

bluekeyes Apr 20, 2021

Choose a reason for hiding this comment

IainSteers Apr 30, 2021

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

bluekeyes May 3, 2021

Choose a reason for hiding this comment

IainSteers commented Apr 16, 2021 •

edited