Consolidate certificate expiry logic #2504

mtrmac · 2022-12-02T19:52:00Z

Summary

This is a subset of #2482 as requested in #2482 (comment) (a bit larger than that):

User-observable: verify RFC 3161 timestamps, and Rekor data, first, before processing certificates. Conceptually we need to do that because certificate processing requires a timestamp (although that’s not yet what this PR finishes doing). This can affect user-observable error messages, which can now complain about untrusted signed Rekor data instead of untrusted signed certificates.
Behavior change: If there is a certificate, always verify it against some timestamp (in particular, the current time, if there is neither a RFC 3161 timestamp nor any Rekor data. (This is more than was asked — I included it because the current behavior is such a surprise for callers, and because always verifying actually makes the code simpler. I can split that into some future PR, and add an explicit “are we dealing with Rekor” condition instead, to preserve the current behavior.)
Code consolidation: verifyInternal now has one place (although not yet the correct place) to do certificate expiry checks, instead of that happening in VerifyRFC3161Timestamp

See #2482 for previous discussion, and the general rationale of only using data after it is verified (which is not what this PR really does, but it’s a prerequisite).

Release Note

cosign verify-blob --cert-chain … --certificate … --skip-tlog-verify now requires the leaf certificate to not be expired.

Documentation

codecov-commenter · 2022-12-02T19:55:50Z

Codecov Report

Merging #2504 (22e5a0e) into main (7813599) will decrease coverage by 0.01%.
The diff coverage is 51.42%.

@@            Coverage Diff             @@
##             main    #2504      +/-   ##
==========================================
- Coverage   30.16%   30.14%   -0.02%     
==========================================
  Files         139      139              
  Lines        8583     8595      +12     
==========================================
+ Hits         2589     2591       +2     
- Misses       5621     5629       +8     
- Partials      373      375       +2

Impacted Files	Coverage Δ
pkg/cosign/verify.go	`41.31% <51.42%> (-0.35%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

haydentherapper

Thanks so much for splitting this up! This looks great.

pkg/cosign/verify.go

cmd/cosign/cli/verify/verify_blob_test.go

pkg/cosign/verify.go

cmd/cosign/cli/verify/verify.go

haydentherapper · 2022-12-03T00:00:48Z

@cpanato Any idea about why the windows test is failing? Not sure how best to debug this.

pkg/cosign/verify.go

asraa

awaiting the rebase, but lgtm! don't block on that comment, since it's fairly minor

Should not change behavior now, but this should make it easier to move code around. Signed-off-by: Miloslav Trmač <[email protected]>

Let's decrease the risk of a caller not noticing an error, and make it a bit shorter to read and more clear that they are all, in fact, error paths. This may change the return value in some cases. Signed-off-by: Miloslav Trmač <[email protected]>

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

This will allow us to move the certificate expiry responsibility to the caller. Should not change behavior, assuming timestamp.ParseResponse can't fail for an alraedy verified response. Signed-off-by: Miloslav Trmač <[email protected]>

... from VerifyRFC3161Timestamp, which has no reason to care, to verifyInternal. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Don't repeat the conditions, and make the flow a bit clearer. Should not change behavior, unless there are multiple reasons to reject the signature. Signed-off-by: Miloslav Trmač <[email protected]>

We will use them to decouple the bundle handling from certificate expiry verification. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Should not change behavior, just to prepare a further move Signed-off-by: Miloslav Trmač <[email protected]>

Another small step. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Now, we always validate certificate expiration against _some_ time. Even if we don't interact with Rekor bundles at all, we validate it against the current time. Signed-off-by: Miloslav Trmač <[email protected]>

Consolidate all the expiry checks into one place. Should not change behavior, unless there are multiple reasons to reject the signature. Signed-off-by: Miloslav Trmač <[email protected]>

Do them before looking at the certificate at all; we need to do this first to obtain signature creation times. This may affect user-visible error messages; adjust a test. Signed-off-by: Miloslav Trmač <[email protected]>

Signed-off-by: Hayden B <[email protected]>

Signed-off-by: Hayden Blauzvern <[email protected]>

pkg/cosign/verify.go

Signed-off-by: Hayden Blauzvern <[email protected]>

cmd/cosign/cli/verify/verify_blob_test.go

Signed-off-by: Hayden Blauzvern <[email protected]>

pkg/cosign/verify_test.go

mtrmac mentioned this pull request Dec 2, 2022

RFC: When verifying, only use data after it is considered acceptable #2482

Closed

haydentherapper requested a review from asraa December 2, 2022 20:28

haydentherapper reviewed Dec 2, 2022

View reviewed changes

pkg/cosign/verify.go Outdated Show resolved Hide resolved

cmd/cosign/cli/verify/verify_blob_test.go Outdated Show resolved Hide resolved

pkg/cosign/verify.go Outdated Show resolved Hide resolved

pkg/cosign/verify.go Show resolved Hide resolved

asraa reviewed Dec 2, 2022

View reviewed changes

pkg/cosign/verify.go Outdated Show resolved Hide resolved

haydentherapper force-pushed the processing-order-1 branch from 229b5b2 to c9af038 Compare December 2, 2022 21:04

asraa reviewed Dec 2, 2022

View reviewed changes

pkg/cosign/verify.go Outdated Show resolved Hide resolved

asraa previously approved these changes Dec 2, 2022

View reviewed changes

cmd/cosign/cli/verify/verify.go Outdated Show resolved Hide resolved

haydentherapper dismissed asraa’s stale review via 7a6bd70 December 2, 2022 22:17

haydentherapper mentioned this pull request Dec 2, 2022

Breaking change: Respect tlog-upload=false, default to true #2505

Merged

haydentherapper mentioned this pull request Dec 5, 2022

Cosign 2.0 Tracking #2376

Closed

3 tasks

haydentherapper requested a review from hectorj2f December 6, 2022 07:31

haydentherapper force-pushed the processing-order-1 branch from 1bd5cda to 23cf1cb Compare December 6, 2022 21:17

asraa reviewed Dec 6, 2022

View reviewed changes

pkg/cosign/verify.go Outdated Show resolved Hide resolved

asraa reviewed Dec 6, 2022

View reviewed changes

mtrmac and others added 14 commits December 6, 2022 23:04

Don't return early in verifyInternal

fb38707

Should not change behavior now, but this should make it easier to move code around. Signed-off-by: Miloslav Trmač <[email protected]>

Introduce acceptedTimestamp to track trust state

a467eb6

Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Move certificate expiration check against the TSA timestamp

424d01a

... from VerifyRFC3161Timestamp, which has no reason to care, to verifyInternal. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Simplify the logic in verifyInternal

9fbe9dd

Don't repeat the conditions, and make the flow a bit clearer. Should not change behavior, unless there are multiple reasons to reject the signature. Signed-off-by: Miloslav Trmač <[email protected]>

Introduce acceptableRFC3161Time and acceptableRekorBundleTime

e544ee3

We will use them to decouple the bundle handling from certificate expiry verification. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

Move the acceptableRFC3161Time enforcement logic a bit

27a8a5d

Should not change behavior, just to prepare a further move Signed-off-by: Miloslav Trmač <[email protected]>

Move the acceptableRekorBundleTime certificate expiry logic

780622d

Another small step. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>

BEHAVIOR CHANGE: Always validate certificate expiration

6a87651

Now, we always validate certificate expiration against _some_ time. Even if we don't interact with Rekor bundles at all, we validate it against the current time. Signed-off-by: Miloslav Trmač <[email protected]>

Reorganize certificate expiry check further

b95b1b9

Consolidate all the expiry checks into one place. Should not change behavior, unless there are multiple reasons to reject the signature. Signed-off-by: Miloslav Trmač <[email protected]>

Move TSA and Rekor checks in verifyInternal

46b8cad

Do them before looking at the certificate at all; we need to do this first to obtain signature creation times. This may affect user-visible error messages; adjust a test. Signed-off-by: Miloslav Trmač <[email protected]>

Apply suggestions from code review

14c0ddc

Signed-off-by: Hayden B <[email protected]>

Add error if rekor client isn't set

2b53511

Signed-off-by: Hayden Blauzvern <[email protected]>

haydentherapper added 8 commits December 6, 2022 23:05

Fix tests

52179b2

Signed-off-by: Hayden Blauzvern <[email protected]>

Fixing e2e tests

81ef16c

Signed-off-by: Hayden Blauzvern <[email protected]>

Update ps1 test

09da0bd

Signed-off-by: Hayden Blauzvern <[email protected]>

Trying to fix ps1

d26a68a

Signed-off-by: Hayden Blauzvern <[email protected]>

Trying to fix ps1 again

6389b08

Signed-off-by: Hayden Blauzvern <[email protected]>

Fix ps1 test

6d8ae28

Signed-off-by: Hayden Blauzvern <[email protected]>

Remove println

6529d99

Signed-off-by: Hayden Blauzvern <[email protected]>

Address nit

844de7c

Signed-off-by: Hayden Blauzvern <[email protected]>

haydentherapper force-pushed the processing-order-1 branch from 7349b6c to 844de7c Compare December 6, 2022 23:09

Remove line

34ea8c0

Signed-off-by: Hayden Blauzvern <[email protected]>

mtrmac commented Dec 7, 2022

View reviewed changes

pkg/cosign/verify.go Show resolved Hide resolved

Clean up error

0f7136d

Signed-off-by: Hayden Blauzvern <[email protected]>

asraa reviewed Dec 7, 2022

View reviewed changes

cmd/cosign/cli/verify/verify_blob_test.go Outdated Show resolved Hide resolved

haydentherapper added 2 commits December 7, 2022 19:10

Fix test for invalid root CA

325ef6c

Signed-off-by: Hayden Blauzvern <[email protected]>

Merge in upstream changes

22e5a0e

Signed-off-by: Hayden Blauzvern <[email protected]>

asraa reviewed Dec 7, 2022

View reviewed changes

pkg/cosign/verify_test.go Show resolved Hide resolved

asraa approved these changes Dec 7, 2022

View reviewed changes

asraa enabled auto-merge (squash) December 7, 2022 20:13

asraa merged commit 09f023f into sigstore:main Dec 7, 2022

github-actions bot added this to the v1.14.0 milestone Dec 7, 2022

mtrmac deleted the processing-order-1 branch December 7, 2022 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate certificate expiry logic #2504

Consolidate certificate expiry logic #2504

mtrmac commented Dec 2, 2022 •

edited

Loading

codecov-commenter commented Dec 2, 2022 •

edited

Loading

haydentherapper left a comment

haydentherapper commented Dec 3, 2022

asraa left a comment

Consolidate certificate expiry logic #2504

Consolidate certificate expiry logic #2504

Conversation

mtrmac commented Dec 2, 2022 • edited Loading

Summary

Release Note

Documentation

codecov-commenter commented Dec 2, 2022 • edited Loading

Codecov Report

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Dec 3, 2022

asraa left a comment

Choose a reason for hiding this comment

mtrmac commented Dec 2, 2022 •

edited

Loading

codecov-commenter commented Dec 2, 2022 •

edited

Loading