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.

Sign up for GitHub

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

Jump to bottom

Control exposing hashing algorithms using Cabal flags #63

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Control exposing hashing algorithms using Cabal flags #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5f6100
Put all hashing algorithms behind Cabal flags
ivanbakel Feb 8, 2023
d6800fd
Expose Data.Password.Internal source code to tests
ivanbakel Feb 8, 2023
54d924a
Version bump, ChangeLog update
ivanbakel Feb 8, 2023
1b9d29c
Merge remote-tracking branch 'origin/master' into cabal-flags
ivanbakel Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions password/ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# Changelog for `password`

## 3.0.2.1

- Add Cabal flags to control which hashing algorithms are exported. These flags are
`argon2`, `bcrypt`, `pbkdf2`, and `scrypt`. Each flag is enabled by default -
disabling it will elide the corresponding module from the library. This allows
downstream packagers to disable hashing algorithms which aren't supported on
certain platforms.
Thanks to [@ivanbakel](https://github.com/ivanbakel)
[#63](https://github.com/cdepillabout/password/pull/63)

## 3.0.2.0

- Add `extractParams` on `PasswordHash`s
Expand Down
70 changes: 61 additions & 9 deletions password/password.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
cabal-version: 1.12

name: password
version: 3.0.2.0
version: 3.0.2.1
category: Data
synopsis: Hashing and checking of passwords
description:
Expand Down Expand Up @@ -46,6 +46,26 @@ extra-source-files:
README.md
ChangeLog.md

flag argon2
description: Compile with argon2 support?
default: True
manual: True

flag bcrypt
description: Compile with Scrypt support?
default: True
manual: True

flag pbkdf2
description: Compile with PBKDF2 support?
default: True
manual: True

flag scrypt
description: Compile with Scrypt support?
default: True
manual: True

custom-setup
setup-depends:
base
Expand All @@ -60,11 +80,19 @@ library
hs-source-dirs:
src
exposed-modules:
Data.Password.Argon2
Data.Password.Bcrypt
Data.Password.PBKDF2
Data.Password.Scrypt
Data.Password.Validate
if flag(argon2)
exposed-modules:
Data.Password.Argon2
if flag(bcrypt)
exposed-modules:
Data.Password.Bcrypt
if flag(pbkdf2)
exposed-modules:
Data.Password.PBKDF2
if flag(scrypt)
exposed-modules:
Data.Password.Scrypt
other-modules:
Paths_password
Data.Password.Internal
Expand Down Expand Up @@ -106,22 +134,33 @@ test-suite password-tasty
type:
exitcode-stdio-1.0
hs-source-dirs:
src
test/tasty
main-is:
Spec.hs
other-modules:
Argon2
, Bcrypt
Data.Password.Internal
, Internal
, PBKDF2
, Scrypt
, TestPolicy
, Validate
, Paths_password
if flag(argon2)
other-modules:
Argon2
if flag(bcrypt)
other-modules:
Bcrypt
if flag(pbkdf2)
other-modules:
PBKDF2
if flag(scrypt)
other-modules:
Scrypt
Comment on lines +147 to +158
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for sending this PR! This seems reasonable to me.

I don't feel super strongly either way, but I think I'd prefer to remove all of the changes in this PR for the tests. If end-users want to disable certain algorithms, then it is okay to me if they can't run the test suite.

Or, you could possibly set the test suite as non-buildable if any of the flags is disabled.

Maybe @Vlix has a strong opinion on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see why you'd want this.
I do have strong opinions about the implementation, but no big reservations about adding this functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cdepillabout if you meant if I have strong opinions about adding the flags to the test suite or not, I don't see why we'd make the test-suite non-buildable. Just testing what's "activated" seems like an ok decision (even though it adds flags and CPP to the test suite 🤷 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to implement this in either way, but having a subset of tests be runnable was nice for me, since it checked that the remaining algorithms still worked as expected on Mac.

ghc-options:
-threaded -O2 -rtsopts -with-rtsopts=-N
build-depends:
base >=4.9 && <5
, base64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where did this suddenly come from?

Is this (together with , template-haskell) a remnant of some testing?

, password
, password-types
, bytestring
Expand All @@ -132,6 +171,19 @@ test-suite password-tasty
, tasty
, tasty-hunit
, tasty-quickcheck
, template-haskell
, text
default-language:
Haskell2010
if flag(argon2)
cpp-options:
-DCABAL_FLAG_argon2
if flag(bcrypt)
cpp-options:
-DCABAL_FLAG_bcrypt
if flag(pbkdf2)
cpp-options:
-DCABAL_FLAG_pbkdf2
if flag(scrypt)
cpp-options:
-DCABAL_FLAG_scrypt
4 changes: 2 additions & 2 deletions password/test/tasty/Internal.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import Test.Tasty (TestTree)
import Test.Tasty.QuickCheck
import Test.QuickCheck.Instances.Text ()

import Data.Password.Types (mkPassword, Password, PasswordHash)
import Data.Password.Bcrypt (PasswordCheck(..), Salt(..))
import Data.Password.Types (mkPassword, Password, PasswordHash, Salt(..))
import Data.Password.Internal (PasswordCheck(..))


testCorrectPassword :: (Eq params, Show params)
Expand Down
18 changes: 18 additions & 0 deletions password/test/tasty/Spec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,41 @@
{-# LANGUAGE CPP #-}

import Test.Tasty
import Test.Tasty.QuickCheck
import Test.Tasty.Runners (NumThreads(..))

import Data.Password.Types

#ifdef CABAL_FLAG_argon2
import Argon2 (testArgon2)
#endif
#ifdef CABAL_FLAG_bcrypt
import Bcrypt (testBcrypt)
#endif
#ifdef CABAL_FLAG_pbkdf2
import PBKDF2 (testPBKDF2)
#endif
#ifdef CABAL_FLAG_scrypt
import Scrypt (testScrypt)
#endif
import Validate (testValidate)

main :: IO ()
main = defaultMain $ localOption (NumThreads 1) $
testGroup "Password"
[ testProperty "Password" $ \pass ->
unsafeShowPassword (mkPassword pass) === pass
#ifdef CABAL_FLAG_argon2
, testArgon2
#endif
#ifdef CABAL_FLAG_bcrypt
, testBcrypt
#endif
#ifdef CABAL_FLAG_pbkdf2
, testPBKDF2
#endif
#ifdef CABAL_FLAG_scrypt
, testScrypt
#endif
, testValidate
]