Add pattern based from/to validity for keys #193

kipz · 2024-10-08T13:09:54Z

Add support for customizing key expiry on a per repo/platform basis
Add support for arbitrary string/string input parameters to Rego policies

codecov · 2024-10-08T13:16:24Z

Codecov Report

Attention: Patch coverage is 68.75000% with 30 lines in your changes missing coverage. Please review.

Project coverage is 69.90%. Comparing base (2d7f6ca) to head (ca5f535).

Files with missing lines	Patch %	Lines
attestation/verify.go	65.21%	12 Missing and 12 partials ⚠️
tlog/mock.go	77.77%	2 Missing and 2 partials ⚠️
signerverifier/parse.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
- Coverage   70.06%   69.90%   -0.17%     
==========================================
  Files          44       44              
  Lines        2576     2655      +79     
==========================================
+ Hits         1805     1856      +51     
- Misses        474      488      +14     
- Partials      297      311      +14

Flag	Coverage Δ
unittests	`69.90% <68.75%> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonnystoten · 2024-10-09T12:02:44Z

attestation/verifier.go

+		toMatch := false
+		fromMatch := false
+		// must match at least one - nil is open ended
+		for _, filter := range keyMeta.Expiries {
+			if filter.To == nil || (filter.To != nil && integratedTime.Before(*filter.To)) {
+				toMatch = true
+			}
+			if filter.From == nil || (filter.From != nil && integratedTime.After(*filter.From)) {
+				fromMatch = true
+			}
+			if toMatch && fromMatch {
+				break
+			}
+		}


I don't think this is right - this will match if the To of one expiry matches and the From of another matches, even if the windows don't overlap. Does it even make sense to have more than one expiry that matches the repo/platform?

Now we return the first match.

jonnystoten · 2024-10-09T12:04:09Z

policy/rego.go

+	for i := range opts.Keys {
+		key := opts.Keys[i]


Any reason not to do

Suggested change

for i := range opts.Keys {

key := opts.Keys[i]

for _, key := range opts.Keys {

Is this a linting rule?

jonnystoten · 2024-10-09T12:11:11Z

policy/rego.go

+						if parsedPlatform.Equals(*platform) {
+							expiries = append(expiries, expiry)
+							break
+						}


Is the intent here to break the outer loop, like on line 358 above? You can name the loop with a label and break label.

Assuming I've got that right, I think this answers my previous question - if every time we append to expiries we also break the loop over key.Expiries, the new key.Expiries will only ever have 0 or 1 entries. Maybe we can instead store the only match in another field so that the usage is clearer?

Along the same lines, it might be better not to break, and instead error on multiple matches. WDYT?

I've refactored this whole abstract because it stinks :)

jonnystoten · 2024-10-09T12:15:41Z

attestation/verifier.go

+	// important to allow an empty array here so that we don't fail open
+	// search for test 'no match should fail closed'
+	if keyMeta.Expiries != nil {
+		// any repo expirey still on the keys must match the times


Suggested change

// any repo expirey still on the keys must match the times

// any repo expiry still on the keys must match the times

(there are a few of this same typo in the PR)

policy/types.go

mrjoelkamp

Have the pros and cons been considered on adding complexity to key management for this vs just managing separate keys with less code and key management complexity?

I look at this and get very uneasy about the potential for bugs in selecting the appropriate key to use for signature verification.

mrjoelkamp · 2024-10-09T19:47:24Z

attestation/sign_test.go

-			name:          "key already revoked",
+			name:          "key was expired",
 			keyID:         keyID,
 			pem:           pem,
 			distrust:      false,
 			from:          time.Time{},
 			to:            new(time.Time),
 			status:        "revoked",
-			expectedError: "already revoked",
+			expectedError: "was expired",
 		},


it looks like this test case was repurposed? can we test a revoked key still?

it's just a log message, and nothing is actually testing the status of a key. The test is the same.

mrjoelkamp · 2024-10-09T19:53:14Z

attestation/types.go

 	publicKey     crypto.PublicKey
+	expired       bool
+	Expiries      []*KeyExpiry `json:"expiries,omitempty" yaml:"expiries,omitempty"`


expiries is a strange word, at first I thought it was a spelling mistake!

So this becomes an additional layer of from/to values on top of the key's main from/to value. To me this seems quite complex for key management but I am guessing that it is the only way to handle key usage for platforms and repos?

It's really less of an expiry object or list of expiry objects as much as it is an applicability object that determines which artifacts this key should be used for. I think that naming would also make it more clear what the purpose of it is and separate it from the key's main from/to expiry.

I'm up for some new words here for sure, but I don't understand your comment about complexity. The problem we're solving here is complex, so the solution needs to account for that, and I think it does that in a clear and concise way.

I was wondering if we should deprecate the to/from at the key level and only use the expiries (or validity_ranges?) because with empty platforms and a wildcard for the pattern it covers the current behaviour....

mrjoelkamp · 2024-10-09T19:57:30Z

attestation/verify.go

+		// if we get here, and no expirey match the image, the key is expired
+		km.expired = true


shouldn't the top-level from/to date determine if the key is expired and this really just let you know that the key is applicable for image/platform provided?

what would happen if we set the main to date to rotate the key completely and one of these nested "expiry" objects overwrites the to date? It seems that it would continue to be active.

to/from at the key level is not permitted when expiries are used. I did that to simplify the whole thing. I'm happy to change it so that these set the upper and lower bounds, but I think that could get a bit complicated.

On reflection, I think you're right. I'll make the key level 'to/from' be the upper and lower bounds. That's what you'd expect I think.

mrjoelkamp · 2024-10-09T19:58:45Z

attestation/verify.go

+				if len(expiry.Platforms) == 0 {
+					km.To = expiry.To
+					km.From = expiry.From
+					km.expired = false


how do we know that expired = false here without calculating if the expiry.To time is less than the current time?

Time is checked later. This is just about choosing the first expiry that matches and setting that on the key. Maybe there's another abstraction here that would be less confusing....

mrjoelkamp · 2024-10-09T20:13:08Z

I think there should probably be updates to README.md to cover this

kipz · 2024-10-10T10:47:04Z

@jonnystoten @mrjoelkamp moved back to draft while we settle on the approach.

Changed:

move logic into VerifyOptions and KeyMetadata receivers
Delay range matching until log entry integration time checking (cleans up the abstraction - e.g. removes that expired flag whic was confusing)
expiries -> validity (a bit better, but maybe not there yet). validity_ranges?
much more pre-checking of the to/from ranges to ensure that each range is within the key's to/from
allow only one matching validity range for image (or fail)

TODO:

many more tests around date ranges
README update as requested

kipz · 2024-10-10T14:51:01Z

As discussed in the sync today, we'll assume a batched approach to signature rollout, thus removing that would be introduced here, with hopefully only the minor inconvenience of creating and expiring different keys for each batch.

kipz added 4 commits October 8, 2024 14:03

feat: add more expressive key validity policy

c6f4792

feat: add support for arbitrary rego input params

94ed370

check len

6fcfbca

fix after merge

7b42be3

kipz added 2 commits October 9, 2024 11:12

more test. fail closed

f2e7e03

fail closed - add test

47e4a96

kipz changed the title ~~Add Rego Input parameter and per repo key validity support~~ Add pattern based from/to validity for keys Oct 9, 2024

Merge branch 'main' into kipz_exp

c9964cd

kipz marked this pull request as ready for review October 9, 2024 11:33

kipz requested a review from a team as a code owner October 9, 2024 11:33

jonnystoten reviewed Oct 9, 2024

View reviewed changes

kipz added 2 commits October 9, 2024 15:34

better abstraction?

c322816

slight abstraction improvement

9d88c87

mrjoelkamp reviewed Oct 9, 2024

View reviewed changes

Try a new abstraction and semantics

ca5f535

kipz marked this pull request as draft October 10, 2024 10:41

kipz closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pattern based from/to validity for keys #193

Add pattern based from/to validity for keys #193

kipz commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

jonnystoten Oct 9, 2024

kipz Oct 9, 2024

jonnystoten Oct 9, 2024

jonnystoten Oct 9, 2024

kipz Oct 9, 2024

jonnystoten Oct 9, 2024

kipz Oct 9, 2024

mrjoelkamp left a comment

mrjoelkamp Oct 9, 2024

kipz Oct 10, 2024

mrjoelkamp Oct 9, 2024

kipz Oct 10, 2024

kipz Oct 10, 2024

mrjoelkamp Oct 9, 2024

mrjoelkamp Oct 9, 2024

kipz Oct 10, 2024

kipz Oct 10, 2024

mrjoelkamp Oct 9, 2024

kipz Oct 10, 2024

mrjoelkamp commented Oct 9, 2024

kipz commented Oct 10, 2024

kipz commented Oct 10, 2024

	for i := range opts.Keys {
	key := opts.Keys[i]
	for _, key := range opts.Keys {

	// any repo expirey still on the keys must match the times
	// any repo expiry still on the keys must match the times

		// if we get here, and no expirey match the image, the key is expired
		km.expired = true

Add pattern based from/to validity for keys #193

Add pattern based from/to validity for keys #193

Conversation

kipz commented Oct 8, 2024

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjoelkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjoelkamp commented Oct 9, 2024

kipz commented Oct 10, 2024

kipz commented Oct 10, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading