Control exposing hashing algorithms using Cabal flags #63
Conversation
| if flag(argon2) | ||
| other-modules: | ||
| Argon2 | ||
| if flag(bcrypt) | ||
| other-modules: | ||
| Bcrypt | ||
| if flag(pbkdf2) | ||
| other-modules: | ||
| PBKDF2 | ||
| if flag(scrypt) | ||
| other-modules: | ||
| Scrypt |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I can see why you'd want this.
I do have strong opinions about the implementation, but no big reservations about adding this functionality.
There was a problem hiding this comment.
@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 🤷 )
There was a problem hiding this comment.
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.
|
I feel like this would be an ok addition to the library, but I think we don't have to expose the I was gonna say I'd like the library not to build if you set all 4 flags, but on second thought, there's no real harm there. Since you don't use any of the modules either 🤷 I also think it might be a good idea to set the flags to |
This causes Cabal to spit out some false positives about elided modules when building the test suite: It doesn't stop the tests from running, though. Do you want me to go ahead with the change? |
|
With the test suite, I feel that is acceptable. I wonder where the right place would be for a comment describing why that warning is expected 🤔 |
This allows a developer depending on `password` to toggle on and off support for different password-hashing algorithms at the package level. The motivator for this is that `cryptonite`'s Argon2 implementation is broken on M1 Macs (or any `aarch64-darwin` machine), and in the Nix package set, `cryptonite` has been patched so that any calls to the Argon2 implementation will cause a compiler error telling the developer that it is broken. However, as a user of `password`, I don't use Argon2, and I still want to be able to compile `password` by simply ignoring Argon2 as a possible hashing algorithm. That's what these flags allow me to do.
The tests previously relied on the Bcrypt module being exposed to access certain re-exports from the Internal module. Now that Bcrypt is no longer guaranteed to be included in the library (i.e. if the brcrypt cabal flag is disabled), we can't rely on it to be able to run the tests. Instead, we add the `src/` folder to the test suite, so that the tests can directly access the Internal module without needing to expose it to public users of the library. This change generates some spurious warnings about "undeclared modules" when building the tests, but these can safely be ignored. Those modules are accessed through the `password` library, not via the source code.
ivanbakel
force-pushed
the
cabal-flags
branch
from
bd710cc
to
7829194
Compare
|
Alright, the change to how the internal module is used has been made. I also made the flags manual. |
| ghc-options: | ||
| -threaded -O2 -rtsopts -with-rtsopts=-N | ||
| build-depends: | ||
| base >=4.9 && <5 | ||
| , base64 |
There was a problem hiding this comment.
Where did this suddenly come from?
Is this (together with , template-haskell) a remnant of some testing?
|
@ivanbakel You can also add |
ivanbakel
force-pushed
the
cabal-flags
branch
from
7829194
to
da3eb33
Compare
ivanbakel
force-pushed
the
cabal-flags
branch
from
da3eb33
to
1b9d29c
Compare
|
Thank you for the contribution :) |
This adds 4 Cabal flags to
passwordwhich control whether or not the various hashing algorithms are exposed by the library. Each flag controls one corresponding hashing algorithm, and disabling the flag removes that flag's module from the library.To allow for the tests to compile and run in all instances, this also exposes the
Internalmodule as a non-PVP'd module to avoid relying on any particular hashing algorithm module for re-exports.NB: the doctests in
password-instancesdo not pass whenbcryptis disabled forpassword. This is because they rely on theBcryptmodule. Unfortunately, there's not much I can do about this - Cabal has been very slow to even consider the possibility that you'd want to use flags for controlling what parts of the API are exposed.Motivation
The argon2 implementation from
cryptoniteis still broken on M1 Macs. I've got a project that uses this library for the scrypt implementation, and I can't compile it on M1 Macs so long aspasswordexports argon2, thanks to this Nix patch which causes usage of argon2 to type-error.Currently, I'm solving this by manually patching the library in Nix, but I'd prefer not to maintain a manual patch. It would be easier for me if I could simply specify that the package should be built without argon2 support, which I don't use.