Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 43 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
f8299e8
Rename sig to untrustedSignature
mtrmac Nov 25, 2022
a155a19
Rename cert to untrustedCert
mtrmac Nov 25, 2022
cb0f9eb
Rename chain to untrustedChain
mtrmac Nov 25, 2022
2ce0cd0
Rename CheckOpts.IntermediateCerts to UntrustedIntermediateCerts
mtrmac Nov 25, 2022
f44d639
Rename pool to untrustedPool
mtrmac Nov 25, 2022
17cad99
Rename cert to untrustedCert
mtrmac Nov 25, 2022
58380e3
Rename parameters to TrustedCert to indicate untrusted...
mtrmac Nov 25, 2022
21d19e8
Rename TrustedCert to CertificateSignedByTrustedRoot
mtrmac Nov 25, 2022
5ae11b5
Introduce a correctlySignedCert variable
mtrmac Nov 25, 2022
5de8a27
Rename cert to correctlySignedCert
mtrmac Nov 25, 2022
9831a1d
Rename validateCertIdentity to validateCertSubject
mtrmac Nov 25, 2022
e3d4d9e
Rename cert to correctlySignedCert
mtrmac Nov 25, 2022
9286e3a
Split validateIssuerPolicy from CheckCertificatePolicy
mtrmac Nov 25, 2022
b8ad87a
Validate the certificate issuer before worrying about the subject
mtrmac Nov 25, 2022
784d491
Rename CheckCertificatePolicy to CheckCertificateIssuerAndSubject
mtrmac Nov 25, 2022
d4a62a5
Only create a verifier from a certificate _after_ it passes conditions.
mtrmac Nov 25, 2022
5120f37
Only worry about certificate issuer and subject if the SCT is acceptable
mtrmac Nov 25, 2022
8aa0f7d
Split ValidateAndUnpackCert
mtrmac Nov 25, 2022
bc50a08
Replace ValidateAndUnpackCert in verifyInternal by its components
mtrmac Nov 25, 2022
91c6593
Warn more explicitly in some functions that ignore certificate validi…
mtrmac Nov 25, 2022
0d4bda1
Rename sig to untrustedSig
mtrmac Nov 25, 2022
8b4421e
Rename ts to untrustedTimestamp
mtrmac Nov 25, 2022
e7b3199
Rename b64sig to untrustedB64Sig
mtrmac Nov 25, 2022
43564e8
Rename signedPayload to untrustedPayload
mtrmac Nov 25, 2022
0ace845
Rename rawSig to untrustedRawSig
mtrmac Dec 7, 2022
07ffbb8
Rename tsBytes to untrustedTSAArtifact
mtrmac Nov 25, 2022
c37d3f8
Rename sig to untrustedSig
mtrmac Nov 25, 2022
d7028e7
Rename bundle to untrustedBundle
mtrmac Nov 25, 2022
7136f69
Rename parameters of VerifySET
mtrmac Nov 25, 2022
c9c92e2
Introduce acceptableBundleBody
mtrmac Nov 25, 2022
5aa2e8e
Only validate the public key and signature _after_ we accept the SET
mtrmac Nov 25, 2022
b150bcf
Rename payload to untrustedPayload
mtrmac Nov 25, 2022
7428fc3
Rename signature to untrustedSignature
mtrmac Nov 25, 2022
ea498b2
Document VerifyBundle a bit more
mtrmac Nov 25, 2022
0321492
Rename pemBytes to untrustedPEMBytes
mtrmac Nov 25, 2022
0771a5a
Rename sig to untrustedSig
mtrmac Nov 25, 2022
28cb652
Rename pem to untrustedPEM
mtrmac Nov 25, 2022
5c35a52
Rename b64sig to untrustedB64sig
mtrmac Nov 25, 2022
66a6644
Rename payload to untrustedPayload
mtrmac Nov 25, 2022
75cd9e8
BEHAVIOR CHANGE: Don't check against current time if we have a RFC 31…
mtrmac Nov 25, 2022
2c2f0da
Eliminate the redundant cert variable
mtrmac Nov 25, 2022
36f9fa1
Introduce acceptableCert
mtrmac Nov 25, 2022
deebfc5
Update comments in verifyInternal
mtrmac Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cosign/cli/sign/sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ func signerFromKeyRef(ctx context.Context, certPath, certChainPath, keyRef strin
for _, c := range certChain[:len(certChain)-1] {
subPool.AddCert(c)
}
if _, err := cosign.TrustedCert(leafCert, rootPool, subPool); err != nil {
if _, err := cosign.CertificateSignedByTrustedRoot(leafCert, rootPool, subPool); err != nil {
return nil, fmt.Errorf("unable to validate certificate chain: %w", err)
}
// Verify SCT if present in the leaf certificate.
Expand Down
4 changes: 2 additions & 2 deletions cmd/cosign/cli/verify/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
Expand Down Expand Up @@ -205,7 +205,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()
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/cosign/cli/verify/verify_attestation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
}
co.IntermediateCerts, err = fulcio.GetIntermediates()
co.UntrustedIntermediateCerts, err = fulcio.GetIntermediates()
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
Expand Down Expand Up @@ -182,7 +182,7 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
}
co.IntermediateCerts, err = fulcio.GetIntermediates()
co.UntrustedIntermediateCerts, err = fulcio.GetIntermediates()
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/cosign/cli/verify/verify_blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func (c *VerifyBlobCmd) Exec(ctx context.Context, blobRef string) error {
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
}
co.IntermediateCerts, err = fulcio.GetIntermediates()
co.UntrustedIntermediateCerts, err = fulcio.GetIntermediates()
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
Expand Down
Loading