match saml sso issuer exactly instead of by prefix#3281
Conversation
| // 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())) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
It'll be a week or so until I get to look at this, sorry for the delay
3 participants
validateIssuer in the SAML 2.0 SSO response validator checks the received issuer against the configured issuer IDP with issuerIDP.startsWith(issuer.getValue()), so any value that is only a prefix of the expected issuer satisfies the enforceKnownIssuer check, right down to a single character. That means the known-issuer control does not really pin the issuer, and where a service provider trusts more than one identity provider it weakens the binding between an assertion and the IdP it is meant to have come from. Compare the issuer to the configured value exactly instead, which is how SAML entityIDs are meant to be matched.