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

Should we store JWT secret keys in Base64? #8165

Closed
jdubois opened this issue Aug 27, 2018 · 17 comments
Closed

Should we store JWT secret keys in Base64? #8165

jdubois opened this issue Aug 27, 2018 · 17 comments
Milestone

Comments

@jdubois
Copy link
Member

jdubois commented Aug 27, 2018

Please note this is for the master branch: this comes from an upgrade to JJWT 0.10.5, which pushes us to store secret keys in Base64. This is not what we have in our latest release (v5.2.1), and would therefore be a breaking change if we do this.

This question is mostly for @mraible as he works with the people from JJWT ( @lhazlewood I think?), and hopefully he can have a correct answer.

So here is our current code on the master branch:

  • We store a token in Base64 in our Spring Boot config file here
  • It is read an transformed to a byte array using the code from JJWT here

Besides being a breaking change, I don't get why this key should be encoded in Base64. Why couldn't we just store the secret key as a String, and read it as a byte array using:

jHipsterProperties.getSecurity().getAuthentication().getJwt()
        .getSecret().getBytes(StandardCharsets.UTF_8))

I find this code mode simpler for us, but also the configuration would be much easier for our users. As this doesn't seem to be the approach recommended by JJWT, is there any reason to use a Base64-encoded String?

@lhazlewood
Copy link

lhazlewood commented Aug 27, 2018

JJWT has no requirements on how you store your keys. You're free to store and obtain your key bytes however you wish. JJWT actually prefers that you use java.security.Key instances, and if you don't, it has helper methods to transform raw byte arrays or Base64 text into byte arrays which can then be used to create a Key instance.

With regards to your question about the secret bytes:

It boils down to security (entropy specifically) and clarity:

  • "this is my password with more than 512 bits of string length".getBytes(UTF_8) has much less entropy (source of randomness) than a secure-random generated byte array.

    In this example, the source character set is only ascii and there are a ton of predictable zeros in the byte representation.

    In contrast, a secure-random byte array has no discernible patterns, with the side effect being the only way to represent such a byte array is via text encoding (like Base64).

  • In general, human-readable strings are less secure compared to randomly generated keys. This is the reason why algorithms like PBKDF2 exist: to take a human-readable password and derive a secure key from it (and a salt among other things) - because human-readable passwords are often insecure. And as much as we all like XKCD, the author is not a cryptanalyst and his theory about concatenating human-friendly dictionary words into larger strings doesn't hold in many contexts (they are still susceptible to larger rainbow table attacks and the entropy is still lower - see above).

  • To achieve the same entropy of a secure-random key (256, 384, or 512 bits) with only western language words, you need more characters - i.e. a longer 'password'. This can often be a burden or confuse users as to why it should be so long.

    A short secure-random base64-encoded byte array is all that is necessary to meet the minimum strength requirements for the chosen algorithm and the Base64 string clearly indicates "This is random stuff and I'm not really supposed to try and memorize it and the security model is probably better as a result."

So while JJWT doesn't require any particular byte array format - it just checks that the byte array length minimums are met per the JWT RFC specification's length requirements - secure-random is always the safest bet and a best practice if you're generating secrets as a convenience to others. Do with that as you will :)

Finally, if you want to generate secure-random keys that are appropriate for a given algorithm that adheres to the tenets covered here, you can use JJWT's Keys utility class to generate safe keys for your desired algorithm(s).

If you want to represent that securely-generated key in a String form:

SecretKey key = Keys.secretKeyFor(SignatureAlgorithm.HS256); //or HS384 or HS512
String base64Key = Encoders.BASE64.encode(key.getEncoded());
// Note that base64Key is just encoded - not encrypted - so it's still not safe to share with others

Anyway, I hope that helps!

@jdubois
Copy link
Member Author

jdubois commented Aug 27, 2018

Thank you so much @lhazlewood - yes, that definitely helps, and this ticket is for sure going to be helpful for a lot of people, as I'm sure we'll have lots of questions about this.

So, if I understand well, encoding the String in Base64 allows to store more secure keys. Of course, users need to generate that secure key correctly, and I'm not sure everyone will worry about this, but at least we will support this and document this, so we're doing our best!

As a result, I'm closing this, and we'll keep the Base64 encoding for the secret key. Thanks again!

@jdubois jdubois closed this as completed Aug 27, 2018
@lhazlewood
Copy link

I'd say that's generally correct. Just remember (or document?) that the Base64 text value is still considered a secret value (since encoding != encryption) and should never be visible by unintended parties.

Happy to help!

@jdubois
Copy link
Member Author

jdubois commented Aug 27, 2018

Oh yes @lhazlewood that should already be good everywhere in our documentation. The only downside is that now you can't read the value anymore (as it's in Base64) so we can't give hints like we used to do here.

@lhazlewood
Copy link

Ah, I see. Yeah, you'd have to have a comment to the right of the line or above it. Got it.

@deepu105
Copy link
Member

@jdubois but IMO its not nice to do a breaking change for users in a minor release. May be we can find a way for existing apps that upgrade to work with string keys as well

@jdubois
Copy link
Member Author

jdubois commented Aug 28, 2018

@deepu105 OK I got an idea during the night:

  • Let's keep the old "secret", and continue reading it with .getBytes(UTF_8). As @lhazlewood writes, it's going to work, but it's less secure -> our old way of working will still be valid. Please note it will break for many people as now JJWT requires longer keys than what we use to have, but that's a smaller problem, and the stack trace is clear. I'll also add a specific log for that.
  • I will add a new "base64Secret": this one will be the "good" one, and if it will have a higher priority than the other key. So if you use this field, you'll have better security, otherwise the old configuration will still work. I will also write a specific log if it is not used.

Then, for JHipster 6, we could drop the old "secret". Having the new key called "base64Secret" isn't bad, as it shows you need to encode your data.

@deepu105 WDYT?

@jdubois jdubois reopened this Aug 28, 2018
@deepu105
Copy link
Member

@jdubois that sounds perfect. Lets do it

@jdubois
Copy link
Member Author

jdubois commented Aug 29, 2018

OK I'm on it, should be done today.

jdubois added a commit to jhipster/jhipster that referenced this issue Aug 29, 2018
@pascalgrimaud
Copy link
Member

@jdubois : do we need to apply the changes to the JHipster Registry too ?

@jdubois
Copy link
Member Author

jdubois commented Aug 29, 2018

@pascalgrimaud yes of course

@jdubois
Copy link
Member Author

jdubois commented Aug 29, 2018

@pascalgrimaud I'll do it

jdubois added a commit to jhipster/jhipster-registry-sample-config that referenced this issue Aug 30, 2018
jdubois added a commit to jhipster/jhipster.github.io that referenced this issue Aug 30, 2018
@jdubois
Copy link
Member Author

jdubois commented Aug 30, 2018

I'm realizing that if you have a microservice architecture, with a mix of older and newer JHipster applications, you cannot mix base64 en clear text secrets with the current config.

What needs to be done is have the clear text secret have a higher priority than the base64 one: that way older and newer applications can work together.

Not the best thing for security, but I've put some big warning log if you don't use base64 encoding, so that should be enough. And we'll migrate totally in JHipster 6.

@jdubois jdubois added this to the 5.3.0 milestone Sep 3, 2018
@pascalgrimaud
Copy link
Member

@jdubois :

  • I saw your commit 70b8474 -> we use jwtSecretKey in jhipster.security.authentication.jwt.base64-secret
  • it means we need to update all jwtSecretKey, right ?

For example, for the jhipster-sample-app-gateway project:

Caused by: io.jsonwebtoken.security.WeakKeyException: The specified key byte array is 240 bits which is not secure enough for any JWT HMAC-SHA algorithm.  The JWT JWA Specification (RFC 7518, Section 3.2) states that keys used with HMAC-SHA algorithms MUST have a size >= 256 bits (the key size must be greater than or equal to the hash output size).  Consider using the io.jsonwebtoken.security.Keys#secretKeyFor(SignatureAlgorithm) method to create a key guaranteed to be secure enough for your preferred HMAC-SHA algorithm.  See https://tools.ietf.org/html/rfc7518#section-3.2 for more information.
	at io.jsonwebtoken.security.Keys.hmacShaKeyFor(Keys.java:81)
	at io.github.jhipster.sample.security.jwt.TokenProvider.init(TokenProvider.java:55)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.beans.factory.annotation.InitDestroyAnnotationBeanPostProcessor$LifecycleElement.invoke(InitDestroyAnnotationBeanPostProcessor.java:366)
	at org.springframework.beans.factory.annotation.InitDestroyAnnotationBeanPostProcessor$LifecycleMetadata.invokeInitMethods(InitDestroyAnnotationBeanPostProcessor.java:309)
	at org.springframework.beans.factory.annotation.InitDestroyAnnotationBeanPostProcessor.postProcessBeforeInitialization(InitDestroyAnnotationBeanPostProcessor.java:136)
	... 58 common frames omitted

Tell me if I'm wrong ?

@jdubois
Copy link
Member Author

jdubois commented Sep 3, 2018

@pascalgrimaud oh yes, I forgot to update the keys of the sample applications! I'll do it right now.

@pascalgrimaud
Copy link
Member

It will impact users who use microservices application and want to upgrade. But it is mentionned in our patch notes, so it's OK for me.
We just need to have all sample applications to be consistent with the code from generator-jhipster :)

@jdubois
Copy link
Member Author

jdubois commented Sep 3, 2018

For microservices, if you have a mix of older and newer versions, you should use the old "secret" key property: it will work with all versions. However, it needs to be long enough.

In the long term, the idea is to have "base64-secret" everywhere, but I didn't do it yet as indeed that would block people who don't upgrade all their microservices at once.

asfgit pushed a commit to apache/ofbiz-framework that referenced this issue Dec 15, 2022
Ensures the length of the secret is at least 512 bit long
https://www.rfc-editor.org/rfc/rfc7518#page-7
https://javadoc.io/doc/com.auth0/java-jwt/latest/com/auth0/jwt/algorithms/Algorithm.html#HMAC512
We should follow the rule and give a 512 bit key by default and provide
validation based on the same rule.

jleroux:
based on recommendation by Les Hazlewood (JJWT founder, Apache Shiro founder):
jhipster/generator-jhipster#8165 (comment)
I used a 512 bits key I created using https://www.allkeysgenerator.com
(Encryption key mode).
But I got this error:
EntitySaxReader               |E| Fatal Error reading XML on line 23, column 155
org.xml.sax.SAXParseException: The reference to entity "F" must end with the ';'
delimiter. It was due to SSOJWTDemoData content. So I removed security.token.key
from this file and used only the property in security.properties.

Thanks: Ayan Farooqui for report and suggestion
asfgit pushed a commit to apache/ofbiz-framework that referenced this issue Dec 15, 2022
Ensures the length of the secret is at least 512 bit long
https://www.rfc-editor.org/rfc/rfc7518#page-7
https://javadoc.io/doc/com.auth0/java-jwt/latest/com/auth0/jwt/algorithms/Algorithm.html#HMAC512
We should follow the rule and give a 512 bit key by default and provide
validation based on the same rule.

jleroux:
based on recommendation by Les Hazlewood (JJWT founder, Apache Shiro founder):
jhipster/generator-jhipster#8165 (comment)
I used a 512 bits key I created using https://www.allkeysgenerator.com
(Encryption key mode).
But I got this error:
EntitySaxReader               |E| Fatal Error reading XML on line 23, column 155
org.xml.sax.SAXParseException: The reference to entity "F" must end with the ';'
delimiter. It was due to SSOJWTDemoData content. So I removed security.token.key
from this file and used only the property in security.properties.

Thanks: Ayan Farooqui for report and suggestion
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