Skip to content

Add OpenSamlAssertingPartyDetails #10794

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

Merged
merged 1 commit into from
Feb 7, 2022
Merged

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Jan 31, 2022

Closes gh-10781

@jzheaux jzheaux self-assigned this Jan 31, 2022
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Jan 31, 2022
@jzheaux jzheaux added this to the 5.7.0-M2 milestone Jan 31, 2022
@jzheaux
Copy link
Contributor Author

jzheaux commented Jan 31, 2022

@OrangeDog, does this meet the use case you identified in this comment:

RelyingPartyRegistrations.fromMetadataLocation only results in a single RelyingPartyRegistration, and the full entity descriptor does not appear to be available from that - e.g. the <EntityAttributes> and <Extensions>.

@OrangeDog
Copy link
Contributor

I assume it does. You guys tested it, right?

It will still need verification of the EntitiesDescriptor signature, especially when SSL is not available.
And ideally the ability to see and use the response's cache details, otherwise both users and the federation host get annoyed with you.

Let me go put some of my code examples on my top-level issue and you can hopefully put together an equivalent example for Spring 5/6.

@jzheaux
Copy link
Contributor Author

jzheaux commented Feb 1, 2022

It will still need verification of the EntitiesDescriptor signature, especially when SSL is not available.

In this situation, I think an application can use OpenSAML to verify the signature themselves, for example:

InputStream unverified = // ... retrieve metadata over HTTP
InputStream verified = // ... verify signature using OpenSAML
List<RelyingPartyRegistrations.Builder> registrations = 
        RelyingPartyRegistrations.collectionFromMetadata(verified);

I'm not really inclined to further simplify pulling metadata over a non-TLS endpoint.

That said, I appreciate the insight -- to discuss the merits of Spring Security doing signature verification further, I'd recommend opening a separate ticket to keep this PR focused on #10781.

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 1, 2022
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

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

Great @jzheaux. I've left some feedback inline.

* {@link org.springframework.security.saml2.provider.service.registration.RelyingPartyRegistration.AssertingPartyDetails}
* @param entity the {@link EntityDescriptor} to use
* @return the
* {@link org.springframework.security.saml2.provider.service.registration.OpenSamlAssertingPartyDetails.Builder}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* {@link org.springframework.security.saml2.provider.service.registration.OpenSamlAssertingPartyDetails.Builder}
* {@link org.springframework.security.saml2.provider.service.registration.OpenSamlMetadataAssertingPartyDetailsConverter.AssertingPartyDetails.Builder}

Comment on lines 273 to 277
/**
* Build an
* {@link org.springframework.security.saml2.provider.service.registration.OpenSamlAssertingPartyDetails}
* @return
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* Build an
* {@link org.springframework.security.saml2.provider.service.registration.OpenSamlAssertingPartyDetails}
* @return
*/
/**
* Build an
* {@link org.springframework.security.saml2.provider.service.registration.OpenSamlMetadataAssertingPartyDetailsConverter.AssertingPartyDetails}
* @return the {@link org.springframework.security.saml2.provider.service.registration.OpenSamlMetadataAssertingPartyDetailsConverter.AssertingPartyDetails}
*/

@jzheaux jzheaux merged commit 541a1e4 into spring-projects:5.7.x Feb 7, 2022
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 type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants