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

PasswordPolicyControl ambiguous graceAuthNsRemaining #131

Closed
dfish3r opened this issue Oct 16, 2017 · 7 comments
Closed

PasswordPolicyControl ambiguous graceAuthNsRemaining #131

dfish3r opened this issue Oct 16, 2017 · 7 comments

Comments

@dfish3r
Copy link
Member

dfish3r commented Oct 16, 2017

The PasswordPolicyControl implementation defaults graceAuthNsRemaining to zero.
Per the RFC, zero is a valid value and could be returned by the server.
The client has no way to determine if the server sent the value zero or no value at all.

See https://groups.google.com/forum/#!topic/ldaptive/nzj62B_Z2js

@marwatk
Copy link

marwatk commented Oct 16, 2017

If it helps, the apache api handles this case in two ways:

  • The default values for both are -1 and are only set if present
  • The controls object itself isn't returned if there is no warning

If you have a path you want to take I can try to code something up...

@dfish3r
Copy link
Member Author

dfish3r commented Oct 18, 2017

I reviewed the behavior against OpenLDAP ppolicy and got the following results:

successful bind graceAuthNsRemaining=2
successful bind graceAuthNsRemaining=1
failed bind error=PASSWORD_EXPIRED

So the behavior is certainly different than ApacheDS.
Could you put ldaptive logging in TRACE and perform a test against ApacheDS?
Step down grace logins until you get a failed bind and look for logs of the form: decoding control:
Post that output to this issue.

dfish3r added a commit that referenced this issue Oct 19, 2017
Change default warning values from 0 to -1.
See #131.
Update integration test.
@dfish3r
Copy link
Member Author

dfish3r commented Oct 19, 2017

Pushed a potential fix to 1.2.4-SNAPSHOT
I'd still like to get some TRACE data for what ApacheDS is actually sending for the control.

@marwatk
Copy link

marwatk commented Oct 20, 2017

Working on getting trace data, I'll see if I can get that today. We swapped out for using org.apache in our code for now so I have to swap it back.

My reading of 8.1.2.3.2 in the rfc leads me to think the final error should be invalid credentials, with the password policy also showing password expired as informational aside. But the invalid credentials is much more important to see, because in that state it is not possible for the user to change the password anymore, so seeing the expired password message is more of a red herring because it can't be fixed like a normal expired password.

After grace auths are used, only a password reset is allowed to change the password, not a modify using the existing password (since the user can no longer bind to perform the password change).

@marwatk
Copy link

marwatk commented Oct 20, 2017

Here are the results of the trace:

decoding control: MAWgA4EBAg==
produced response controls: [[org.ldaptive.control.PasswordPolicyControl@-350057145::criticality=false, timeBeforeExpiration=0, graceAuthNsRemaining=2, error=null]]
decoding control: MAWgA4EBAQ==
produced response controls: [[org.ldaptive.control.PasswordPolicyControl@-350057258::criticality=false, timeBeforeExpiration=0, graceAuthNsRemaining=1, error=null]]
decoding control: MAWgA4EBAA==
produced response controls: [[org.ldaptive.control.PasswordPolicyControl@-350057371::criticality=false, timeBeforeExpiration=0, graceAuthNsRemaining=0, error=null]]
decoding control: MAOBAQA=
produced response controls: [[org.ldaptive.control.PasswordPolicyControl@1125954332::criticality=false, timeBeforeExpiration=0, graceAuthNsRemaining=0, error=PASSWORD_EXPIRED]]

@marwatk
Copy link

marwatk commented Oct 20, 2017

Reading through your commit I think it'll mitigate the issue. A consumer can use the -1 values to determine if values were specified by the server.

Though it feels a little wrong to have to test for a special value. Maybe methods like isTimeBeforeExpirationPresent() and isGraceAuthsRemainingPresent()? That feels weird too, though. Ideally you'd hand back nullable objects, but that would break existing stuff. I guess I don't really have good solution, so maybe the special -1 is ideal in this case.

dfish3r added a commit that referenced this issue Oct 23, 2017
@dfish3r
Copy link
Member Author

dfish3r commented Oct 23, 2017

Thanks for the report and test data, that should close this out.

@dfish3r dfish3r closed this as completed Oct 23, 2017
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

No branches or pull requests

2 participants