-
Notifications
You must be signed in to change notification settings - Fork 15
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
Adjusting for base64 1.0 #74
Conversation
* also dropped CI for GHC 8.8.4
… is still 'cryptonite'
password/password.cabal
Outdated
@@ -56,6 +56,11 @@ flag bcrypt | |||
default: True | |||
manual: True | |||
|
|||
flag crypton | |||
description: Compile with Scrypt support? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: Compile with Scrypt support? | |
description: Use the crypton library as the cryptographic backend |
Feel free to reword this
description: Compile with Scrypt support? | ||
default: False | ||
manual: True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it might be nice to go ahead and add the cryptonite
flag in right now, in case there are users that want to set this in advance of the switch.
flag cryptonite | |
description: Use the cryptonite library as the cryptographic backend | |
default: True | |
manual: True | |
Also, I guess we should have some sort of assertion that exactly one of the crypton
/ cryptonite
flags is set. I'm not sure how to do this off the top of my head though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it's a good idea to include the flag, but I'd argue to add to the cryptonite
flag description: "... (does nothing until crypton
becomes the default)"
And then when we switch in a major version, that part is removed and the crypton
flag description turns into "This flag is here for backwards compatibility"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds good!
guard $ length paramList == 4 | ||
let [ algT, | ||
iterationsT, | ||
salt64, | ||
hashedKey64 ] = paramList | ||
pbkdf2Algorithm <- textToAlg algT | ||
pbkdf2Iterations <- readT iterationsT | ||
salt <- from64 salt64 | ||
hashedKey <- from64 hashedKey64 | ||
let pbkdf2OutputLength = fromIntegral $ C8.length hashedKey | ||
pbkdf2Salt = fromIntegral $ C8.length salt | ||
return (PBKDF2Params{..}, Salt salt, hashedKey) | ||
case paramList of | ||
[algT, iterationsT, salt64, hashedKey64] -> do | ||
pbkdf2Algorithm <- textToAlg algT | ||
pbkdf2Iterations <- readT iterationsT | ||
salt <- from64 salt64 | ||
hashedKey <- from64 hashedKey64 | ||
let pbkdf2OutputLength = fromIntegral $ C8.length hashedKey | ||
pbkdf2Salt = fromIntegral $ C8.length salt | ||
return (PBKDF2Params{..}, Salt salt, hashedKey) | ||
_ -> Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice rewrite :-)
Looks good? @cdepillabout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I'll merge tomorrow and push to hackage. |
Took the opportunity to also add a flag to build with
crypton
.Bumped GHC version testing to 9.8.1 and dropped 8.8.4.
Bumped LTS to 22.7 to build with GHC 9.6.4 normally.