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

Use hashed rekord type for tlog upload #1081

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Nov 19, 2021

Signed-off-by: Asra Ali [email protected]

Summary

  • Use a hashed rekord type for tlog upload to allow for signing large blobs with keyless flow.

THIS IS A NOT A DRAFT ANYMORE BECAUSE...

  1. Prod rekor needs to be deployed with the new type (DONE)
  2. The bigger issue is that if someone is using COSIGN_EXPERIMENTAL and uploading signed entries, let's say, with a KMS provider that uses a specific hash algorithm, then defaulting to the SHA256 I hard-coded in rekorEntry will be incorrect and will fail signature verification. The hash algorithm needs to be plumbed through, so I imagine that SignerVerifier would have to expose a new method to it's interface providing the hash function that it uses by default (doesn't look like cosign will ever use a WithCryptoSignerOpts(otherHashFunc) on its SignMessages
  • to get around this I guess I could specifically only have hashedRekord type being used for fulcio generated certs that we know will be using sha256? that'll at least help keyless sign-blobs. but i would rather not solve just a tiny part of the problem

Ticket Link

Fixes sigstore/rekor#481

Release Note


@asraa asraa marked this pull request as draft November 19, 2021 15:44
@dlorenc
Copy link
Member

dlorenc commented Nov 19, 2021

2. The bigger issue is that if someone is using COSIGN_EXPERIMENTAL and uploading signed entries, let's say, with a KMS provider that uses a specific hash algorithm, then defaulting to the SHA256 I hard-coded in rekorEntry will be incorrect and will fail signature verification. The hash algorithm needs to be plumbed through, so I imagine that SignerVerifier would have to expose a new method to it's interface providing the hash function that it uses by default (doesn't look like cosign will ever use a WithCryptoSignerOpts(otherHashFunc) on its SignMessages

The spec pretty much requires SHA256 right now. People can technically use other ones in a KMS, but I'd be fine fixing that later, maybe built on #1071

@asraa
Copy link
Contributor Author

asraa commented Nov 19, 2021

The spec pretty much requires SHA256 right now.

Do you mean the spec for keyless signing? Or more generally. I'm just worried about someone doing KMS w/ sha512 + rekor upload

@asraa asraa changed the title WIP: Use hashed rekord type for tlog upload Use hashed rekord type for tlog upload Nov 19, 2021
@asraa asraa marked this pull request as ready for review November 19, 2021 19:59
@dlorenc
Copy link
Member

dlorenc commented Nov 19, 2021

Do you mean the spec for keyless signing? Or more generally. I'm just worried about someone doing KMS w/ sha512 + rekor upload

The spec for all of cosign: https://github.com/sigstore/cosign/blob/main/specs/SIGNATURE_SPEC.md

@asraa
Copy link
Contributor Author

asraa commented Nov 19, 2021

The spec for all of cosign: https://github.com/sigstore/cosign/blob/main/specs/SIGNATURE_SPEC.md

doh right, thanks! yeah i suppose working out the issues for sign-blobs with KMS can come later

go.mod Outdated Show resolved Hide resolved
@dlorenc
Copy link
Member

dlorenc commented Nov 23, 2021

We should get this one in before v1.4.0 as well!

@asraa
Copy link
Contributor Author

asraa commented Nov 29, 2021

QQ about this: while this change happens, should cosign look for both the rekord proposed entry and, if it doesn't exist, the hashedrekord proposed entry? i think it's okay, because two sigs are always gonna be unique because of randomization, so one or the other should exist, not both. (I suppose it's possible for someone to hand craft a rekord and hashedrekord with the same exact signature, but then it shouldn't matter unless we accidentally serve a different form of the public key, e.g. the x509 cert vs the extracted public key that misses some client verification)

@dlorenc
Copy link
Member

dlorenc commented Nov 29, 2021

QQ about this: while this change happens, should cosign look for both the rekord proposed entry and, if it doesn't exist, the hashedrekord proposed entry? i think it's okay, because two sigs are always gonna be unique because of randomization, so one or the other should exist, not both. (I suppose it's possible for someone to hand craft a rekord and hashedrekord with the same exact signature, but then it shouldn't matter unless we accidentally serve a different form of the public key, e.g. the x509 cert vs the extracted public key that misses some client verification)

Just using the new format seems fine, most lookups go through the offline bundle verification anyway.

@asraa
Copy link
Contributor Author

asraa commented Nov 29, 2021

Done! Updated the offline bundle verification to unmarshal either or. Tested both container/blob verification

https://gist.github.com/asraa/c2ef0bd3aacb7073b1e0736c0f9fd557

@@ -469,20 +471,32 @@ func bundleHash(bundleBody, signature string) (string, string, error) {
return *intotoObj.Content.Hash.Algorithm, *intotoObj.Content.Hash.Value, nil
}

err = json.Unmarshal(bodyDecoded, &rekord)
if err := json.Unmarshal(bodyDecoded, &rekord); err == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you double check this actually fails? Sometimes unmarshal will succeed even if it's the wrong type. We might need to get a bit more clever here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! can confirm that for a hashed rekord signature, it actually travels the second path and doesn't unmarshal to a rekord without an error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just checked how it errors: errors out on "invalid kind value" (rekor checks to make sure that the type we want to unmarshal to is the kind in the JSON)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@dlorenc dlorenc merged commit 9555f33 into sigstore:main Nov 29, 2021
@github-actions github-actions bot added this to the v1.4.0 milestone Nov 29, 2021
@dlorenc dlorenc mentioned this pull request Nov 29, 2021
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.

504 Gateway timeout on large file
2 participants