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

Change argon2 salt length to recommended value (16 bytes) #275

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

falko17
Copy link
Contributor

@falko17 falko17 commented Jun 8, 2022

This pull request changes the Ksf implementation for Argon2 and sets the salt length to 16 bytes (instead of the minimal length of 8 bytes), the value recommended for password hashing in section 3.1 of the Argon2 specification.
This also serves to improve interoperability: for example, with this change, it's possible to use opaque-ke in conjunction with libopaque, since it also uses a 16-byte salt by default.

Note that this will make existing registrations (i.e., those made with an 8-byte salt) invalid, since KSF parameters (including salt length) must stay the same across registration and login.

@kevinlewi
Copy link
Contributor

Given RustCrypto/password-hashes#306, perhaps it would be more apt to use the constant newly-exported constant RECOMMENDED_SALT_LEN?

@falko17
Copy link
Contributor Author

falko17 commented Jun 9, 2022

I agree it would be cleaner to use that, but the constant is unfortunately only visible when the password-hash feature is enabled, which would add the corresponding crate as a transitive dependency. Apart from that, the PR containing the constant was just merged yesterday, so it isn't in the published crate yet.

@daxpedda
Copy link
Contributor

As far as I know, the only reason we are using a salt at all, is because it's required. After all, we are using a static salt, so it's security value is zero. So following the recommendation doesn't make straightforward sense to me. Specifically, the minimum amount was used because it's the minimum required.

Of course, the issue, as pointed out by @falko17, is compatibility, we all have to somehow agree on the same salt to be compatible with each other.

@kevinlewi can this be coordinated by the spec somehow? Maybe go a bit further and specify a static salt for MHFs, instead of using zeroes?

Copy link
Contributor

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just submitted RustCrypto/password-hashes#307 for us. The RustCrypto team is quite quick with releasing new versions on demand.

Despite my comment before, I think the PR is still fine, we can follow-up afterwards.

@kevinlewi
Copy link
Contributor

kevinlewi commented Jun 10, 2022

It's outside the purview of the OPAQUE spec to specify the length of the salt for the key stretching function (argon in this case).

Agreed, pending the outcome of that PR, we can follow up afterwards. Thanks for working on this @falko17, this should be good to merge.

src/ksf.rs Show resolved Hide resolved
Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to add feature-gating for argon2

This commit changes the `Ksf` implementation for Argon2
and sets the salt length to 16 bytes, the value
recommended in section 3.1 of the Argon2 specification.
@falko17
Copy link
Contributor Author

falko17 commented Jun 10, 2022

Thanks for your comments! Should we wait until the changes from RustCrypto/password-hashes#306 and RustCrypto/password-hashes#307 are released in a new crate version so that the constant can be used, or do we want to address this later in a follow-up PR? I'm not sure how long this will take, so perhaps addressing this in a follow-up is the better option here.

@kevinlewi
Copy link
Contributor

Agreed, we can land now. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants