cdepillabout / password

datatypes and functions for easily working with passwords in Haskell
http://hackage.haskell.org/package/password
55 stars 16 forks source link

Control exposing hashing algorithms using Cabal flags #63

Closed ivanbakel closed 1 year ago

ivanbakel commented 1 year ago

This adds 4 Cabal flags to password which 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 Internal module as a non-PVP'd module to avoid relying on any particular hashing algorithm module for re-exports.

NB: the doctests in password-instances do not pass when bcrypt is disabled for password. This is because they rely on the Bcrypt module. 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 cryptonite is 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 as password exports 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.

Vlix commented 1 year ago

I feel like this would be an ok addition to the library, but I think we don't have to expose the Data.Password.Internal module if we'd add src as a hs-source-dir to the test suite and added it to the other-modules 🤔

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 manual: True, since I feel cabal shouldn't try every iteration of flags to try and build this package. Though maybe this Argon2 issue might warrant the argon2 flag to not be set to manual.

ivanbakel commented 1 year ago

I feel like this would be an ok addition to the library, but I think we don't have to expose the Data.Password.Internal module if we'd add src as a hs-source-dir to the test suite and added it to the other-modules

This causes Cabal to spit out some false positives about elided modules when building the test suite:

Warning: The following modules should be added to exposed-modules or other-modules in ...:
             - In test:password-tasty:
                 Data.Password.Argon2
                 Data.Password.Bcrypt
                 Data.Password.PBKDF2
                 Data.Password.Scrypt
                 Data.Password.Validate

It doesn't stop the tests from running, though. Do you want me to go ahead with the change?

Vlix commented 1 year ago

With the test suite, I feel that is acceptable. There are ways to remedy this, but it's not necessary at the moment.

I wonder where the right place would be for a comment describing why that warning is expected :thinking:

ivanbakel commented 1 year ago

Alright, the change to how the internal module is used has been made. I also made the flags manual.

Vlix commented 1 year ago

@ivanbakel I've updated dependency boundaries and the CI, could you merge in master?

You can also add Thanks to [@ivanbakel](https://github.com/ivanbakel) to the Changelog entry if you're so inclined.

Vlix commented 1 year ago

Thank you for the contribution :)

sternenseemann commented 2 weeks ago

We should also make the build-depends conditonal based on the flags. It's a bit nonsensical to require scrypt to be installed if scrypt is disabled via the flag.