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

fix addRandomSalt #142

Closed
wants to merge 2 commits into from
Closed

Conversation

mr-ken
Copy link

@mr-ken mr-ken commented Mar 6, 2024

BUGFIX:
Changing the default character set to UTF-8 in v1.7.3 seems to have broken HashBuilder.addRandomSalt() and HashBuilder.addRandomSalt(int length). Using addSalt(String salt) with a properly encoded String fixes that.

@firaja
Copy link
Member

firaja commented Mar 7, 2024

BUGFIX: Changing the default character set to UTF-8 in v1.7.3 seems to have broken HashBuilder.addRandomSalt() and HashBuilder.addRandomSalt(int length). Using addSalt(String salt) with a properly encoded String fixes that.

Hi @mr-ken

can you provide an example of the issue?
Thank you very much!

@mr-ken
Copy link
Author

mr-ken commented Mar 7, 2024

BUGFIX: Changing the default character set to UTF-8 in v1.7.3 seems to have broken HashBuilder.addRandomSalt() and HashBuilder.addRandomSalt(int length). Using addSalt(String salt) with a properly encoded String fixes that.

Hi @mr-ken

can you provide an example of the issue? Thank you very much!

If you run the provided tests without the changes to HashBuilder, they will fail. I assume they should pass?

@firaja firaja linked an issue Mar 7, 2024 that may be closed by this pull request
@firaja
Copy link
Member

firaja commented Mar 7, 2024

@mr-ken thank you for the test and yes they should pass.

In your PR you convert the salt into a String and then back to bytes[]. This rung me a bell and in fact the problem was about the encoding of the salt in the Hash object.

So I changed this line in Argon2Function (#internalHash)

Hash result = new Hash(this, encodeHash(hash, salt), hash, Utils.fromBytesToString(salt));

with

Hash result = new Hash(this, encodeHash(hash, salt), hash, salt);

and your tests are working fine!
This is the commit ded2c06 and the related issue #143

I also deprecated Hash#Hash(HashingFunction, String, byte[], String) because working with a mixture of strings and bytes create inconsistencies; and in 1.7.x Argon2 gave problems with that, so that's why I reworked the way data is encoded, but I totally missed your use case in my tests 😞

So even this PR cannot be merged I really want to thank you!
But your tests will be included anyway in 1.8.1.

PS: let me know if this implementation is working fine in your project (branch master).

@mr-ken
Copy link
Author

mr-ken commented Mar 7, 2024

Do you have an estimate when you'll release v1.8.1?

@firaja
Copy link
Member

firaja commented Mar 7, 2024

Do you have an estimate when you'll release v1.8.1?

I think within this week.

@firaja firaja closed this Mar 8, 2024
@firaja
Copy link
Member

firaja commented Mar 8, 2024

@mr-ken 1.8.1 released, thank you for your collaboration!

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.

Argon2: fix addRandomSalt
2 participants