#49327 password expired control not sent during grace logins
Closed: wontfix 4 years ago Opened 4 years ago by gparente.

Issue Description

when the password is already expired and user is doing grace login, the password expired control is not returned.


Metadata Update from @lkrispen:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1464505
- Custom field type adjusted to defect

4 years ago

Hey @mreynolds you know this code better than I do. Could the fault be that we are not setting need_pw during grace logins? I think sending this back always isn't the logic we intend here.

Metadata Update from @gparente:
- Issue assigned to gparente

4 years ago

My understanding of this issue is that in any case, when we are in grace periods, any grace login should return the control password expired, since it's exactly the case. In the code, we check that password is expired, then we want to see if it's a grace login. Only in that case we return to client side without error but the password was expired and we should send the control back to the client application.

Metadata Update from @gparente:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to review
- Custom field version adjusted to None

4 years ago

@mreynolds Do you mind checking this patch for @gparente ?? Thanks mate :)

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.3.7.0

4 years ago

Metadata Update from @mreynolds:
- Issue assigned to mreynolds (was: gparente)

4 years ago

@gparente I've investigated your patch, and it is good. However, the way new_new_passwd() was used was complicated, and required the caller to add the controls. So I cleaned all of this up and simplified it. Here is the new patch:

0001-Ticket-49327-password-expired-control-not-sent-durin.patch

Mark,

in fact, Thierry has shown in the password policy draft, that this control is not "password expired" but "password should be changed".

That changes completely semantics and I think it should not be sent during grace period.

I think this should be closed as "not a bug".

Regards,

German.

Mark,
in fact, Thierry has shown in the password policy draft, that this control is not "password expired" but "password should be changed".
That changes completely semantics and I think it should not be sent during grace period.
I think this should be closed as "not a bug".

https://tools.ietf.org/html/draft-vchu-ldap-pwd-policy-00

Fell free to close it then, but this also means that we have not been using it correctly for a long time (according to this draft). More importantly we never stated that we support that draft either.

Anyway, we don't comply with the draft as of today, so we either fix our current implementation to follow this draft (and potentially break existing clients that expect these controls), or we continue doing it our own way. In the later case I see no problem returning "our" expired control during the grace period.

Perhaps in DS 1.4.0 (RHEL 8) we can discuss this and look into if we should be complying with the draft. We should also consider what other ldap vendors do as well.

I agree with you mark. We are not committed to the ietf draft even if our current implement of this specific control is following it.

IMHO we need to document this control and potentially extend its use like @gparente proposed.
DS 1.4.0 looks good to me

I agree with you mark. We are not committed to the ietf draft even if our current implement of this specific control is following it.
IMHO we need to document this control and potentially extend its use like @gparente proposed.
DS 1.4.0 looks good to me

Sounds good, milestone adjusted. Perhaps the bugzilla should be closed in the mean time @gparente ?

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.4 backlog (was: 1.3.7.0)

4 years ago

Update: There is inconsistency in how the control is interpreted according to different sources:

http://www.alvestrand.no/objectid/2.16.840.1.113730.3.4.4.html

So I think the patch/fix is still valid, and adds value.

Please review patch...

@mreynolds . The fix looks good.
This is extending the meaning of that control and a doc bug should be opened.

Metadata Update from @tbordaz:
- Custom field reviewstatus adjusted to ack (was: review)

4 years ago

This is mentioned in the deployment guide:

https://access.redhat.com/documentation/en-us/red_hat_directory_server/10/html/deployment_guide/Designing_a_Secure_Directory-Designing_a_Password_Policy#Password_Policy_Attributes-Grace_Login_Limit

It states that the password is "expired", but the login is allowed. So this enforces the idea that we should be sending the expired control. Also, there is no mention in any of our docs about password policy controls at all, so I will get a doc bug filed to cover this topic.

02d76b6..fbd32c4 master -> master

84109ee..5173add 389-ds-base-1.3.6 -> 389-ds-base-1.3.6

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

4 years ago

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.3.6.0 (was: 1.4 backlog)

4 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to review (was: ack)

4 years ago

I am okay with a code, so only minor issues:

We import docstrings to Polarion, so they should be in some exact format. There is an example:

118 +    """ Test for expiration control when password must be changed because an
119 +    admin reset the password

I am not sure if the additional whitespace in the beginning of the string will be processed correctly (and it probably won't look consistent)

122 +    :feature: Test the password policy expiration control after password reset

The :feature: is not mandatory field anymore, I've removed it from create_test.py.

123 +    :setup: Configure password policy

Setup usually contains the fixtures. Also you already have the :step: with "Configure password policy". So the :setup: will look like "Standalone instance, a user for testing"

124 +    :steps:
125 +        1. Configure password policy and reset password as admin
126 +        2. Bind, and check for expired control withthe proper error code "2"
127 +    :expectedresults:
128 +        1. The EXPIRED control is returned, and we the expected error code

For the Polarion we need 1-to-1 steps-expectedresults mapping. So it will look like:

124 +    :steps:
125 +        1. Configure password policy
125 +        2. Reset password as admin
126 +        3. Bind, and check for expired control with the proper error code "2"
127 +    :expectedresults:
128 +        1. Password policy should be configured successfully
128 +        2. Password should be successfully reset
129 +        3. The EXPIRED control is returned, and we have the expected error code

And one more thing. Could you please add more logging? I think we can use comments "# password expired" as the logs - "Wait 6 second for the password to be expired".

Metadata Update from @spichugi:
- Custom field reviewstatus adjusted to ack (was: review)

4 years ago

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2386

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: fixed)

a year ago

Login to comment on this ticket.