Skip to content
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

File-based Configuration for Asserting Party Metadata #9028

Closed
ryan13mt opened this issue Sep 17, 2020 · 6 comments
Closed

File-based Configuration for Asserting Party Metadata #9028

ryan13mt opened this issue Sep 17, 2020 · 6 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@ryan13mt
Copy link

ryan13mt commented Sep 17, 2020

Expected Behavior

String metadataLocation = "C:\\local\\temp\\saml\\meta.xml";

RelyingPartyRegistration registration = RelyingPartyRegistrations.fromMetadataLocation(metadataLocation)
 .registrationId("registration-id").build();

Current Behavior

String metadataLocation = "https://testAssertingParty.com/meta.xml";

RelyingPartyRegistration registration = RelyingPartyRegistrations.fromMetadataLocation(metadataLocation)
 .registrationId("registration-id")
.build();

Context

Currently the RelyingPartyRegistrations.fromMetadataLocation(metadataLocation) only accepts a URL. Basing on my old implementation with Spring Security SAML Extension it would be good if we could also provide a local directory path to this method. Some asserting parties will not host their metadata files but will instead email it to the relying party.

Suggested Solution

The OpenSamlRelyingPartyRegistrationBuilderHttpMessageConverter class introduced in 5.4 can still be used but the parsing logic should be extracted into a separate utility class that can be used elsewhere.

An example of the changes needed in RelyingPartyRegistrations.fromMetadataLocation(metadataLocation):

public static RelyingPartyRegistration.Builder fromMetadataLocation(String metadataLocation) {
	try {
		if (metadataLocation.matches("^(https?)://.*$")) {
			RestOperations rest = new RestTemplate(Arrays.asList(new OpenSamlRelyingPartyRegistrationBuilderHttpMessageConverter()));
			return rest.getForObject(metadataLocation, RelyingPartyRegistration.Builder.class);
		} else {
			OpenSamlRelyingPartyRegistrationBuilderConverter converter = new OpenSamlRelyingPartyRegistrationBuilderConverter();
			return converter.read(RelyingPartyRegistration.Builder.class, new FileInputStream(metadataLocation));
		}
	}
	catch (RestClientException | FileNotFoundException ex) {
		if (ex.getCause() instanceof Saml2Exception) {
			throw (Saml2Exception) ex.getCause();
		}
		throw new Saml2Exception(ex);
	}
}

This way if the metadataLocation is a url, it will use the OpenSamlRelyingPartyRegistrationBuilderHttpMessageConverter, if not, it will use the Utility class that holds the parsing code which in my example is named OpenSamlRelyingPartyRegistrationBuilderConverter.

Obviously my code can be greatly improved upon but this change should be an easy one since most of the logic and tests regarding the parsing is already there.

@ryan13mt ryan13mt added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Sep 17, 2020
@Koshux
Copy link
Contributor

Koshux commented Sep 17, 2020

+1

Would greatly appreciate it if this effort could be looked into!

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

jzheaux commented Sep 17, 2020

Thanks for the suggestion, @ryan13mt.

I think the proposal is a good start. Some things come to mind:

  • We should leave the conversion strategy from InputStream to RelyingPartyRegistration.Builder package-private for now. We can always make it public later if needed.
  • OpenSamlRelyingPartyRegistrationBuilderConverter sounds a little bit vague. I'm wondering if OpenSamlAssertingPartyMetadataConverter is better since that indicates that it's converting metadata from the IdP.
  • ResourceLoader might be better than RestTemplate since it offers classpath-, file-, and URL-based resource retrieval. At that point, RelyingPartyRegistrations could do instead:
try (InputStream source = resourceLoader.getResource(metadataLocation).getInputStream()) {
    return assertingPartyMetadataConverter.convert(source);
} ...

Do those changes make sense and would you be able to submit a PR to add this feature?

@jzheaux jzheaux changed the title Support for dynamic configuration using IDP metadata file for SAML SSO integration File-based Configuration for Asserting Party Metadata Sep 21, 2020
@ryan13mt
Copy link
Author

@jzheaux I opened a pull request to see if im heading in the right direction with this

@dawi
Copy link

dawi commented Oct 23, 2020

When will this feature be available? Could it be part of the next 5.4.2 release?

@ryan13mt
Copy link
Author

When will this feature be available? Could it be part of the next 5.4.2 release?

I believe it will be in the 5.5.0 release

@jzheaux jzheaux added this to the 5.5.0-M1 milestone Oct 26, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Oct 26, 2020

Hi, @dawi, thanks for your interest.

Spring Security follows semantic versioning, so only bug fixes will go into 5.4.2. As @ryan13mt said, it will be available in 5.5.0-M1.

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

Successfully merging a pull request may close this issue.

4 participants