Add support for crypto.Signer when signing and verifying signatures #48
Conversation
wolfeidau
changed the title
wolfeidau
changed the title
| @@ -113,16 +120,28 @@ func Sign(_ context.Context, key jwk.Key, sf SignedFielder, opts ...Option) (*pi | |||
| return nil, err | |||
| } | |||
|
|
|||
| if options.logger != nil { | |||
| switch key := key.(type) { | |||
There was a problem hiding this comment.
This is new to me!
I haven't done serious Golang development for a while, you used to need to do Reflect here but that was pretty slow. Is this some newish Golang feature?
There was a problem hiding this comment.
Type switches have been around for a very long time: https://web.archive.org/web/20120331010210/golang.org/ref/spec
There was a problem hiding this comment.
How had I never come across this before!
I needed the magic words to google, it's called a TypeSwitchGuard
| @@ -113,16 +120,28 @@ func Sign(_ context.Context, key jwk.Key, sf SignedFielder, opts ...Option) (*pi | |||
| return nil, err | |||
| } | |||
|
|
|||
| if options.logger != nil { | |||
| switch key := key.(type) { | |||
| case jwk.Key: | |||
| pk, err := key.PublicKey() | |||
There was a problem hiding this comment.
This method isn't implemented in the above interface
There was a problem hiding this comment.
Within this case, key has the type jwk.Key, since it was redeclared within the switch line (key := key.(type)). This is probably the most magical of all Go syntax.
| @@ -73,9 +75,14 @@ func configureOptions(opts ...Option) options { | |||
| return options | |||
| } | |||
|
|
|||
| type Key interface { | |||
| Algorithm() jwa.KeyAlgorithm | |||
There was a problem hiding this comment.
I'm a bit confused, how could crypto.Signer implement this interface?
There was a problem hiding this comment.
A caller to Sign would supply a key having some concrete type that implements Key. Whether that value has a concrete type that also implements crypto.Signer can be determined by the type switch.
| return nil, fmt.Errorf("failed to marshal public key: %w", err) | ||
| } | ||
|
|
||
| debug(options.logger, "Public Key Thumbprint (sha256): %x", sha256.Sum256(data)) |
There was a problem hiding this comment.
While we're here, we could replace hex.EncodeToString with a %x verb in the other debug log (line 135)
signature/sign_test.go
Outdated
| @@ -134,6 +139,126 @@ func TestSignVerify(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| var _ crypto.Signer = MockCryptoSigner{} | |||
|
|
|||
| type MockCryptoSigner struct { | |||
There was a problem hiding this comment.
My only quibble here (very minor) is I would call this a fake instead of a mock, since it lacks the ability to check whether its methods were called (a good thing).
| RepositoryURL: "fake-repo", | ||
| } | ||
|
|
||
| cases := []struct { |
There was a problem hiding this comment.
Since there's only one test case, it's probably overkill to set up the table-driven test infrastructure like this, but OTOH might be useful if we plan to add more cases?
There was a problem hiding this comment.
@DrJosh9000 yeah I figure at some point we will need more test cases so i left it a table.
signature/sign_test.go
Outdated
|
|
||
| block, _ := pem.Decode([]byte(pemPrivateKey)) | ||
| x509Encoded := block.Bytes | ||
| privateKey, _ := x509.ParseECPrivateKey(x509Encoded) |
There was a problem hiding this comment.
It's unlikely, but if we accidentally screw up the private key file such that this fails, we should probably t.Fatalf out and report the error.
| t.Fatalf("os.ReadFile(%q) error = %v", privateKeyPath, err) | ||
| } | ||
|
|
||
| block, _ := pem.Decode([]byte(pemPrivateKey)) |
There was a problem hiding this comment.
Funny how this looks like an ignored error, but isn't. According to the docs, block would be nil if it failed to parse. Arguably we could check that and t.Fatalf, but since the following line would panic, it's not a big deal.
This change will enable the use of KMS signing in AWS and possibly GCP as these services can implement crypto.Signer and provide signatures for the pipeline signing feature.