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

Argon2 v10 fix #56

Merged
merged 4 commits into from
Mar 8, 2022
Merged

Argon2 v10 fix #56

merged 4 commits into from
Mar 8, 2022

Conversation

Vlix
Copy link
Collaborator

@Vlix Vlix commented Feb 13, 2022

Aims to fix #54

Argon2 hash without a version part will be interpreted as having been hashed with version 1.0 (v=16)

Not so much a "bug", especially since almost all usages of Argon2 are generally version 1.3, but it's a small effort to also accept hashes without a version part.

@Vlix
Copy link
Collaborator Author

Vlix commented Feb 13, 2022

Just remembered it might be a good idea to also mention this in the documentation of Argon2? Or is that unnecessary? 🤔

@cdepillabout
Copy link
Owner

Sorry for taking a long time to respond to this. I hope to get to this in a few days.

@Vlix
Copy link
Collaborator Author

Vlix commented Mar 3, 2022

No rush, this isn't a pressing fix 👌

let ps = T.split (== ',') params
guard $ Prelude.length ps == 3
go ps (Nothing, Nothing, Nothing)
parseAll argon2Variant argon2Version parametersT salt64 hashedKey64 = do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nitpick, but I felt like I wanted a type signature on this function. I thought it might be a little easier to see exactly what it is doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseAll :: Argon2.Variant -> Argon2.Version -> Text -> Text -> Text -> Maybe (Argon2Params, Salt Argon2, ByteString)

Sure, I'll add this in before I merge it.

Copy link
Owner

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

@Vlix Vlix merged commit 3a6db2b into master Mar 8, 2022
@cdepillabout cdepillabout deleted the argon2-v10-fix branch March 9, 2022 00:05
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 v1.0 hashes might not include "v=16" parameter
2 participants