Repository navigation
Enforce at least one bearer subject confirmation method and also fail for unsupported methods - #55013
Conversation
… for unsupported methods Closes keycloak#54711 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The documentation incorrectly implies that unsupported subject-confirmation methods cause login failure even when a valid bearer confirmation is present.
What changed in this PR
This PR tightens SAML identity-broker validation to require a valid bearer subject confirmation, preventing holder-of-key-only assertions from being accepted without proof of key possession.
Changes:
- Reject assertions without a bearer confirmation while preserving acceptance of mixed-method assertions with a valid bearer confirmation.
- Add regression tests and helpers for unsupported methods and expired bearer data.
- Document the bearer-confirmation requirement in administrator and upgrade guides.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/saml/SamlResponseDocument.java |
Adds helpers to modify subject confirmations and expire their data. |
tests/base/src/test/java/org/keycloak/tests/broker/saml/KcSamlBrokerTest.java |
Tests unsupported-only and mixed-method assertions. |
services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java |
Rejects assertions with no bearer confirmation. |
docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc |
Adds an upgrade note about unsupported confirmation methods. |
docs/documentation/server_admin/topics/identity-broker/saml.adoc |
Documents subject-confirmation support for SAML brokering. |
Review effort: Balanced
| {project_name} accepts the assertion only if a bearer confirmation is present and contains a `<SubjectConfirmationData>` element that validates, including the recipient, the `InResponseTo` value, and the validity window. | ||
|
|
||
| {project_name} does not support a subject confirmation method other than `bearer`. | ||
| Any other unsupported method is rejected and the login fails. |
ssilvert
left a comment
There was a problem hiding this comment.
Looks good. The port matches the 26.6 fix, and the new tests cover the holder-of-key-only, unknown-method-only, bearer + holder-of-key, and expired-bearer + holder-of-key cases.
Two doc comments inline: one wording fix that I think is worth taking, and one nit on bullet placement.
The External links check failure is https://developer.linkedin.com returning 403, which is unrelated to this PR.
| {project_name} accepts the assertion only if a bearer confirmation is present and contains a `<SubjectConfirmationData>` element that validates, including the recipient, the `InResponseTo` value, and the validity window. | ||
|
|
||
| {project_name} does not support a subject confirmation method other than `bearer`. | ||
| Any other unsupported method is rejected and the login fails. |
There was a problem hiding this comment.
This reads as if an assertion carrying both a bearer and a holder-of-key confirmation is rejected. It is accepted: non-bearer confirmations are ignored as long as a valid bearer one is present, which is what testBearerAndHolderOfKeySubjectConfirmationIsAccepted asserts.
| Any other unsupported method is rejected and the login fails. | |
| Confirmations using any other method are ignored, and the login fails unless a valid `bearer` confirmation is also present. |
|
|
||
| * The assertion's `Subject` must contain a `SubjectConfirmation` with `Method` set to Bearer, and it must carry a `SubjectConfirmationData` element. A missing `SubjectConfirmation`, or one without `SubjectConfirmationData`, is now rejected; previously it was silently ignored. If the assertion carries multiple Bearer confirmations, they are treated as alternatives and only one of them needs to validate. | ||
| * `SubjectConfirmationData` must carry a `NotOnOrAfter` attribute. | ||
| * Other subject confirmation methods, including holder-of-key, are not supported; an assertion without a valid bearer confirmation is rejected. |
There was a problem hiding this comment.
Nit: this bullet lands between the NotOnOrAfter and Recipient bullets, splitting the three "SubjectConfirmationData must carry ..." items. It would read better directly after the first bullet (the one about the Bearer SubjectConfirmation) or at the end of the list.
|
I'm sending an update to docs as a follow-up. |
Closes #54711