SAML: OpenSaml4AuthenticationProvider.createDefaultAssertionValidator() should make it easier to add ValidationContext static parameters

[handcraftedbits]

Expected Behavior

Ideally I would be able to create a default assertion validator via createDefaultAssertionValidator() and be able to pass a set of static parameters for the default ValidationContext.

Current Behavior

Currently, I can create a ValidationContext with static parameters OR create a default ValidationContext, not both (with one caveat, see Context).

Context

Let's say I want to create an assertion validator (Converter<AssertionToken, Saml2ResponseValidatorResult>) that has a custom clock skew parameter associated with the validator's ValidationContext. I can call OpenSaml4AuthenticationProvider.createDefaultAssertionValidator(Converter<AssertionToken, ValidationContext> contextConverter):

authenticationProvider.setAssertionValidator(
            OpenSaml4AuthenticationProvider.createDefaultAssertionValidator(assertionToken -> {
                params.put(SAML2AssertionValidationParameters.CLOCK_SKEW, Duration.ofSeconds(100l);
                return new ValidationContext(params);
            }));

The created ValidationContext will only check CLOCK_SKEW, not other important parameters like COND_VALID_AUDIENCES or SC_VALID_RECIPIENTS, because OpenSaml4AuthenticationProvider.createDefaultAssertionValidator(Converter<AssertionToken, ValidationContext> contextConverter) does not let me append to or overwrite the default static parameters, which are defined in the private method OpenSaml4AuthenticationProvider.createValidationContext()

I could of course call OpenSaml4AuthenticationProvider.createDefaultAssertionValidator(), which has a hardcoded CLOCK_SKEW parameter of 5 minutes and then use Saml2ResponseValidatorResult.concat() to add my 100 second CLOCK_SKEW check, but what if I really wanted clock skew to be 6 minutes? The initial hardcoded check would cause a validation failure to occur. If I really, really wanted this 6 minute clock skew I would have to call OpenSaml4AuthenticationProvider.createDefaultAssertionValidator(Converter<AssertionToken, ValidationContext> contextConverter), copy in all the logic from OpenSaml4AuthenticationProvider.createValidationContext(), and change the clock skew parameter.

It's a contrived example but the root issue is that there's no way to call createValidationContext(AssertionToken assertionToken, Consumer<Map<String, Object>> paramsConsumer). This would make life so much easier -- I could reuse the existing logic to create a default ValidationContext and, say, change the value associated with SC_VALID_RECIPIENTS, SC_VALID_IN_RESPONSE_TO, CLOCK_SKEW, etc. without affecting any of the other static parameters.

[jzheaux]

Hi, @handcraftedbits, thanks for the suggestion. There's a lot of detail in your description -- can you please clarify what it is specifically that you are not able to do that you need to do? Knowing this will help guide us to the right change.

[handcraftedbits]

@jzheaux the long and short of it is that you can't create the default assertion validator and overlay param changes on top of it. You can create your own assertion validator completely from scratch OR create the default assertion validator and concat params to it.

Specifically, I want to create an assertion validator with all the default params EXCEPT clock skew, which is hardcoded to 5 minutes. The problem with concatenating params to the default assertion validator is, let's say I want a clock skew of 6 minutes. The current, hardcoded clock skew in OpenSaml4AuthenticationProvider.createDefaultAssertionValidator() would reject that before the concatenated param could be evaluated. I realize such a long clock skew is dumb, but this is a thing we let customers do.

My proposal is: make createValidationContext() not private so I would be able to create a default ValidationContext with the option of overriding/appending some of the default params you add in createValidationContext().

[jzheaux]

@handcraftedbits, thanks, makes sense.

I think it will pay dividends to leave createValidationContext private and allow the class to continue to evolve along with OpenSAML. Instead, I think that exposing a Converter like so will offer the same level of control:

public static Converter<AssertionToken, ValidationContext> createDefaultAssertionValidationContext();

This will allow for constructs like:

Converter<AssertionToken, ValidationContext> delegate = createDefaultAssertionValidationContext();
Converter<AssertionToken, Saml2ResponseValidatorResult> validator = createDefaultAssertionValidator((token) -> {
	ValidationContext context = delegate.convert(token);
	Map<String, Object> params = new HashMap<>(context.getStaticParameters());
	params.put(SAML2AssertionValidationParameters.CLOCK_SKEW, Duration.ofMinutes(6));
	return new ValidationContext(params);
});

This approach gives a lot of flexibility to applications to mutate the map in any way as well as having all the same context that createValidationContext does.

Would exposing the default ValidationContext strategy address your issue, and if so, would you be able to submit a PR to add it?

[jzheaux]

Either way, in the mean time, there is a fair amount of complexity inside createValidationContext that you might not need. For example, in 5.8.0-M2, you can do:

Converter<AssertionToken, ValidationContext> context = (assertionToken) -> {
	Saml2AuthenticationToken token = assertionToken.token;
	RelyingPartyRegistration registration = token.getRelyingPartyRegistration();
	Map<String, Object> params = new HashMap<>();
	params.put(SC_VALID_IN_RESPONSE_TO, token.getAuthenticationRequest().getId());
	params.put(COND_VALID_AUDIENCES, Collections.singleton(registration.getEntityId()));
	params.put(SC_VALID_RECIPIENTS, Collections.singleton(registration.getAssertionConsumerServiceLocation()));
	params.put(VALID_ISSUERS, Collections.singleton(registration.getAssertingPartyDetails().getEntityId()));
	params.put(CLOCK_SKEW, Duration.ofMinutes(6));
	return new ValidationContext(params);
};
Converter<AssertionToken, Saml2ResponseValidatorResult> validator = createDefaultAssertionValidator(context);

[handcraftedbits]

@jzheaux

Would exposing the default ValidationContext strategy address your issue, and if so, would you be able to submit a PR to add it?

Yes that would work I think. I'll get a PR up.

[marcusdacoregio]

Hi @handcraftedbits. Have you had the chance to work on the PR?

Do not hesitate to contact us if you need any support.

[jzheaux]

Added a commit that allows doing the following:

Duration duration = Duration.ofMinutes(6);
OpenSaml4AuthenticationProvider provider = new OpenSaml4AuthenticationProvider();
Consumer<Map<String, Object>> parameters = (params) -> params.put(CLOCK_SKEW, duration);
provider.setAssertionValidator(createDefaultAssertionValidatorWithParameters(parameters));

and also deprecates createDefaultAssertionValidator(Converter). I committed this to 5.8 because it's arguably a bug that the deprecation of setResponseTimeValidationSkew is incomplete.