Remove signer hardcoding of SHA-256 #328

malancas · 2023-04-19T20:20:33Z

Summary

Closes #304

Currently when signers are created, SHA-256 is always used. A KMS based key may use a different supported algorithm, like SHA-384 or SHA-512, so this hardcoding should be removed.

Adds a new timestamp-server flag signer-hash-function that allows the user to specify a signer hash function. This is used in several different signer schemes
Adds two new fetch-tsa-certs flags, intermediate-hash-function and leaf-hash-function, that allow the user to specify the leaf and intermediate key hash functions
Updates the signer code to handle a specified hash function

Release Note

Documentation

Signed-off-by: Meredith Lancaster <malancas@github.com>

codecov · 2023-04-19T20:25:45Z

Codecov Report

Merging #328 (b41aa63) into main (c5ee8cb) will decrease coverage by 0.15%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
- Coverage   53.11%   52.96%   -0.15%     
==========================================
  Files          20       20              
  Lines        1188     1229      +41     
==========================================
+ Hits          631      651      +20     
- Misses        496      514      +18     
- Partials       61       64       +3

Impacted Files	Coverage Δ
pkg/signer/tink.go	`42.45% <50.00%> (-1.78%)`	⬇️
pkg/signer/signer.go	`39.58% <51.61%> (+16.85%)`	⬆️
pkg/signer/memory.go	`60.97% <53.84%> (-2.19%)`	⬇️
pkg/signer/file.go	`62.06% <100.00%> (+4.37%)`	⬆️
pkg/tests/server.go	`78.57% <100.00%> (+1.64%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas · 2023-04-19T20:27:57Z

pkg/signer/signer.go

+	}
+}
+
+func NewCryptoSigner(ctx context.Context, signer, kmsKey, tinkKmsKey, tinkKeysetPath, hcVaultToken, fileSignerPath, fileSignerPasswd, signerHashFunc string) (crypto.Signer, error) {


I think this function and related helper functions are ready for a refactor but given that this PR is already big, it can be tackled in a follow up.

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas · 2023-04-19T23:25:49Z

pkg/signer/tink.go

@@ -129,6 +129,9 @@ func KeyHandleToSigner(kh *keyset.Handle) (crypto.Signer, error) {
 		p.PublicKey.X, p.PublicKey.Y = c.ScalarBaseMult(privKey.GetKeyValue())
 		return p, nil
 	case ed25519SignerTypeURL:
+		if hashFunc != crypto.SHA512 {
+			fmt.Printf("Ed25519 only supports SHA512, specified hash func is %s. Using hash function specified by Ed25519\n", hashFunc)


Regarding how to handle a hash function that doesn't match the one used by Ed25519, I thought it may make sense to simply inform the user that the provided algorithm will be disregarded and move on with setup. The downside here is that we are passing in a hash function just to print this warning message but it could be addressed in a refactor in a follow up. Thoughts?

Given this runs only on setup, I'd prefer we err out rather than allow for misconfiguration.

haydentherapper

I think we actually need to fix this in the timestamp library, not here. sigstore/sigstore exposes a SignMessage interface that takes the hash passed into it, but that's only used by sigstore services. The crypto.Signer interface provides Sign which takes in a hash function in crypto.SignerOpts, but that's only to check that the digest provided matches the hash function expectations (like digest length). Sign is what's called in the pkcs7 library

The hash function is passed in the timestamp library here - https://github.com/digitorus/timestamp/blob/d542479a242518f315934e8d79c8e077ddbe4990/timestamp.go#L599, and you can see it's hardcoded. I don't think any changes are needed in the pkcs7 library, as it supports other hash algorithms already - https://github.com/digitorus/pkcs7/blob/master/sign.go#L150-L156

so tl;dr - https://github.com/digitorus/timestamp/blob/d542479a242518f315934e8d79c8e077ddbe4990/timestamp.go#L599 needs to be configurable

Leaving up the comments I've typed up already, but they're moot given the above.

haydentherapper · 2023-05-08T18:27:31Z

pkg/signer/signer.go

+		if err != nil {
+			return nil, err
+		}
+		signer, err := kms.Get(ctx, kmsKey, hashFunc)


From @codysoyland's other PR, we've realized that the kms provider just ignores the hash function, so either we can leave this in, or just keep a hardcoded hash with a comment that it's ignored.

haydentherapper · 2023-05-08T19:16:06Z

pkg/signer/file.go

 		if err != nil {
 			return nil, err
 		}
 		return &File{signer}, nil
 	case ed25519.PrivateKey:
+		if hashFunc != crypto.SHA512 {
+			fmt.Printf("Ed25519 only supports SHA512, specified hash func is %s. Using hash function specified by Ed25519\n", hashFunc)


Should we err out instead of printing?

haydentherapper · 2023-05-08T19:19:02Z

pkg/signer/memory.go

@@ -30,12 +30,25 @@ import (
 	tsx509 "github.com/sigstore/timestamp-authority/pkg/x509"
 )

+func getCurveFromSigner(signer crypto.Signer) (elliptic.Curve, error) {


memory signer hardcodes the signer's curve -

timestamp-authority/pkg/signer/signer.go

Line 42 in 403a0a1

sv, _, err := signature.NewECDSASignerVerifier(elliptic.P256(), rand.Reader, crypto.SHA256)

- so this isn't needed

I think this is reasonable, as it's only meant for testing.

haydentherapper · 2023-05-08T19:19:48Z

pkg/signer/signer.go

 	switch signer {
 	case MemoryScheme:
-		sv, _, err := signature.NewECDSASignerVerifier(elliptic.P256(), rand.Reader, crypto.SHA256)
+		hashFunc, curve, err := getHashFuncEllipticCurve(signerHashFunc)


I'd vote to keep this hardcoded, as it's only meant for testing and should need no additional configuration.

haydentherapper · 2023-05-08T19:22:01Z

pkg/signer/tink.go

@@ -129,6 +129,9 @@ func KeyHandleToSigner(kh *keyset.Handle) (crypto.Signer, error) {
 		p.PublicKey.X, p.PublicKey.Y = c.ScalarBaseMult(privKey.GetKeyValue())
 		return p, nil
 	case ed25519SignerTypeURL:
+		if hashFunc != crypto.SHA512 {
+			fmt.Printf("Ed25519 only supports SHA512, specified hash func is %s. Using hash function specified by Ed25519\n", hashFunc)


Given this runs only on setup, I'd prefer we err out rather than allow for misconfiguration.

haydentherapper · 2023-05-08T19:22:32Z

pkg/signer/tink.go

@@ -51,7 +51,7 @@ var (
 )

 // NewTinkSigner creates a signer by decrypting a local Tink keyset with a remote KMS encryption key
-func NewTinkSigner(ctx context.Context, tinkKeysetPath string, primaryKey tink.AEAD) (crypto.Signer, error) {
+func NewTinkSigner(ctx context.Context, tinkKeysetPath string, primaryKey tink.AEAD, hashFunc crypto.Hash) (crypto.Signer, error) {


Is hash func needed here? This only builds a signer

malancas · 2023-08-24T21:02:57Z

Closing as the hardcoding bug should be resolved by #452

malancas added 6 commits April 19, 2023 13:16

add support for specifying signer hash function

b0e0210

Signed-off-by: Meredith Lancaster <malancas@github.com>

add test for initializing signer with hash func

3fa31d3

Signed-off-by: Meredith Lancaster <malancas@github.com>

get curve from signer

59aa09b

Signed-off-by: Meredith Lancaster <malancas@github.com>

add handling for server arg hash func for file, tink signers

e04e2ab

Signed-off-by: Meredith Lancaster <malancas@github.com>

add args for intermediate, leaf hash funcs

42f40de

Signed-off-by: Meredith Lancaster <malancas@github.com>

add license

72b2412

Signed-off-by: Meredith Lancaster <malancas@github.com>

include new flags in comment

72fe910

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas commented Apr 19, 2023

View reviewed changes

malancas added 3 commits April 19, 2023 15:53

udpate comment

dd6be2b

Signed-off-by: Meredith Lancaster <malancas@github.com>

include signer-hash-function flag

3924a98

Signed-off-by: Meredith Lancaster <malancas@github.com>

update error messages

b41aa63

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas commented Apr 19, 2023

View reviewed changes

malancas marked this pull request as ready for review April 19, 2023 23:59

malancas requested a review from a team as a code owner April 19, 2023 23:59

haydentherapper requested changes May 8, 2023

View reviewed changes

malancas closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove signer hardcoding of SHA-256 #328

Remove signer hardcoding of SHA-256 #328

malancas commented Apr 19, 2023 •

edited

Loading

codecov bot commented Apr 19, 2023 •

edited

Loading

malancas Apr 19, 2023 •

edited

Loading

malancas Apr 19, 2023 •

edited

Loading

haydentherapper May 8, 2023

haydentherapper left a comment

haydentherapper May 8, 2023

haydentherapper May 8, 2023

haydentherapper May 8, 2023

haydentherapper May 8, 2023

haydentherapper May 8, 2023

haydentherapper May 8, 2023

malancas commented Aug 24, 2023

Remove signer hardcoding of SHA-256 #328

Remove signer hardcoding of SHA-256 #328

Conversation

malancas commented Apr 19, 2023 • edited Loading

Summary

Release Note

Documentation

codecov bot commented Apr 19, 2023 • edited Loading

Codecov Report

malancas Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

malancas Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

haydentherapper May 8, 2023

Choose a reason for hiding this comment

malancas commented Aug 24, 2023

malancas commented Apr 19, 2023 •

edited

Loading

codecov bot commented Apr 19, 2023 •

edited

Loading

malancas Apr 19, 2023 •

edited

Loading

malancas Apr 19, 2023 •

edited

Loading