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

Remove En/DecodeSegment deprecation notice #153

Closed
albertteoh opened this issue Jan 13, 2022 · 4 comments
Closed

Remove En/DecodeSegment deprecation notice #153

albertteoh opened this issue Jan 13, 2022 · 4 comments

Comments

@albertteoh
Copy link

I'd like to raise a proposal to remove the deprecation notice in EncodeSegment.

Our use case:

We sign JWTs using AWS KMS. Because we create an asymmetric key for signing/verification in KMS, the private key never leaves KMS, and so we have to sign our JWTs within KMS via their API. The returned signature is a binary blob which needs to be base64 encoded.

Currently, we do this manually (which took me some time to figure out which encoding type to use):

signature := base64.RawURLEncoding.EncodeToString(rawSignature)

but discovered EncodeSegment does the same thing, and makes the code a little cleaner as we offload this implementation detail to the more appropriate jwt lib, especially because the same encoding is required for decoding by the consumer of the JWT. In fact, we leverage the JWT lib to perform the JWT verification, instead of KMS. This requires Decoding the signature; so having consistent encoding and decoding methods is important for use cases like ours where signing and verification are performed by different libraries.

It would be a shame to lose this capability, and so would like to request for it to remain exported.

If we agree to this proposal, I can put together the PR.

@mfridman
Copy link
Member

mfridman commented Feb 8, 2022

Thanks for the detailed explanation. We will certainly not remove these functions in the current version /v4.

I also don't think we can remove them without thinking through what a good alternative user experience would be. There might be a good argument for keeping them exported to be used as building blocks.

Let's keep this issue open and see if others have an opinion.

@tt
Copy link

tt commented May 31, 2022

I came here with the same concern.

However, it seems the underlying problem is that the SigningMethod interface requires Sign to return an encoded signature. This feels like a leaky abstraction and it's causing (minor) duplication as all signing methods must encode.

I realize it would be a breaking change but maybe the improvement is to change the contract so that the raw signature bytes are returned and then (*Token).SignedString is responsible for encoding. That would allow to not export EncodeSegment while also making it easier to implement another signing method.

I pushed a branch, main...tt:return-raw-signature, to show what this would look like. I'm happy to open a pull request if this seems like a reasonable direction.

@oxisto
Copy link
Collaborator

oxisto commented Mar 25, 2023

We decided to keep them, but they have been moved to Token and Parser.

@oxisto oxisto closed this as completed Mar 25, 2023
@oxisto
Copy link
Collaborator

oxisto commented Mar 25, 2023

I came here with the same concern.

However, it seems the underlying problem is that the SigningMethod interface requires Sign to return an encoded signature. This feels like a leaky abstraction and it's causing (minor) duplication as all signing methods must encode.

I realize it would be a breaking change but maybe the improvement is to change the contract so that the raw signature bytes are returned and then (*Token).SignedString is responsible for encoding. That would allow to not export EncodeSegment while also making it easier to implement another signing method.

I pushed a branch, main...tt:return-raw-signature, to show what this would look like. I'm happy to open a pull request if this seems like a reasonable direction.

The implementation of PR #278 is very similar to what you had in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants