crypto-password icon indicating copy to clipboard operation
crypto-password copied to clipboard

Add argon2 hash functions

Open gmsvalente opened this issue 2 years ago • 5 comments

Adds argon2 encryption to the encryption methods offered by this library

gmsvalente avatar Mar 02 '23 13:03 gmsvalente

Thanks for the patch. Can you remove any code unrelated to the feature being implemented?

weavejester avatar Mar 03 '23 16:03 weavejester

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 lane

Which 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))))


gmsvalente avatar Mar 19 '23 16:03 gmsvalente

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 https://github.com/weavejester/crypto-password/pull/16#discussion_r1161291286 :

  • 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 https://github.com/weavejester/crypto-password/pull/16#discussion_r1161291777 :

    • 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 https://github.com/weavejester/crypto-password/pull/16#discussion_r1161291967 :

  • (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 https://github.com/weavejester/crypto-password/pull/16#discussion_r1161292131 :

+(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 https://github.com/weavejester/crypto-password/pull/16#pullrequestreview-1376915809, or unsubscribe https://github.com/notifications/unsubscribe-auth/AW2SZV523Q7AVMETBRE6D6DXAK7OBANCNFSM6AAAAAAVNNKOVA . You are receiving this because you authored the thread.Message ID: @.***>

gmsvalente avatar May 04 '23 21:05 gmsvalente

Hi Mr. Reeves, please see if the changes are ok. And sorry about the delay!

gmsvalente avatar Jun 01 '23 16:06 gmsvalente

Hi @weavejester @gmsvalente, any update about this PR :)

Thanks,

Marius

puchka avatar Dec 28 '23 11:12 puchka