Repository navigation
fix: LDAP password-policy response control parser — warning/error tag dispatch and zero-length error element - #52403
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation matches the LDAP encoding requirements and includes comprehensive regression coverage.
Pull request overview
Fixes LDAP password-policy response parsing for constructed warning wrappers and malformed zero-length errors.
Changes:
- Correctly skips spec-compliant constructed warnings before parsing errors.
- Handles zero-length error values without propagating exceptions.
- Adds regression tests and the constructed-tag constant.
File summaries
| File | Description |
|---|---|
PasswordPolicyControl.java |
Fixes warning dispatch and malformed error handling. |
BERDecoder.java |
Adds the constructed BER tag-form constant. |
PasswordPolicyControlTest.java |
Covers wrapped warnings, subsequent errors, and zero-length values. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Thank you for the PR. Please add a sign-off to the commit as outlined in the CONTRIBUTING.md. Once you do that, the DCO check will turn green.
@tsaarni - as the original author, can you please comment on this PR? Also on the suggested parser error handling of logging vs. throwing an exception?
| } catch (BERDecoder.DecodeException | NumberFormatException e) { // NumberFormatException: zero-length error element defeats BigInteger | ||
| logger.errorf("Failed to parse PasswordPolicyResponseValue: %s", e.getMessage()); |
There was a problem hiding this comment.
Instead of logging a warning, a unusaleable data block might better be handled by throwing a NamingException as long as we're sure that this is a problem. This would allow Keycloak a safe-by-default approach, where a login would fail if the data can't be parsed.
See below the JavaDoc.
/**
* Creates a control using this control factory.
*<p>
* The factory is used by the service provider to return controls
* that it reads from the LDAP protocol as specialized control classes.
* Without this mechanism, the provider would be returning
* controls that only contained data in BER encoded format.
*<p>
* Typically, {@code ctl} is a "basic" control containing
* BER encoded data. The factory is used to create a specialized
* control implementation, usually by decoding the BER encoded data,
* that provides methods to access that data in a type-safe and friendly
* manner.
* <p>
* For example, a factory might use the BER encoded data in
* basic control and return an instance of a VirtualListReplyControl.
*<p>
* If this factory cannot create a control using the argument supplied,
* it should return null.
* A factory should only throw an exception if it is sure that
* it is the only intended factory and that no other control factories
* should be tried. This might happen, for example, if the BER data
* in the control does not match what is expected of a control with
* the given OID. Since this method throws {@code NamingException},
* any other internally generated exception that should be propagated
* must be wrapped inside a {@code NamingException}.
*
* @param ctl A non-null control.
*
* @return A possibly null Control.
* @throws NamingException If {@code ctl} contains invalid data that prevents it
* from being used to create a control. A factory should only throw
* an exception if it knows how to produce the control (identified by the OID)
* but is unable to because of, for example invalid BER data.
*/
public abstract Control getControlInstance(Control ctl) throws NamingException;
0e72b0d to
b0531ef
Compare
|
Thanks for the review — the commit is signed off now, so the DCO check should turn green. On logging vs. throwing: fail-closed via If we go that way, I'd suggest two scoping points:
Happy to rework the PR in that direction once you and @tsaarni pick one. |
There was a problem hiding this comment.
Thanks @0xCcSec for spotting the parser bug. Looks good to me, just one comment online.
Regarding the throw vs error log:
When this parser runs, the bind response itself was successful, so depending on interpretation allowing login is ok.
-
The whole password policy extension is non-critical (=LDAP server can ignore it).
-
The spec only has a MAY requirement for the client to act on
warninganderror(link)For every bind response received, the client checks the resultCode of the bindResponse and checks for a passwordPolicyResponse control to determine if any of the following conditions are true and MAY prompt the user accordingly.
That said, when a user enables this feature in Keycloak, they expect the policy extension to be enforced. From that perspective, I'm ok denying the login when parsing fails.
However, I consider this a 500 internal server error rather than an authentication failure, since it is an implementation bug somewhere. The authentication itself succeeded and the bind was successful. Authentication failure can mislead the troubleshooting efforts from the bug.
Also clarification regarding the consequences discussed in the report:
OpenLDAP does not set both error + warning at the same time, so this bug was not triggered and was previously missed. As the spec mentions
Typically, only either a warning or an error will be encoded though there may be exceptions. For example, if the user is required to change a password after the password administrator set it, and the password will expire in a short amount of time, the control may include the timeBeforeExpiration warning and the changeAfterReset error.
If another LDAP server sends both the timeBeforeExpiration warning and the changeAfterReset error, the forced password change will be suppressed, as mentioned in this PR. However, it will not misread a warning as changeAfterReset, because read offset does not advance after unrecognized 0xA0 tag.
| // A tag applied to a CHOICE is always explicit (X.690), so compliant encoders | ||
| // wrap the warning in a CONSTRUCTED [0] element holding the inner | ||
| // timeBeforeExpiration [0] / graceAuthNsRemaining [1] value. Warning values are | ||
| // not used; skip the whole element. The bare primitive [0] form is | ||
| // non-compliant but historically tolerated. | ||
| if (ber.isNextTag(BERDecoder.TAG_CLASS_CONTEXT_SPECIFIC, BERDecoder.TAG_FORM_CONSTRUCTED, 0) // warning [0] CHOICE, spec-compliant form | ||
| || ber.isNextTag(BERDecoder.TAG_CLASS_CONTEXT_SPECIFIC, BERDecoder.TAG_FORM_PRIMITIVE, 0)) { // legacy bare form |
There was a problem hiding this comment.
I think the explanations are unnecessary as well as keeping the "legacy" bug. LLM tends to be verbose and only add code, but I think those can be removed.
… and zero-length error element The ppolicy response control grammar defines warning as a CHOICE of timeBeforeExpiration [0] or graceAuthNsRemaining [1]. A tag applied to a CHOICE is always explicit (X.690), so compliant encoders wrap the warning in a constructed [0] element. The parser only skipped a bare primitive [0] warning, so a wrapped graceAuthNsRemaining warning (context tag [1] inside) fell through and was parsed as the error [1] ENUMERATED that shares the same wire tag — a grace-login warning was misread as changeAfterReset (forcing a password change the directory never mandated), and a warning preceding a real error consumed its tag position, silently suppressing a directory- mandated forced password change. Skip both the spec-compliant constructed [0] wrapper and the legacy bare primitive [0] form, so a following error [1] is always examined. Also treat a zero-length error element as a decode failure: BigInteger on empty bytes throws NumberFormatException, which escaped the constructor's catch clause and failed the login with HTTP 500. Closes keycloak#52400 Signed-off-by: 0xCcSec <zcc890826@gmail.com>
b0531ef to
72637a3
Compare
|
Thanks for the review and the clarifications, @tsaarni. Addressed the inline comment: the verbose explanations and the legacy bare primitive You're right on both corrections, and I've double-checked against our reproduction notes:
|
There was a problem hiding this comment.
🟡 Changes recommended
Legacy primitive warning support was replaced rather than preserved, potentially suppressing a following password-reset error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| try { | ||
| ber.startSequence(); // PasswordPolicyResponseValue ::= SEQUENCE | ||
| if (ber.isNextTag(BERDecoder.TAG_CLASS_CONTEXT_SPECIFIC, BERDecoder.TAG_FORM_PRIMITIVE, 0)) { // warning [0] CHOICE | ||
| if (ber.isNextTag(BERDecoder.TAG_CLASS_CONTEXT_SPECIFIC, BERDecoder.TAG_FORM_CONSTRUCTED, 0)) { // warning [0] CHOICE |
|
This trade-off is deliberate per @tsaarni's review above: he asked for the legacy bare primitive Rationale for dropping it:
That said, the behavioral change Copilot flags is real: a bare warning followed by an error now leaves the error unexamined (suppression) instead of being tolerated. If the maintainers would rather keep tolerating the legacy form for robustness, I'm happy to restore the branch plus a regression case — one line each way. |
|
No LDAP server never sent the warning as a |
|
Exactly — thanks for confirming, @tsaarni. With no server ever emitting the primitive form, there is nothing to be compatible with; the branch stays out. (This also settles the Copilot thread above: what it proposed to preserve was the decoding bug itself, not compatibility.) |
|
I manually tested this, and the changes look good. I prepared some test setups here: keycloak/keycloak-quickstarts#790 Having a safe side fallback is IMHO difficult and needs more discussion: Throwing an exception is not good as it would not allow the user to log in. Maybe always to require the password change, but this would then become more complicated once other elements might be parsed. Let's move on with this one, and have a discussion in a separate issue. |
Closes #52400
Problem
The LDAP password-policy response control parser (
PasswordPolicyControl) has two defects, triaged as a weakness by the Keycloak security team:1. warning/error tag dispatch. The ppolicy grammar defines
warningas a CHOICE oftimeBeforeExpiration [0]/graceAuthNsRemaining [1]. A tag applied to a CHOICE is always explicit (X.690), so compliant encoders (e.g. OpenLDAP) wrap the warning in a constructed [0] element (0xA0) holding the inner value. The parser only skipped a bare primitive[0]element, so a wrappedgraceAuthNsRemainingwarning was not recognized as a warning at all — its inner context[1]tag shares the wire tag witherror [1] ENUMERATED(0x81). Two consequences:changeAfterReseterror, forcing a password change the directory never mandated;2. Zero-length error element. A zero-length
error [1]element (30 02 81 00) madenew BigInteger(...)throwNumberFormatException, which escaped the constructor'scatch (BERDecoder.DecodeException)clause and failed the login with HTTP 500.Fix
PasswordPolicyControl: skip the spec-compliant constructed[0]warning wrapper in addition to the legacy bare primitive[0]form, so a followingerror [1]is always examined; catchNumberFormatExceptionalongsideDecodeExceptionso a malformed error element is logged as a decode failure instead of breaking the login flow.BERDecoder: add the missingTAG_FORM_CONSTRUCTEDconstant.Tests
Four regression cases in
PasswordPolicyControlTest:graceAuthNsRemaining(2)is not misread aschangeAfterReset(no forged forced password change);changeAfterReseterror still raises it (no suppression);timeBefo 8000 reExpirationparses cleanly;All 12 tests of the class pass locally (standalone compile+run of
BERDecoder,PasswordPolicyControl,PasswordPolicyControlTestagainst JDK 21 / JUnit 4.13.2).Reported via the Keycloak security team; they invited an upstream contribution.
AI assistance disclosure (per CONTRIBUTING.md): this fix was drafted with AI-agent assistance under human direction — analysis of the defect, the patch, and the tests were produced by an AI agent and reviewed/verified by the human operator, who understands the change end to end (independent runtime reproductions of both defects exist against 26.7.2). Released under Apache License 2.0.