Skip to content

Saml2 support overriding the SAMLRequest parameters #9199

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

Open
ghost opened this issue Nov 11, 2020 · 14 comments
Open

Saml2 support overriding the SAMLRequest parameters #9199

ghost opened this issue Nov 11, 2020 · 14 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement

Comments

@ghost
Copy link

ghost commented Nov 11, 2020

Expected Behavior
As a service provider, it would be great if we could override the following parameter in a SAMLRequest

  • saml:AuthnContextClassRef
  • ForceAuthn
  • IsPassive

Current Behavior
Currently, we are having the default values configured which is

Context
As a service provider, we want to enforce the user is always prompted for authentication while access some sensitive service so we would like to set the following values.

saml:AuthnContextClassRef = urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport
ForceAuthn =true

@ghost ghost added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Nov 11, 2020
@jzheaux jzheaux self-assigned this Nov 11, 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 Nov 11, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Nov 11, 2020

If these values are the same all the time, then the easiest way is to register your own custom AuthnRequestMarshaller with OpenSAML.

Or, if the value needs to be changed on a per-request basis, you can register your own Saml2AuthenticationRequestContextResolver.

Will either of those work in your case?

@ghost
Copy link
Author

ghost commented Nov 12, 2020

Awesome that would work for me. Maybe in the future if there are multiple requests we could expose it as a property that can be configured?

@jzheaux
Copy link
Contributor

jzheaux commented Nov 12, 2020

Yes, @Adi-devops, that's a good point. ForceAuthn and AuthnContextClassRef have been requested a few times now.

Could you elaborate on what you mean by "expose it as a property"? I'm not sure it's common enough for a Boot property, but I could imagine a scenario where adding properties to Saml2AuthenticationRequestContext could be helpful.

@ghost
Copy link
Author

ghost commented Nov 13, 2020

My bad I did not know Saml2RelyingPartyProperties was part of the boot property. I am not very familiar with the codebase, I think that would be a good location to set the property as it's used while creating the Saml2AuthenticationRequestFactory.If you could please refer me to a similar config I could try creating a PR 🙂

@jzheaux
Copy link
Contributor

jzheaux commented Nov 13, 2020

No problem, @Adi-devops. You are correct that adding something to Saml2RelyingPartyProperties would mean adding a Boot property. In that case, you'd want to file a ticket with the Boot team; however, I'm thinking that these settings are not common enough to expose in that way.

What I think could be done is to add these properties to Saml2AuthenticationRequestContext. Then, applications could set them in a custom Saml2AuthenticationRequestContextResolver like so:

@Bean 
public Saml2AuthenticationRequestContextResolver 
        authenticationRequestContextResolver(RelyingPartyRegistrationRepository relyingParties) {

        DefaultRelyingPartyRegistrationResolver resolver = 
                new DefaultRelyingPartyRegistrationResolver(relyingParties);
        return (request) -> Optional.of(request).map(this.resolver::resolve)
                .map((relyingParty) -> Saml2AuthenticationRequestContext.builder().relyingParty(relyingParty)
                        .forceAuthn(true)
                        // etc
                ).orElse(null);
}

I like that since it reduces a three-step process to a one-step process for these more common settings.

Would you be able to contribute a PR that adds these properties to Saml2AuthenticationRequestContext and then also to OpenSamlAuthenticationRequestFactory to read them?

@ghost
Copy link
Author

ghost commented Nov 14, 2020

Absolutely can I give it a try? If I have any doubts can we use this same issue thread to discuss the implementation too?

@jzheaux
Copy link
Contributor

jzheaux commented Nov 17, 2020

Sounds great, @Adi-devops.

@ghost
Copy link
Author

ghost commented Nov 18, 2020

@jzheaux I have created an initial PR by adding new properties in Saml2AuthenticationRequestContext and reading them in OpenSamlAuthenticationRequestFactory can you please review it? Do we need to update DefaultSaml2AuthenticationRequestContextResolver#L70 and set these parameters as default false?

@jzheaux
Copy link
Contributor

jzheaux commented Nov 25, 2020

Good question, @Adi-devops. Since the defaults are passive, I believe that DeafultSaml2AuthenticationRequestContextResolver can stay as it is.

@amergey
Copy link
Contributor

amergey commented Nov 30, 2020

Also ability to set the NameIDPolicy format would be great and useful. By experience with AuthnContextClassRef, this is something that we are used to configure to be able to connect to various saml idp.

@jzheaux
Copy link
Contributor

jzheaux commented Nov 30, 2020

Thanks for the feedback, @amergey. I believe that could be inferred from metadata, so I think a good start would be to add it to RelyingPartyRegistration.

If you agree, would you add some detail to that ticket (#9115) and indicate whether you are able to contribute a PR?

@kavi87
Copy link
Contributor

kavi87 commented Feb 4, 2021

BTW, the example at https://docs.spring.io/spring-security/site/docs/current/reference/html5/#servlet-saml2login-opensaml-customization does not even compile and doesn't show how to actually register the custom AuthnRequestMarshaller (I guess through the factory param that is not used).

@kavi87
Copy link
Contributor

kavi87 commented Feb 4, 2021

Also, while the converter is useful, I think that what most people will want is a hook into the default AuthnRequest before it is sent, in order to override parameters, instead of having to recreate an entire request from the saml context.

@phuongnq
Copy link

Hi, is there a way to override these parameters while using an XML-based configuration?

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 type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants