Skip to content
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

RFC9106: change defaults and add profiles #106

Closed
wants to merge 1 commit into from

Conversation

B-McDonnell
Copy link
Contributor

Description of changes

Change the default parameter choice to RFC9106's recommended "low memory" option and provide named profiles for both high-memory (recommended on systems that can support it) and low-memory profiles.

ph = PasswordHasher.from_profile(argon2.profiles.RFC9106HighMemory)

Also adds the ability to create Profile instances (or subclasses) that wrap PasswordHasher's parameters.

my_profile = argon2.profiles.Profile(time_cost=1, memory_cost=2, parallelism=3, hash_len=4, salt_len=5)
ph = PasswordHasher.from_profile(my_profile)

Remaining tasks

  • Provide documention/changelog.
  • Write tests

Both of these will be done when the implementation strategy is confirmed

Questions

  • Singletons vs subclasses: The named RFC9106 profiles are implemented as import-time Profile singletons. Another option would be to subclass Profile and exclude the values from the __init__ provided from dataclasses. This comes with inconsistent initialization behavior between Profile and its provided subclasses but avoids singletons. This would be okay if Profile was no longer exposed to the user.
  • Duplicated parameters: Currently, Profile defaults encoding and type just as PasswordHasher does. If we want to avoid this duplication, we could remove all defaults from Profile and enforce all parameters are explicit.

Closes #101

@hynek
Copy link
Owner

hynek commented Nov 10, 2021

  • Singletons are fine, but let's make them frozen=True?
  • encoding and type should be kept out for now – if something earth-shattering happens and we'll need them, we can always add them later
  • the version aside, a Profile ist 1:1 the same thing as Parameters – shouldn't we try to somehow unify those two? It probably makes sense to extract parameters and the create a PasswordHasher from it?

@B-McDonnell
Copy link
Contributor Author

Yes, the only difference between Profile and Parameters is that Parameters also stores the version. When combining the two, I'm not sure of the best way to handle the version parameters.

Is there any reason that Parameters is not a dataclass? When reading over it, the only difference I can see is that Parameters uses slots which is only supported in dataclasss in 3.10+. Is there a typical use case where enough Parameters are in memory simultaneously that slots are providing a real benefit? If not I will probably turn Parameters into a dataclass to remove boilerplate

@hynek
Copy link
Owner

hynek commented Nov 19, 2021

dataclasses were introduced in Python 3.7. We still support 3.6 and until quite recently supported 2.7. :)

As a side excursion: slotted classes have more benefits that space efficiency including being faster at instantiation – so it's a good default to go for.

@hynek
Copy link
Owner

hynek commented Nov 22, 2021

FWIW, you can do the dataclass stuff anyway and add a conditional dependency for 3.6 for https://pypi.org/project/dataclasses/. I only want to do one 3.6 release (it gets EOL in December), but I think I'd like to have one 3.6 release with the new parameters.

As for version…just add a default to the only version we support and the factory within PasswordHasher ensures it's always just the one we support (until a new one drops…)?

@hynek
Copy link
Owner

hynek commented Dec 5, 2021

Since you're busy, I went ahead and implemented what we've talked about in #110. It would be nice if you could give it a glance and tell me what you think!

@hynek
Copy link
Owner

hynek commented Dec 8, 2021

fixed by #110, thank you for the inspiration

@hynek hynek closed this Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC is no longer a draft (RFC9106); default parameter choice out of date
2 participants