search

riverrun / argon2_elixir

Elixir wrapper for the Argon2 password hashing algorithm
Apache License 2.0
229 stars 34 forks source link

Nif telemetry #32

Closed LostKobrakai closed 5 years ago

LostKobrakai commented 5 years ago

I'm again hitting some memory issues with Argon2 v1. I don't want to argue that I need to increase the available memory, but I'm wondering if it wouldn't be useful to have some way of instrumenting argon2 for detecting issues. Currently verify_pass does return a boolean and that's it in terms of how much information I can get from the library. hash_password seems to report issues in way more detail.

riverrun commented 5 years ago

Will adding a log error help you in your situation?

LostKobrakai commented 5 years ago

It would at least be detectable, but it would be even nicer to have for example telemetry for any "there are issues with the execution" type of errors. Personally I'd like to harness e.g. the :alarm_handler module of the :sasl application to report such issues, where a log alone is not the best place of getting notified.

riverrun commented 5 years ago

Ok. Could you give me a little more info (just a few sentences, maybe with an example) about telemetry, and the alarm_handler module that you mentioned? I can then make a more informed decision.

One more thing, how much memory is currently allotted to your server and and are you still setting the memory to 64 MiB?

LostKobrakai commented 5 years ago

Ok. Could you give me a little more info (just a few sentences, maybe with an example) about telemetry, and the alarm_handler module that you mentioned? I can then make a more informed decision.

Sure. From an operational standpoint I'd really like to know if failures are because of the nif having some issue like to less memory or if it worked like it's supposed to work, but e.g. arguments where incorrect or passwords simply don't match. hash_password seems to already return the information for me (as the client of the library) to detect problems. verify_pass on the other hand doesn't, but certainly it's called way more often than I'm hashing a new password. The information could either be given via the return value like for hash_password, which would probably need an update in the comonin interface, or via a side effect like a raised event with the telemetry library. Either would allow me to wire issues into :alarm_handler, which I wouldn't expect your library to do.

The problem I'm trying to break down in this issue really is that currently a user simply cannot login if there's an issue with memory without me as an operator being able to detect the issue. It just looks like the password was wrong.

riverrun commented 5 years ago

I finally had some time to look at this issue again today. I think the first thing I need to do is make sure that errors are properly reported by verify_pass. I will update verify_pass so that it raises if there is insufficient memory - which is the same as hash_password.

riverrun commented 5 years ago

The v2.1 branch has the changes mentioned above - verify_pass now raises when there is an error. If you have any opinions about whether it should raise or not, please let me know - that is open for discussion.

As for adding telemetry, I am happy to do so, but I am just not sure how to proceed with that at the moment.

riverrun commented 5 years ago

Merged v2.1 to master and released version 2.1.0 to hex.

riverrun commented 5 years ago

Closing this for now - adding telemetry is still an option, but at the moment I don't have enough time to look into how best to do it.