Fix #8693 Support SAML 2.0 SP Metadata Endpoints #8795

jkubrynski · 2020-07-04T08:21:43Z

No description provided.

jzheaux

Thanks, @jkubrynski! I've left some feedback inline.

...rg/springframework/security/saml2/provider/service/servlet/filter/SamlMetadataGenerator.java

...mework/security/saml2/provider/service/servlet/filter/SamlServiceProviderMetadataFilter.java

...pringframework/security/saml2/provider/service/servlet/filter/SamlMetadataGeneratorTest.java

...rg/springframework/security/saml2/provider/service/servlet/filter/SamlMetadataGenerator.java

jzheaux

Thanks for the updates, @jkubrynski. I've left some additional feedback inline.

jzheaux · 2020-07-15T19:43:55Z

...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java

 	private AuthenticationManager authenticationManager;

 	private Saml2WebSsoAuthenticationFilter saml2WebSsoAuthenticationFilter;

+	private Saml2MetadataFilter saml2MetadataFilter;


It seems a little odd to provide this as part of saml2Login(). For now, let's leave it out of the DSL as an application can add the filter directly with addFilter

Entity ID patterns used as defaults across spring-security-saml map to the URL, which for me suggest this URL should be available without any external configuration. Why do you think it's odd?

Why do you think it's odd?

Because saml2Login() indicates an authentication mechanism, so it should only contain filters related to that.

For example, saml2Login() isn't the place to configure SAML logout.

without any external configuration

Can you elaborate on this point? What I'm suggesting is that an application do:

http .saml2Login(saml2 -> {}) .addFilter(new Saml2MetadataFilter(new OpenSamlMetadataResolver()));

which already seems simple.

jzheaux · 2020-07-15T19:47:51Z

...g/springframework/security/saml2/provider/service/registration/RelyingPartyRegistration.java

@@ -299,12 +300,14 @@ public static Builder withRelyingPartyRegistration(RelyingPartyRegistration regi
 		private final String entityId;
 		private final boolean wantAuthnRequestsSigned;
 		private final String singleSignOnServiceLocation;
+		private final String nameIdFormat;


AssertingPartyDetails represents metadata for the asserting party, i.e. IDPSSODescriptor.

Since you are generating relying party metadata, I would have thought this would be defined in the relying party section.

NameID is a part of the IDPSSODescriptor

I believe it's in both. You can see it listed for SPSSODescriptor in this metadata overview document on page 8, section 2.14.

There may be value in adding this in both places, but since we're generating an SPSSODescriptor for this ticket, let's please focus on that use case.

...n/java/org/springframework/security/saml2/provider/service/web/OpenSamlMetadataResolver.java

jzheaux · 2020-07-15T20:06:30Z

...n/java/org/springframework/security/saml2/provider/service/web/OpenSamlMetadataResolver.java

+
+		assertionConsumerService.setLocation(
+				resolveTemplate(registration.getAssertionConsumerServiceLocation(), registration, request));
+		assertionConsumerService.setBinding(registration.getAssertingPartyDetails().getSingleSignOnServiceBinding().getUrn());


The AssertingPartyDetails#singleSignOnServiceBinding conceptually maps to the <SingleSignOnService binding='...'/> attribute.

Instead, I think this should probably be set to POST until RelyingPartyRegistration#assertionConsumerServiceBinding is added in #8776

You cannot simply hardcode POST here, as REDIRECT is pretty common in Saml. I don't get the problem with such usage as above

Are you thinking #8776 should be done first? I think it's okay to wait for that.

I don't get the problem with such usage as above

The problem with the above usage is that SingleSignOnService#binding is IDP metadata, but you are generating SP metadata. Since you are wanting to show AssertionConsumerService#binding, you'll want to retrieve that from a semantically equivalent source.

REDIRECT is pretty common in Saml

AFAIK, SAML doesn't allow the usage of REDIRECT for SAML assertions. In the spec line 421 says:

In step 5, the identity provider issues a message to be delivered by the user agent to the service provider. Either the HTTP POST, or HTTP Artifact binding can be used to transfer the message to the service provider through the user agent. The message may indicate an error, or will include (at least) an authentication assertion. The HTTP Redirect binding MUST NOT be used, as the response will typically exceed the URL length permitted by most user agents.

That doesn't mean that ppl don't do it, but it does mean simplifying that specific use case is a lower priority.

jzheaux · 2020-07-15T20:37:49Z

...org/springframework/security/saml2/provider/service/web/Saml2WebSsoAuthenticationFilter.java

@@ -14,7 +14,7 @@
 * limitations under the License.
 */

-package org.springframework.security.saml2.provider.service.servlet.filter;
+package org.springframework.security.saml2.provider.service.web;


I appreciate you being proactive here, but moving things will take a bit more work so that things stay backward compatible. While new filters like Saml2MetadataFilter should go into web, let's please wait on moving existing filters until #8819

I cannot move new filters without moving the rest or changing the visibility of *Utils classes to public. Are you OK with the second one?

No, the Utils classes should not be made public. I'd recommend inlining that support, as was done with DefaultSaml2AuthenticationRequestContextResolver. Once everything gets moved, then the code can be changed to call the Utils classes again.

.../main/java/org/springframework/security/config/annotation/web/builders/FilterComparator.java

Closes spring-projectsgh-8828

Issue spring-projectsgh-8172

jzheaux · 2020-07-16T01:30:29Z

@jkubrynski It looks like something funny happened in a recent push -- I'm seeing other commits in your PR. Can you rebase so that only your commits show?

jzheaux · 2020-07-23T22:56:17Z

@jkubrynski Are you able to make the requested changes or is there something else that I can help with?

jkubrynski · 2020-07-27T19:11:42Z

Hi, sorry but I don't have more time for this PR. Feel free to polish it on your side or simply close it.

jzheaux · 2020-08-05T17:55:07Z

Thanks for your contributions here, @jkubrynski, I was able to complete the PR.

I took your commits and squashed them in 8a35524 and then applied a polish in b999faa.

Some of your contributions were left out since more discussion is probably needed to introduce them. First, metadata configuration was left out of the saml2Login DSL. Second, support for SPSSODescriptor#nameIDFormat was left out of OpenSamlMetadataResolver. These can be revisited in additional tickets if you need that support.

Also, to align with #8887 I opted for a simpler contract of just one parameter for Saml2MetadataResolver.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 4, 2020

jkubrynski force-pushed the features/saml-metadata-generator branch from dcfc676 to 901a2f3 Compare July 4, 2020 08:39

jzheaux requested changes Jul 8, 2020

View reviewed changes

jzheaux self-assigned this Jul 8, 2020

jzheaux added in: saml2 An issue in SAML2 modules type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 8, 2020

jkubrynski added 2 commits July 12, 2020 14:21

Fix spring-projects#8693 Support SAML 2.0 SP Metadata Endpoints

89e48df

Fixes after code review

d4e6701

jkubrynski force-pushed the features/saml-metadata-generator branch from 5fd41c6 to d4e6701 Compare July 14, 2020 20:03

jzheaux requested changes Jul 15, 2020

View reviewed changes

eleftherias and others added 5 commits July 16, 2020 00:33

Restrict mockito to 3.3.+

aeabf06

Closes spring-projectsgh-8828

Add Kotlin WebFlux header configuration to docs

08670aa

Issue spring-projectsgh-8172

Set RSocket version to Snapshot in snapshot build

9f0e8eb

Checkstyle fixes

aec89c3

Fixes after code review spring-projects#3

6fefb2d

jzheaux added this to the 5.4.0-RC1 milestone Aug 5, 2020

jzheaux added the status: duplicate A duplicate of another issue label Aug 5, 2020

jzheaux closed this Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #8693 Support SAML 2.0 SP Metadata Endpoints #8795

Fix #8693 Support SAML 2.0 SP Metadata Endpoints #8795

jkubrynski commented Jul 4, 2020

jzheaux left a comment

jzheaux left a comment

jzheaux Jul 15, 2020

jkubrynski Jul 15, 2020

jzheaux Jul 16, 2020

jzheaux Jul 15, 2020

jkubrynski Jul 15, 2020

jzheaux Jul 16, 2020

jzheaux Jul 15, 2020

jkubrynski Jul 15, 2020

jzheaux Jul 16, 2020

jzheaux Jul 15, 2020

jkubrynski Jul 15, 2020

jzheaux Jul 16, 2020

jzheaux commented Jul 16, 2020 •

edited

Loading

jzheaux commented Jul 23, 2020

jkubrynski commented Jul 27, 2020

jzheaux commented Aug 5, 2020

Fix #8693 Support SAML 2.0 SP Metadata Endpoints #8795

Fix #8693 Support SAML 2.0 SP Metadata Endpoints #8795

Conversation

jkubrynski commented Jul 4, 2020

jzheaux left a comment

Choose a reason for hiding this comment

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux commented Jul 16, 2020 • edited Loading

jzheaux commented Jul 23, 2020

jkubrynski commented Jul 27, 2020

jzheaux commented Aug 5, 2020

jzheaux commented Jul 16, 2020 •

edited

Loading