Skip to content
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

Extend jose.NewSigner support for other types #689

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Jan 28, 2025

This commit adds support for custom crypto.Signer types for signing tokens. With this, one can use the signer implemented in a KMS or even step + step-kms-plugin to sign JWTs.

This commit adds support for custom crypto.Signer types for signing
tokens. With this, one can use the signer implemented in a KMS or even
 step + step-kms-plugin to sign JWTs.
@maraino maraino requested a review from hslatman January 28, 2025 23:18
maraino added a commit to smallstep/cli that referenced this pull request Jan 28, 2025
The crypto PR allows to sign tokens using step-kms-plugin.
maraino added a commit to smallstep/cli that referenced this pull request Jan 28, 2025
The crypto PR allows to sign tokens using step-kms-plugin.
Comment on lines 360 to +361

func guessOpaqueSigner(key crypto.PrivateKey) crypto.PrivateKey {
Copy link
Member

@hslatman hslatman Jan 29, 2025

Choose a reason for hiding this comment

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

Should this be called guessSigner, since it can also return other types of signers, specifically the X25519Signer (and will leave others untouched)?

A small blurb of doc might be good too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

X25519Signer is an implementation of the OpaqueSigner interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method intends to return an OpaqueSigner only if necessary.

Comment on lines +43 to +53
func (w wrapSigner) Public() crypto.PublicKey {
if w.Signer == nil {
return nil
}
return w.Signer.Public()
}

func (w wrapSigner) Sign(r io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) {
if w.Signer == nil {
return nil, errors.New("not implemented")
}
Copy link
Member

Choose a reason for hiding this comment

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

Since wrapSigner is only used in tests, you could make it take t *testing.T, and fail immediately when Signer is nil. Then you can remove some test cases that are only testing this test struct's functioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are specific cases where I don't want the test to fail. For example, when I create an opaque signer with NewOpaqueSigner(wrapSigner{}), this method will end up calling signer.Public(), I don't care about the output, but I don't want the method to fail, I want only that the opaqueSigner is not able to recognize the type of the key.

@maraino maraino requested a review from hslatman January 29, 2025 18:35
@maraino maraino merged commit e19adc5 into master Jan 29, 2025
12 checks passed
@maraino maraino deleted the mariano/jose-wrapped-signer branch January 29, 2025 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants