-
Notifications
You must be signed in to change notification settings - Fork 21
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 Fulcio support to signing #158
Conversation
This involved a bit of a refactor to support MessageSignature and DSSE bundle content. Also, we should probably start adding tests soon. Signed-off-by: Zach Steindler <steiza@github.com>
Signed-off-by: Zach Steindler <steiza@github.com>
This PR is kind of large, and it's likely it'll grow with some more testing. If it would help reviewing, I could separate out the Fulcio addition and the bundle content supporting Message Signature and DSSE Envelope - let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with the size and content of the PR! Was easy to read through and I think tests will be straightforward.
The main comment to discuss is the one about Sign
being responsible for both certificate requesting and artifact signing, lemme know your thoughts. I don't think it's a significant change from this PR, mostly about providing an interface to request certificates as an optional step of signing.
Signed-off-by: Zach Steindler <steiza@github.com>
`Content` now clearly owns generating and remembering the hash digest. `Keypair` communicates the hash algorithm to `Content`, and generates the signature. Signed-off-by: Zach Steindler <steiza@github.com>
Signed-off-by: Zach Steindler <steiza@github.com>
Signed-off-by: Zach Steindler <steiza@github.com>
Signed-off-by: Zach Steindler <steiza@github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the changes, this looks great. I have some thoughts on the top level sign.Bundle
method, but I think it'll naturally evolve as we continue adding features. Just a couple last comments.
Signed-off-by: Zach Steindler <steiza@github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for all the discussion here! @kommendorkapten any other thoughts?
I think this looks good for an incremental PR! I have two comments I would like to see answered before merging. |
…gning Signed-off-by: Zach Steindler <steiza@github.com>
Nice work @steiza ! |
Summary
Continuing to work towards #136 with small / incremental diffs.
This involved a bit of a refactor to support MessageSignature and DSSE bundle content.
Also, we should probably start adding signing tests soon.
Release Note
NONE - still no Rekor support or testing via conformance tests or integration tests with mocks
Documentation
NONE