Skip to content

OpenSamlAuthenticationRequestFactory has a very closed design #8232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fpagliar opened this issue Mar 30, 2020 · 6 comments
Closed

OpenSamlAuthenticationRequestFactory has a very closed design #8232

fpagliar opened this issue Mar 30, 2020 · 6 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue

Comments

@fpagliar
Copy link

Summary

It is impossible to create AuthNRequests aside from the default configuration with the current implementation. The class is not designed for extension, offers no configuration options and composition is also out of the picture for creating requests.

A developer needing to customize the experience of creating requests has no option but to create a copy of this class which is undesirable.

spring-security-saml used to offer the WebSSOProfileOptions for this case. Even setting ForceAuthN=True on a request requires writing a custom version of the factory.

Moreover, the WebSSOProfile allowed for customizing the AuthNRequest further (including extended attributes) before building and signing the http request.

It is also to be mentioned that the Saml2AuthenticationRequestContext does contain a limited set of information and again supports no extension, so within the factory it is not possible to look at the http-request to try to craft an appropriate AuthNRequest.

Expected Behavior

1 - An accessible hook to manage the AuthnRequest instance before building the Saml2RedirectAuthenticationRequest.

2- Configuration options for:
auth.setForceAuthn(Boolean.FALSE);

auth.setIsPassive(Boolean.FALSE);

auth.setProtocolBinding(protocolBinding);
and setIncludeScoping;

Version

Spring Security 5.2.2 RELEASE

@fpagliar
Copy link
Author

Actually support for AuthNContextClassRef falls in this category too.

@fpagliar
Copy link
Author

For the moment I think my workaround is using XMLObjectProviderRegistrySupport.registerObjectProvider plus registering a filter to inject the information into either the SecurityContext or some other global variable. But this is a big hack for pretty standard functionality.

@rwinch rwinch removed the status: waiting-for-triage An issue we've not yet triaged label Mar 31, 2020
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 31, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Apr 2, 2020

Related to #8141

@jzheaux jzheaux self-assigned this Apr 2, 2020
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 2, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Apr 2, 2020

@fpagliar Thanks for chiming in on this. Spring Security's SAML 2.0 Service Provider MVP was released with the expectation that it would be evolved in future releases. I think the idea of custom attributes makes sense.

To better determine the right place(s) to enhance the API, can you elaborate on this:

so within the factory it is not possible to look at the http-request to try to craft an appropriate AuthNRequest

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

@Primedo
Copy link

Primedo commented Apr 3, 2020

Spring Security's SAML 2.0 Service Provider MVP was released with the expectation that it would be evolved in future releases

@jzheaux Evolving is ambiguous, when you go from "makes sense" to "work with OpenSAML directly" in no time. Could you elaborate on what you mean by MVP and evolve?

@jzheaux
Copy link
Contributor

jzheaux commented Apr 3, 2020

Sure, @Primedo, good questions, I'll see if I can be clearer.

In 5.2, Spring Security released SAML 2.0 Service Provider support as a minimally-viable product. It supports a very narrow, but complete, use case. Future releases will support more use cases. Resource Server was released in the same way in 5.1 - it only supported authorizing JWT bearer tokens against a JWK Set Uri using RS256, but in subsequent releases, more features have been added. SAML is a much larger spec, so it's no surprise that Spring Security's existing SAML feature set seems quite small relative to the community's existing use cases.

By "evolve", I mean that the API will need to change over time. New interfaces will be needed, new methods will need to be added, etc. For example, in order to allow an application to affect the Saml2AuthenticationRequestContext before the factory is invoked I believe will require a new interface. As was discussed in #8141 and also here, it appears that new methods may be necessary in Saml2AuthenticationRequestContext to support custom attributes.

you go from "makes sense" to "work with OpenSAML directly" in no time

I think I may have communicated the wrong sentiment by having two conversations going at once about similar feature requests. These two quotes are actually about two different things: The "makes sense" comment is about custom attributes. The "work with OpenSAML directly" is about waiting until there is an obvious and concrete need to pull from the HttpServletRequest. As far as I can tell, my position hasn't changed about either.

Your point is correct, though. Let's mark this ticket as a duplicate, reopen #8141, and continue the conversation over there.

@jzheaux jzheaux closed this as completed Apr 3, 2020
@jzheaux jzheaux added the status: duplicate A duplicate of another issue label Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

5 participants