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
Redesign the library #130
Comments
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 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. |
|
@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 This will mean that most developers will not need to add If any of you have any questions / comments, please let me know. |
Maybe the implementation in this gist could be a useful reference. |
The master branch has now been updated with the changes. |
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:
behaviour
incomeonin
(eg.Comeonin.Hasher
)comeonin
and implement the behaviourcomeonin
which 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.The text was updated successfully, but these errors were encountered: