Skip to content

SAML attributes not parsed correctly with prefixed XML elements #8864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JoakimLofgren opened this issue Jul 21, 2020 · 11 comments
Closed

SAML attributes not parsed correctly with prefixed XML elements #8864

JoakimLofgren opened this issue Jul 21, 2020 · 11 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules status: feedback-provided Feedback has been provided type: bug A general bug
Milestone

Comments

@JoakimLofgren
Copy link
Contributor

Describe the bug
When grabbing the email attribute (urn:oid:0.9.2342.19200300.100.1.3) from the SimpleSaml2AuthenticatedPrincipal / DefaultSaml2AuthenticatedPrincipal the output is XML and not the expected email.

Until #8861 is merged, the following workaround is used to access the attributes:

val principal = objectMapper.convertValue(auth.principal, Map::class.java)
val attributes = principal["attributes"] as Map<String, List<String>>

The value returned is:

<?xml version="1.0" encoding="UTF-8"?>
<saml2:AttributeValue 
    xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" 
    xmlns:xs="http://www.w3.org/2001/XMLSchema" 
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:type="xs:anyType">
  [email protected]
</saml2:AttributeValue>

This seems to be due to prefixed XML elements, as the issue does not occur when non-prefixed XML elements that specify the xmlns attribute are used instead.

Google SAML produces XML with prefixed XML elements, e.g.:

<saml2:AttributeStatement>
    <saml2:Attribute Name="urn:oid:0.9.2342.19200300.100.1.3">
        <saml2:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema"
                              xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:anyType">
            [email protected]
        </saml2:AttributeValue>
    </saml2:Attribute>
</saml2:AttributeStatement>

And e.g. https://github.com/amdonov/lite-idp produces non-prefixed XML elements:

<AttributeStatement xmlns="urn:oasis:names:tc:SAML:2.0:assertion">
    <Attribute xmlns="urn:oasis:names:tc:SAML:2.0:assertion" FriendlyName="urn:oid:0.9.2342.19200300.100.1.3"
               Name="urn:oid:0.9.2342.19200300.100.1.3"
               NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
        <AttributeValue xmlns="urn:oasis:names:tc:SAML:2.0:assertion">[email protected]
        </AttributeValue>
    </Attribute>
</AttributeStatement>

This was tested with the versions 5.4.0-M2 and 5.4.0-SNAPSHOT (as of today).

To Reproduce

  • Setup a Google SAML SP (or other SAML software that produces prefixed XML elements).
  • Try to grab the email attribute (urn:oid:0.9.2342.19200300.100.1.3) from SimpleSaml2AuthenticatedPrincipal / DefaultSaml2AuthenticatedPrincipal.

Expected behavior
The email is returned.

@JoakimLofgren JoakimLofgren added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jul 21, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Jul 21, 2020

@JoakimLofgren thanks for the report.

Would you be able to add a unit test that fails in the way that you describe? You could use a static response payload, for example.

@JoakimLofgren
Copy link
Contributor Author

Is there any existing tests I could tweak to do that?

I'll try and take a stab at it tomorrow.

@jzheaux
Copy link
Contributor

jzheaux commented Jul 21, 2020

You can tweak authenticateWhenAssertionContainsAttributesThenItSucceeds in OpenSamlAuthenticationProviderTests.

I haven't played around much with configuring the namespace with OpenSAML, so I don't have specific advice for you along those lines; however, let me know if you run into trouble, and I'm happy to help.

@JoakimLofgren
Copy link
Contributor Author

After debugging it appears that the issue is not that elements are prefixed.

The culprit seems to be this: https://github.com/spring-projects/spring-security/blob/master/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/OpenSamlAuthenticationProvider.java#L428

Doing xsAny.getTextContent(); returns the email correctly, but the this.saml.serialize(xsAny) seems to just convert the whole node to a string.

@jzheaux Do you have any idea what the purpose of that serialize is?

@jzheaux
Copy link
Contributor

jzheaux commented Jul 23, 2020

The serialize is so that applications can customize how to handle xs:any types.

Were you able to develop a unit test? Specifically, I'm curious why the registry returned a marshaller instead of null.

@jzheaux jzheaux self-assigned this Jul 23, 2020
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 23, 2020
@JoakimLofgren
Copy link
Contributor Author

Not yet. Might take a stab at it today if I have the energy/time.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 29, 2020
@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jul 30, 2020
JoakimLofgren added a commit to JoakimLofgren/spring-security that referenced this issue Aug 5, 2020
@JoakimLofgren
Copy link
Contributor Author

@jzheaux Reproduced the issue in a unit test: JoakimLofgren@4f7499b

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 5, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Aug 19, 2020

I was able to reproduce using the test you created, thank you.

I'm thinking that #8010 will be a better vehicle for allowing an application to customize the way that an xs:any is serialized. To that end, I'll remove the code that tries to look up the marshaller as it seems quite a bit more common that an application will just want the content inside the attribute.

@JoakimLofgren
Copy link
Contributor Author

Great. Will it be included in the 5.4 release?

@jzheaux jzheaux added this to the 5.4.0 milestone Aug 25, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Aug 25, 2020

Yes, it will. It should be in the SNAPSHOT. Can you confirm that the fix addresses your issue?

@JoakimLofgren
Copy link
Contributor Author

Oh, I missed your message. I verified it in 5.4.0. It works. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants