-
Notifications
You must be signed in to change notification settings - Fork 2k
Improved Credential to allow stronger password checksums. #13806
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
Conversation
Signed-off-by: Simone Bordet <[email protected]>
| OBF:1yta1t331v8w1v9q1t331ytc <3> | ||
| MD5:5eBe2294EcD0E0F08eAb7690D2A6Ee69 <4> | ||
| ... | ||
| MD:SHA-1:E5E9Fa1bA31eCd1aE84f75CaAa474f3a663f05F4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OBF can be reversed to a password.
MD5, SHA1-, SHA-256 is 1 direction, it cannot be reversed into a password.
Is that nuance worth mentioning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This nuance is important for reversible passwords.
A stored password for a database connection? needs to be reversible, so that the code can actually submit that password to the database server.
A stored password for verifying a submitted password (like a user login), that can be a MD based 1-way password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost like describing a stored password as verifying an incoming password (all storage types supported) or obtaining an outgoing password (only reversible supported)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joakime we always had MD5, so this PR adds just a pluggable format for MD-based checksums.
I have added in the docs an example for 1-way password.
Where do you think we need to add more in the documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't a need for a code change, it feels more documentation specific.
I'll rereview the docs shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change LGTM, but I would like to remind that keeping passwords as String objects is not very secure as they would be included in a heap dump for instance, so they should be kept in char[] and the array should be wiped immediately after use.
That's probably a non-issue for a short-lived command-line tool like the one here, though. And this would need to be solved in a different PR.
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
No description provided.