ELY-5 - Implemented Unix SHA Crypt #5
Conversation
| private Charset charset; | ||
|
|
||
| /** | ||
| * Accepts a formatted string with the specifications plus the password. Examples for the specification are: |
There was a problem hiding this comment.
This implementation is not meant to accept the raw string; that is the role of CryptStringPasswordSpec. This implementation should only consist of the salt, iteration count, and hash.
There was a problem hiding this comment.
Makes sense. Will remove it, and work on the Spec.
|
So, I'm sending another commit, just to check if this is going on the right direction. I'll add more javadoc and change a bit how the encode is organized, but I've shuffled things a bit to see if it fits better the architecture. |
|
By the way: for the "real" PR, I'd be squashing the commits. |
|
At this point I think it needs a rebase in order to understand the changes. |
|
Rebased, I hope it looks better :-) |
| this.salt = password.getSalt(); | ||
| this.iterationCount = password.getIterationCount(); | ||
| this.id = password.getId(); | ||
| this.encoded = password.getEncoded(); |
There was a problem hiding this comment.
The "encoded" field should remain null. There should be a "byte[] hash" field instead. Also, charset encoding should not be used for hash password types.
There was a problem hiding this comment.
True, forgot to make a note somewhere that I wasn't sure if "encoded" was what I thought it was. It seems it isn't :-) Will change it.
There was a problem hiding this comment.
About the charset: I haven't given much thought to this, but my first intuition would be that it would be sensible to set a default charset to a common value. A byte is a byte no matter the charset, but when working on the hash for a given password, it might make a difference. I haven't tested, but wouldn't it cause problems when a password is hashed on one machine with ISO-8859-5, and read/verified on a machine whose default charset is UTF-8?
There was a problem hiding this comment.
This password class should only deal with the decoded material. Since it only deals in bytes, the encoded field should be null, as should the reported charset/encoding.
When we do the CryptStringPasswordSpec handling, after all the various crypt types have been implemented, that will be a different story. The key material will be a string (like what you'd find in a /etc/shadow file or similar) and will be translatable to/from various algorithms' "raw" form.
|
Take 3 is ready for review. As seen on the SCRAM example, I'm throwing the InvalidKeySpec when facing exceptions at the PasswordFactorySpiImpl level. |
…set for the guess
|
Any further feedback on this one? |
| @@ -48,6 +69,20 @@ protected Password engineGeneratePassword(final String algorithm, final KeySpec | |||
| return abstractPassword.getKeySpec(keySpecType); | |||
| } | |||
| } | |||
|
|
|||
| switch (algorithm) { | |||
There was a problem hiding this comment.
This bit shouldn't be required. If UnixSHACryptPassword extends AbstractPasswordImpl (as it should), then the method to convert it to a key spec should be right on that class.
|
I've sent a few more commits, that should be addressing the comments. The only think I didn't work on yet is on converting the tests into a higher level usage of the API, as it's not very clear to me how they should be used from the outside. |
|
This PR should be closed if #10 is merged, because that PR incorporates this one. |
|
Being merged under #10 Thank you very much for the work on this. |
[ELY-1711] Create new `wildfly-elytron-sasl-digest` module
This is a first refactored version of the first implementation jpkrohling/wildfly-elytron/ELY-5-UnixSHACrypt, and is not meant to be merged, as I'm mostly looking for feedback.
Things that I'd particularly like to get reviewed are:
But of course, any kind of comment is welcomed. As I mentioned on the IRC, I found this JIRA and decided to give it a try, just for fun. I'm not sure if you already have a specific implementation in mind for this, but if not, this could be a good candidate.