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

JwtSecretKey takes a String #449

Closed
hunterpayne opened this issue Jun 15, 2024 · 7 comments
Closed

JwtSecretKey takes a String #449

hunterpayne opened this issue Jun 15, 2024 · 7 comments

Comments

@hunterpayne
Copy link
Contributor

hunterpayne commented Jun 15, 2024

Strings in the JVM can be internalized. For this reason, it has long been the case that storing cryptographic material in a Java String is verboten (forbidden). Please change the signature of JwtSecretKey from taking a String to something more appropriate for cryptographic material.

java.security.PrivateKey would probably be the best choice but others like Array[Byte] or Array[Char] are probably good choices too.

@froth
Copy link
Collaborator

froth commented Jun 18, 2024

Thanks for opening this issue, I agree. Would you be interested in opening a pull request?

@gvolpe this is a breaking change, we would have to release 2.0.0. Is there something else we should change if a breaking release happens anyways?

@hunterpayne
Copy link
Contributor Author

Which type should we choose or should I do something that allows all the referenced types?

@gvolpe
Copy link
Member

gvolpe commented Jun 19, 2024

should I do something that allows all the referenced types?

This should be ideal if possible 👍🏽

this is a breaking change, we would have to release 2.0.0.

Indeed. I think the best way to go about it would be to deprecate the existing method that takes a String noticing it's unsafe and make the 2.0.0 release. It can then be removed in the following major release.

@hunterpayne
Copy link
Contributor Author

Which branch should I make the changes on? master or 1.x?

@froth
Copy link
Collaborator

froth commented Jul 1, 2024

Master is pretty outdated I think. I would say base it on 1.0 for now and we will sort it out and maybe create a series/2.0 branch or so.

@hunterpayne
Copy link
Contributor Author

#452

@froth
Copy link
Collaborator

froth commented Jul 25, 2024

fixed in #452 thanks!

@froth froth closed this as completed Jul 25, 2024
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

3 participants