Support additionnal hash algorithms for artifact signing #181
Conversation
alexiscampan
force-pushed
the
feat/additionnal-key-types
branch
from
5e61cf1
to
abc2014
Compare
There was a problem hiding this comment.
Hey, thanks for the PR. The issue for log key types wasn’t about more hash algorithms for the log, it was to support different key algorithms. sha256 is fairly standard for log deployment at this point, and I’d prefer to not add support for other digests if it’s not easily possible to deploy such a log.
As for the second part of this change to add more hash algorithms for artifact signing, this looks good at a glance. Can you add tests for this too?
Signed-off-by: alexiscampan <alexis.campan@ut-capitole.fr>
alexiscampan
force-pushed
the
feat/additionnal-key-types
branch
from
abc2014
to
372c758
Compare
alexiscampan
changed the title
|
Hey @haydentherapper, thanks for the review. I've just added some tests for the artifact signing part and refined the scope of this PR. I'll do the support of new algorithms for the verify part in another PR. |
| @@ -117,6 +118,8 @@ func main() { | |||
| trustedRootPath = &os.Args[i+1] | |||
| case "--staging": | |||
| staging = true | |||
| case "--signing-algorithm": | |||
There was a problem hiding this comment.
We want the conformance command line options to match https://github.com/sigstore/sigstore-conformance/blob/main/docs/cli_protocol.md. If we decide to continue with this change, we could add this as an option to examples/signing/main.go (for signing) or cmd/sigstore-go/main.go (for verifying).
| Hint []byte | ||
| // TODO: support additional key algorithms | ||
| Hint []byte | ||
| HashAlgorithm protocommon.HashAlgorithm |
There was a problem hiding this comment.
So this was actually proposed before, and we decided not to allow hash algorithm options for ECDSA P256, because even if you specify a hash with more bits it will get truncated to match the key size.
I guess to take a step back, what problem are we trying to solve here? What use-case are we trying to enable?
There was a problem hiding this comment.
I'd like to see cryptographic agility. Agreed that specifying the hash here doesn't make sense.
Instead of HashAlgorithm, should we be taking in PublicKeyDetails - https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L62, and initializing both a signing algorithm and hash algorithm based on that.
There was a problem hiding this comment.
Ah, yeah, we can support some additional keys in https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L62, especially for key types and hashes we expect to be very common.
But I don't think that pkg/sign/keys.go should implement every possible combination. The point of the Keypair interface is so that sigstore-go doesn't have to implement every key type - you can provide your own implementation to that interface.
That's why I was curious what @alexiscampan's use-case is. If they were anticipating lots of clients needing to sign with a specific key type and hash, or if it would make more sense for them to implement their own Keypair when they use sigstore-go.
There was a problem hiding this comment.
That makes sense for KMS keys or other key interfaces that pull in dependencies or experimental algorithms, e.g. PQC. Or for signing where keys are managed, e.g on disk or HSMs.
For algorithms that are natively supported in Go and where keys are used in ephemeral signing, why not support them here too? The default of ECDSA-P256 is reasonable, but in the spirit of cryptographic agility (#74), I would like to add in a way to configure RSA, ed25519, and other elliptic curves. I recognize this is a case of YAGNI, but it's straightforward to add this support and removes a barrier for potential integrators while still directing them to the default, preferred Sigstore signing workflow.
There was a problem hiding this comment.
Ah, yeah, we can support some additional keys in https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L62, especially for key types and hashes we expect to be very common.
But I don't think that
pkg/sign/keys.goshould implement every possible combination. The point of theKeypairinterface is so thatsigstore-godoesn't have to implement every key type - you can provide your own implementation to that interface.That's why I was curious what @alexiscampan's use-case is. If they were anticipating lots of clients needing to sign with a specific key type and hash, or if it would make more sense for them to implement their own
Keypairwhen they usesigstore-go.
Hey @steiza, I have no particular use case for this specific feature. With my company we are going to need the signing part, we could use other librairies like the one in python to sign our images but all our applications are written in Go so we wanted to stay consistant. I also think that the default ECDSA-P256 is sufficient for our use case. I just wanted to do a first contribution before contributing on the signing part. I will close this PR for now as the priority of this feature is not high and we don't really know if it will be useful. I will try to give some support on the signing part which is the main feature of this library. Thanks for your fast review.
| Hint []byte | ||
| // TODO: support additional key algorithms | ||
| Hint []byte | ||
| HashAlgorithm protocommon.HashAlgorithm |
There was a problem hiding this comment.
I'd like to see cryptographic agility. Agreed that specifying the hash here doesn't make sense.
Instead of HashAlgorithm, should we be taking in PublicKeyDetails - https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L62, and initializing both a signing algorithm and hash algorithm based on that.
No description provided.