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

update rekor intoto/0.0.2 entry hash calculation #151

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

bdehamer
Copy link
Collaborator

@bdehamer bdehamer commented Nov 3, 2022

Signed-off-by: Brian DeHamer bdehamer@github.com

Summary

Updates the logic to properly calculate the digest of a DSSE envelope when creating an intoto/0.0.2 style entry in Rekor.

Signed-off-by: Brian DeHamer <bdehamer@github.com>
@bdehamer bdehamer requested a review from a team as a code owner November 3, 2022 00:42
@bdehamer
Copy link
Collaborator Author

bdehamer commented Nov 3, 2022

/cc @codysoyland

// * payload is base64 encoded
// * signature is base64 encoded (only the first signature is used)
// * keyid is included ONLY if it is NOT an empty string
// * The resulting JSON is canonicalized and hashed to a hex string
Copy link
Member

Choose a reason for hiding this comment

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

Yikes!

Copy link
Member

Choose a reason for hiding this comment

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

Is it worth writing some kind of tests around this behaviour to make sure we don't deviate from it in future?

Copy link
Member

Choose a reason for hiding this comment

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

Do we have some tests in src/tlog/verify.test.ts that create different kinds of envelopes with and without a keyid? Maybe with multiple sigs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@feelepxyz I added some more tests around the DSSE hash calculation to ensure that we don't unintentionally alter it.

Copy link
Member

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

Gnarly.. nice work figuring this out! Just had one comment around tests, otherwise LGTM 👍

codysoyland
codysoyland previously approved these changes Nov 3, 2022
Copy link
Member

@codysoyland codysoyland left a comment

Choose a reason for hiding this comment

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

🎉

Signed-off-by: Brian DeHamer <bdehamer@github.com>
Copy link
Member

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests!

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.

3 participants