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

Feature/argon2 contexts secret and associated data #86

Merged
merged 4 commits into from
Apr 12, 2021
Merged

Feature/argon2 contexts secret and associated data #86

merged 4 commits into from
Apr 12, 2021

Conversation

suniastar
Copy link
Contributor

I've added some advanced hash and verify methods to support argon2's secret and associated data.
I needed this for one of my projects and also saw issue #83.

Added:

  • Argon2_context Structure Class as the representation of its C struct.
  • Argon2_version as a better representation of its C counterpart.
  • The C argon2*_ctx(argon2_context *context); methods to Argon2Library class.
  • 2 rawHashAdvanced and verifyAdvanced methods to Argon2Advanced because the argon2*_ctx(argon2_context *context); methods do not encode the hash.
  • Also callLibraryContext and callLibrarayVerifyContext methods to BaseArgon2, Argon2i, Argon2d, and Argon2id to support the new native library calls.
  • Minor private helper methods in BaseArgon2.
  • Some tests for the new methods.

Modified:

  • JavaVersion from 1.6 to 1.8 as my gradle wouldn't compile the project otherwise.
  • Argon2Version to fit the new Argon2_version JnaUint32 class.

Testing:

  • I've tested the new methods with the added tests but only on a Linux x64 machine (running ArchLinux) and they worked so far.

As this is one of my first real Github contributions feedback is much appreciated.

@suniastar
Copy link
Contributor Author

BTW, is there a reason for the current java target and source version 1.6?

@suniastar
Copy link
Contributor Author

suniastar commented Apr 10, 2021

I don't know why but there seems to be an issue with the mac Github checks.
Only a few lines of code where changed with my last commit but the tests seem to be stuck or don't provide an error message.

EDIT: Seems to be a temporary issue. All checks after the last commit passed.

Signed-off-by: Frederik Enste <frederik@fenste.de>
@phxql
Copy link
Owner

phxql commented Apr 12, 2021

Wow, thanks for that contribution! I'm gonna take a look and test it.

Regarding to Java 1.6: That was done to support Android, afaik they need 1.6 bytecode.

@phxql phxql self-requested a review April 12, 2021 07:00
@phxql phxql self-assigned this Apr 12, 2021
@suniastar
Copy link
Contributor Author

suniastar commented Apr 12, 2021

Should I revert it to 1.6 in this case or will you do it as part of your review?

Just FYI: Android uses 1.6 bytecode by default but most developers (incl. Google) suggest setting target and source compatibility to at least 1.8 in order to support all API features.
https://developer.android.com/studio/write/java8-support

@phxql
Copy link
Owner

phxql commented Apr 12, 2021

Should I revert it to 1.6 in this case or will you do it as part of your review?

Just FYI: Android uses 1.6 bytecode by default but most developers (incl. Google) suggest setting target and source compatibility to at least 1.8 in order to support all API features.
https://developer.android.com/studio/write/java8-support

I would prefer to stay on 1.6 just to be on the safe side. What's the problem with 1.6?

I added some comments to your changes, please review. And thanks again for your contribution, it's great!

- Reverted Java source and target back to 1.6
- Fixed swapped Library method javadoc
- Added comment to context parallelism
- Changed the hashLenght check
- Renamed Argon2Version enum
@suniastar suniastar requested a review from phxql April 12, 2021 13:39
@suniastar
Copy link
Contributor Author

Thanks for the review. I've adjusted the things we discussed in the conversations.
Please take a look.

Let me also give you some praise for your dedicated work.
Not every reviewer/project owner would have seen that I accidentally swapped the java doc for argon2i_ctx and argon2d_ctx.

@phxql phxql merged commit f53b3b9 into phxql:develop Apr 12, 2021
@phxql
Copy link
Owner

phxql commented Apr 12, 2021

Thank you! I've merged it into develop branch :)

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.

2 participants