Possible issue with SAML metadata merge in pac4j >= 6.1.2

[Nathan Cailbourdin]

Hello everyone,

I’ve been encountering an issue since the latest CAS upgrade, which appears to be related to pac4j-saml. After each CAS server restart, some lines in the SAML client metadata files are duplicated (especially the SingleLogoutService and AssertionConsumerService entries).

This behavior seems to have been introduced by the following commit : https://github.com/pac4j/pac4j/commit/4ecf729b9de59396c33fef932883c3e8d0dc0ae1

The conditions required to reproduce the issue are as follows:
- An SAML2 configuration with setForceServiceProviderMetadataGeneration(false)
- Metadata generation using a mechanism that supports merge (e.g. SAML2FileSystemMetadataGenerator)
- Generating the metadata once (with it being saved to a file)
- Run the same generation code again

On the second execution, the merge process doesn’t behave correctly: pac4j believes some lines are missing and adds them again, even though they already exist. As a result, each server restart keeps appending these entries, causing the metadata files to grow rapidly.

The issue seems to come from the merge method in the BaseSAML2MetadataGenerator class:

currentSP.getSingleLogoutServices().forEach(service -> {
    if (!existingSP.getSingleLogoutServices().contains(service)) {
        service.detach();
        existingSP.getSingleLogoutServices().add(service);
        logger.debug("Adding single logout service {} to existing entity", service.getLocation());
        currentEntityHasChanged.set(true);
    }
});
currentSP.getAssertionConsumerServices().forEach(service -> {
    if (!existingSP.getAssertionConsumerServices().contains(service)) {
        service.detach();
        service.setIndex(existingSP.getAssertionConsumerServices().size());
        existingSP.getAssertionConsumerServices().add(service);
        logger.debug("Adding assertion consumer service {} to existing entity", service.getLocation());
        currentEntityHasChanged.set(true);
    }
});

The problem is that .contains() is used on AssertionConsumerService and SingleLogoutService objects, which do not override equals(). Therefore, the code always assumes the entries are missing and adds them again.

Here is a unit test that reproduces the issue (you may need to run another test before to generate the metadata file in target/, if the file is missing the test passes, if the file is present the test fails):

@Test
public void resolveServiceProviderMetadataViaExistingClasspathWithMerge() {
    var httpClient = new SAML2HttpClientBuilder();
    httpClient.setConnectionTimeout(Duration.ofSeconds(1));
    httpClient.setSocketTimeout(Duration.ofSeconds(1));
    val config = new SAML2Configuration();
    config.setHttpClient(httpClient.build());
    config.setKeystorePath("target/keystore.jks");
    config.setKeystorePassword("pac4j");
    config.setPrivateKeyPassword("pac4j");
    config.setSignMetadata(true);
    config.setForceKeystoreGeneration(true);
    config.setForceServiceProviderMetadataGeneration(false);
    config.setServiceProviderMetadataResource(new FileSystemResource("target/sample-sp-metadata.xml"));
    config.setServiceProviderEntityId("urn:mace:saml:pac4j.org");
    config.setSingleSignOutServiceUrl("http://localhost:8080/callback?client_name=SAML2Client&logoutendpoint=true");
    config.setIdentityProviderMetadataResource(new ClassPathResource("idp-metadata.xml"));
    var attribute =
        new SAML2ServiceProviderRequestedAttribute("urn:oid:1.3.6.1.4.1.5923.1.1.1.6", "eduPersonPrincipalName");
    attribute.setServiceLang("fr");
    attribute.setServiceName("MySAML2ServiceProvider");
    config.getRequestedServiceProviderAttributes().add(attribute);
    config.init();
    final SAML2MetadataResolver metadataResolver = new SAML2ServiceProviderMetadataResolver(config);
    assertNotNull(metadataResolver.resolve());
    try {
        EntityDescriptor descriptor = metadataResolver.resolve().resolveSingle(
            new CriteriaSet(new EntityIdCriterion("urn:mace:saml:pac4j.org"))
        );
        var sp = descriptor.getSPSSODescriptor(SAMLConstants.SAML20P_NS);
        assertEquals(4, sp.getSingleLogoutServices().size());
        assertEquals(1, sp.getAssertionConsumerServices().size());
    } catch (ResolverException e) {
        throw new RuntimeException(e);
    }
}

This test shows that the metadata file should not be modified, since it already contains the correct SingleLogoutService and AssertionConsumerService entries. However, when the test runs, those lines get duplicated (so the asserts are failing).

Here’s an example of a potential fix:

currentSP.getSingleLogoutServices().forEach(service -> {
    var alreadyPresent = existingSP.getSingleLogoutServices().stream()
        .anyMatch(s ->
            Objects.equals(s.getLocation(), service.getLocation()) && Objects.equals(s.getBinding(), service.getBinding())
        );
    if (!alreadyPresent) {
        ...
    }
});
currentSP.getAssertionConsumerServices().forEach(service -> {
    var alreadyPresent = existingSP.getAssertionConsumerServices().stream()
        .anyMatch(s ->
            Objects.equals(s.getLocation(), service.getLocation()) && Objects.equals(s.getBinding(), service.getBinding())
        );
    if (!alreadyPresent) {
        ...
    }
});

With this change, the test passes: metadata remains unchanged because it is already correct — which I think is the expected behavior.

Does this look like a real issue in pac4j, or am I missing something?
I’m not a pac4j expert and this is my first time working with it, but I started noticing this odd behavior since CAS 7.3 (which updated pac4j version).
Does the proposed fix and the test case seem reasonable to you?

Thanks in advance for your feedback.

Best regards
Nathan

[Jérôme LELEU]

Hi,

This fully makes sense to me.

Would you mind submitting a PR with the fix and the related test?

After the merge, with the version 6.3.1-SNAPSHOT used in place in CAS v7.3.1, you should be able to confirm the fix more globally.

Thanks.

Best regards,

Jérôme

--
You received this message because you are subscribed to the Google Groups "Pac4j development mailing list" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-dev+...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/pac4j-dev/f89af781-63d5-40c0-b683-28fd4133e021n%40googlegroups.com.

[Nathan Cailbourdin]

Thanks for your quick response.
I just submitted a PR here : https://github.com/pac4j/pac4j/pull/3666

[Jérôme LELEU]

Hi,

Great! I merged the PR and updated the release notes.

A version 6.3.1 release will be cut at the beginning of December.

Thanks.

Best regards,

Jérôme

To view this discussion visit https://groups.google.com/d/msgid/pac4j-dev/ef0a56aa-6391-494a-9769-10fba7f45a20n%40googlegroups.com.