Code looks good to me but I'd like to clarify that I understand it correctly.

Do I understand right that pw_response_requested (which is basicaly extracted pwpolicy control?) can be used only on our local backend?
And if we are binding to the remote backend we should check the attributes remotely?

So in the check_must_change_pw function, you check passwordExpirationTime attribute. And if it is 0, you check password policy which should have 'mast change' flag.
Then you set pb_conn->c_needpw to out pblock connection (it will be processed later).

Code looks good to me but I'd like to clarify that I understand it correctly.
Do I understand right that pw_response_requested (which is basicaly extracted pwpolicy control?) can be used only on our local backend?

The control is used by the client not the backend. Maybe I'm not understanding your question.

And if we are binding to the remote backend we should check the attributes remotely?

In this case using "chain-on-update" & passwordIsGlobalPolicy, then yes. Really "chain-on-update" is a workaround to make password policy truly global and strict (in regards to password retry attempts). One could argue that this is not necessary as long as you are okay with each replica allowing the max retries. Most brute force attempts would still be ineffective, but customers want the retry limit to be strictly enforced.

So in this case the client binds to a replica, and the bind is chained to the master, where it it sees that the user must change their password. It then marks that connection (the chaining connection replica -> master) with "c_needpw", but then the clients attempts a search on the replica and the "c_needpw" is not set on that connection (client -> replica), and the search incorrectly succeeds.

So when we are using chaining (remote_data) we must still check for "user must change password". The reason why the code was not calling need_new_pw() is because that function would then update password policy attributes (or even reject the bind), when in fact this was already done on the remote/chained server. So we needed a one-off function just to check if the password was reset by an admin for chained binds.

The control is used by the client not the backend. Maybe I'm not understanding your question.

Sure, it was just a very bad wording.
I meant something like 'our local machine', of course. And it is the controls for the client. Understood.

In this case using "chain-on-update" & passwordIsGlobalPolicy, then yes. Really "chain-on-update" is a workaround to make password policy truly global and strict (in regards to password retry attempts). One could argue that this is not necessary as long as you are okay with each replica allowing the max retries. Most brute force attempts would still be ineffective, but customers want the retry limit to be strictly enforced.
So in this case the client binds to a replica, and the bind is chained to the master, where it it sees that the user must change their password. It then marks that connection (the chaining connection replica -> master) with "c_needpw", but then the clients attempts a search on the replica and the "c_needpw" is not set on that connection (client -> replica), and the search incorrectly succeeds.
So when we are using chaining (remote_data) we must still check for "user must change password". The reason why the code was not calling need_new_pw() is because that function would then update password policy attributes (or even reject the bind), when in fact this was already done on the remote/chained server. So we needed a one-off function just to check if the password was reset by an admin for chained binds.

Ookey, got it :)
So I am okay with the code but maybe someone else would like to check it also.

arem you sure that always an entry will be found ? or do we still need a check that bind_target_entry != NULL (or check in check_pw_must_change and return

made one comment in the patch, otherwise ok for me

Something not clear to me. The bind being chained to the master, the master enforced the "remote" password policy it found on the master. Here we are using the "local" password policy. Can "local" and "remote" password policies (specifically pw_must_change) be different ?

If the entry has no passwordExpirationTime attribute and pwpolicy->pw_must_change, should not we set c_needpw ?

No, because that means the password was not reset if there is no passwordExpirationTime - so there is nothing to enforce

If the entry is NULL it just works as expected, and no password change is enforced. We properly check for NULL entries in the chain of function calls.

It's possible an admin could use different policies by mistake. We can not stop admin's from shooting themselves in the foot though. Maybe I am not understanding your question?

That was exactly my question, if an admin defines different password policies, that is the local policy (replica) that will decide if the password needs to be reset or not.
I agree it looks admin error to define different password policy

@mreynolds Thanks for your comments . The patch looks good to me. Ack

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/2814

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.