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

Add --signing-algorithm flag #3497

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ret2libc
Copy link

Summary

Give the user the option to choose which signing algorithm to use when generating keypairs (#3271).

Code based on #3479 .

Release Note

Documentation

Comment on lines 141 to 153
// Always default to ECDSA_SHA2_256_NISTP256 for now
if ko.SigningAlgorithm == pb_go_v1.KnownSignatureAlgorithm_KNOWN_SIGNATURE_ALGORITHM_UNSPECIFIED {
ko.SigningAlgorithm = pb_go_v1.KnownSignatureAlgorithm_ECDSA_SHA2_256_NISTP256
}

if !cosign.ClientAlgorithmsRegistry.HasAlgorithmDetails(ko.SigningAlgorithm) {
return fmt.Errorf("unsupported signing algorithm: %s", ko.SigningAlgorithm.String())
}

algorithmDetails, err := cosign.ClientAlgorithmsRegistry.GetAlgorithmDetails(ko.SigningAlgorithm)
if err != nil {
return err
}
Copy link
Author

Choose a reason for hiding this comment

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

This does not make too much sense when the user specifies --key. What should be the behaviour in that case? Right now a user can even specifies a e.g. RSA key with --key and this check would not detect it (because the signing algorithm defaults to ECDSA_SHA2_256_NISTP256).

How do we want cosign to behave? Shall the registry block also --key algorithms? If so, we should probably add RSA to the list of accepted client signing algorithms as that is already valid in the current state.

@ret2libc ret2libc closed this Jan 23, 2024
@ret2libc ret2libc reopened this Jan 23, 2024
@ret2libc
Copy link
Author

Shall we add the --signing-algorithm to the verify/verify-blob commands as well?

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.

Do we need a client algorithm registry for the sign path? There are roughly three places the client interacts with a key: Generation, signing, and verification.

For generation, it makes sense that the client specify which algorithms are supported for the generated key. This can be for both ephemeral and long-lived key generation.

For the verification path, supported algorithms could be a part of the verification policy, so that makes sense to allow a user to specify a set of trusted algorithms.

For signing, I'm not sure it's needed. When a key is provided, the user is specifying that's the key they want to use (whether it was generated ephemerally or self-managed). The backend (fulcio or rekor) could choose to reject it, which will be surfaced as a response error.

* Use ED25519ph algorithm with sign/verify-blob commands

Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
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.

Overall LGTM, I'll need to do a deep dive once this is out of draft but overall this seems solid. Can we add e2e tests that exercise generation, signing and verification?

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.

None yet

2 participants