Saml Enhancements

[jzheaux]

This PR is for two different features, both requested by @OrangeDog.

With this change, applications can specify query parameters in the authenticationRequestUri value through a new method: authenticationRequestUriQuery:

http
    .saml2Login((saml2) -> saml2
        .authenticationRequestUri("/saml/authenticate?registrationId={registrationId}")
    )

Also, this PR introduces RelyingPartyRegistrationsDecoder, an interface that allows for making what is otherwise hidden behind RelyingPartyRegistrations configurable. For example, before this change, you would do:

RelyingPartyRegistration.Builder builder = RelyingPartyRegistrations.fromMetadataLocation("uri");

With this change, you can do:

var registrationsDecoder = new OpenSamlRelyingPartyRegistrationsDecoder();
Collection<RelyingPartyRegistration.Builder> builders = registrationsDecoder.decode("uri");

By default, the registration ids are URL-safe.

OpenSamlRelyingPartyRegistrationsDecoder itself comes with support for verifying the metadata signature via provided keys, specifying an id generator for registrationId values, and a post-processor for the corresponding AssertingPartyDetails.Builder:

CredentialResolver credentials = new CollectionsCredentialResolver(Set.of(new BasicCredential(publicKey())));
var registrationsDecoder = new OpenSamlRelyingPartyRegistrationsDecoder(credentials);
registrationsDecoder.setRegistrationIdGenerator(EntityDescriptor::getEntityID);
registrationsDecoder.setAssertingPartyDetailsCustomizer((parameters) -> {
    EntityDescriptor descriptor = parameters.getEntityDescriptor();
    AssertingPartyDetails.Builder builder = parameters.getAssertingPartyDetails();
    // ... customize as needed
});
Collection<RelyingPartyRegistration.Builder> builders = registrationsDecoder.decode("uri");

[jzheaux]

@OrangeDog will you please take a look at this PR and see if it addresses the use cases you described in https://github.com/spring-projects/spring-security/issues/15090, https://github.com/spring-projects/spring-security/issues/15017, and https://github.com/spring-projects/spring-security/issues/15018 (related to https://github.com/spring-projects/spring-security/issues/12116)?

[OrangeDog]

• #15090 ❌ - I still need correct expiry-aware metadata refreshing, so will continue to adapt an org.opensaml.saml.metadata.resolver.MetadataResolver . Thus I still need a way to turn a single EntityDescriptor into a RelyingPartyRegistration.
• #15017 ✔- This looks like it works. I will have a look later to test it. However, what happens if the default encoding turns two different entity ids into the same registration id?
• #12116 ❓ - The default configuration needs to be secure (and the current lack of security should have a CVE filed). If untrusted metadata is provided (no signature from a remote source, no trust material available) it needs to fail. It should have the same behaviour as the deprecated implementation which spent a lot of time ensuring correct security. But for me, as I still need MetadataResolver, I will be getting that to do it for me (Shibboleth have spent even more time getting the security correct) rather than use your code.

[OrangeDog]

I've tested what I'm using (see above) and it's definitely an improvement 👍 . Please make OpenSamlRelyingPartyRegistrationsDecoder.convert(EntityDescriptor) public though.

For the signing, perhaps it should always use the filter, but with an empty/system trust store if none is provided. Then when decoding, set requireSignedRoot based on the URI scheme, or provide some customisation for what is trusted (with a safe default).

---

With this change, applications can specify query parameters in the authenticationRequestUri value through a new method: authenticationRequestUriQuery:

Rather than parse a query string template, perhaps a method that just takes the parameter name would be better?

http
    .saml2Login((saml2) -> saml2
        .authenticationRequestUri("/saml/login")
        .authenticationRequestUriParam("peerEntityId")
    )

or

http
    .saml2Login((saml2) -> saml2
        .authenticationRequestUri((uri) -> uri
            .path("/saml/login")
            .queryParam("peerEntityId")
        )
    )

[jzheaux]

But for me, as I still need MetadataResolver, I will be getting that to do it for me (Shibboleth have spent even more time getting the security correct) rather than use your code. ... Please make OpenSamlRelyingPartyRegistrationsDecoder.convert(EntityDescriptor) public though.

I think what may be better than this is enhancing OpenSamlAssertingPartyDetails#withEntityDescriptor to do the work of preparing the asserting party details instead of the decoder doing that. Then I think you'd not use the decoder at all.

[OrangeDog]

I think what may be better than this is enhancing OpenSamlAssertingPartyDetails#withEntityDescriptor to do the work of preparing the asserting party details instead of the decoder doing that. Then I think you'd not use the decoder at all.

I don't care where that code is, as long as I can get at it without reflection hacks during my EntityDescriptor -> RelyingPartyRegistration conversion in my findUniqueByAssertingPartyEntityId adaptor.

[jzheaux]

Thanks for all the feedback and insight, @OrangeDog. I'm enjoying this collaboration with you.

Specifically, your comments about MetadataResolver inspired a different design that is hopefully more useful in the way of creating a refreshing, expiry-aware repository.

Rather than parse a query string template, perhaps a method that just takes the parameter name would be better?

I'd like to keep the placeholder there as I think a nice feature one day would be to allow for other placeholders that are already supported in other parts of Spring Security like {relyingPartyEntityId} and {assertingPartyEntityId}. Given that, I think there is less cognitive load for the user to supply the query as a string, though I'd be open to doing something like:

.authenticationRequestUri("/saml2/authenticate", "registrationId={registrationId}")

Such just didn't seem to buy very much since I'm already parsing the =.

The default configuration needs to be secure (and the current lack of security should have a CVE filed). If untrusted metadata is provided (no signature from a remote source, no trust material available) it needs to fail.

I'd prefer to leave the defaults as they are, which means that if no credentials are provided, then no signature verification is performed. This is also what MetadataResolver in OpenSAML does by default.

I disagree that this is a CVE on the grounds that HTTPS is the standard for secure endpoints across Spring Security. When deviating from that standard, Security tries to make the secure option the easy one. But going further than that, we can easily get in the way -- for example. it's not true that every HTTP endpoint requires trust material as http://localhost/ does not.

[jzheaux]

Also, in the meantime, I've addressed #15090 in the way we already discussed and committed that to main.

[OrangeDog]

a different design that is hopefully more useful

Very interesting. I'll have a deeper look at that...

Yes, that looks like it would work for common cases. Though I don't think it should be described as "expiry aware", because that entirely depends on the MetadataResolver you give it. Not all subclasses do automatic refreshing.

For the default one your builder constructs, you may want to pass your own Timer so it can have a name identifying it.

I'm using custom Criteria queries too, so this isn't a direct drop-in, but I think I can make use of this and delete more of my code.

HTTPS is the standard for secure endpoints

Not in SAML. That's why everything has XML-level signatures and optional encryption.

it's not true that every HTTP endpoint requires trust material

That may be true, but disabling security should be a conscious choice by the developer, not the default.

[OrangeDog]

I think this needs more test coverage, in particular the parts that didn't work that I pointed out above.