Redesign the library
NobbZ opened this issue · 9 comments
Hello, currently the library depends optionally on the hashing packages and creates modules dynamically that wrap calls into those packages.
The current structure makes it hard to add additional hashing algorithms, as this always requires changes to comeonin.
Id therefore suggest to massively rewrite comeonin and the currently available hashing libraries.
My (very) rough idea is this:
- make comeonin not depend on the algorithms at all
- introduce a
behaviourincomeonin(eg.Comeonin.Hasher) - make the hashing packages depend on the new
comeoninand implement the behaviour - Provide a single wrapper module in
comeoninwhich is used like this:Comeonin.add_hash(Bcrypt, password, opts)instead ofComeonin.Bcrypt.add_hash(password, opts)
This would definitively be a breaking change and it would require coordinated work on all 4 repositories.
I'd give the implementation of comeonin according to this proposal a try over the next 2 or 3 weeks (I'm quite busy at work), if you want me to.
First of all, apologies for not getting back to you sooner - really busy at the moment.
I think your suggestions are really interesting, and if I was starting on the project today, I might well adopt this approach. My main problem with making changes to Comeonin to incorporate these ideas is that it results in breaking the api, and I want to avoid that at all costs. There is already a lot of code and many tutorials out there which use a huge variety of the different versions of the library, and I want those to stay relevant. Another problem is that when I make any changes to the library, it is sometimes difficult for me to publicize them sufficiently, and that leads to many issues being raised by developers who expect the already existing / previous behavior.
About adding additional hashing algorithms, do you have any algorithms in mind?
Anyway, I will leave this issue open, so that other people can comment on it. It would be good to hear what other people think.
I think that being able to support multiple hashing algorithms in a pluggable way might be important for the long term use of this library. If people are in agreement on this, then the question of the rewrite would be more about when and how than if. The alternative might look like a competing library emerging at some point, which seems like an undue maintenance burden if it could be avoided.
It would be useful to get a sense of how many and which APIs would be broken by this change, and if there are options to retain and deprecate some or all of these. Could Comeonin.Bcrypt.add_hash(password, opts), for example, be retained as a logic-free wrapper module that delegates to the new APIs with a compile-time warning? That seems like a reasonable way to educate users who may be following an old tutorial with a new library version.
One unintended effect I could see in making the hashing packages pluggable and dependent on a behaviour implemented in this library is that it could be possible for behaviour implementation to be mistakenly taken as implicit endorsement of soundness of the underlying hashing algorithm.
Yes, your last point is important. One of the goals of this library is that it provides advice (access to research, etc.) to developers, and the choice of algorithms is very much part of this because it does imply an endorsement of those algorithms.
Currently, as far as password hashing algorithms are concerned, I don't think there are any other algorithms that we need to support, and I don't see this changing in the near future.
do you have any algorithms in mind?
- scrypt
- anything more optimised for embedded when using with Nerves
@hauleth thanks for your input. I think the fact that there are other algorithms that developers might want to use helps me make up my mind on this issue.
Not that I needed to use them, but these are the use cases I see for other algorithms.
I have started work on a similar implementation to the one suggested by @NobbZ
You can see the current work at the v5.0 branch of Comeonin and v2.0 branch of argon2_elixir.
The plan is to have Comeonin define two behaviours, Comeonin and Comeonin.PasswordHash, together with an implementation of the Comeonin behaviour. The password hashing algorithm will then use Comeonin, which will add the functions add_hash and check_pass to the password hashing library (in this case, Argon2).
This will mean that most developers will not need to add comeonin to their dependencies - only password hashing libraries will need to have comeonin as a dependency.
If any of you have any questions / comments, please let me know.
The master branch has now been updated with the changes.