search

OpenConext / Stepup-Gateway

Stepup Gateway
Apache License 2.0
6 stars 3 forks source link

Incorrect gateway NameID behaviour #386

Closed phavekes closed 3 days ago

phavekes commented 3 days ago

This issue is imported from pivotal - Originaly created at Jan 13, 2016 by Pieter van der Meulen

The Subject nameID in the Assertion for the SP that is generated by the stepup-gateway must be set to the nameId in the eduPersonTargetedID attribute. The attribute(s) containing the nameID must be passed unmodified. Currently the gateway modifies the attribute(s) containing nameIDs and does not update the Subject.

Current implementation: https://github.com/SURFnet/Stepup-Gateway/blob/1.2.1/src/Surfnet/StepupGateway/GatewayBundle/Service/ProxyResponseService.php#L109

Correct behaviour

The Assertion received from the remote IdP (i.e. OpenConext/SURFconext) contains an "//Assertion/Subject/NameID" element. This element contains the identity of the user for use by the stepup-gateway. In addition the received Assertion contains one (or two) SAML attributes with a NameID. These attributes are for use by the SP. The names of these attributes are "urn:oid:1.3.6.1.4.1.5923.1.1.1.10" and "urn:mace:dir:attribute-def:eduPersonTargetedID". These two attributes are generated by the remote IdP for the SP and, like any other attributes in the response received from the remote idp, these must be copied verbatim in the assertion generated by the stepup-gateway.

A SAML attribute containing a NameID has the following form:

<saml:Attribute Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.10" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
    <saml:AttributeValue>
        <saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent">abcd-some-value-xyz</saml:NameID>
    </saml:AttributeValue>
</saml:Attribute>

The "//Assertion/Subject/NameID" element in the generated assertion must be set to the value of the of the "urn:oid:1.3.6.1.4.1.5923.1.1.1.10" attribute, and if that attribute does not exist to the value of the "urn:mace:dir:attribute-def:eduPersonTargetedID" attribute. If neither of these two attribute exists, this is an error (urn:oasis:names:tc:SAML:2.0:status:Responder, these is no suitable second-level status code available).

The Subject element in the Assertion received from the remote idp may look like this:

<saml:Assertion>
...
    <saml:Subject>
        <saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">urn:collab:person:example.edu:jdoe</saml:NameID>
        ...

So, using the NameID from the SAML Attribute in the example above the Assertion generated by the stepup-gateway for the SP must have the following Subject:

<saml:Assertion>
...
    <saml:Subject>
        <saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent">abcd-some-value-xyz</saml:NameID>
        ...

Rationale

This mechanism of copying the NameID from the attributes allows the remote IdP to control the NameId that is sent to the SP. This allows it to issue different persistent identifiers to the SP. Using the NameID from the subject the stepup-gateway can uniquely identify the user. This unique identifier must be hidden from the SP however as it would allow identifying the user by SPs that would otherwise only receive a transient of persistent identifier.

phavekes commented 3 days ago

Problemen proxying van Assertion vanuit IdP naar Assertion naar SP

1. EPTI wordt overschreven door Subject NameID

De MACE-versie van het EPTI-attribuut, of bij afwezigheid de OID-versie, wordt overschreven met het Subject NameID van de Assertion die van de IdP vandaan komt.

Oorzaak

Er is een logica aanwezig die exact de beschreven handeling uitvoert: er wordt een NameID gegenereerd adhv het Subject van de IdP en deze wordt aan de attributen-array toegevoegd.

Oplossing

Deze logica kan worden verwijderd. https://github.com/SURFnet/Stepup-Gateway/blob/79adaebc740fd15fb55bcb0dee051467926de12b/src/Surfnet/StepupGateway/GatewayBundle/Service/ProxyResponseService.php#L135-L157 Dit is echter een deeloplossing, daar het MACE-EPTI nu aan hetzelfde probleem onderhevig zal zijn als probleem 2:

2. EPTI wordt omgezet naar een string

De OID-versie van het EPTI-attribuut wordt omgezet naar een string-waarde (<saml:AttributeValue xsi:type="xs:string">) (alleen bij afwezigheid van de MACE-versie) met de originele NameID-Value.

Oorzaak

De SAML2-library vertaalt attribuutwaarden naar strings. Complexe values, zoals een volledig NameID gaan verloren, waarbij de tekstuele content van de <AttributeValue/> wordt overgenomen (https://github.com/simplesamlphp/saml2/blob/da4f356dfee6eaabac4fb8afb0de045e571c476b/src/SAML2/Assertion.php#L495). Het schrijven van complexe waarden d.m.v. DOMNodeLists is wel mogelijk.

Oplossing

Bespreken hotfix in GW of fix in SAML2.

3. NameID uit EPTI wordt niet volledig overgenomen in Subject naar SP

Het NameID uit één van de EPTI-attributen wordt niet volledig overgenomen als Subject NameID van de Assertion die naar de SP gaat. De Value wordt overgenomen, maar zaken als Format, SPNameQualifier, etc. gaan verloren. Het Format \'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified\' wordt hardcoded gebruikt.

Oorzaak

Zoals eerder beschreven vertaalt de SAML2-library attribuutwaarden naar strings. Het volledige <NameID/> voor het Subject overnemen uit het EPTI is dan ook niet mogelijk.

Oplossing

Bespreken hotfix in GW of fix in SAML2. (Reinier Kip - Jan 15, 2016)

phavekes commented 3 days ago

Boy Baukema: https://github.com/simplesamlphp/saml2/blob/da4f356dfee6eaabac4fb8afb0de045e571c476b/src/SAML2/Assertion.php#L495

Betreft het parsen van inkomende attribuutwaarden. EB heeft hier geen last van aangezien het de EPTID zet dit werkt wel, maar is redelijk ongedocumenteerd en ongetest gedrag. (Reinier Kip - Jan 15, 2016)

phavekes commented 3 days ago

Complex type parsing is not possible in the SAML2 library. In order to implement the correct behaviour, this needs to be introduced. Once that has been done, the Stepup-Saml-Bundle can be updated to include the new functionality, after which the fix in the Gateway is simple, as we do not need to be concerned about copying the EPTI by hand anymore.

The NameID format must be adjusted to take the value of the EPTI AttribyteValue NameID Format.

After that has been done it should function correctly. (Daan van Renterghem - Jan 22, 2016)

phavekes commented 3 days ago

PR that introduces complex type handling when parsing attribute values in SAML2: https://github.com/simplesamlphp/saml2/pull/60 (Daan van Renterghem - Jan 22, 2016)

phavekes commented 3 days ago

putting issue on-hold until the patch for SAML2 has landed. (Daan van Renterghem - Jan 22, 2016)

phavekes commented 3 days ago

SAML2 has been fixed in v1.8 and v2.1 (Daan van Renterghem - Jan 27, 2016)