dxbjavid commented on code in PR #3281:
URL: https://github.com/apache/cxf/pull/3281#discussion_r3537318508
##########
rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidator.java:
##########
@@ -169,8 +169,8 @@ private void
validateIssuer(org.opensaml.saml.saml2.core.Issuer issuer) throws W
return;
}
- // Issuer value must match (be contained in) Issuer IDP
- if (enforceKnownIssuer && (issuer.getValue() == null ||
!issuerIDP.startsWith(issuer.getValue()))) {
+ // Issuer value must match the configured Issuer IDP
+ if (enforceKnownIssuer && !issuerIDP.equals(issuer.getValue())) {
Review Comment:
That's the commit, yes. As far as I can tell the reason for the prefix check
is that validateIssuer compares the response Issuer against
requestState.getIdpServiceAddress() (set in
AbstractRequestAssertionConsumerHandler), which is the IdP's SSO endpoint URL
rather than its entityID, and the entityID is often a prefix of that address.
The trouble is that a bare startsWith accepts any prefix at all, down to a
single character, so with enforceKnownIssuer on the issuer isn't really pinned
to anything. If exact matching against the service address is too strict for
existing deployments, a separate expected issuer property might be the safer
route, but happy to rework it whichever way you and Colm prefer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
