Should SAML metadata EntityDescriptor tag have the md: prefix?

[ClaudioConsolmagno]

Expected Behavior

All tags in the metadata xml have md: or other appropriate prefixes, except for the EntityDescriptor tag. For consistency purposes and better integration with other services, it should be changed to include the md: prefix. E.g.:

<?xml version="1.0" encoding="UTF-8"?>
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="saml2.foobar.com">
    <md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
        <md:KeyDescriptor use="signing">
            <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
                <ds:X509Data>
                    <ds:X509Certificate>foobar=</ds:X509Certificate>
                </ds:X509Data>
            </ds:KeyInfo>
        </md:KeyDescriptor>
        <md:NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress</md:NameIDFormat>
        <md:AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://foobar.com/login/saml2/sso/foobar.com" index="1"/>
    </md:SPSSODescriptor>
</md:EntityDescriptor>

Current Behavior

The default metadata xml file contains an EntityDescriptor tag without the md: prefix. E.g.:

<?xml version="1.0" encoding="UTF-8"?>
<EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="saml2.foobar.com">
    <md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
        <md:KeyDescriptor use="signing">
            <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
                <ds:X509Data>
                    <ds:X509Certificate>foobar=</ds:X509Certificate>
                </ds:X509Data>
            </ds:KeyInfo>
        </md:KeyDescriptor>
        <md:NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress</md:NameIDFormat>
        <md:AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://foobar.com/login/saml2/sso/foobar.com" index="1"/>
    </md:SPSSODescriptor>
</EntityDescriptor>

Context

How has this issue affected you? / What are you trying to accomplish?
Consuming the current metadata file in Azure fails. From testing, Azure fails to process the metadata file when prefixed md: tags are mixed with non-prefixed tags. The fix is to either add md: prefix to EntityDescriptor tag or to remove all tags that have the md: prefix.

Are you aware of any workarounds?
We use Spring Boot 2.7.0 so we are on the latest Spring Security version 5.7.1. Our current workaround is to use the setEntityDescriptorCustomizer() method:

private Saml2MetadataFilter getSaml2MetadataFilter() {
    final var saml2MetadataResolver = new OpenSamlMetadataResolver();
    saml2MetadataResolver.setEntityDescriptorCustomizer(entityDescriptorParameters ->
            ((AbstractXMLObject) entityDescriptorParameters.getEntityDescriptor()).setElementNamespacePrefix(SAMLConstants.SAML20MD_PREFIX)
    );
    final var relyingPartyRegistrationResolver = new DefaultRelyingPartyRegistrationResolver(this.relyingPartyRegistrationRepository);
    final var filter = new Saml2MetadataFilter((request, id) -> relyingPartyRegistrationResolver.convert(request), saml2MetadataResolver);
    filter.setRequestMatcher(new AntPathRequestMatcher(SAML2_METADATA_URL, HttpMethod.GET.name()));
    return filter;
}

This isn't too bad of a workaround but should we be doing this? In any case, this is better than the workaround we had for previous Spring Boot/Security versions where we had to copy the whole OpenSamlMetadataResolver class (as it's final) and change a single line of code.

Additional Information

• I tried to read through SAML spec documents but couldn't myself find exactly what is expected from it and maybe that's the issue, it lies on implementations and Azure seems to be strict that it needs either all have the prefix or none of them.
• Though I don't think Spring should make changes to fix specific client implementation cases, I think it might have been an oversight that EntityDescriptor doesn't have the prefix, it doesn't feel like this was intentional? It looks odd that only this one tag doesn't have it. Using the prefix there would make the xml more consistent in that all tags would have the namespace prefix.

[jzheaux]

@ClaudioConsolmagno, thanks for the report.

I wonder if the error is here:

EntityDescriptor entityDescriptor = build(EntityDescriptor.ELEMENT_QNAME);

It seems to me that it should be:

EntityDescriptor entityDescriptor = build(EntityDescriptor.DEFAULT_ELEMENT_NAME);

Would you be able to provide a PR of the 5.8.x branch that makes this change?

[ClaudioConsolmagno]

The change you suggested is exactly what we had for our previous work around where we copied the whole OpenSamlMetadataResolver.java class (as it's final) in order to change this one line.

Sure, will have a look at making this change PR.

[jzheaux]

The change you suggested is exactly what we had for our previous work

Glad to hear!

we copied the whole OpenSamlMetadataResolver.java class (as it's final) in order to change this one line.

I'm sorry that you ended up copying the class; I imagine we'll be able to backport this fix, so it shouldn't be for long.

I'm hearing a little bit of surprise in your comment about the fact that the class is final, so this might be a good place to reinforce Spring Security's design philosophy.

Generally speaking, Spring Security only opens up classes when doing so simplifies customization. What we don't want to do is open a class for the primary purpose of giving an escape hatch to workaround a bug. When we do that, bugs live for a longer time in the code, which isn't ideal. Given that, many classes begin as final in Spring Security, waiting for a time when there is a clear use case that is best solved by opening it for extension.

[ClaudioConsolmagno]

I'm hearing a little bit of surprise in your comment

No, I'm not surprised at all! I just wanted to give you (or anyone else reading this) context on why I ended up going to great lengths for this one change.

I understand why you'd want to keep the class final and support you on that. I just wished I would have opened this ticket sooner when I first came across this issue. I was reluctant on doing this until I was sure 100% there was no other way or if there was something else I was missing. I finally got time to spend on it this week with our Spring Boot 2.7 + Spring Security OAuth decommissioning (migration to Spring Auth Server) work.

I never contributed to Spring before so will take some time to set it up and make the PR this weekend.

[ClaudioConsolmagno]

Have created the PR now: #11300