SAML-Toolkits / python3-saml

MIT License
671 stars 302 forks source link

“Invalid issuer in the Assertion/Response”: superfluous check? #329

Closed obscherler closed 1 year ago

obscherler commented 1 year ago

Last week, one of our users ran into a login problem through SAML that resulted in the following error message:

Invalid issuer in the Assertion/Response (expected [SAML entity ID of the IdP we sent them to], got [SAML entity ID of another IdP, associated with their e-mail address, but not configured in our system])

We send them to an identity provider that provides services to many organisations, and sometimes they have users who work for several of them, or change jobs between them, and end up having one or more organisational identities in addition to their private identity.

After checking with the administrators of the identity provider in question, what surfaced is that there is no requirement in the SAML specification that the assertion comes from the same entityID you sent your user to with the AuthnRequest if the entity ID in the response is a valid issuer according the the metadata. They say they have verified this with one of the main authors of said specification.

The bottom line seems to be that the issuer != idp_entity_id check in src/onelogin/saml2/response.py, which seems a reasonable thing to do, is actually superfluous and not compliant with the specs.

Now I don’t know much about SAML, so I am not in a position to advocate for the removal of this check, but I thought it would be sensible to report it here so that people more knowledgeable about SAML can consider the matter if they want, and also so that other people encountering this issue, however unlikely, can find some information about it.

For additional context, there’s a discussion about this in the SimpleSAMLphp project with more information, which resulted in replacing the exception with a warning. I am available if more context is needed.

pitbulk commented 1 year ago

I disagree that is not compliant with the specs in my opinion.

From SAML Spec Core, page 16

<Issuer> [Required]
The SAML authority that is making the claim(s) in the assertion. The issuer SHOULD be unambiguous
to the intended relying parties.
This specification defines no particular relationship between the entity represented by this element
and the signer of the assertion (if any). Any such requirements imposed by a relying party that
consumes the assertion or by specific profiles are application-specific.

...

Depending on the requirements of particular protocols or profiles, the issuer of a SAML assertion may
often need to be authenticated, and integrity protection may often be required. Authentication and
message integrity MAY be provided by mechanisms provided by a protocol binding in use during the
delivery of an assertion (see [SAMLBind]). The SAML assertion MAY be signed, which provides both
authentication of the issuer and integrity protection.

From the security part of the spec:

 The browser/POST profile requires the SAML response carrying SAML assertions to
be signed, thus providing both message integrity and authentication. The Service Provider site MUST
verify the signature and authenticate the issuer.

SAML sets a circle of trusts, if I receive a message that does not belong my circle, I reject it.

lhaemmerle commented 1 year ago

The proposal to remove the superfluous issuer check is in line to the specification that was cited above, basically it's orthogonal to it. Yes, the SP should only accept signed (and ideally even encrypted) assertions from issuers it (uniquely) knows and of course only if the signature is valid. However, the spec does not say that when a user initiates a login with IdP with entityID A that then the assertion also must come from an IdP with entityID A. There are legit use-cases where SPs get (unsolicited) assertions without the user having been first at the SP and without the SP knowing with which IdP the user is going to authenticate.

The only thing that counts is that the assertion is signed by an Issuer that the SP trusts. From the three most used SAML implementations Shibboleth Service Provider, SimpleSAML PHP and Microsoft ADFS none have such a check.

pitbulk commented 1 year ago

@lhaemmerle , @jma

The situation described on the SSP ticket is totally different than the use case here.

On SSP the exception happened when the SP sent an AuthNRequest to IdP A, stored in the status the IdP Entity ID and then while processing the SAMLResponse, checked that the Issuer of such Response matched the IdP Entity ID stored.

python3-saml does not store such initial IdP Entity ID and do later any check. The check you reference in this ticket is done between the Entity ID lf the IdP that generated the SAMLResponse received and the Entity ID of the IdPs that are trusted, a legit check.

SSP and Shibboleth do also this check, otherwise, how SAML settings/metadata of such IdP are retrieved to be managed by the toolkit during the SAMLResponse validation? If they dont recognize the Entity ID, they raise an exception.

pitbulk commented 1 year ago

The only thing that counts is that the assertion is signed by an Issuer that the SP trusts.

That exactly what is done by the check in python3-saml, confirm the Issuer is a trusted one.

lhaemmerle commented 1 year ago

php-saml does not store such initial IdP Entity ID and do later any check.

Ok, then all is good :-) Sorry, based on the initial description I was under the (wrong) impression that there would be such a check. Reading the error message again, I realize that python3-saml apparently did not know the IdP it got the assertion from ("but not configured in our system").

SSP and Shibboleth do also this check, otherwise, how SAML settings/metadata of such IdP are retrieved to be managed by the toolkit during the SAMLResponse validation? If they dont recognize the Entity ID, they raise an exception.

Yes, of course the SP must know (from configuration and/or metadata) which entityIDs it accepts assertions from.

obscherler commented 1 year ago

@pitbulk @lhaemmerle I made another test with another identity provider that is known and properly configured. The user chooses to sign in with IdP A (SWITCH edu-ID), is sent there, is offered to choose between their personal or institution identity (Unil), choose their institution identity (as they’re very well allowed to), and when they return, are “greeted” with the same error:

Invalid issuer in the Assertion/Response (expected [SAML entity ID of SWITCH edu-ID], got [SAML entity ID of Unil])

This exception is verbatim from the python3-saml exception found in the source code, and literally states that it’s comparing the entity ID in the response with exactly one entity ID it’s expecting. Not “the entity ID of one of the IdPs that are trusted.”

The metadata for both SWITCH edu-ID and Unil are identical XML documents containing all the IdPs provided by SWITCH, including their public keys for signature verification, so I don’t know what’s happening. Is Django Third Party Auth not providing the full configuration to python3-saml?