search

technion / ruby-argon2

A Ruby gem offering bindings for Argon2 password hashing
MIT License
229 stars 30 forks source link

Fix salt reuse vulnerability #51

Closed joshbuker closed 3 years ago

joshbuker commented 3 years ago

Closes #50

This introduces breaking changes that will require a major version bump.

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 24ed7ef2143abf515b25a92506db701ddc8668bc on athix:bug/fix-salt-reuse-vulnerability into ad6b30542ef313a1160de7a783ca968715de9e10 on technion:master.

technion commented 3 years ago

It's been said on many occasions at this point that I won't be taking large, breaking-changes PRs. Whilst I agree that the potential for API misuse is worth fixing, I don't agree with framing that as a vulnerability or using that to push such a PR.

I've made a two line change (+tests) that makes this go away.

joshbuker commented 3 years ago

It's been said on many occasions at this point that I won't be taking large, breaking-changes PRs. Whilst I agree that the potential for API misuse is worth fixing, I don't agree with framing that as a vulnerability or using that to push such a PR.

I've made a two line change (+tests) that makes this go away.

Unchanging code is neither secure nor maintainable in the long term. The whole point of major versions in semver is to support breaking changes when necessary, and I have yet to see any reasons that the existing api flow is beneficial or preferable in any way. That, and the changes suggested are incredibly easy to account for downstream.

This is about as small as a breaking change as possible, and it directly benefits developers both by reducing complexity, and better following object-oriented design.

joshbuker commented 3 years ago

@technion can you explain your position behind why Argon2::Password.new should represent configuration details? The class name suggests that it's for generating/reading Argon2 Passwords, not configuration information. Likewise, the helpers also tell the same story.

technion commented 3 years ago

I disagree with an assertion that constant API changes benefit developers benefits security, and I've spent more time debating that than I'm comfortable.