RelyingPartyRegistrations does not verify signatures

[OrangeDog]

Describe the bug
When an EntitiesDescriptor, EntityDescriptor, or IDPSSODescriptor is being parsed, any included Signatures must be verified against a supplied trust store. This is especially critical if the metadata is fetched over an insecure channel. However, the methods in RelyingPartyRegistrations do not do this. Nor is it clear how someone might do it after the fact.

The underlying OpenSaml library provides all the necessary functionality. Spring Security just needs to call it. The now unsupported spring-security-saml does this correctly.

To Reproduce
The current API does not provide any way to provide trust material, even if it tried to verify signatures.

Expected behavior
See above.

Additional Information
This may qualify as a security issue, if people were expecting the previous behaviour of SAML metadata being cryptographically verified.

[OrangeDog]

Nor is it clear how someone might do it after the fact.

This I think does a good enough job, at least for me, but may have some flaws. I have no mapping of which metadata sources are expected to be using which signing certificates, so it just trusts all certs for all sources.

It will need some additional logic if some sources (e.g. https/local) should be trusted without signatures.
I'm also not sure in what situation you would be trusting signed RoleDescriptors but not their EntityDescriptor, so it skips checking those.

// Configure trust engine
Collection<Credential> certificates = new ArrayList<>();
keyStore.aliases().asIterator().forEachRemaining(alias -> {
    try {
        if (keyStore.isCertificateEntry(alias)) {
            certificates.add(new BasicX509Credential((X509Certificate) keyStore.getCertificate(alias)));
        }
    } catch (KeyStoreException | ClassCastException ex) {
        throw new RuntimeException(ex);
    }
});
CredentialResolver credentialResolver = new CollectionCredentialResolver(certificates);
KeyInfoCredentialResolver keyInfoResolver = DefaultSecurityConfigurationBootstrap
        .buildBasicInlineKeyInfoCredentialResolver();
SignatureTrustEngine trustEngine = new ExplicitKeySignatureTrustEngine(credentialResolver, keyInfoResolver);
SAMLSignatureProfileValidator profileValidator = new SAMLSignatureProfileValidator();

// Collect signatures from the metadata
Set<Signature> signatures = new HashSet<>();
for (RelyingPartyRegistration reg : relyingPartyRegistrationRepository) {
    if (reg.getAssertingPartyDetails() instanceof OpenSamlAssertingPartyDetails details) {
        EntityDescriptor descriptor = details.getEntityDescriptor();
        if (descriptor.hasParent() && descriptor.getParent() instanceof EntitiesDescriptor parent) {
            if (parent.isSigned()) {
                signatures.add(parent.getSignature());
            } else {
                throw new Saml2Exception("EntitiesDescriptor is not signed for " + descriptor.getEntityID());
            }
        } else if (descriptor.isSigned()) {
            signatures.add(descriptor.getSignature());
        } else {
            throw new Saml2Exception("EntityDescriptor is not signed for " + descriptor.getEntityID());
        }
    } else {
        throw new Saml2Exception("Cannot verify " + reg.getAssertingPartyDetails().getEntityId());
    }
}

// Verify all signatures
X509CertSelector certSelector = new X509CertSelector();
certSelector.setCertificateValid(Date.from(Instant.now()));
CriteriaSet criteria = new CriteriaSet(new EvaluableX509CertSelectorCredentialCriterion(certSelector));
for (Signature signature : signatures) {
    boolean verified;
    try {
        profileValidator.validate(signature);
        verified = trustEngine.validate(signature, criteria);
    } catch (SignatureException ex) {
        throw new Saml2Exception("Metadata validation failed", ex);
    }
    if (!verified) {
        throw new Saml2Exception("Metadata validation failed");
    }
}

[OrangeDog]

Indeed, there's a big warning in the OpenSAML documentation against the design of spring-security-saml-provider:

It is very dangerous to attempt to use parts of the library in isolation without making use of all of its relevant components. In particular, implementing your own XML processing code, using XML parsing classes other than the ParserPool components provided by the library, using your own security processing code, omitting proper support for SAML metadata, etc. are all risky choices that may lead to security flaws and incomplete, unsafe, and ill-advised SAML solutions. The Shibboleth Project discourages such approaches in the strongest possible terms. Use all of it that applies to the task at hand, or use none of it.

[OrangeDog]

A safer approach is to have OpenSAML do the fetch and validation together, as the previous extension did.

Some example configuration:

@Bean(initMethod = "initialize", destroyMethod = "destroy")
public BasicParserPool parserPool() {
    return new BasicParserPool();
}

@Bean
public HttpClient httpClient() {
    return new net.shibboleth.utilities.java.support.httpclient.HttpClientBuilder().buildClient();
}

@Bean
public MetadataFilter metadataFilter(SignatureTrustEngine trustEngine) {
    MetadataFilter roleFilter = new EntityRoleFilter(List.of(IDPSSODescriptor.DEFAULT_ELEMENT_NAME));
    SignatureValidationFilter validationFilter = new SignatureValidationFilter(trustEngine);
    validationFilter.setRequireSignedRoot(METADATA_URI.getScheme().equals("http"));
    validationFilter.setSignaturePrevalidator(new SAMLSignatureProfileValidator());
    MetadataFilterChain filterChain = new MetadataFilterChain();
    filterChain.setFilters(List.of(validationFilter, roleFilter));
    return filterChain;
}

@Bean(initMethod = "initialize", destroyMethod = "destroy")
public HTTPMetadataResolver metadataResolver(HttpClient httpClient) {
    HTTPMetadataResolver resolver = new HTTPMetadataResolver(httpClient, METADATA_URI.toString());
    resolver.setId("default");
    resolver.setRequireValidMetadata(true);
    return resolver;
}

You then need an adaptor to expose the resulting EntityDescriptors to Spring Security.

[jzheaux]

Thanks for the report and the suggested changes. Since this is connected with an earlier ticket, I'll close this as a duplicate and continue from there.