Add support for ED25519ph for sign/verify(-blob) commands #3479
Conversation
|
Will review once 1595 is agreed upon and merged |
ret2libc
force-pushed
the
support-ed25519ph
branch
from
976d483
to
4418d76
Compare
ret2libc
force-pushed
the
support-ed25519ph
branch
from
4418d76
to
95dff84
Compare
| func getHashAlgorithmFromSignerVerifier(sv *SignerVerifier) (crypto.Hash, error) { | ||
| publicKey, err := sv.SignerVerifier.PublicKey() | ||
| if err != nil { | ||
| return crypto.Hash(0), err | ||
| } | ||
|
|
||
| switch publicKey.(type) { | ||
| case *ecdsa.PublicKey: | ||
| return crypto.SHA256, nil | ||
| case *rsa.PublicKey: | ||
| return crypto.SHA256, nil | ||
| case ed25519.PublicKey: | ||
| return crypto.SHA512, nil | ||
| default: | ||
| return crypto.Hash(0), fmt.Errorf("unsupported public key type") | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe this would be solved with sigstore/sigstore#860 . Shall I add a reference to this issue in the code (and back-ref)?
There was a problem hiding this comment.
Would you be interested in taking a look at the failing test in sigstore/sigstore#1426? I think it's just a failure due to a mock not having an expectation met. We could get that merged in then to use here.
There was a problem hiding this comment.
Isn't sigstore/sigstore#1426 just about the KMS SignerVerifier? I think we'd need that HashFunc feature at the sigstore.SignerVerifier level.
ret2libc
changed the title
|
@haydentherapper good call on the rekord vs hashedrekord issue! Currently there are problems indeed and with this change we can't verify rekord types with ed25519 keys The reason is that ed25519ph is assumed in all sign-blob/verify-blob paths. I'm going to try and fix it. |
I'm not 100% confident here, but at least in the bundle case you should be able to decode |
|
What do you think about this solution? diff --git a/pkg/cosign/verify.go b/pkg/cosign/verify.go
index b7d15e2c..17627188 100644
--- a/pkg/cosign/verify.go
+++ b/pkg/cosign/verify.go
@@ -19,6 +19,7 @@ import (
"context"
"crypto"
"crypto/ecdsa"
+ "crypto/ed25519"
"crypto/sha256"
"crypto/sha512"
"crypto/x509"
@@ -233,7 +234,7 @@ func verifyOCISignature(ctx context.Context, verifier signature.Verifier, sig pa
if err != nil {
return err
}
- signature, err := base64.StdEncoding.DecodeString(b64sig)
+ decodedSignature, err := base64.StdEncoding.DecodeString(b64sig)
if err != nil {
return err
}
@@ -241,7 +242,33 @@ func verifyOCISignature(ctx context.Context, verifier signature.Verifier, sig pa
if err != nil {
return err
}
- return verifier.VerifySignature(bytes.NewReader(signature), bytes.NewReader(payload), options.WithContext(ctx))
+ // For compatibility reasons, if ED25519ph is used, we try both ED25519 and ED25519ph.
+ // Refusing to verify ED25519 signatures (used e.g. by rekord entries) would break compatibility.
+ // The signature algorithm to use should be uniquely determined before this point.
+ verificationErr := verifier.VerifySignature(bytes.NewReader(decodedSignature), bytes.NewReader(payload), options.WithContext(ctx))
+ if verificationErr == nil {
+ return nil
+ }
+
+ switch verifier.(type) {
+ case *signature.ED25519phVerifier:
+ publicKey, err := verifier.PublicKey()
+ if err != nil {
+ return err
+ }
+
+ if edPublicKey, ok := publicKey.(ed25519.PublicKey); ok {
+ altVerifier, err := signature.LoadED25519Verifier(edPublicKey)
+ if err != nil {
+ return err
+ }
+
+ fmt.Fprintf(os.Stderr, "Failed to verify signature with ED25519ph, falling back to ED25519 for backward compatibility.\n")
+ verificationErr = altVerifier.VerifySignature(bytes.NewReader(decodedSignature), bytes.NewReader(payload), options.WithContext(ctx))
+ }
+ }
+
+ return verificationErr
}
|
Trial verification is nasty, but I think it's sound in this case (the classic examples of it being unsound are symmetric/asymmetric confusion, like in JWTs). So I'm not a huge fan of it, but it's probably an acceptable tradeoff vs. a backwards-incompatible change. CCing @haydentherapper for thoughts as well. |
woodruffw
force-pushed
the
support-ed25519ph
branch
from
8da076a
to
6e7d374
Compare
|
I'm good with this approach to attempt verification twice. The other option would be plumbing through which rekor type is being verified, and only use the -ph variant for hashedrekord. I suspect that's gonna be messy though. |
|
Will take a closer look Monday, but overall LGTM |
ret2libc
force-pushed
the
support-ed25519ph
branch
from
afdad84
to
a8fd1d7
Compare
ret2libc
force-pushed
the
support-ed25519ph
branch
4 times, most recently
from
a8367c9
to
0928190
Compare
|
This is also blocked by Fulcio support, correct? Either we'll need to be OK with Fulcio certifying ed25519ph keys as ed25519 (my preference), or fork x509.go edit: sorry, ignore part of this: fulcio is not a blocker assuming we are good with fulcio certifying ed25519ph keys as ed25519 |
The only real problem for Fulcio is the hash function at this point. See https://github.com/sigstore/fulcio/pull/1517/files#diff-648d47fb9eeb444c1a09095dd41e4012ee5aafcb37b712f7f3bf492d8410017dR149 . Cosign(and various clients) should be changed to use Pure Ed25519 when Ed25519ph (or other Ed25519 variants) is used and it interacts with Fulcio. |
ret2libc
force-pushed
the
support-ed25519ph
branch
from
0928190
to
0bf4bce
Compare
| func getHashAlgorithmFromSignerVerifier(sv *SignerVerifier) (crypto.Hash, error) { | ||
| publicKey, err := sv.SignerVerifier.PublicKey() | ||
| if err != nil { | ||
| return crypto.Hash(0), err | ||
| } | ||
|
|
||
| switch publicKey.(type) { | ||
| case *ecdsa.PublicKey: | ||
| return crypto.SHA256, nil | ||
| case *rsa.PublicKey: | ||
| return crypto.SHA256, nil | ||
| case ed25519.PublicKey: | ||
| return crypto.SHA512, nil | ||
| default: | ||
| return crypto.Hash(0), fmt.Errorf("unsupported public key type") | ||
| } | ||
| } |
There was a problem hiding this comment.
Would you be interested in taking a look at the failing test in sigstore/sigstore#1426? I think it's just a failure due to a mock not having an expectation met. We could get that merged in then to use here.
|
|
||
| switch publicKey.(type) { | ||
| case *ecdsa.PublicKey: | ||
| return crypto.SHA256, nil |
There was a problem hiding this comment.
Maybe for a later PR to keep this smaller in scope - Should this be based on curve too? This would be a change from current behavior, since I believe we're using sha256 regardless of curve. This might be an unexpected change for custom verifiers though who likely have always assumed sha256.
There was a problem hiding this comment.
Yes I agree, there should be more flexibility here.
| @@ -214,11 +215,16 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) { | |||
| } | |||
| } | |||
|
|
|||
| svOpts := []signature.LoadOption{ | |||
| signatureoptions.WithHash(crypto.SHA256), | |||
| signatureoptions.WithED25519ph(), | |||
There was a problem hiding this comment.
Do we need the trial verification you mentioned in the comment, verifying both as ed25519 and ed25519ph?
There was a problem hiding this comment.
At this stage, yes, I think that's needed.
However, once we add support for --signing-algorithm flag, the user can specific the algorithm he wants and we can add or not this option.
There was a problem hiding this comment.
I realize now that you might be saying there is no trial verification in the PR, but this is already implemented in https://github.com/sigstore/cosign/pull/3479/files#diff-8a85c8e688d61e16b8af8e09832ed2bef89c1163b0e9601a8363c782c387c006R272-R298 . Was that what you were referring to or did I misinterpret your messages?
| sv, err := SignerFromKeyOpts(ctx, signOpts.Cert, signOpts.CertChain, ko) | ||
| svOptions := []signature.LoadOption{ | ||
| signatureoptions.WithHash(crypto.SHA256), | ||
| signatureoptions.WithED25519ph(), |
There was a problem hiding this comment.
To confirm, this would mean that self-managed ed25519 keys would now generate signatures with the pre-hashed variant, correct? This would mean that a verifier using an older Cosign version could not verify a signature from the latest Cosign version. Could we set this only when tlogupload (or the equivalent config) is true? This preserves the same signing behavior then for self-managed keys with sigs not being uploaded to Rekor.
|
I believe this is missing the trial verification and the last comment about limiting when the key is the prehash variant? |
* 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>
ED25519-ph is not widely supported and it is not an accepted option in x509 Certificates/CSR, so Fulcio does not accept them. Instead, clients are supposed to use PureED25519 when interacting with Fulcio. This commit provides to the Fulcio code a separate SignerVerifier created from the one loaded from the private key. This SignerVerifier is usually of the same type, except when dealing with ED25519ph. Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
ret2libc
force-pushed
the
support-ed25519ph
branch
from
1f5a737
to
19222ef
Compare
Signed-off-by: Riccardo Schirone <562321+ret2libc@users.noreply.github.com>
ret2libc
force-pushed
the
support-ed25519ph
branch
from
19222ef
to
e01c68c
Compare
Summary
Integrate sigstore/sigstore#1595 into cosign, allowing sign-blob/verify-blob commmands to use ED25519ph as necessary.
This needs both sigstore/rekor#1959 and sigstore/rekor#1945
How I tested this:
Release Note
LoadPrivateKeyWithOpts,TLogUploadWithCustomHash,ValidateAndUnpackCertWithOpts,VerifierForKeyRefWithOpts,LoadPublicKeyRawWithOpts,SignerVerifierFromKeyRefWithOpts,PublicKeyFromKeyRefWithOptsfor passingsigstore.signature.SignerVerifierOptions aroundDocumentation