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.

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

add sha256:... as alternative way to specify a blob #4016

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wolfv
Copy link

@wolfv wolfv commented Jan 17, 2025

Hey, looking for some early feedback on this change to make validation of blobs without providing the actual blob easier.

Summary

With this change, one can validate blobs without the blob - just by sha256:<hash>.

Release Note

  • Allow passing a hash instead of an actual blob / file by using sha256:<hash>.

@haydentherapper
Copy link
Contributor

@woodruffw, @kommendorkapten and I had a discussion in sigstore/protobuf-specs#444. There was some concern that providing only the hash of an attestation would lead to a lack of verification of the document itself.

@wolfv
Copy link
Author

wolfv commented Jan 17, 2025

Sure, but in our case the server application already checks the hash and we don't want to send the file around to the microservice to validate the signature :)

Also sigstore-python already supports this.

But also fine with me if it isn't accepted!

@woodruffw
Copy link
Member

Hey @wolfv! Good to see you 🙂

Per sigstore/protobuf-specs#444, I think there are two subtly different cases here: when doing a "bare" verification (cosign verify-blob) having a hash instead of an artifact is OK, since the signature is over a "bare" payload and the hash is equivalently binding to it.

However, in the attestation verification context, just a hash is insufficient (since it binds to the subject's digest, but not the subject's name.

So, I think cosign could support sha256:... in the "bare" case, but forbid it for the attestation case. OTOH, I could see that being confusing for users -- I'm curious what @haydentherapper thinks.

(In sigstore-python we allow sha256:... in both cases, which was arguably a mistake 😅 -- I'm going to investigate removing or augmenting it in the next stable release. In particular, if we decide to keep it, I'll likely add some kind of required --subject flag to complement the digest.)

@haydentherapper
Copy link
Contributor

I agree with William. This PR is for the attestation case, but if this were for the "bare" case, that can be accepted.

With that said, I'm realizing that Cosign's attestation verification here is incomplete, missing subject name verification. I'd be open to a change that accepts a digest and a subject name, with the accompanying changes for the latter.

@codysoyland
Copy link
Member

However, in the attestation verification context, just a hash is insufficient (since it binds to the subject's digest, but not the subject's name.

Just to be clear, the current implementation doesn't check the file name -- it requires a file on disk and just computes its digest. This PR just adds the ability to specify a digest as an input instead of a file. We can discuss validating that the subject name matches, but that is orthogonal to this PR.

@codysoyland
Copy link
Member

@haydentherapper @woodruffw I'm curious how you'd respond to #4019? It's a very related ask with regard to passing a subject digest as an input for attestation verification, but in the context of verifying git commits (not files on disk) using cosign. I think it's a reasonable ask and I'm not quite sure I understand the argument against providing digests as input for attestation verification.

@wolfv
Copy link
Author

wolfv commented Jan 23, 2025

Yeah after diving deeper into the whole sigstore / in-toto validation topic I understand that you would want to validate the in-toto statement subject.name against the file name of the artifact that we're verifying.

I guess that makes a lot of sense, but this PR seemed to work without providing a subject or alternative filename so I do not think that's currently implemented (as @codysoyland mentions).

We'll do these kind of validations on our end either way but yeah, might be nice to (also) add a --subject flag and require that together with a sha hash, but I think that would be a different PR :)

@TomHennen
Copy link
Contributor

Note that subject.name is not a required field in statements, so if the proposal is to check that value against something it won't work for a number of use cases.

@woodruffw
Copy link
Member

I think it's a reasonable ask and I'm not quite sure I understand the argument against providing digests as input for attestation verification.

I guess the basic argument is that it's confusing 🙂 -- it makes sense to allow "bare" verifications to be verified against digests because the signed "statement" (i.e. blob) is fully enclosed by the digest. In contrast, it's not clear what the user is intending when they pass a digest during attestation verification: they might be communicating that they don't care about the subject, or that they don't know the subject, or even an unknown unknown (they don't know the subject even exists as a verifiable piece of state).

This could be fixed with UX work, e.g. requiring users to pass something like --no-subject or --ignore-subject when they want to verify an attestation by digest without checking the subject. But I think that should be an explicit user decision that can be substantiated from a security model, not something that Sigstore clients should do implicitly.

Alternatively, we could firm up the client spec to make it clear where the division of responsibilities between Sigstore clients and downstreams lies: I would be OK with Sigstore adopting the opinion that it only cares about digests, and that it's the responsibility of downstreams to adequately verify the remainder of the subject/statement modulo their own policies! However, I think we'd need to make that very explicit, because on its own an attestation verification conveys almost none of the "trust" that a normal signal verification does (since the bundle is essentially verifying its own contents).

@codysoyland
Copy link
Member

codysoyland commented Jan 23, 2025

In contrast, it's not clear what the user is intending when they pass a digest during attestation verification

Interesting, I hadn't considered that the user's intent might be unclear if we offer a flag to specify a digest. The cosign verify-blob-attestation command help text is fairly clear that the blob is checked against the attestation subjects, so I guess it feels like a natural extension to allow passing the digest as a means connecting the bundle to the artifact, but you're right we need to be careful about the documentation/UX here.

I'm actually more receptive to the notion of disallowing digest as inputs in "raw" message signature verification (as opposed to attestations)... In sigstore/protobuf-specs#444, @kommendorkapten brought up three scenarios where providing a digest may be considered harmful, and two applied to message signatures: poor crypto library support for verifying signatures with only a message digest, and some algorithms (i.e. ed25519) incapable of verification with just a digest. The third argument was related to api security and could possible apply to DSSE, and I admit I don't understand the threat model aside from some potentially confusing UX like you pointed out.

Alternatively, we could firm up the client spec to make it clear where the division of responsibilities between Sigstore clients and downstreams lies

Hard agree! Sigstore is mainly concerned with tying an identity to a claim/subject, and it seems sensible for the verifier to open the attestation and confirm the subject is present, but the breakdown in responsibilities is definitely unclear to the average user. This is sort of covered in the client spec:

Verifier MUST ensure that the artifact's digest/algorithm tuple is present in the list of subjects in the in-toto statement.

@kommendorkapten
Copy link
Member

I think I can't add much content to the discussion here, most of pitfalls have been mentioned with passing in digests. And as @codysoyland mentions the client spec mentions that

Verifier SHOULD accept the raw artifact and compute the message digest to minimize any risk for confusion attacks

So i think the spec is ok, it's more about getting the cli flags in order.

But another thing briefly mentioned here, and something I took up in sigstore/protobuf-specs#444 is the effect this would have in the future for PQC. The current standardized algorithms follows Ed25519 in that they require the complete message. There are pre-hashed versions defined too, but will they be common/supported? My understanding now is that Ed25519ph is not very common/supported, and exposing an API that will break for the common scenarios is not that good (note that his will be a primary concerns for blobs, not in-toto/dsse as the dsse envelope is the message being signed and verified).

@codysoyland
Copy link
Member

@wolfv I spoke with @kommendorkapten this morning, and we're in agreement that we could accept this PR with some caveats/changes. We want to make sure that the UI is clear and explicit about what is checked when you enter a digest. I would recommend:

  • Add a new flag, something like --expect-subject-digest=sha256:xxxxx (this name is up for debate)
    • Add help text to this flag to make it obvious what it does
    • Do not inspect blob filename to see if it looks like a digest
    • This allows for arbitrary algorithms instead of just sha256 (for @TomHennen's case, he would like to verify git commit shas)
  • You will need to modify the logic here to make the blob filename not required if the new flag is provided.

@haydentherapper
Copy link
Contributor

Agreed with @codysoyland on the suggested changes. You'll also need to update

for _, subj := range st.StatementHeader.Subject {
dgst, ok := subj.Digest["sha256"]
if !ok {
continue
}
subjDigest := "sha256:" + dgst
if subjDigest == imageDigest.String() {
return nil
}
to remove the assumption the digest is sha256.

Note that subject.name is not a required field in statements, so if the proposal is to check that value against something it won't work for a number of use cases.

TIL!

the effect this would have in the future for PQC. The current standardized algorithms follows Ed25519 in that they require the complete message. There are pre-hashed versions defined too, but will they be common/supported?

Let's discuss more outside of this PR, but the short answer is that for now, I'd say it's on the verifier to know if providing only a digest is possible for the given algorithm. This is meant to be an optimization, and the common use case is still to provide the blob.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants