Spring-security: SAML: Add RequestedAuthnContext to AuthnRequest in OpenSamlAuthenticationRequestFactory

[Primedo]

Summary

Add RequestedAuthnContext with Comparison and AuthnContextClassRef to require a certain authentication from the IdP.

Actual Behavior

OpenSamlAuthenticationRequestFactory creates the AuthnRequest with an Saml2AuthenticationRequest, but isn't possible to modify the AuthnRequest.

Expected Behavior

Either transport the required information via Saml2AuthenticationRequestContext or allow the modification of the created AuthnRequest before it is serialized.

Version

5.3.0-RELEASE

Additional Information

I am willing to work on this issue, but I am uncertain, what the expected direction could be. Personally I would prefer something like an ObjectPostProcessor where I would also have access to the HttpServletRequest so I could adjust the AuthnRequest according to the current user.

[jzheaux]

Thanks for the suggestion, @Primedo.

I think it makes sense to add support for custom attributes to Saml2AuthenticationRequestContext.

Since AuthenticationRequestFactory is abstracted away from the Servlet API, though, a post processor that includes HttpServletRequest in the contract likely wouldn't work.

What would you think of a Converter<Saml2AuthenticationRequestContext, AuthnRequest> setter on OpenSamlAuthenticationRequestFactory? The converter could use the custom context attributes to control how the AuthnRequest is configured.

[Primedo]

Josh, thank you for your feedback.

This is what I came up with until now and I hope it isn't the complete opposite of what you suggested: https://github.com/spring-projects/spring-security/compare/master...Primedo:gh-8141

I removed Saml2AuthenticationRequest since it was deprecated and I couldn't create a Saml2AuthenticationRequestContext from it.

Since I didn't want to create a new AuthnRequest and wanted to reuse the OpenSamlImplementation to create Objects I created a different converter with again another Context Converter<OpenSamlAuthenticationRequestContext, AuthnRequest>

What do you think, is this going in the right direction and what should I change?

[jzheaux]

Thanks for taking some time to try this out, @Primedo.

Let's see if it's possible to only focus on customizing the AuthnRequest. For example, I don't think we should remove methods in this PR. Indeed, if they must be removed for this feature to be added, then we'd need to wait until 6.0 to do it.

Regarding custom attributes, let's follow the pattern set out in OAuth2AuthorizationContext. Note that there the method is called getAttributes and the builder has a method called attributes that takes a Consumer.

As for a composite OpenSamlAuthenticationRequestContext object, I'm hesitant to add a new type to address this enhancement. Can you elaborate on precisely what you are wanting to add to the AuthnRequest and what additional context you need to be able to configure it correctly?

[Primedo]

@jzheaux, what is your take on this change?
https://github.com/spring-projects/spring-security/compare/master...Primedo:gh-8141

I skipped the suggested Converter for this change.

[jzheaux]

I think I need to have a clearer idea on what inputs are determining what outputs. So far, I understand that you'd prefer to take something from the HttpServletRequest, but I'm not clear on what and on how that manifests itself in the resulting AuthnRequest.

Can you elaborate on precisely what you are wanting to add to the AuthnRequest and what inputs determine what gets added?

[Primedo]

I want the current user (anonymous or authenticated) to authenticate with an exact or better authentication method based on the needed authentication level for the desired action (e. g. second factor to change password, identity card to change address, username-password or better for initial authentication). And RequestedAuthnContext could be used to transport the desired authentication level.

https://www.oasis-open.org/committees/download.php/35711/sstc-saml-core-errata-2.0-wd-06-diff.pdf

Does this explain the requirement?

[jzheaux]

Yes, that helps, @Primedo.

When it's an authenticated user, are the details you need tied to the current Authentication? For example, could you do Authentication current = SecurityContextHolder.getContext().getAuthentication(); and find your inputs from current?

[Primedo]

and find your inputs from current

Yes, that would work.

And for anonymous users it would be helpful to have some context (cookie, URL, session or something else), so that the desired RequestedAuthnContext is known before creating the SAML Request.

[jzheaux]

And for anonymous users it would be helpful to have some context (cookie, URL, session or something else)

Do you have a concrete requirement here? Otherwise, maybe we wait on that aspect for now.

Yes, that would work.

One thing you should be able to do then, which would require no enhancement at all, is for you to extend OpenSAML's AuthnRequestMarshaller, registering it with XMLObjectProviderRegistrySupport.registerObjectProvider. There, you'd have full control over post-processing the AuthnRequest.

[Primedo]

Do you have a concrete requirement here? Otherwise, maybe we wait on that aspect for now.

An anonymous user is on the public site and wants to change his address. Since address change does require an authentication by id card or better this information should be transported to the IdP, otherwise the anonymous user would maybe have to authenticate twice with different methods.

which would require no enhancement at all,

Thanks, is this your preferred solution for this issue?

[jzheaux]

Yes, @Primedo, if you can work with OpenSAML directly for your customization of the AuthnRequest, that would be ideal.

Of course, feel free to reopen this ticket if you run into trouble and want to take another look at changing OpenSamlAuthenticationRequestFactory.

[fpagliar]

This request was actually totally on point about limited support in the creation of AuthNRequests.
See this issue

[jzheaux]

To reiterate, I think it would be helpful to answer the following question:

What specifically do you need to get from HttpServletRequest and how does it affect the AuthnRequest?

[fpagliar]

I understand that SAML is currently on MVP state for Spring Security 5.2, but spring-security-saml is no longer supported, so 5.2 is the only valid option at the moment.
There will be an increasing flow of teams collaborating to achieve feature parity and common migration patterns.

Back to the issue, the goal as a developer using the framework is to be able to customize request creation without re-writing Saml2WebSsoAuthenticationRequestFilter and Saml2AuthenticationRequestFactory.
Instead I want to be able to create and enhanced Saml2AuthenticationRequestContext with the information needed, and use composition on the factory to customize the AuthnRequest but then delegate the rest of the logic (XML creation, signature) on the default implementation.

What specifically do you need to get from HttpServletRequest and how does it affect the AuthnRequest?

Sample scenarios for this case would be:

• Override ForceAuthN based on a querystring attribute. If the SAML response is rejected due to MaxAuthenticationAge, a simple solution and great user experience is to trigger a new SAML flow with ForceAuthN = True to fetch a fresh sign in from the IdP. So on the SAMLResponse error handling, we can redirect back to the entry point with ForceAuthN=true.

• Different connections. We can look at the incoming http request, for example look at private vs public IP addresses and require the external connections to do a 2FA on the IdP by adding AuthnContextClassRef tags.

• Extended Attributes. SAML supports sending custom attributes to the IdP within the request.
  Those would be passed on the original http request and could contain information on what type of resource the user tried to access that triggered the redirect and SP initiated flow, for example.

[jzheaux]

@fpagliar good points, @Primedo as well, and I'm happy to have both of you as collaborators to help discover the right contract here.

I agree that it will likely be helpful to modify the AuthnRequest based on request material. Saml2AuthenticationRequestFactory is abstracted away from the request, though, so I imagine that we'll need three things:

• A way to construct a custom Saml2AuthenticationRequestContext
• A way to provide custom attributes to Saml2AuthenticationRequestContext
• A way to post-process the AuthnRequest

@Primedo and I made some initial progress on these points. Building off of that progress, let me know what you both think of the following contract for the ForceAuthN use case (as it seems pretty simple). Most of this support doesn't exist yet, so it's just for illustration on what might be possible:

@Bean 
Saml2AuthenticationRequestContextResolver authenticationRequestContextResolver() {
    Saml2AuthenticationRequestContextResolver delegate =
            new DefaultSaml2AuthenticationRequestContextResolver();
    return request -> {
        Saml2AuthenticationRequestContext.Builder builder = delegate.resolve(request);
        return builder.attribute("is-force-authn", request.getParameter("force"));
    }
}

@Bean 
Saml2AuthenticationRequestFactory authenticationRequestFactory() {
    OpenSamlAuthenticationRequestFactory factory =
            new OpenSamlAuthenticationRequestFactory();
    factory.setAuthnRequestPostProcessor((context, authnRequest) -> {
        if (StringUtils.hasText(context.getAttribute("is-force-authn"))) {
            authnRequest.setForceAuthn(true);
        }
    });
    return factory;
}

The idea here is that the filter would call the Saml2AuthenticationRequestContextResolver, get a Saml2AuthenticationRequestContext.Builder, build it, and pass it to the Saml2AuthenticationRequestFactory. The factory would use the post-processor (a BiConsumer, I'm thinking), to post-process the initial AuthnRequest that the factory constructed.

The SAML 2.0 specification is obviously large and I'd prefer to manage Saml2AuthenticationRequestContext's growth carefully. It's tricky to know where to draw the line on what properties to introduce and what to leave out. So, what I like about this approach is it adds a small amount of code on the Spring Security side while allowing a lot of flexibility from applications to customize the AuthnRequest, e.g. we don't need to add a setForceAuthN method, a setAuthnContextClassRef method, etc.

I also like that this leaves all OpenSAML references inside of OpenSamlAuthenticationRequestFactory. It favors composition, which is nice.

Do you believe this would address this use case as well as the others? Where do you see areas for improvement?

[Primedo]

Do you believe this would address this use case as well as the others?

Yes, and I agree upon managing Saml2AuthenticationRequestContext's growth carefully.

Where do you see areas for improvement?

Just two cents, where I also find the other solution fine:

• I had a similar method in my branch as your suggested post-processor and also returned the AuthnRequest, but I was not sure, if this hides that the object is also modified.

• Rethink Saml2AuthenticationRequestFactory's usage and goal and maybe also provide the request within the post-processor so someone could skip copying attributes from the Resolver to the Factory.

[fpagliar]

I think the overall strategy looks good.
I agree that we want to keep the context's growth under control.
Turning the context into an interface or allowing it to be extended and passing on a generic T extends Saml2AuthenticationRequestContext to the postProcessor would enable the devs to have a more rich/complex typesafe passage of arguments.

Although I think the current proposal with a string map of custom attributes is probably good enough for my current usecase.

[jzheaux]

I was not sure, if this hides that the object is also modified.

I think that the method name could take care of this, @Primedo. I'm thinking setAuthnRequestPostProcessor, but do you see a different method name that clarifies that the AuthnRequest parameter is the one being modified?

(Also note that I modified my sample above as I found a bug in it -- see if that also resolves your concern.)

and also returned the AuthnRequest

That's a good point, we'd originally talked about it being a Converter.

It depends on whether the application is constructing an AuthnRequest from scratch or post-processing one that Spring Security constructs. If generated from scratch, I think a Converter<Saml2AuthenticationRequestContext, AuthnRequest> makes sense. Otherwise, BiConsumer<Saml2AuthenticationRequestContext, AuthnRequest> makes sense.

The reason I proposed a BiConsumer was because of:

Since I didn't want to create a new AuthnRequest and wanted to reuse the OpenSamlImplementation to create Objects

combined with the fact that if an application wants to customize the construction of an AuthnRequest, then they can do that by registering a custom AuthnRequestBuilder.

Rethink Saml2AuthenticationRequestFactory's usage and goal and maybe also provide the request within the post-processor so someone could skip copying attributes from the Resolver to the Factory.

I agree that this would be a nice thing to consider down the road, perhaps in a new component. As it is, the factory is at the service level so we don't want to have the HttpServletRequest as part of the contract.

I suppose an application could add the request as a custom attribute if they aren't concerned about leaking the request into the service layer. I don't think we'd want OpenSamlAuthenticationRequestFactory to treat HttpServletRequest as a first-class citizen, though.

[jzheaux]

... allowing [the context] to be extended

@fpagliar, I think this is a polish we could possibly do later on - I'd like to wait a bit before making Saml2AuthenticationRequestContext's constructor non-private, which I believe is a requirement to making it extensible.

That said, if you've got a concrete use case that's a great deal simpler with a custom context, then it's something worth considering.

[jzheaux]

@fpagliar Thinking a bit more about your recommendation that Saml2AuthenticationRequestContext be opened up, I think this would be a good way to begin. Adding custom data, whether via a Map of attributes or via extension, is really at the center of all of these use cases: https://github.com/spring-projects/spring-security/issues/8356

[jzheaux]

Regarding the other two items, I've created a ticket for adding the resolver next: https://github.com/spring-projects/spring-security/issues/8360

The reason that this is next in priority is that it will address additional cases, like for custom implementations of Saml2AuthenticationRequestFactory.

I'm still analyzing the implications of exposing an AuthnRequest post-processor in OpenSamlAuthenticationRequestFactory. I think OpenSamlAuthenticationRequestFactory may expose other components over time, and I'd like to see how they interact before moving forward with a post-processor. Also, I'd like to see in what way post-processing OpenSAML components might be helpful in OpenSamlAuthenticationProvider. In the meantime, please consider implementing Saml2AuthenticationRequestFactory to customize the AuthnRequest instance.

[fpagliar]

This is definitely a step in the right direction, but I definitely feel like we need the post processor.
For the ForceAuthN case for example, how would it be implemented without it?
It is necessary to have that information stored in the context and that part is now covered.
But the only way I see of using it, is copying the whole OpenSamlAuthenticationRequestFactory and replacing just the one line that does auth.setForceAuthn(Boolean.FALSE);