Add argon2 hash functions #16
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the patch. Can you remove any code unrelated to the feature being implemented? |
weavejester
requested changes
Mar 10, 2023
src/crypto/password/argon2.clj
Outdated
| See: | ||
| https://infosecscout.com/best-algorithm-password-storage | ||
| https://github.com/phxql/argon2-jvm" | ||
| (:import (de.mkammerer.argon2 Argon2 Argon2Factory Argon2Advanced))) |
There was a problem hiding this comment.
Can you change the () into [] to ensure it's consistent with the rest of the library?
|
Hello Mr. Reeves, Firstly, thank you for your patience, and sorry for the mess I will squash down my commits, just see if it is ok for you now. About the parameters, I followed this https://github.com/phxql/argon2-jvm#recommended-parameters, and take the default-iterations number by running the function, (de.mkammerer.argon2.Argon2Helper/findIterations argon2 1000 default-memory-cost default-parallelization-parameter)
;;; (findIterations argon2 maxMillisecs memory parallelism)
;;; argon2 <-- argon2 instance
;;; maxMillisecs <-- Millisecs that the hash must take
;;; memory <-- Sets memory usage to x kibibytes
;;; parallelism <-- Number of threads and compute laneWhich give me the answer 22 used at default-iterations For reference: I was thinking of doing something like this on my side here: (def default-memory-cost ,,,)
(def default-parallelization-parameter ,,,)
(defn n-iters []
(Argon2Helper/findIterations argon2 1000 default-memory-cost default-parallelization-parameter))
(def ^:private default-iterations
(Long/parseLong (System/getProperty "crypto.password.argon2.default-iterations" (n-iters))))
|
weavejester
requested changes
Apr 9, 2023
There was a problem hiding this comment.
Thanks for the PR, and apologies for the delay. There's some very minor styling changes needed, but other than that the code looks fine. After you've addressed the changes, can you squash your commits down?
I have a small contributing document for my other repos that I haven't copied into this one quite yet.
src/crypto/password/argon2.clj
Outdated
| (def ^:private default-parallelization-parameter | ||
| (Long/parseLong (System/getProperty "crypto.password.argon2.default-parallelization-parameter" "1"))) | ||
|
|
||
|
|
There was a problem hiding this comment.
Can you remove this extra blank line?
src/crypto/password/argon2.clj
Outdated
Comment on lines
24
to
25
| * `iter` - the number of iterations, defaults to 22 (see: | ||
| Argon2Helper/findIterations) | ||
| * `mem` - the memory cost, defaults to 65536 | ||
| * `parallel` - the parallelization parameter, defaults to 1" |
There was a problem hiding this comment.
I don't think the default needs to be justified in the docstring. Better to just keep the relevant information for usage, and line up the definitions for clarity. So:
* `iter` - the number of iterations, defaults to 22
* `mem` - the memory cost, defaults to 65536
* `parallel` - the parallelization parameter, defaults to 1
src/crypto/password/argon2.clj
Outdated
| * `mem` - the memory cost, defaults to 65536 | ||
| * `parallel` - the parallelization parameter, defaults to 1" | ||
| ([raw] | ||
| (encrypt raw default-iterations default-memory-cost default-parallelization-parameter)) |
There was a problem hiding this comment.
Can you keep this line within 80 characters? I know this rule is violated for the default parameter variables in all the namespaces, but we can fix that separately.
src/crypto/password/argon2.clj
Outdated
|
|
||
| (defn check | ||
| "Compare a raw string with a string encrypted with the [[encrypt]] | ||
| function. Returns true if the string matches, false otherwise." |
There was a problem hiding this comment.
Should be a single space after the .. So:
function. Returns true if the string matches, false otherwise."
|
Hi, Mr. Reeves sorry the delay, busy here, but did not forget it. I will
finished by this weekend.
Thanks again,
Gustavo Valente
…On Sun, Apr 9, 2023 at 11:10 AM James Reeves ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks for the PR, and apologies for the delay. There's some very minor
styling changes needed, but other than that the code looks fine. After
you've addressed the changes, can you squash your commits down?
I have a small contributing
<https://github.com/weavejester/contributing/blob/master/CONTRIBUTING.md>
document for my other repos that I haven't copied into this one quite yet.
------------------------------
In src/crypto/password/argon2.clj
<#16 (comment)>
:
> + See: https://infosecscout.com/best-algorithm-password-storage
+ https://github.com/phxql/argon2-jvm"
+ (:import [de.mkammerer.argon2 Argon2 Argon2Factory Argon2Advanced]))
+
+(def argon2 (Argon2Factory/create))
+
+(def ^:private default-iterations
+ (Long/parseLong (System/getProperty "crypto.password.argon2.default-iterations" "22")))
+
+(def ^:private default-memory-cost
+ (Long/parseLong (System/getProperty "crypto.password.argon2.default-memory-cost" "65536")))
+
+(def ^:private default-parallelization-parameter
+ (Long/parseLong (System/getProperty "crypto.password.argon2.default-parallelization-parameter" "1")))
+
+
Can you remove this extra blank line?
------------------------------
In src/crypto/password/argon2.clj
<#16 (comment)>
:
> + * `iter` - the number of iterations, defaults to 22 (see:
+ Argon2Helper/findIterations)
+ * `mem` - the memory cost, defaults to 65536
+ * `parallel` - the parallelization parameter, defaults to 1"
I don't think the default needs to be justified in the docstring. Better
to just keep the relevant information for usage, and line up the
definitions for clarity. So:
* `iter` - the number of iterations, defaults to 22
* `mem` - the memory cost, defaults to 65536
* `parallel` - the parallelization parameter, defaults to 1
------------------------------
In src/crypto/password/argon2.clj
<#16 (comment)>
:
> + (Long/parseLong (System/getProperty "crypto.password.argon2.default-memory-cost" "65536")))
+
+(def ^:private default-parallelization-parameter
+ (Long/parseLong (System/getProperty "crypto.password.argon2.default-parallelization-parameter" "1")))
+
+
+(defn encrypt
+ "Encrypt a password string using the argon2 algorithm. This function takes
+ three optional parameters:
+
+ * `iter` - the number of iterations, defaults to 22 (see:
+ Argon2Helper/findIterations)
+ * `mem` - the memory cost, defaults to 65536
+ * `parallel` - the parallelization parameter, defaults to 1"
+ ([raw]
+ (encrypt raw default-iterations default-memory-cost default-parallelization-parameter))
Can you keep this line within 80 characters? I know this rule is violated
for the default parameter variables in all the namespaces, but we can fix
that separately.
------------------------------
In src/crypto/password/argon2.clj
<#16 (comment)>
:
> +(defn encrypt
+ "Encrypt a password string using the argon2 algorithm. This function takes
+ three optional parameters:
+
+ * `iter` - the number of iterations, defaults to 22 (see:
+ Argon2Helper/findIterations)
+ * `mem` - the memory cost, defaults to 65536
+ * `parallel` - the parallelization parameter, defaults to 1"
+ ([raw]
+ (encrypt raw default-iterations default-memory-cost default-parallelization-parameter))
+ ([raw iter mem parallel]
+ (.hash argon2 iter mem parallel raw)))
+
+(defn check
+ "Compare a raw string with a string encrypted with the [[encrypt]]
+ function. Returns true if the string matches, false otherwise."
Should be a single space after the .. So:
function. Returns true if the string matches, false otherwise."
—
Reply to this email directly, view it on GitHub
<#16 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AW2SZV523Q7AVMETBRE6D6DXAK7OBANCNFSM6AAAAAAVNNKOVA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Co-authored-by: Soulflyer <[email protected]> Add deps.edn Fix bad url in README Remove unrelated code Co-authored-by: Iain Wood <[email protected]> Remove more unrelated code Co-authored-by: Iain Wood <[email protected]> Fix README and docstrings length Fix format
|
Hi Mr. Reeves, please see if the changes are ok. And sorry about the delay! |
|
Hi @weavejester @gmsvalente, any update about this PR :) Thanks, Marius |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds argon2 encryption to the encryption methods offered by this library