Don't fail open in VerifyBundle #1648
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
+ Coverage 32.16% 32.73% +0.57%
==========================================
Files 146 147 +1
Lines 9208 9301 +93
==========================================
+ Hits 2962 3045 +83
- Misses 5898 5902 +4
- Partials 348 354 +6
Continue to review full report at Codecov.
|
|
cc @priyawadhwa |
pkg/cosign/verify.go
Outdated
| @@ -614,7 +614,7 @@ func VerifyBundle(ctx context.Context, sig oci.Signature) (bool, error) { | |||
| if err != nil { | |||
| return false, err | |||
| } else if cert == nil { | |||
| return true, nil | |||
| return false, errors.New("signature does not include a certificate") | |||
There was a problem hiding this comment.
I think this codepath was for the case when a user signs with a key (instead of the "keyless" signing flow) and uploads to the tlog, in which case there would be no cert.
e.g. something like
COSIGN_EXPERIMENTAL=1 cosign sign --key cosign.key MYIMAGE
There was a problem hiding this comment.
Thanks. Why would it be acceptable to skip the bundlehash != payloadHash check in that case? (Or a different way to ask this: if it is not necessary to check the payload hash for signatures with --key, why is it checked in the other cases?)
There was a problem hiding this comment.
good catch, we should do the bundleHash != payloadHash check before returning.
There was a problem hiding this comment.
Drive-by opinion: I think there's a lot of harm that comes from maintaining one codepath with a lot of branches based on keyless vs. keyfull signing flow and that splitting this into two functions with some "duplicate" code will make it much easier to reason about.
There was a problem hiding this comment.
+1 in general but I think it's actually slightly more complicated than that: there are actually 3 modes, because the transparency log inclusion is separate:
- keyless, (always tlog)
- keyed (tlog)
- keyed (no tlog)
the case where a keyed signature is in the tlog is the one that's always easiest to forget.
There was a problem hiding this comment.
I think this codepath was for the case when a user signs with a key (instead of the "keyless" signing flow) and uploads to the tlog
Updated to handle that, now.
There was a problem hiding this comment.
Drive-by opinion: I think there's a lot of harm that comes from maintaining one codepath with a lot of branches based on keyless vs. keyfull signing flow and that splitting this into two functions with some "duplicate" code will make it much easier to reason about.
(Another drive-by opinion: I’m not so worried the code having branches than about having a clear and constrained state of knowledge, something that could be expressed as simple assertions.
E.g here, the certificate’s acceptability only validated at this point, way after VerifyImageSignature has already extracted the public key and used it to verify a signature. When the certificate’s acceptability is not yet established, the signature’s verification status is indeterminate until that happens.
I’d love to see something like
func verifySignature(configuredRootOfTrust …, untrustedRekorBundle, untrustedCertificate, untrustedSignature, untrustedPayload []byte) acceptedPayloadData {
var acceptablePublicKey
switch configuredRootOfTrust.signerMode {
case .fulcioCertificate:
correctlySignedCertificate, err = validateCertificateSignature(configuredRootOfTrust.fulcioCACertificates, untrustedCertificate)
if err { fail }
acceptableCertificate, err = verifyCertificateDataConstraints(configuredRootOfTrust.fulcioAcceptedIdentities, correctlySignedCertificate)
if err { fail }
acceptablePublicKey = acceptableCertificate.PublicKey
case .rawPublicKey:
acceptablePublicKey = configuredRootOfTrust.trustedPublicKey
}
correctlySignedPayloadBlob, err = verifySignature(acceptablePublicKey, untrustedSignature, untrustedPayload)
if err { fail }
if configuredRootOfTrust.requireRekorPresence {
acceptableRekorBundle, err = verifyRekorBundleIsAcceptable(configuredRootOfTrust.rekorTrustedCertificates, untrustedRekorBundle)
if err { fail }
err = verifyPayloadIsConfirmedByRekorBundle(acceptableRekorBundle, correctlySignedPayload)
if err { fail }
decodablePayloadBlob = correctlySignedPayloadBlob
} else {
decodablePayloadBlob = correctlySignedPayloadBlob
}
acceptablePayload, err = parseAndValidatePayload(configuredRootOfTrust.payloadClaimVerifier, decodablePayloadBlob)
if err { fail }
return acceptablePayload.ParsedData
}where every piece of data either comes from configuredRootOfTrust, or it starts as untrusted…, and is only ever used for any purpose after it becomes acceptable….
I feel that this would greatly simplify reasoning about the code, and possible states of the world — every piece of data is only “interesting” on the paths where it turns from untrusted… into acceptable…, and those should be linear and fairly easy to follow, perhaps even with various branches.
I hope to contribute work towards that direction.)
mtrmac
force-pushed
the
cert-fail-open
branch
2 times, most recently
from
a27ad0f to
1378dbb
Compare
We do need to accept a missing certificate here (to accept raw signatures which are uploaded in a transparency log), but that's not a reason to bypass all other checks in this function. Signed-off-by: Miloslav Trmač <[email protected]>
We do need to accept a missing certificate here (to accept raw signatures which are uploaded in a transparency log), but that's not a reason to bypass all other checks in this function. Signed-off-by: Miloslav Trmač <[email protected]>
Summary
Remove a “fail-open” code path in
VerifyBundleTicket Link
Release Note
NONE
This code path succeeding and bypassing all future checks worries me greatly, and I can't find any documentation nor explanation for why that is necessary, so let's close this avenue and see what breaks.