389-ds-base

#50450 Ticket 49421 - Implement password hash upgrade on bind.
Closed by spichugi. Opened by firstyear.
firstyear/389-ds-base update_on_bind  into  master

Download 50450.patch

Bug Description: As time goes on, password hash mechanisms
change and need to become more resistant to brute force and
other attacks. However long lived, and service passwords do
not change frequently - and in fact, frequent password changes
is a security anti-pattern which is now discouraged.

As a result, it's important to be able to improve the
cryptographic strength and resitance of our passwords for
users as time goes on.
Fix Description: We can implement this because during a bind
operation we have short amount of access to the plaintext
password - we then use that to upgrade the content of the
hash.

Later need_new_pw will check if the password will be updated soon (password expired or about to expire). In such case this is not urgent to check/reencode the current password. Could it be done only if need_new_pwd has not requested a new passwd ?

The idea and the patch are nice. I was wondering if the detection should be part of a kind of healthcheck tools (kind of insight rules), to let the admin decide what/when to do the update.

I think the issue with the healthcheck approach is we would need to substring index on userPassword, and the admin would have to search for these which could be really costly? I think also having a choice of "when" could be really complex to implement policy wise, and also relies on the admin to fix it.

In my mind the healthcheck tool is so that things we can't automatically fix, can be raised. I think given how simple this is, automatically fixing it is more reasonable than a warning/review because we can easily update it. An example we can't automaticalyl fix is weak ACI's for example, or a missing index (because we don't know if the query is flawed or some other issue exists). So I'd rather just have this "as it is" because it's simple, and most admins will not have to think about the process at all.

I think the need_new_pwd idea is good, but I would wonder if it adds complexity to testing an asserting the behaviour? The test case now is "correct and simple". We can really strongly look at the test here and say "yes, it works!". To add in the need_new_pw behaviour, maybe this is an optimisation for time-performance that is not such a big concern, but brings larger complexity in testing that the code works? So I think I'm going to avoid over-complicating this until it becomes a problem, because I think in most cases the upgrade would be avoided (already done), and the cost is low when it is done anyway.

There is another update to the patch about to come which avoids updating clear/crypt to avoid breaking super-legacy and weird integrations, and adds an on-off switch too.

rebased onto 4b5c9c8ecf16fc20e8c1bc492245f6df8c5faa1c

Okay, this has been updated now. Please review @tbordaz and @codehotter!

Awesome!

What happened to the concern about repurposing replication flag for a non-obvious purpose?

@codehotter has reminded me about this issue, so I should double check about the correct way to handle this because op_repl may prevent csn creation?

Okay, so just thinking about it, using op_flag_repl, even without any other possible side effects, is wrong as it's confusing and not clear, especially if we change op-flag-repl later to do more. So I'm going to add a new flag (OP_FLAG_ACTION_SKIP_PWDPOLICY) which has a specific behaviour to only allow this change to skip pw policy checking and manipulations.

rebased onto 1873d94be70adf6a78e8f6425795c435bab078c6

Okay, this update changes the flag and adds the needed check to update_pw_info to avoid the history and other update. I'd like @mreynolds to check this though because he knows this part of the code better than I do, and I'd appreciate his expert input if I have done this correctly.

It's probably worth checking in the test the pwdHistory isn't modified - is that as simple as reading the passwordHistory attribute from the entry?

Okay, this update changes the flag and adds the needed check to update_pw_info to avoid the history and other update. I'd like @mreynolds to check this though because he knows this part of the code better than I do, and I'd appreciate his expert input if I have done this correctly.

What about FIPS mode and PBDFK2? If you enable fips after server install the default hashing scheme is still PBDFK2. I think there was code to us a different scheme, but I don't recall if it updates the scheme in the internal password policy. Please test this works correctly in FIPS mode. If it does you have my ack!

But I'm wondering if this should go into 1.4.2? Which means I need to branch master to 1.4.1 first.

@mreynolds I don't have a fips capable system?

Saying that it sounds like the fix is as you say, to have the different scheme used when fips is active ... but I'm not sure what would be the best way to implement that.

An option is to merge this now as default off, then on 1.4.2 we flick it to default on?

In libglobs.c:4126 couldn't we do something like:

new_scheme = NULL
if (fips) {
    new_scheme = pw_name2scheme(DEFAULT_FIPS_SCHEME)
} else {
    new_scheme = pw_name2scheme(DEFAULT_PASSWORD_SCHEME)
}

That would take care of this and other concerns I think, because then it would cause us to be able to on-upgrade improve both the fips and non-fips value, and means we'd properly respect and change the scheme based on the fips flag.

But I also think this may be out of scope of the ticket here ... thoughts?

When in FIPS aren't we doomed already since we cannot even check if the password is valid since the algorithm is not allowed? Like:

Jul 12 02:58:08 b ns-slapd[19129]: [12/Jul/2019:02:58:08.611940106 +0000] - ERR - PBKDF2_SHA256 - Unable to extract hash output.
Jul 12 02:58:08 b ns-slapd[19129]: [12/Jul/2019:02:58:08.618619292 +0000] - ERR - PBKDF2_SHA256 - Unable to hash userpwd value

@mhonek If that's still an issue, I think we need some more concrete fips handling of the pw algo setup then, so this sounds like it's own issue ....

@mhonek If that's still an issue, I think we need some more concrete fips handling of the pw algo setup then, so this sounds like it's own issue ....

Correct. The hashing fails because PK11_ExtractKeyValue call is disallowed in FIPS. We'd need to find a different way of storing the symmetric key, which is a different issue.

Okay, I think we are off track here - there is clearly a FIPS and PBKDF2 issue - lets open an issue for that, and focus on the correct fix for it there. I think it's not related to or part of this issue.

With that in mind, any comments on this patch then?

ack

rebased onto 3aa14161cfe827ce0f29ce3c0059a10305149b91

Pull-Request has been merged by firstyear

@firstyear. This really needs a design doc, and it should be added to the wiki: https://www.port389.org/docs/389ds/design/design.html

The doc team uses the design page for updating the admin guide.

Thanks!

Yep, i'll write this up today.

https://github.com/marcus2376/389wiki/pull/18

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3508

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.