jwt icon indicating copy to clipboard operation
jwt copied to clipboard

Remove En/DecodeSegment deprecation notice

Open albertteoh opened this issue 3 years ago • 2 comments

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.

albertteoh avatar Jan 13 '22 03:01 albertteoh

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.

mfridman avatar Feb 08 '22 01:02 mfridman

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, https://github.com/golang-jwt/jwt/compare/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.

tt avatar May 31 '22 18:05 tt

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

oxisto avatar Mar 25 '23 14:03 oxisto

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.

oxisto avatar Mar 25 '23 14:03 oxisto