Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New API Proposal: PBKDF2 #98
New API Proposal: PBKDF2 #98
Changes from all commits
01bebe2
ab4881f
b9b7fb2
51fda71
910fc84
f056b79
78f15ed
4e23a81
2c51e73
9af5c40
ee887f1
7e545be
5e95576
add4813
c96811a
746fd49
506278d
4fea9d5
f8b571b
f5785f4
867eace
589a5a6
ee2b43d
7bd24df
00205b2
3932d8a
635ea61
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API generally looks good, but I'm wondering if we should add a minimum round count to avoid folks naively using the number
1
. We should certainly add documentation suggesting a high round count (in the hundreds of thousands), but I can't see any world where we'd want to accept less than 1k.If we have use-cases that absolutely need lower round counts I'd love to know, but those might be best served the way we serve RSA key sizes: with a second API that is a bit harder to justify typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do actually have such a use case. User+password authentication in Oracle typically encrypts the password with 4096 iterations. Hashes it alongside other data using SHA512, does a bit of CBC de- and encryption with it and some more data. And finally encrypts the end result using another 3 rounds of PBKDF2. (https://github.com/lovetodream/oracle-nio/blob/7fee78535f68c9f3a451beeed15483bc6ac2877b/Sources/OracleNIO/Messages/Coding/OracleFrontendMessageEncoder.swift#L926-L974)
It might not be the same for every version of Oracle because this is controlled by the db server, but I confirmed it is the default behaviour with Oracle 23ai (newest release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ace, that's helpful. So I think that this then suggests we want two APIs, one like the one here and one closer that maybe uses the label
unsafeUncheckedRounds
(or similar).My desire is to enable the use-case you have, while strongly discouraging users who pick up PBKDF2 without much thought from putting in extraordinarily low values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the
unsafeUncheckedRounds
API method and aguard
check in the regular API method to throw an incorrect parameter size error whenever the rounds is less than 1000. Please verify if this is acceptable.If we now have the unsafe API method, is the 1000 rounds requirement enough though, as it was proposed way back in original RFC with intent to increase number of rounds over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. OWASP suggests a minimum round count of 210k for SHA512-based PBKDF2, and so maybe we should consider that the lower bound.