-
Notifications
You must be signed in to change notification settings - Fork 38
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
Remove signer hardcoding of SHA-256 #328
Remove signer hardcoding of SHA-256 #328
Conversation
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Codecov Report
@@ 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
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>
} | ||
} | ||
|
||
func NewCryptoSigner(ctx context.Context, signer, kmsKey, tinkKmsKey, tinkKeysetPath, hcVaultToken, fileSignerPath, fileSignerPasswd, signerHashFunc string) (crypto.Signer, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this runs only on setup, I'd prefer we err out rather than allow for misconfiguration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if err != nil { | ||
return nil, err | ||
} | ||
signer, err := kms.Get(ctx, kmsKey, hashFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we err out instead of printing?
@@ -30,12 +30,25 @@ import ( | |||
tsx509 "github.com/sigstore/timestamp-authority/pkg/x509" | |||
) | |||
|
|||
func getCurveFromSigner(signer crypto.Signer) (elliptic.Curve, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
I think this is reasonable, as it's only meant for testing.
switch signer { | ||
case MemoryScheme: | ||
sv, _, err := signature.NewECDSASignerVerifier(elliptic.P256(), rand.Reader, crypto.SHA256) | ||
hashFunc, curve, err := getHashFuncEllipticCurve(signerHashFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to keep this hardcoded, as it's only meant for testing and should need no additional configuration.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this runs only on setup, I'd prefer we err out rather than allow for misconfiguration.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is hash func needed here? This only builds a signer
Closing as the hardcoding bug should be resolved by #452 |
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.
signer-hash-function
that allows the user to specify a signer hash function. This is used in several different signer schemesintermediate-hash-function
andleaf-hash-function
, that allow the user to specify the leaf and intermediate key hash functionsRelease Note
Documentation