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

Support for multi-subject attestations using different hash algorithms #361

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

codysoyland
Copy link
Member

Summary

Support for attestations with multiple subjects using different hash algorithms.

#360

Release Note

Documentation

@codysoyland
Copy link
Member Author

Currently, this PR is a draft containing only a multi-digest hashing tool.

Signed-off-by: Cody Soyland <codysoyland@github.com>
…sfy subject discovery in multi-subject attestations

Signed-off-by: Cody Soyland <codysoyland@github.com>
pkg/verify/signature.go Outdated Show resolved Hide resolved
Signed-off-by: Cody Soyland <codysoyland@github.com>
Signed-off-by: Cody Soyland <codysoyland@github.com>
Signed-off-by: Cody Soyland <codysoyland@github.com>
@codysoyland codysoyland marked this pull request as ready for review December 20, 2024 16:47
@codysoyland codysoyland requested a review from a team as a code owner December 20, 2024 16:47
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Had some suggestions for performance improvements to minimize iterations over the statement and compute fewer digests.

// go through the statement and make a simple data structure to hold the
// list of hash funcs for each subject (subjectHashFuncs)
for i, subject := range statement.Subject {
for alg := range subject.Digest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we err out if digest is empty? This is the current behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Interpreting the spec to say the subject is invalid if the digest is the empty string seems pretty defensible to me / 👍 to erroring out on empty digests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a little iffy on this, because verification may still pass if there exists a subject with the given digest, even if a subject exists that does not contain a digest. I'm not sure it's the responsibility of the verifier to make sure every subject contains a digest.

return nil, errors.New("no subjects found in statement")
}

supportedHashFuncs := []crypto.Hash{crypto.SHA512, crypto.SHA384, crypto.SHA256}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Slices are mutable and therefore cannot be declared as constants.

pkg/verify/signature.go Outdated Show resolved Hide resolved
pkg/verify/signature.go Show resolved Hide resolved

// Look for artifact digest in statement
for _, subject := range statement.Subject {
for alg, digest := range subject.Digest {
hexdigest, err := hex.DecodeString(digest)
for alg, hexdigest := range subject.Digest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another idea for an optimization is to create the mapping between subject and the algorithms and expected digests in one pass. With this proposed solution, we make two passes, one to build the digest algorithms and one to verify the digests, which would be a performance regression from the previous solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that would require shifting a lot of logic into the already tested getHashFunctions (if I understand correctly), I'm not too concerned about the performance penalty of iterating through the digests here again. I do think it's worth revisiting when/if we implement #363.

Co-authored-by: Hayden B <hblauzvern@google.com>
Signed-off-by: Cody Soyland <codysoyland@github.com>
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.

4 participants