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

Replace HMAC with ED25519 in examples #310

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

G1gg1L3s
Copy link

While HMAC remains relatively popular in some contexts, it's generally considered as insecure, especially in untrustworthy environments, as it's easy for the verifier to forge the JWT token. OWASP, for example, recommends using signature algorithms instead of HMAC.

Since ED25519 is state-of-the-art among signature schemes, this PR adds it to the examples. This is to ensure that the examples are "secure by default". As a result, users have less chances of using weak algorithms when they blindly copy the examples.

Though, I didn't touch the hmac_example_test.go, so if user really need HMAC tokens, they can explicitly go the corresponding examples.

@mfridman
Copy link
Member

Should we have both examples? If we're missing ed25519 examples then for sure we should add them, but given how prevalent HMAC is it seems worth keeping?

@oxisto
Copy link
Collaborator

oxisto commented May 16, 2023

The examples are a little bit of a mess since we have different ones in different files; I agree that we definitely want to have ones with priv/public key - maybe ECDSA as well. The question is also whether we really want to have them in the code repository anymore or on our documentation page, like here https://golang-jwt.github.io/jwt/usage/create/#with-additional-claims

@G1gg1L3s
Copy link
Author

Should we have both examples? If we're missing ed25519 examples then for sure we should add them, but given how prevalent HMAC is it seems worth keeping?

Yes, this is exactly why I left the hmac_example_test.go untouched! Users can explicitly go for it if they really need. At the same time, the other examples provide algorithms more suitable for general use.

@G1gg1L3s
Copy link
Author

The examples are a little bit of a mess since we have different ones in different files; I agree that we definitely want to have ones with priv/public key - maybe ECDSA as well. The question is also whether we really want to have them in the code repository anymore or on our documentation page, like here https://golang-jwt.github.io/jwt/usage/create/#with-additional-claims

I don't see problems having the examples here, considering also that they are runnable tests. But if you want, I would like to contribute to the examples on the https://golang-jwt.github.io!

@oxisto
Copy link
Collaborator

oxisto commented May 20, 2023

The examples are a little bit of a mess since we have different ones in different files; I agree that we definitely want to have ones with priv/public key - maybe ECDSA as well. The question is also whether we really want to have them in the code repository anymore or on our documentation page, like here https://golang-jwt.github.io/jwt/usage/create/#with-additional-claims

I don't see problems having the examples here, considering also that they are runnable tests. But if you want, I would like to contribute to the examples on the https://golang-jwt.github.io!

Sure! You can find the docs repo here: https://github.com/golang-jwt/jwt-docs

example_test.go Outdated Show resolved Hide resolved
@@ -14,24 +16,46 @@ import (
// no way to retrieve other fields after parsing.
// See the CustomClaimsType example for intended usage.
func ExampleNewWithClaims_registeredClaims() {
mySigningKey := []byte("AllYourBase")
publicKey, privateKey, err := ed25519.GenerateKey(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether we should load one of the keys from the test repository instead of generating one. This would probably be a bit more "realistic". Hopefully people will understand to load their own key instead of using the one in the test folder then.

Copy link
Author

Choose a reason for hiding this comment

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

I saw that other tests load the key from the filesystem, so here I decided to show that they can also be generated if needed. At least it's secure and useful for quick prototypying.

Co-authored-by: Christian Banse <oxisto@aybaze.com>
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.

3 participants