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

Attestations require uploading entire payload to rekor #3599

Open
jonjohnsonjr opened this issue Mar 14, 2024 · 9 comments
Open

Attestations require uploading entire payload to rekor #3599

jonjohnsonjr opened this issue Mar 14, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@jonjohnsonjr
Copy link
Contributor

Description

Recently ran into an issue where we have an absurdly large attestation (130MB) that is rejected by rekor due to its size.

Arguably, this is a bug in rekor, but I think it's really a bug in how cosign attestations work. Both the dsse and intoto record types require uploading the entire attestation payload to rekor. If they're small enough, rekor will store the intoto attestation. AFAICT, rekor doesn't store the dsse payload at all, but it does require you to upload the entire thing.

Cosign should support a form of attestation that doesn't require the entire payload be uploaded to rekor.

This is part of why I initially proposed an OCI descriptor as the payload format, since embedding data is an optional thing. I think sigstore ended up using dsse so that the payload type could be signed, but I can't remember the details.

Anyway, it would be great if someone could tell me I'm just holding this wrong or something, but I'd love an easy way to attest a large thing. Should I just attest an OCI descriptor that points to the large thing? Is there an existing pattern that people use for this?

It would take a lot of load off of rekor if it could just verify a hash instead of the entire payload, so maybe what we need is a form of cosign attest that stores and verifies a hashedrekord? Does that even make sense? If someone can hold all of this in their head, I'd love your opinions on what the right path forward would be.

@jonjohnsonjr jonjohnsonjr added the enhancement New feature or request label Mar 14, 2024
@mrjoelkamp
Copy link
Contributor

mrjoelkamp commented Mar 14, 2024

One solution to this would be to use the HaskedRekord type (where the uploaded data is just a hash of the payload) for attestations. cosign.TLogUpload() implements this.

It looks like HashedRekord just needs to be an option for RekorEntryType here

if c.RekorEntryType == "intoto" {
return cosign.TLogUploadInTotoAttestation(ctx, r, signedPayload, b)
} else {
return cosign.TLogUploadDSSEEnvelope(ctx, r, signedPayload, b)
}

I can't think of any reason why that wouldn't be acceptable. Pretty sure that the TL entry is just for checking the inclusion proof and verifying that the signed entry timestamp fits into the validity window of the signing cert. That could be done with a hash of the attestation.

@bobcallaway
Copy link
Member

Arguably, this is a bug in rekor, but I think it's really a bug in how cosign attestations work. Both the dsse and intoto record types require uploading the entire attestation payload to rekor. If they're small enough, rekor will store the intoto attestation. AFAICT, rekor doesn't store the dsse payload at all, but it does require you to upload the entire thing.

Originally we had envisioned Rekor storing the full attestation (and implemented the intoto type to do just that), but after some experience and hindsight realized that it was probably better to not position Rekor to need full search capabilities (since folks want to be able to look up all log entries /attestations for arbitrary subjects). We added the dsse type to clean up some code issues, as well as start enforcing the pattern that Rekor wouldn't store the attestation when an entry was made with that type.

One way you could think of a DSSE envelope is that it has "embedded" signatures (similar to an RPM or an APK file), since the payload and signature are part of the same JSON document.

It is easier for Rekor clients to upload the DSSE envelope in its entirety to verify the signature over the entire envelope VS having the client break it apart manually. It's also arguably stronger from a claimant point of view because Rekor has computed the hash directly (during the certificate's validity period) and thus 'observed' the attestation, versus trusting that the digest provided in a hashedrekord entry actually corresponds to a DSSE envelope that exists and was signed.

It's not ideal for Rekor to be parsing multi-hundred MB+ JSON documents just for signature verification, and that's why there's a size limit on what you can upload to the public Rekor instance.

@mrjoelkamp
Copy link
Contributor

It is easier for Rekor clients to upload the DSSE envelope in its entirety to verify the signature over the entire envelope VS having the client break it apart manually.

The client can upload the digest of the DSSE envelope in its entirety without having to break it apart.

It's also arguably stronger from a claimant point of view because Rekor has computed the hash directly (during the certificate's validity period) and thus 'observed' the attestation, versus trusting that the digest provided in a hashedrekord entry actually corresponds to a DSSE envelope that exists and was signed.

Verifying that the Rekor HashedRekord log entry payload matches the hash of the DSSE envelope can and should be checked by the client by computing the DSSE hash during verification.

I have an example of this implemented for OpenPubkey attestations (experimental) here: VerifyEntryPayload

@jonjohnsonjr
Copy link
Contributor Author

What would the process be for adding support for a hashedrekord of a dsse envelope to cosign attest and cosign verify-attestation? Just send a PR, or do we need to gather some consensus around that?

@priyawadhwa
Copy link
Contributor

I think sending a PR would be fine. @bobcallaway do you have any concerns?

@haydentherapper
Copy link
Contributor

It would slightly complicate verification, as we don't know if an entry is a hashedrekord or dsse type. We would have to canonicalize the entry as both.

It would also mean that entries uploaded in Cosign couldn't be verified by other clients, as currently it's assumed that an attestation is uploaded as an intoto or dsse type. I'll create an issue on #clients mentioning this to see if they have any thoughts.

@lkatalin
Copy link
Contributor

I want to note that the problem is broader than attestations - we have this same issue with RPMs, which can be large. Uploading an RPM as a hashedrekord type does sidestep that problem but ignores the signature embedded in the RPM. I will try to follow the solution here (and look into DSSE envelopes) to see if it's applicable to RPMs; but any attestation-specific solution may need to be broadened to other "types".

@mrjoelkamp
Copy link
Contributor

It would slightly complicate verification, as we don't know if an entry is a hashedrekord or dsse type. We would have to canonicalize the entry as both.

Isn't this already solved since an attestation entry can be either intoto or dsse entries? AFAIK this is abstracted away in the verification path using models.LogEntryAnon and RekorBundle

It would also mean that entries uploaded in Cosign couldn't be verified by other clients, as currently it's assumed that an attestation is uploaded as an intoto or dsse type. I'll create an issue on #clients mentioning this to see if they have any thoughts

Similar to the above, I believe this can be abstracted away using models.LogEntryAnon correct? And then can be unmarshalled using the generic UnmarshalProposedEntry and UnmarshalEntry functions like so.

@haydentherapper
Copy link
Contributor

Yea, I noted on the Slack thread that this may not be an issue if the kind or canonicalized entry is persisted.

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

No branches or pull requests

6 participants