Skip to content

Configure SAML 2.0 Service Provider via Metadata #23045

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Configure SAML 2.0 Service Provider via Metadata #23045

wants to merge 2 commits into from

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Aug 21, 2020

Closes gh-22986

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 21, 2020
@wilkinsona wilkinsona mentioned this pull request Aug 25, 2020
Closed
@jzheaux
Copy link
Contributor Author

jzheaux commented Aug 25, 2020

The build appears to be failing due to something unrelated:

Found test failures in 1 test task:

:spring-boot-project:spring-boot-autoconfigure:test
    org.springframework.boot.autoconfigure.session.SessionAutoConfigurationMongoTests > defaultConfigWithUniqueStoreImplementation()

and

runc run: exit status 1: container_linux.go:346: starting container process caused "process_linux.go:449: container init caused \"rootfs_linux.go:58: mounting \\\"tmpfs\\\" to rootfs \\\"/var/vcap/data/worker/work/volumes/live/80eb908b-5ccf-4263-65e6-2f4c827cb6bf/volume/rootfs\\\" at \\\"/dev/shm\\\" caused \\\"mkdir /var/vcap/data/worker/work/volumes/live/80eb908b-5ccf-4263-65e6-2f4c827cb6bf/volume/rootfs/dev/shm: no space left on device\\\"\""

Is there something I should do to address the build failures?

@snicoll
Copy link
Member

snicoll commented Aug 25, 2020 •
edited
Loading

Is there something I should do to address the build failures?

Thanks for asking @jzheaux. We had some CI issues recently, we will take care of that as part of reviewing the PR.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 25, 2020
@philwebb philwebb added this to the 2.4.x milestone Aug 25, 2020
snicoll
snicoll reviewed Sep 1, 2020
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I've made a few comments, would you have time to review?

.../java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyProperties.java Outdated
/**
* Endpoint for discovery-based configuration.
*/
private String metadataUrl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that make sense to move that to a metadata sub-namespace the way we do for singlesignon and verification. Not sure if this could be extended in the future though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of a use case where there would be more to specify or configure regarding the identity provider's metadata. I'd recommend it stay at this level. In most cases, an application would either specify metadata-url or the rest of the properties.

That said, it occurs to me that there's metadata-based configuration support for OAuth 2.0 where the property is named issuer-uri. It may be better to call this metadata-uri.

...gframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java
@@ -28,6 +28,7 @@
import org.springframework.boot.autoconfigure.security.saml2.Saml2RelyingPartyProperties.Identityprovider.Verification;
import org.springframework.boot.autoconfigure.security.saml2.Saml2RelyingPartyProperties.Registration;
import org.springframework.boot.autoconfigure.security.saml2.Saml2RelyingPartyProperties.Registration.Signing;
import org.springframework.boot.context.properties.PropertyMapper;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of PropertyMapper adds a lot of noise to what this PR actually changes. I'd prefer if we did that separately (if at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we leave the associated code as-is, then nulls will override what was specified through the metadata endpoint.

Would surrounding each of those lines with if statements be preferrable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed that. PropertyMapper it is then!

...ringframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java Outdated
@@ -55,6 +58,15 @@
private final WebApplicationContextRunner contextRunner = new WebApplicationContextRunner().withConfiguration(
AutoConfigurations.of(Saml2RelyingPartyAutoConfiguration.class, SecurityAutoConfiguration.class));

private MockWebServer server;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only one test that uses this. An argument could be made that we could make this local to that test only.

...ringframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java Outdated
@@ -176,6 +201,46 @@ private boolean hasFilter(AssertableWebApplicationContext context, Class<? exten
return filters.stream().anyMatch(filter::isInstance);
}

private void setupMockResponse() {
String metadataResponse = "<md:EntityDescriptor entityID=\"https://idp.example.com/idp/shibboleth\"\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this XML file to a test resource perhaps?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Sep 1, 2020
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Sep 2, 2020
@snicoll snicoll self-assigned this Sep 2, 2020
@snicoll snicoll modified the milestones: 2.4.x, 2.4.0-M3 Sep 2, 2020
snicoll pushed a commit that referenced this pull request Sep 2, 2020
@jzheaux @snicoll
Configure SAML 2.0 Service Provider via Metadata
5187c01 
See gh-23045
snicoll added a commit that referenced this pull request Sep 2, 2020
@snicoll
Polish "Configure SAML 2.0 Service Provider via Metadata"
681abcc 
See gh-23045
@snicoll snicoll closed this in 1926065 Sep 2, 2020
@snicoll
Copy link
Member

snicoll commented Sep 2, 2020

thanks for the follow-up @jzheaux!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.4.0-M3
Development

Successfully merging this pull request may close these issues.

Saml2RelyingPartyAutoConfiguration should use RelyingPartyRegistrations
4 participants
@jzheaux @snicoll @philwebb @spring-projects-issues