-
Notifications
You must be signed in to change notification settings - Fork 547
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.
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
bug: x509 verification broken #2632
Comments
At HEAD, this is WAI: For the second, it looks like you uploaded a public key to Rekor, but tried to verify the returned bundle with a certificate. This would result in an error, because the verifiers don't match. |
To confirm, is this issue saying the docs are broken, or something else? If it's just the docs, can you update #2534? |
You'd expect that I'd uploaded a public key to Rekor looking at the error, but that isn't the case. The rekor bundle has a certificate (the same as the one I'm verifying with). This can actually be seen from the error message. In Lines 1078 to 1081 in 29360f6
Lines 723 to 737 in 29360f6
The If this is WAI at head I suppose that is fine and the docs just need to be updated, but why should I have to specify details like the issuer etc if I've given you the exact certificate I want you to verify with. It feel like an odd little bit of homework for me to pull that out of the cert and as for it to be checked when I assume this pathway should already check the certificate from the signature and the one passed at the command line match |
Ah, this is all scoped to BYO PKI. For the first point, that is reasonable to not require the flags since you’re passing the certificate. The logic would look something like “cert || cert-identity || key” is required. This would also affect verify blob. However I’m also tempted to say that we should always require the identity flags so that we can have clear documentation around why requiring identity is necessary. Saying it’s not for a subset of cases might be confusing. And there’s edge cases too - if I provide a bundle and not a certificate, I should still require the identity flags. Edit: you’ll also get examples that just extract a certificate from a bundle and pass that, that’s not ideal because users can’t easily see what identity they’re verifying with. For the second, yea, this seems buggy. Lemme dig through the code later. I think this is because we extract the public key from the certificate rather than have the certificate be the verifier, when —cert is passed. For the keyless case, I’m assuming we make the certificate the verifier, but I’m not sure right now. |
Yeah exactly, I opened up that feature request issue #2630 because I agree that identity flags + bundle would be a great option, but it feels like a new feature to cosign. The existing approach of BYO PKI where you specify the exact certificate for verification and we don't have strong opinions on its format (e.g we don't require that custom Fulcio extension for the OIDC issuer to be specified) is something I feel like we should still support. In this case it doesn't seem clear to me that we should require the identity flags because those extensions might not be set by the users PKI right? |
Yea, this seems like an issue because we had previously supported certificates like that. A sub command should help this, but for now, I think the easiest option is to say that if you pass a certificate or bundle, we don’t require identity flags. verify-blob wouldn’t require it, only “cosign verify” without a verifier passed to it. |
The other option is adding some flag that says my certificate is non-conforming (effectively a BYO flag), but I don’t really think that is great user experience. cc @znewman01 thoughts on all this? |
@haydentherapper Yup that fixed the second issue!
|
@nsmith5 Been thinking a bit more about how to fix the first issue with requiring the identity flags. The problem is that even if we remove requiring the flag when a certificate or bundle is provided**, what about the case where the signer attaches a BYO PKI certificate without an identity to a container? We would still require the flags, but no identity would be present. The issue is that certificates can be one of two things within Sigstore: Either an identity-based code signing certificate (issued by Fulcio or a managed PKI that issues certificates that adhere to Sigstore's expectations), or just a vehicle for providing a public key (which some managed PKIs may do because they have existing infrastructure). In the latter case, the identity is irrelevant. Rather than remove flags, I would propose that we add a flag to disable the requirement, something like How does this sound? Once again, the long-term solution is probably going to be splitting out these commands into opinionated verification and generic verification. ** I am hesitant still to remove the flag in this case, because it's harder to build verification policies around certificates. I treat these flags as policy, but by CLI flag. Ideally these flags would be a part of a richer policy, and we would want to describe verification using identities rather than raw certificates. I also think we still need the flag for bundles, because it is effectively an opaque object to the lay user (yes, they could parse it to find the identity, but they probably won't). It's easier to visualize correctness with an email, |
I was thinking if a slightly different approach long term, but I think we would both end up at the same place basically. If you give us a leaf certificate and bundle, we apply the strictest possible interpretation -- we require the exact same leaf certificate to be attached to the OCI image / rekor bundle / offline bundle (from If your certificate is just a vehicle for a public key, I feel like you should potentially just pop out the public key with e.g In the near term, we also allow folks to verify without the leaf certificate using the same identity policy flags we have for Fulcio certs (--certificate-identity, --certificate-oidc-issuer, etc etc). This restricts folks to BYO PKI that is Fulcio-like. Longer term, we namespace the BYO PKI stuff like I feel like this way we don't ever really need to open up a flag like |
Commenting on this first, the rest of my comments are extraneous. We're sorta stuck in this state where we have a lot of ongoing work and a lot of work we want to do. I'd prefer we keep what's here, to always require the flags, and keep an eye on user feedback to see how we can make the UX better.
At least in verify-blob, the leaf certificate from the bundle will overwrite the provided leaf certificate, see https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify_blob.go#L211. We could change the behavior to verify that both match, but this is probably fine for now. I think it's an unexpected case to pass both, given I think you'd have to hand-craft a bundle to contain only the rekor payload and not the certificate if the bundle is requested on signing.
I think this is reasonable, though not the stance we've taken currently. I'm totally happy to support this though, it simplifies how we treat certificates, such that we can say that a certificate always means it conforms to fulcio's issued certificate profile. I'd say we proceed with this and if there's any comments from users, we can adapt.
Could you clarify this? Do you mean have policy flags for non-fulcio-like certificates? If so, I don't think this is something we should implement. We will have an endlessly growing list of policy flags if so. For example, someone asked to support subject (not SAN) verification, and we proposed relying on an upcoming |
Yeah so the idea here was just to have one more flag like |
That sounds like a great feature! |
Great discussion. There's a lot, so trying to summarize: ImmediatelyTry to make minimal changes here, in favor of a better long-term solution:
Medium-term
Long-term
@haydentherapper @nsmith5 can you make sure I didn't miss anything? |
LGTM, thanks @znewman01! One more immediate AI, a part of minimal changes, is to complete #2633 as a fix for verifying with a provided Fulcio-like certificate. |
Hi, just wondering where it stands. |
Hey @avishayil, I haven't had a chance to fix this. Can you confirm which command you're running that is failing for you? For OCI, you can also run |
Just following up on this as it seems to still be an issue with cosign v2.1.1. Additionally, I have not had success using For background, we are using internal PKI to sign images using a KMS key. In cosign v1.13.1, we had been signing and attaching the certificate using The following steps should reproduce the (they're essentially the same as above, but to confirm the flow): Sign the image, attaching the cert and bundle in the process cosign sign -y \
--rekor-url='http://rekor.cluster.internal' \
--key awskms:///<id> 192.168.106.3:5000/rekor-test:latest \
--certificate cosign-certificate.pem \
--certificate-chain cosign-ca-bundle.pem Attempt to verify image using Rekor cosign verify \
--rekor-url http://rekor.cluster.internal \
--certificate cosign-certificate.pem \
--certificate-chain cosign-ca-bundle.pem \
--certificate-identity me@whatever.io \
--certificate-oidc-issuer-regexp ".*" \
--insecure-ignore-sct 192.168.106.3:5000/rekor-test:latest This results in the same error described by the OP:
Meanwhile, attempting to
|
To confirm, does that error occur during |
The last error above occurs during |
Again the problem we're trying to solve with Rekor is:
|
Ah, I think I see the issue. You need to also attach the Rekor response ("rekor bundle") to the OCI image to resolve the issue. The workaround using Just for background context: To verify short-lived certificates, a Sigstore client needs either:
Cosign also expects a Rekor bundle/response regardless of how the timestamp is obtained, since part of Sigstore's security model is requiring transparency. There's a few issues on GitHub discussing handling private instances better - 1.x had better support, 2.x swung the pendulum a bit too far but made usage of the public instance more secure. |
I will confirm once I get Rekor stood up again (colima has been sad since Ventura), but I also tried without We did have success with the first option, but wanted to see about the second. We were utilizing |
Yeah, confirmed that the issue remains without
Throws:
|
You can drop
Rekor acts as a witness to a signing event and will provides a timestamp on upload. If a signer wants to distribute trust to a third-party timestamp authority, it can do so by fetching a timestamp that is signed over the artifact/container signature. The timestamp that comes from either Rekor or the timestamp authority is used during verification. |
Without
Is this a quirk of self-hosted Rekor? |
This is because you have to configure Cosign with the roots of trust (Rekor pub key, Fulcio cert chain) from your self-hosted instance. You can either specify trusted roots via CLI flag ( By default, Cosign is configured with roots of trust from the public instance that we store in a TUF/TheUpdateFramework repository. You can also set up a TUF repo and initialize Cosign with your own TUF repository. We created a guide for this if you want to pursue this, https://blog.sigstore.dev/sigstore-bring-your-own-stuf-with-tuf-40febfd2badd/, one of the benefits being you can easily update roots of trust and you'll have a setup similar to how we run the public instance of sigstore. |
Gotcha, thank you @haydentherapper I'll review the Rekor helm chart. |
Hi, I am hitting the same error as the OP, but in a different scenario:
But when I checkout 2.2.1 release as source, cherry-pick the commit from #2633 and build it, the verify works without errors:
Is it possible to merge #2633? Or maybe there is another solution in my scenario? |
#2633 (comment) is the correct fix for this issue, but this is a larger refactor that I haven't been able to get to. I can try to work on this in the coming weeks, or if anyone wants to pick up #2633 and get it finished and tested, that would be appreciated. |
Description
The documented x509 certificate verification isn't working as expected. This is broken in two different ways at HEAD (29360f6) and v2.0.0-rc0
HEAD
v2.0.0-rc0
Version
The text was updated successfully, but these errors were encountered: