Skip to content

Add support for ED25519ph for sign/verify(-blob) commands #3479

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Add support for ED25519ph for sign/verify(-blob) commands #3479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f0717d
Used LoadOptions for more flexibility
ret2libc Jan 17, 2024
260978c
Attempt verification with ED25519 if ED25519ph is used for compatibility
ret2libc Jan 19, 2024
1046eea
Use a diffrent Signer when interacting with Fulcio
ret2libc Jan 30, 2024
e01c68c
Use ED25519ph only when uploading to TLog
ret2libc Feb 29, 2024
83c0cb5
Merge remote-tracking branch 'origin/main' into support-ed25519ph
ret2libc Jan 20, 2025
0ae10be
Merge remote-tracking branch 'origin/main' into support-ed25519ph
ret2libc Feb 6, 2025
7a48924
update rekor code
ret2libc Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cmd/cosign/cli/fulcio/fulcio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ type Signer struct {
signature.SignerVerifier
}

func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerVerifier) (*Signer, error) {
func NewSignerWithAdapter(ctx context.Context, ko options.KeyOpts, signer signature.SignerVerifier, fulcioSigner signature.SignerVerifier) (*Signer, error) {
fClient, err := NewClient(ko.FulcioURL)
if err != nil {
return nil, fmt.Errorf("creating Fulcio client: %w", err)
Expand Down Expand Up @@ -167,7 +167,7 @@ func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerV
}
flow = flowNormal
}
Resp, err := GetCert(ctx, signer, idToken, flow, ko.OIDCIssuer, ko.OIDCClientID, ko.OIDCClientSecret, ko.OIDCRedirectURL, fClient) // TODO, use the chain.
Resp, err := GetCert(ctx, fulcioSigner, idToken, flow, ko.OIDCIssuer, ko.OIDCClientID, ko.OIDCClientSecret, ko.OIDCRedirectURL, fClient) // TODO, use the chain.
if err != nil {
return nil, fmt.Errorf("retrieving cert: %w", err)
}
Expand All @@ -182,6 +182,10 @@ func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerV
return f, nil
}

func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerVerifier) (*Signer, error) {
return NewSignerWithAdapter(ctx, ko, signer, signer)
}

func (f *Signer) PublicKey(opts ...signature.PublicKeyOption) (crypto.PublicKey, error) { //nolint: revive
return f.SignerVerifier.PublicKey()
}
Expand Down
8 changes: 6 additions & 2 deletions cmd/cosign/cli/fulcio/fulcioverifier/fulcioverifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ import (
"github.com/sigstore/sigstore/pkg/signature"
)

func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerVerifier) (*fulcio.Signer, error) {
fs, err := fulcio.NewSigner(ctx, ko, signer)
func NewSignerWithAdapter(ctx context.Context, ko options.KeyOpts, signer signature.SignerVerifier, fulcioSigner signature.SignerVerifier) (*fulcio.Signer, error) {
fs, err := fulcio.NewSignerWithAdapter(ctx, ko, signer, fulcioSigner)
if err != nil {
return nil, err
}
Expand All @@ -46,3 +46,7 @@ func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerV

return fs, nil
}

func NewSigner(ctx context.Context, ko options.KeyOpts, signer signature.SignerVerifier) (*fulcio.Signer, error) {
return NewSignerWithAdapter(ctx, ko, signer, signer)
}
62 changes: 51 additions & 11 deletions cmd/cosign/cli/sign/sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ func SignCmd(ro *options.RootOptions, ko options.KeyOpts, signOpts options.SignO
ctx, cancel := context.WithTimeout(context.Background(), ro.Timeout)
defer cancel()

sv, err := SignerFromKeyOpts(ctx, signOpts.Cert, signOpts.CertChain, ko)
svOptions := []signature.LoadOption{
signatureoptions.WithHash(crypto.SHA256),
signatureoptions.WithED25519ph(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented!

}

sv, err := signerFromKeyOptsWithSVOpts(ctx, signOpts.Cert, signOpts.CertChain, ko, svOptions...)
if err != nil {
return fmt.Errorf("getting signer: %w", err)
}
Expand Down Expand Up @@ -261,7 +266,12 @@ func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko opti
if err != nil {
return err
}
s = irekor.NewSigner(s, rClient)

hashAlgorithm, err := getHashAlgorithmFromSignerVerifier(sv)
if err != nil {
return err
}
s = irekor.NewSigner(s, rClient, hashAlgorithm)
}

ociSig, _, err := s.Sign(ctx, bytes.NewReader(payload))
Expand Down Expand Up @@ -391,8 +401,8 @@ func signerFromSecurityKey(ctx context.Context, keySlot string) (*SignerVerifier
}, nil
}

func signerFromKeyRef(ctx context.Context, certPath, certChainPath, keyRef string, passFunc cosign.PassFunc) (*SignerVerifier, error) {
k, err := sigs.SignerVerifierFromKeyRef(ctx, keyRef, passFunc)
func signerFromKeyRef(ctx context.Context, certPath, certChainPath, keyRef string, passFunc cosign.PassFunc, opts ...signature.LoadOption) (*SignerVerifier, error) {
k, err := sigs.SignerVerifierFromKeyRefWithOpts(ctx, keyRef, passFunc, opts...)
if err != nil {
return nil, fmt.Errorf("reading key: %w", err)
}
Expand Down Expand Up @@ -521,12 +531,12 @@ func signerFromKeyRef(ctx context.Context, certPath, certChainPath, keyRef strin
return certSigner, nil
}

func signerFromNewKey() (*SignerVerifier, error) {
func signerFromNewKey(svOpts ...signature.LoadOption) (*SignerVerifier, error) {
privKey, err := cosign.GeneratePrivateKey()
if err != nil {
return nil, fmt.Errorf("generating cert: %w", err)
}
sv, err := signature.LoadECDSASignerVerifier(privKey, crypto.SHA256)
sv, err := signature.LoadSignerVerifierWithOpts(privKey, svOpts...)
if err != nil {
return nil, err
}
Expand All @@ -542,12 +552,17 @@ func keylessSigner(ctx context.Context, ko options.KeyOpts, sv *SignerVerifier)
err error
)

fulcioSV, err := adaptSignerVerifierToFulcio(sv)
if err != nil {
return nil, fmt.Errorf("adapting signer verifier to Fulcio: %w", err)
}

if ko.InsecureSkipFulcioVerify {
if k, err = fulcio.NewSigner(ctx, ko, sv); err != nil {
if k, err = fulcio.NewSignerWithAdapter(ctx, ko, sv, fulcioSV); err != nil {
return nil, fmt.Errorf("getting key from Fulcio: %w", err)
}
} else {
if k, err = fulcioverifier.NewSigner(ctx, ko, sv); err != nil {
if k, err = fulcioverifier.NewSignerWithAdapter(ctx, ko, sv, fulcioSV); err != nil {
return nil, fmt.Errorf("getting key from Fulcio: %w", err)
}
}
Expand All @@ -559,19 +574,19 @@ func keylessSigner(ctx context.Context, ko options.KeyOpts, sv *SignerVerifier)
}, nil
}

func SignerFromKeyOpts(ctx context.Context, certPath string, certChainPath string, ko options.KeyOpts) (*SignerVerifier, error) {
func signerFromKeyOptsWithSVOpts(ctx context.Context, certPath string, certChainPath string, ko options.KeyOpts, svOpts ...signature.LoadOption) (*SignerVerifier, error) {
var sv *SignerVerifier
var err error
genKey := false
switch {
case ko.Sk:
sv, err = signerFromSecurityKey(ctx, ko.Slot)
case ko.KeyRef != "":
sv, err = signerFromKeyRef(ctx, certPath, certChainPath, ko.KeyRef, ko.PassFunc)
sv, err = signerFromKeyRef(ctx, certPath, certChainPath, ko.KeyRef, ko.PassFunc, svOpts...)
default:
genKey = true
ui.Infof(ctx, "Generating ephemeral keys...")
sv, err = signerFromNewKey()
sv, err = signerFromNewKey(svOpts...)
}
if err != nil {
return nil, err
Expand All @@ -584,6 +599,10 @@ func SignerFromKeyOpts(ctx context.Context, certPath string, certChainPath strin
return sv, nil
}

func SignerFromKeyOpts(ctx context.Context, certPath string, certChainPath string, ko options.KeyOpts) (*SignerVerifier, error) {
return signerFromKeyOptsWithSVOpts(ctx, certPath, certChainPath, ko)
}

type SignerVerifier struct {
Cert []byte
Chain []byte
Expand All @@ -610,6 +629,27 @@ func (c *SignerVerifier) Bytes(ctx context.Context) ([]byte, error) {
return pemBytes, nil
}

// adaptSignerVerifierToFulcio adapts, if necessary, the SignerVerifier to be used to interact with Fulcio.
//
// This is needed in particular for ED25519 keys with the pre-hashed version of
// the algorithm, which is not supported by Fulcio. This function creates a
// ED25519 SignerVerifier based on that instead.
func adaptSignerVerifierToFulcio(sv *SignerVerifier) (*SignerVerifier, error) {
if ed25519phSV, ok := sv.SignerVerifier.(*signature.ED25519phSignerVerifier); ok {
signerVerifier, err := ed25519phSV.ToED25519SignerVerifier()
if err != nil {
return nil, err
}

return &SignerVerifier{
SignerVerifier: signerVerifier,
Cert: sv.Cert,
Chain: sv.Chain,
}, nil
}
return sv, nil
}

func fetchLocalSignedPayload(sig oci.Signature) (*cosign.LocalSignedPayload, error) {
signedPayload := &cosign.LocalSignedPayload{}
var err error
Expand Down
57 changes: 48 additions & 9 deletions cmd/cosign/cli/sign/sign_blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ package sign

import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rsa"
"crypto/sha256"
"crypto/x509"
"encoding/base64"
Expand All @@ -39,9 +43,28 @@ import (
protocommon "github.com/sigstore/protobuf-specs/gen/pb-go/common/v1"
"github.com/sigstore/rekor/pkg/generated/models"
"github.com/sigstore/sigstore/pkg/cryptoutils"
"github.com/sigstore/sigstore/pkg/signature"
signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree, there should be more flexibility here.

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")
}
}
Comment on lines +50 to +66
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this would be solved with sigstore/sigstore#860 . Shall I add a reference to this issue in the code (and back-ref)?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't sigstore/sigstore#1426 just about the KMS SignerVerifier? I think we'd need that HashFunc feature at the sigstore.SignerVerifier level.


// nolint
func SignBlobCmd(ro *options.RootOptions, ko options.KeyOpts, payloadPath string, b64 bool, outputSignature string, outputCertificate string, tlogUpload bool) ([]byte, error) {
var payload internal.HashReader
Expand All @@ -50,27 +73,43 @@ func SignBlobCmd(ro *options.RootOptions, ko options.KeyOpts, payloadPath string
ctx, cancel := context.WithTimeout(context.Background(), ro.Timeout)
defer cancel()

svOptions := []signature.LoadOption{
signatureoptions.WithHash(crypto.SHA256),
}
// Use ED25519 pre-hashed version only when uploading to tlog to maintain
// backwards compatibility. When self-managed keys are used this keeps the
// behavior consistent with older cosign clients, which will still be able
// to verify the newer signatures.
if tlogUpload {
svOptions = append(svOptions, signatureoptions.WithED25519ph())
}

sv, err := signerFromKeyOptsWithSVOpts(ctx, "", "", ko, svOptions...)
if err != nil {
return nil, err
}
defer sv.Close()

hashAlgorithm, err := getHashAlgorithmFromSignerVerifier(sv)
if err != nil {
return nil, err
}

if payloadPath == "-" {
payload = internal.NewHashReader(os.Stdin, sha256.New())
payload = internal.NewHashReader(os.Stdin, hashAlgorithm)
} else {
ui.Infof(ctx, "Using payload from: %s", payloadPath)
f, err := os.Open(filepath.Clean(payloadPath))
defer f.Close()
if err != nil {
return nil, err
}
payload = internal.NewHashReader(f, sha256.New())
payload = internal.NewHashReader(f, hashAlgorithm)
}
if err != nil {
return nil, err
}

sv, err := SignerFromKeyOpts(ctx, "", "", ko)
if err != nil {
return nil, err
}
defer sv.Close()

sig, err := sv.SignMessage(&payload, signatureoptions.WithContext(ctx))
if err != nil {
return nil, fmt.Errorf("signing blob: %w", err)
Expand Down Expand Up @@ -135,7 +174,7 @@ func SignBlobCmd(ro *options.RootOptions, ko options.KeyOpts, payloadPath string
if err != nil {
return nil, err
}
rekorEntry, err = cosign.TLogUpload(ctx, rekorClient, sig, &payload, rekorBytes)
rekorEntry, err = cosign.TLogUploadWithCustomHash(ctx, rekorClient, sig, &payload, rekorBytes)
if err != nil {
return nil, err
}
Expand Down
16 changes: 11 additions & 5 deletions cmd/cosign/cli/verify/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
sigs "github.com/sigstore/cosign/v2/pkg/signature"
"github.com/sigstore/sigstore/pkg/cryptoutils"
"github.com/sigstore/sigstore/pkg/signature"
signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
"github.com/sigstore/sigstore/pkg/signature/payload"
)

Expand Down Expand Up @@ -182,11 +183,16 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
}
}

svOpts := []signature.LoadOption{
signatureoptions.WithHash(crypto.SHA256),
signatureoptions.WithED25519ph(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the trial verification you mentioned in the comment, verifying both as ed25519 and ed25519ph?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

See https://github.com/sigstore/cosign/pull/3497/files#diff-fc9a26a4caa1d8684a4bdd9102f187f44e1f7fce7c06a19f4185035f13317cc8R232-R238 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

}

// Keys are optional!
var pubKey signature.Verifier
switch {
case keyRef != "":
pubKey, err = sigs.PublicKeyFromKeyRefWithHashAlgo(ctx, keyRef, c.HashAlgorithm)
pubKey, err = sigs.PublicKeyFromKeyRefWithOpts(ctx, keyRef, svOpts...)
if err != nil {
return fmt.Errorf("loading public key: %w", err)
}
Expand Down Expand Up @@ -220,7 +226,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
if err != nil {
return fmt.Errorf("getting Fulcio intermediates: %w", err)
}
pubKey, err = cosign.ValidateAndUnpackCert(cert, co)
pubKey, err = cosign.ValidateAndUnpackCertWithOpts(cert, co, cosign.WithSignerVerifierOptions(svOpts...))
if err != nil {
return err
}
Expand All @@ -230,7 +236,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
if err != nil {
return err
}
pubKey, err = cosign.ValidateAndUnpackCertWithChain(cert, chain, co)
pubKey, err = cosign.ValidateAndUnpackCertWithOpts(cert, co, cosign.WithChain(chain), cosign.WithSignerVerifierOptions(svOpts...))
if err != nil {
return err
}
Expand Down Expand Up @@ -267,7 +273,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {

for _, img := range images {
if c.LocalImage {
verified, bundleVerified, err := cosign.VerifyLocalImageSignatures(ctx, img, co)
verified, bundleVerified, err := cosign.VerifyLocalImageSignaturesWithOpts(ctx, img, co, svOpts...)
if err != nil {
return err
}
Expand All @@ -283,7 +289,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
return fmt.Errorf("resolving attachment type %s for image %s: %w", c.Attachment, img, err)
}

verified, bundleVerified, err := cosign.VerifyImageSignatures(ctx, ref, co)
verified, bundleVerified, err := cosign.VerifyImageSignaturesWithOpts(ctx, ref, co, svOpts...)
if err != nil {
return cosignError.WrapError(err)
}
Expand Down
13 changes: 10 additions & 3 deletions cmd/cosign/cli/verify/verify_blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import (
sigs "github.com/sigstore/cosign/v2/pkg/signature"

"github.com/sigstore/sigstore/pkg/cryptoutils"
"github.com/sigstore/sigstore/pkg/signature"
signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
)

func isb64(data []byte) bool {
Expand Down Expand Up @@ -166,10 +168,15 @@ func (c *VerifyBlobCmd) Exec(ctx context.Context, blobRef string) error {
}
}

svOpts := []signature.LoadOption{
signatureoptions.WithHash(crypto.SHA256),
signatureoptions.WithED25519ph(),
}

// Keys are optional!
switch {
case c.KeyRef != "":
co.SigVerifier, err = sigs.PublicKeyFromKeyRef(ctx, c.KeyRef)
co.SigVerifier, err = sigs.PublicKeyFromKeyRefWithOpts(ctx, c.KeyRef, svOpts...)
if err != nil {
return fmt.Errorf("loading public key: %w", err)
}
Expand Down Expand Up @@ -214,7 +221,7 @@ func (c *VerifyBlobCmd) Exec(ctx context.Context, blobRef string) error {
bundleCert, err := loadCertFromPEM(certBytes)
if err != nil {
// check if cert is actually a public key
co.SigVerifier, err = sigs.LoadPublicKeyRaw(certBytes, crypto.SHA256)
co.SigVerifier, err = sigs.LoadPublicKeyRawWithOpts(certBytes, svOpts...)
if err != nil {
return fmt.Errorf("loading verifier from bundle: %w", err)
}
Expand Down Expand Up @@ -297,7 +304,7 @@ func (c *VerifyBlobCmd) Exec(ctx context.Context, blobRef string) error {
if err != nil {
return err
}
if _, err = cosign.VerifyBlobSignature(ctx, signature, co); err != nil {
if _, err = cosign.VerifyBlobSignatureWithOpts(ctx, signature, co, svOpts...); err != nil {
return err
}

Expand Down
Loading
Loading