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

Jump to bottom

Fix verifying when a certificate is provided #2633

Closed
wants to merge 1 commit into from
Closed

Fix verifying when a certificate is provided #2633

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions pkg/cosign/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,15 +725,20 @@ func keyBytes(sig oci.Signature, co *CheckOpts) ([]byte, error) {
if err != nil {
return nil, err
}
// We have a public key.
if co.SigVerifier != nil {
switch {
// certificate provided from the OCI image, or via flag (--certificate or --bundle)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure I understand the logic here, but I'm worried that this bit might have a subtle verification bug. Let me know if this makes sense:

If the user has specified --certificate at the command line my understanding is that it get parsed and its public key is used in initialize co.SigVerifier. This function appears to then return a certificate PEM if found on the OCI signature (which may be different than --certificate 😬 ). The function calling this one

cosign/pkg/cosign/verify.go

Lines 1078 to 1081 in 29360f6

if !bytes.Equal(pemFirst.Bytes, pemSecond.Bytes) {
return fmt.Errorf("comparing public key PEMs, expected %s, got %s",
pemBytes, decodeSecond)
}
then appears to compare the returned certificate to the one it finds in the rekor bundle.

My worry here is that the certificate compared to the rekor bundle might not be the one the user actually specified at the command line, unless that bit of logic is lurking in a different place?

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 have this issue now also, although it doesn’t surface any error. If you provide a certificate that certified the same public key as a certificate on the OCI imagine, but the rest of the cert differs, we wouldn’t notice.

Note this isn’t an issue for verify-blob since we construct a static OCI struct from input. For verify on OCI, theres no way to pass the certificate passed in from input

Sigh…maybe we could add a check that a provided cert matches what’s on the image annotations? Or disallow providing a certificate when the image annotations contain a certificate already?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh…maybe we could add a check that a provided cert matches what’s on the image annotations? Or disallow providing a certificate when the image annotations contain a certificate already?

+1, see my comment below. I think --certificate is a little misleading here. The --certificate flag for OCI image never "makes" it into the pkg verification code (just the pub key does). In the CLI layer there's some verification of it, but once we get into pkg/verify, we rely on sig.Cert().

Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment accurate / misleading? If it's OCI it will always take it in from the OCI image, NOT from the --certificate flag. I think @nsmith5 issue shows a "Red herring" like you mentioned. I think the issue should have happened if the --certificate was specified or not since it's comparing sig.Cert() form the OCI annotation. The OCI verify CLI never sets that. So maybe

// certificate provided from the OCI image, or, in the case of a blob, via flag (--certificate or --bundle)

case cert != nil:
return cryptoutils.MarshalCertificateToPEM(cert)
// no certificate provided, verifier must be a public key
case co.SigVerifier != nil:
pub, err := co.SigVerifier.PublicKey(co.PKOpts...)
if err != nil {
return nil, err
}
return cryptoutils.MarshalPublicKeyToPEM(pub)
default:
return nil, errors.New("no verifier found, must provide either certificate or public key")
}
return cryptoutils.MarshalCertificateToPEM(cert)
}

// VerifyBlobSignature verifies a blob signature.
Expand Down