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

Allow JWT signing method to be configurable. #80

Merged
merged 2 commits into from
Apr 7, 2023

Conversation

wlynch
Copy link
Collaborator

@wlynch wlynch commented Sep 28, 2022

This change creates a new Signer interface which encapsulates jwt.SigningMethod + the key material use to sign JWT tokens.

This allows clients to modify how JWT tokens are signed by passing in their own Signer. In particular, I'm interested in coupling this with something like
https://github.com/golang-jwt/jwt#extensions to allow for JWT signing backed by KMS systems (Vault, Cloud KMS, etc) where the private key never resides on the local client.

Introduces a new AppsTransportOptions to make it easier to make new transport creation options without needing to make new funcs each time. For now only added WithSigner, but we could easily extend this out to other config options (Client, BaseURL, etc.)

Finally, upgrades deprecated jwt.StandardClaims -> jwt.RegisteredClaims.

This change creates a new `Signer` interface which encapsulates
jwt.SigningMethod + the key material use to sign JWT tokens.

This allows clients to do is modify how JWT tokens are
signed by passing in their own Signer. In particular, I'm interested in
coupling this with something like
https://github.com/golang-jwt/jwt#extensions to allow for JWT signing
backed by KMS systems (Vault, Cloud KMS, etc).

Also introduces a new `AppsTransportOptions` to make it easier to make
new transport creation options without needing to make new funcs each
time. For now only added `WithSigner`, but we could easily extend this
out to other config options (Client, BaseURL, etc.)
@bradleyfalzon
Copy link
Owner

This looks good and the I like the AppsTransportOptions. The smallest nit would be whether you wanted to add something to README.

@wlynch
Copy link
Collaborator Author

wlynch commented Oct 4, 2022

Sure thing! Coming right up.

@wlynch
Copy link
Collaborator Author

wlynch commented Oct 17, 2022

@bradleyfalzon This is good for another look! 🙏

@wlynch
Copy link
Collaborator Author

wlynch commented Nov 3, 2022

@bradleyfalzon ping for review

@bradleyfalzon
Copy link
Owner

Sorry @wlynch! I've completely missed this, should we merge? It looks good to me.

@wlynch
Copy link
Collaborator Author

wlynch commented Apr 7, 2023

oh sure! (I also forgot about this 😅 )

@wlynch wlynch merged commit b3e558f into bradleyfalzon:master Apr 7, 2023
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

Successfully merging this pull request may close these issues.

2 participants