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

Issue 6269 - RFE - Add nsslapd-pwdPBKDF2Rounds configuration to PBKDF2-* plugins #6447

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

droideck
Copy link
Member

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This was password hashing round value can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than
what we currently provide.

Fixes: #6269

Reviewed by: ?

@droideck droideck marked this pull request as draft December 12, 2024 01:50
@droideck droideck added the work in progress Work in Progress - can be reviewed, but not ready for merge. label Dec 12, 2024
Copy link
Contributor

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

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

Looks really good, nice to see you looking at the rust side :)

src/slapi_r_plugin/src/error.rs Outdated Show resolved Hide resolved
const MAX_PBKDF2_ROUNDS: usize = 1_000_000;

const PBKDF2_ROUNDS_ATTR: &str = "nsslapd-pwdPBKDF2Rounds";
static PBKDF2_ROUNDS: Lazy<RwLock<usize>> = Lazy::new(|| RwLock::new(DEFAULT_PBKDF2_ROUNDS));
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realized we need to make the PBKDF2_ROUNDS a HashMap, as we might use different values in different schemes.

Do you know what the best option for our case is?

I found dashmap option Lazy<DashMap<MessageDigest, usize>> or we can make a RwLock HashMap like this RwLock<HashMap<MessageDigest, usize>>...
And I think there can be more...

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I tried a few approaches (even Rust's generics...), and I think the one in the last commit will work the best.
It's the simple AtomicUsize variables - one for each digest.

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

C/template part of the patch LGTM. A minor question

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

oppss forgot to add my questions ;)

ldap/schema/01core389.ldif Outdated Show resolved Hide resolved
ldap/schema/01core389.ldif Outdated Show resolved Hide resolved
@droideck droideck force-pushed the rust-pbkdf2-add-param branch 3 times, most recently from d1025bd to 2f0db06 Compare December 19, 2024 23:51
…2-* plugins

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This was password hashing round value can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Fixes: 389ds#6269

Reviewed by: ?
@droideck droideck force-pushed the rust-pbkdf2-add-param branch from 2f0db06 to 3603e54 Compare December 20, 2024 00:43
@droideck
Copy link
Member Author

Okay, it's ready for the final review!
I additionally covered UI, CLI, and tests.

Design doc: 389ds/389ds.github.io#17

Please check! Thank you!

@droideck droideck removed the work in progress Work in Progress - can be reviewed, but not ready for merge. label Dec 20, 2024
@droideck droideck marked this pull request as ready for review December 20, 2024 00:46
@droideck
Copy link
Member Author

Changes are made, please, review

@droideck droideck force-pushed the rust-pbkdf2-add-param branch from 6496b99 to 802fb91 Compare December 23, 2024 16:16
Copy link
Contributor

@progier389 progier389 left a comment

Choose a reason for hiding this comment

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

LGTM

@droideck droideck force-pushed the rust-pbkdf2-add-param branch from d4ec94a to 9156f11 Compare January 3, 2025 04:22
Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Firstyear Firstyear left a comment

Choose a reason for hiding this comment

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

Yep, the changes to the rust code look good to me :)

@droideck droideck merged commit b8e442b into 389ds:main Jan 8, 2025
199 checks passed
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
droideck added a commit that referenced this pull request Jan 8, 2025
…2-* plugins (#6447)

Description: Add nsslapd-pwdPBKDF2Rounds attribute that can be configured in
PBKDF2-* password storage plugin entries. This is a password hashing round value that can be adjusted.
Certain compliance requirements (like from BSI) require specific hashing round values greater than what we currently provide.
Add CLI, Web UI option, and CI tests.

Increase DEFAULT_PBKDF2_ROUNDS to 100_000.

Fixes: #6269

Reviewed by: @Firstyear, @progier389, @tbordaz (Thanks!!!)
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.

[RFE] pbkdf2 hardcoded parameters should be turned into configuration options
5 participants