-
Notifications
You must be signed in to change notification settings - Fork 31
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
Discussion on providing digest/alg for artifact verification #444
Comments
Thanks for opening this @kommendorkapten! I think the situation here is pretty (unfortunately, IMO) nuanced, since Sigstore clients really have two (or more) separate verification flows:
I think the divergence between these flows is difficult to explain to users, and has made it hard to create maximally-misuse-resistant client APIs -- sigstore-python keeps the two flows separate at the API level, but conflates them slightly (incorrectly, IMO!) at the TL;DR: I think it's a good idea to put this behind some kind of insecure/"I know what I'm doing" option/flag, but I also think we need to revisit the concept of "verification" more fundamentally 🙂 -- right now we really have two (or more) flows with subtly different outputs/semantics that aren't well-communicated. |
Thank for a good reply, I will only give a short reply now as its late, but I couldn't resist :) In sigstore-go, we do allow for the "attestation flow" to actually be verified against an artifact: https://github.com/sigstore/sigstore-go/blob/378b249542ef54cb12720de86189af3ebeee67a7/pkg/verify/signature.go#L68 So my primary concern is around mapping the digest to the intended artifact. And offering a policy model where the artifact can be directly provided to the verifier minimizes the risk of a confusion. But in-general I agree, there is a disconnect from the bundle to the artifact, which is very much related to the CVE I linked in the first post. |
Speaking to the impact on verification for signing algorithms that require the pre-image: That's a fair point, and I agree that a comment would be valuable. The Sigstore client specification is underspecified when it comes to DSSE verification and we should flesh this out, what actually is required to be verified vs what's the responsibility of a verifier for a given predicate. |
Originally the verification options supported two options:
This was changed in #406 to be able to support some cases in the conformance tests when verifying against a message digest.
Having this as part of the standard verification path have me wondering if this is really a good idea. The first is that this does not seem to be compatible with Sigstore-js and node.js verify implementation which requires the complete message to verify. Pulling in a dependency for crypto verification is something we would like to avoid I think.
Also, this is not compliant with certain algorithms such as Ed25519 (this will fail today with Sigstore-go if an artifact digest is provided), LMS, ML-DSA and SLH-DSA to name a few. We haven't started to talk about PQC yet, but at some point we need too. All of the listed algorithms have pre-hash version, but I can't guess how popular/common those will be.
My third concern is about API security. Crypto APIs should make it hard to do the wrong thing, and having the Sigstore verification APIs accept a message digest is not optimal. Providing an artefact is more secure as we can guarantee in the Sigstore client that we are verifying against the expected digest. Low level APIs such as Go's
ecdsa.VerifyASN1
is an example where providing the digest makes sense, but for higher level APIs it can be a risk.When providing a bundle with a message signature, we explicitly forbid against using the (untrusted) digest for signature verification: https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L119
But as we are allowing digests for verification, I can see how this can be misused. Although not the same vulnerability but similar is CVE-2022-36056 in Cosign, where the content of a Rekor entry wasn't verified to actually match the artifact in question.
Proposal
I can understand that it's desirable from the conformance test suite to have this feature, but for regular verification I would like to see an comment about that this is not recommended and an optional feature (it breaks Ed25519 today).
Possibly even rename the policy option https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/signed_entity.go#L444 to
WithArtifactDigestUnsafe
similar to when no signer identity is provided.If we also make this an optional feature, how could this be expressed in the conformance test suite?
cc @woodruffw @haydentherapper @bdehamer
The text was updated successfully, but these errors were encountered: