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

mtrmac · 2022-11-25T20:45:20Z

Summary

pkg/cosign.verifyInternal keeps growing conditions (A) at the end of the function, long after already relying (B) on data that is constrained by these conditions. It effectively makes the outcome of B indeterminate until A happens; that’s hard to track, and A might not happen at all if the function returns early. This makes me nervous: personally for me, it makes following the operation and rules of the code much harder than I think is necessary.

So, this is a move somewhat towards, but not completely, the verification organization suggested in #1648 (comment).

Most data in verifyInternal, and closest utility functions, is now stored with variables named untrusted…. Only after all checks on that data are done, it may be stored in a variable named acceptable….

Testing relies on existing unit tests; no new tests were added.

See individual commit messages for details. Most of the commits are trivial variable renames.

Behavior changes:

Certificate expiry is now not tested if we don’t rely on the certificate at all (if the user provides a trusted public key)
If we do rely on the certificate, we now always test expiry against some time. Previously that didn’t happen if there was no RFC 3161 timestamp and Rekor log presence was opted out. (I.e. this should allow using ordinary X.509 CA certificates without relying on a timestamp / Rekor at all, the way the TLS ecosystem does.)
If there is a RFC 3161 timestamp, and Rekor log presence is processed but not required, we only require the certificate to be valid as of the timestamp, not as of the current time.

Basic summary of the ordering changes:

The verification flow now:
- First ensures timestamp / Rekor presence; that obtains a trusted signature creation timestamp, if any
- Then, if necessary, validates the certificate (requires the timestamp from previous step)
- Cryptographically verifies the signature/payload
- Only at the very end, processes the payload
VerifyBundle now first ensures that the entry was actually stored on Rekor, and only afterwards uses it to see whether it matches the supposedly-logged signature.

Release Note

Certificate expiry checking was made a bit more accurate.

Documentation

N/A.

codecov-commenter · 2022-11-25T21:39:18Z

Codecov Report

Merging #2482 (deebfc5) into main (09f023f) will increase coverage by 0.18%.
The diff coverage is 69.32%.

@@            Coverage Diff             @@
##             main    #2482      +/-   ##
==========================================
+ Coverage   30.14%   30.33%   +0.18%     
==========================================
  Files         139      139              
  Lines        8595     8624      +29     
==========================================
+ Hits         2591     2616      +25     
- Misses       5629     5632       +3     
- Partials      375      376       +1

Impacted Files	Coverage Δ
cmd/cosign/cli/verify/verify.go	`19.36% <0.00%> (ø)`
cmd/cosign/cli/verify/verify_attestation.go	`0.00% <0.00%> (ø)`
pkg/cosign/verify.go	`42.74% <70.70%> (+1.43%)`	⬆️
cmd/cosign/cli/sign/sign.go	`15.84% <100.00%> (ø)`
cmd/cosign/cli/verify/verify_blob.go	`49.56% <100.00%> (ø)`

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

znewman01

Hey, thanks for this! Very good idea overall and I'm strongly in favor.

I'd like even better to enforce this at the type system layer, rather than simply via naming. You might be able to do some cool encapsulation tricks. (Don't make it possible to instantiate a Foo struct from outside the module; instead, you must create an UnverifiedFoo and then turn it into a Foo.)

But in the shorter term—is there any chance you could break this up into 2 or 3 smaller PRs? I find once I'm reviewing more than 100–200 lines of code my eyes gloss over and I find it difficult to actually make sure the changes are correct.

haydentherapper · 2022-11-27T20:13:48Z

+1, for PRs that both refactor and modify behavior, we should separate those into separate PRs, especially for critical parts of the code base like verify. Very minimal test changes for refactors, with more unit tests for behavior changes.

If we do rely on the certificate, we now always test expiry against some time. Previously that didn’t happen if there was no RFC 3161 timestamp and Rekor log presence was opted out. (I.e. this should allow using ordinary X.509 CA certificates without relying on a timestamp / Rekor at all, the way the TLS ecosystem does.)

Is this correct? We default to the current time when checking expiration time. If no bundle or timestamp is provided, we use the current time. There should be no case when cert expiration is not checked, but let me know if you found one.

If there is a RFC 3161 timestamp, and Rekor log presence is processed but not required, we only require the certificate to be valid as of the timestamp, not as of the current time.

This sounds like a good change! Note that if both a bundle and 3161 time stamp are present, we want to check the times for both. Maybe we make this configurable in the future, but for now, I’d like to continue checking both, because we do expect both to be valid.

asraa · 2022-11-28T15:15:29Z

YAY! Thank you -- I'll give a start reviewing soon. Also, I'm catching up on some lost context over the holiday week so apologies if I'm behind

Is this correct? We default to the current time when checking expiration time. If no bundle or timestamp is provided, we use the current time. There should be no case when cert expiration is not checked, but let me know if you found one.

We introduced a few as we were making the cosign 2.0 changes, but I ran out of time before the holidays to clean them up. Looking at main right now, you could get around if, let's say, you did skip-tlog-verify without TSACerts specified to validate an RFC 3161 timestamp. We fixed up the later ones, but I like that this one is a lot more explicit about the check.

mtrmac · 2022-11-28T15:27:45Z

Hey, thanks for this! Very good idea overall and I'm strongly in favor.

I'd like even better to enforce this at the type system layer, rather than simply via naming. You might be able to do some cool encapsulation tricks. (Don't make it possible to instantiate a Foo struct from outside the module; instead, you must create an UnverifiedFoo and then turn it into a Foo.)

Go doesn’t have a strong enough visibility system for that, other than using separate subpackages — and I think that would spread the core verification logic over too many separate files, making the end resulting logic hard to follow. (That’s especially the case for certificates, which really have several mostly-independent correctness criteria, like the various subject restrictions that can be configured.)

There’s certainly much more that could be done (e.g. maybe the oci.Signature type should name all fields Untrusted…). I wanted to get the core verification logic ordered, and I can’t immediately spend significantly more time on a style proposal beyond that.

But in the shorter term—is there any chance you could break this up into 2 or 3 smaller PRs? I find once I'm reviewing more than 100–200 lines of code my eyes gloss over and I find it difficult to actually make sure the changes are correct.

Sure, the set of commits should be correct at every point, so It can split in arbitrary places.

asraa · 2022-11-28T17:48:39Z

Sure, the set of commits should be correct at every point, so It can split in arbitrary places.

@mtrmac could we start with the timestamp check changes in the verification flow and do some renaming as follow-ups once there's no functional changes?

I'm looking at
3b21d42
eb1f060
0a343bc

asraa

this is looking great! i've just reviewing the verify main command.

asraa · 2022-12-02T17:45:22Z

pkg/cosign/verify.go

+				return false, fmt.Errorf("offline verification failed")
+			}
+
+			if co.RekorClient != nil {


I think this check was always very confusing to me. It seems like if you aren't skipping TLOG verification, then if there's no rekor client, somehow you can still pass without checking the tlog.

do you think maybe it should be an error that co.RekorClient is nil when we have to do tlog verification and never got a bundle?

Right now, the API is such that external callers can call VerifyImageSignature and get (bundleVerified = false, err = nil). So this PR doesn’t change that, it’s a large enough set of changes already.

I agree that it seems dangerous and undesirable; IMHO external users of pkg/cosign should just provide a policy, and not be relied upon to correctly enforce other conditions on top. Conceptually, I think “change the API philosophy” and “change the implementation philosophy” are fairly separate conversations — OTOH, admittedly, the question of the right timestamp to use for certificate expiry means the two can’t separated that cleanly.

haydentherapper · 2022-12-02T17:48:05Z

cmd/cosign/cli/verify/verify.go

@@ -154,7 +154,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
 		if err != nil {
 			return fmt.Errorf("getting Fulcio roots: %w", err)
 		}
-		co.IntermediateCerts, err = fulcio.GetIntermediates()
+		co.UntrustedIntermediateCerts, err = fulcio.GetIntermediates()


Could we keep the same name, IntermediateCerts?

I would prefer we don't used "Untrusted" in the name. This term comes from openssl from what I've seen, and I personally don't think it's correct to only use intermediate certificates for chain building ("untrusted"). Intermediates should be trusted, or at least come from a trusted source.

This one is confusing. These particular ones are coming from a trusted source, so I can agree to see it named the same. The ones found on the registry through CertChain are untrusted intermediates

Yes, see co.UntrustedIntermediateCerts = untrustedPool. So, in that sense, I think changing the name of CheckOpts to UntrustedIntermediateCerts has been a success exactly because the trusted nature of those coming from fulcio.GetIntermediates is insufficient to make the field trusted.

(I’m also not sure what a “trusted intermediate” means. Is that a root of trust, i.e. a non-intermediate root? Or an intermediate that we still require to be correctly signed by a root of trust, i.e. something where we don’t actually require any trust?)

Actually, what I really think is that the caller-provided policy should be immutable and should not be modified to hold single-instance data within verify.go at all. Then we might even more precisely differentiate between

completely-untrusted signature-carried data

externally Fulcio-provided data (acceptable if the connection to Fulcio is trusted)

completely locally-provided 100%-trusted policy

and potentially make different decisions based on those details. That would be a different PR.

For a core library function, I think we should not be opinionated if the certificates are trusted or untrusted. For example, we don't know where the root certificate came from either when the checkopts is populated. I don't see a reason to distinguish between untrusted and trusted in the CheckOpts. Internally, we can name variables differently based on their source.

I think we should not be opinionated if the certificates are trusted or untrusted.

I… don’t know what that would mean.

The verification code must be certain about which data is a part of the root of trust that is, by necessity, assumed valid, and which data is part of the external attack surface that is, by default, suspect. At the very least these two categories are 100% distinct. There might be even more categories in between (e.g. the intermediate certificates might be more trusted than the underlying signatures, but they are still potentially a part of the attack surface).

(And then the API should help the caller of the API express its assumptions clearly, and to help expose any incorrect assumptions the caller might be making.

From that point of view, calling the certificates “untrusted” is good for the caller, because it tells the caller the caller doesn’t need to make much effort at all to ensure validity of the data. “A trusted system is one whose failure would break a security policy”.)

I don’t care at all about the individual naming of the options. If you don’t want the field to be named Untrusted, and to instead have a CheckOpts.{TrustedSigVerifier,TrustedRootCerts}, sure, that might be even more explicit. If you think the distinction between trusted and untrusted data is sufficient to express in comments and doesn’t need to be in field names, that’s not my preference but I can live with that just fine.

But I read the above as suggesting that it doesn’t make a difference whether the data is trusted or not, and I just don’t know what to do with that. Probably I’m completely misunderstanding your point.

haydentherapper · 2022-12-02T17:55:11Z

My 2cents: I see multiple types of refactoring in this PR - Name changes, reordering, etc. Given the criticality of this package, I think we should scope down the PR even more. I see some of the commits are behavior changes too - These definitely should be in their own PR, especially since I don't see tests that accompany them (which suggests we don't have sufficient testing in Cosign, which we don't :) ).

mtrmac · 2022-12-02T18:15:37Z

Right, I have rebased this first to have a baseline from which I can extract a smaller subset.

Then I intend to keep this PR to be a catch-all that merges branches of the smaller subsets, keeping a record of what is merged/being reviewed / outstanding (and dropping the rejected parts). If/as time allows.

haydentherapper · 2022-12-02T19:10:36Z

@mtrmac Do you think it'd be possible to have a PR just with the behavior changes first? We wanted to cut a release candidate soon for cosign 2.0, and one of the behavior changes in particular is a blocker for release imo:

If we do rely on the certificate, we now always test expiry against some time. Previously that didn’t happen if there was no RFC 3161 timestamp and Rekor log presence was opted out. (I.e. this should allow using ordinary X.509 CA certificates without relying on a timestamp / Rekor at all, the way the TLS ecosystem does.)

This should result in a verification failure. Reproduced with cosign verify-blob --certificate blob.cert --signature blob.sig --insecure-skip-tlog-verify blob, using an expired cert, it says it succeeds.

fyi @priyawadhwa

mtrmac · 2022-12-02T19:55:06Z

@mtrmac could we start with the timestamp check changes in the verification flow and do some renaming as follow-ups once there's no functional changes?

I'm looking at 3b21d42 eb1f060 0a343bc

@mtrmac Do you think it'd be possible to have a PR just with the behavior changes first? We wanted to cut a release candidate soon for cosign 2.0, and one of the behavior changes in particular is a blocker for release imo:
If we do rely on the certificate, we now always test expiry against some time. Previously that didn’t happen if there was no RFC 3161 timestamp and Rekor log presence was opted out. (I.e. this should allow using ordinary X.509 CA certificates without relying on a timestamp / Rekor at all, the way the TLS ecosystem does.)
This should result in a verification failure. Reproduced with cosign verify-blob --certificate blob.cert --signature blob.sig --insecure-skip-tlog-verify blob, using an expired cert, it says it succeeds.

#2504: as it happens, I was planning to add that one on top of the previous request.

If you want to urgently backport the other change, feel free to; I’m afraid I’m done for the week.

mtrmac · 2022-12-02T20:24:51Z

This PR is now based on top of #2504 (and includes an explicit merge commit). So I’m marking it as a draft to direct reviews and conversations to the #2504 subset for now.

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. Signed-off-by: Miloslav Trmač <mitr@redhat.com> Co-authored-by: Hayden B <hblauzvern@google.com>

haydentherapper · 2022-12-07T20:37:48Z

We've merged the initial PR. Note that there have been a lot of changes in verify.go, so you'll likely have conflicts.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to track the trust status of the Rekor bundle Signed-off-by: Miloslav Trmač <mitr@redhat.com>

And validate the public key first, the signature second, just for consistency with the flow of trust (although that really doesn't mattere here, we want all of (key, signature, payload) to match). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

…61 timestamp Reorganize the certificate expiry checks, so that we check against the accepted timestamps if any, and only fall back to current time if there is no data. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Use certWithUnverifiedExpiry , which we already have, and which we care about. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Finally, only create the verifier based on an actually acceptable certificate, instead of creating it first and then hoping not to forget to validate preconditions. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to document the individual stages. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2022-12-07T22:27:22Z

Sure, rebased.

Do you have a preference for a subsection to propose next? Some options:

Pure variable renames
The certificate validation functions, leading up to the split of validateCertIssuanceAndSubject from ValidateAndUnpackCert
The reordering of VerifyBundle (probably smallest)

haydentherapper · 2022-12-07T22:41:34Z

If you want quick reviews, we can start with the smallest, reordering VerifyBundle, then var renames?

github-actions · 2023-01-07T01:59:33Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2023-01-17T02:00:40Z

This PR was closed because it has been stalled for 10 days with no activity.

mtrmac force-pushed the processing-order branch from 6716343 to b16b8b6 Compare November 25, 2022 21:36

haydentherapper requested review from haydentherapper, asraa and znewman01 November 27, 2022 00:56

znewman01 reviewed Nov 27, 2022

View reviewed changes

mtrmac force-pushed the processing-order branch from b16b8b6 to 923c9ef Compare December 2, 2022 17:40

asraa reviewed Dec 2, 2022

View reviewed changes

haydentherapper requested changes Dec 2, 2022

View reviewed changes

mtrmac mentioned this pull request Dec 2, 2022

Consolidate certificate expiry logic #2504

Merged

mtrmac force-pushed the processing-order branch from 923c9ef to 594d1e6 Compare December 2, 2022 20:24

mtrmac marked this pull request as draft December 2, 2022 20:24

haydentherapper mentioned this pull request Dec 5, 2022

Cosign 2.0 Tracking #2376

Closed

3 tasks

mtrmac added 6 commits December 7, 2022 22:31

Rename sig to untrustedSignature

f8299e8

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename cert to untrustedCert

a155a19

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename chain to untrustedChain

cb0f9eb

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename CheckOpts.IntermediateCerts to UntrustedIntermediateCerts

2ce0cd0

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename pool to untrustedPool

f44d639

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename cert to untrustedCert

17cad99

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added 23 commits December 7, 2022 23:18

Rename sig to untrustedSig

0d4bda1

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename ts to untrustedTimestamp

8b4421e

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename b64sig to untrustedB64Sig

e7b3199

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename signedPayload to untrustedPayload

43564e8

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename rawSig to untrustedRawSig

0ace845

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename tsBytes to untrustedTSAArtifact

07ffbb8

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename sig to untrustedSig

c37d3f8

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename bundle to untrustedBundle

d7028e7

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename parameters of VerifySET

7136f69

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Introduce acceptableBundleBody

c9c92e2

... to track the trust status of the Rekor bundle Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename payload to untrustedPayload

b150bcf

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename signature to untrustedSignature

7428fc3

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Document VerifyBundle a bit more

ea498b2

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename pemBytes to untrustedPEMBytes

0321492

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename sig to untrustedSig

0771a5a

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename pem to untrustedPEM

28cb652

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename b64sig to untrustedB64sig

5c35a52

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Rename payload to untrustedPayload

66a6644

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Eliminate the redundant cert variable

2c2f0da

Use certWithUnverifiedExpiry , which we already have, and which we care about. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Introduce acceptableCert

36f9fa1

Finally, only create the verifier based on an actually acceptable certificate, instead of creating it first and then hoping not to forget to validate preconditions. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Update comments in verifyInternal

deebfc5

... to document the individual stages. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the processing-order branch from 594d1e6 to deebfc5 Compare December 7, 2022 22:22

github-actions bot added the no-pr-activity label Jan 7, 2023

github-actions bot closed this Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mtrmac commented Nov 25, 2022

codecov-commenter commented Nov 25, 2022 •

edited

Loading

znewman01 left a comment

haydentherapper commented Nov 27, 2022

asraa commented Nov 28, 2022

mtrmac commented Nov 28, 2022

asraa commented Nov 28, 2022

asraa left a comment

asraa Dec 2, 2022

mtrmac Dec 2, 2022 •

edited

Loading

haydentherapper Dec 2, 2022

asraa Dec 2, 2022

mtrmac Dec 2, 2022

haydentherapper Dec 2, 2022

mtrmac Dec 2, 2022

haydentherapper commented Dec 2, 2022

mtrmac commented Dec 2, 2022

haydentherapper commented Dec 2, 2022

mtrmac commented Dec 2, 2022

mtrmac commented Dec 2, 2022

haydentherapper commented Dec 7, 2022

mtrmac commented Dec 7, 2022

haydentherapper commented Dec 7, 2022

github-actions bot commented Jan 7, 2023

github-actions bot commented Jan 17, 2023

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

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

Conversation

mtrmac commented Nov 25, 2022

Summary

Release Note

Documentation

codecov-commenter commented Nov 25, 2022 • edited Loading

Codecov Report

znewman01 left a comment

Choose a reason for hiding this comment

haydentherapper commented Nov 27, 2022

asraa commented Nov 28, 2022

mtrmac commented Nov 28, 2022

asraa commented Nov 28, 2022

asraa left a comment

Choose a reason for hiding this comment

asraa Dec 2, 2022

Choose a reason for hiding this comment

mtrmac Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

haydentherapper Dec 2, 2022

Choose a reason for hiding this comment

asraa Dec 2, 2022

Choose a reason for hiding this comment

mtrmac Dec 2, 2022

Choose a reason for hiding this comment

haydentherapper Dec 2, 2022

Choose a reason for hiding this comment

mtrmac Dec 2, 2022

Choose a reason for hiding this comment

haydentherapper commented Dec 2, 2022

mtrmac commented Dec 2, 2022

haydentherapper commented Dec 2, 2022

mtrmac commented Dec 2, 2022

mtrmac commented Dec 2, 2022

haydentherapper commented Dec 7, 2022

mtrmac commented Dec 7, 2022

haydentherapper commented Dec 7, 2022

github-actions bot commented Jan 7, 2023

github-actions bot commented Jan 17, 2023

codecov-commenter commented Nov 25, 2022 •

edited

Loading

mtrmac Dec 2, 2022 •

edited

Loading