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

feat(saml): Update SAML to use Spring Security #1744

Merged
merged 12 commits into from May 8, 2024

Conversation

jvz
Copy link
Contributor

@jvz jvz commented Nov 28, 2023

This updates gate-saml to use the built-in SAML support in Spring Security which uses OpenSAML 4.x. Nearly all the previously supported properties for SAML are still supported, though a couple niche options no longer seem to be configurable.

This also introduces AuthenticationService, a variation of PermissionService which can also return a user's granted authorities in one login call. It was also used for exception translation previously as retrofit exceptions are not serializable which would cause errors in Spring Security authentication failure error handlers, but the underlying exception being thrown has since been updated to avoid that problem.

This updates `gate-saml` to use the built-in SAML support in Spring
Security which uses OpenSAML 4.x. Nearly all the previously supported
properties for SAML are still supported, though a couple niche options
no longer seem to be configurable.

This also introduces `AuthenticationService`, a variation of
`PermissionService` which can also return a user's granted authorities
in one login call. It was also used for exception translation previously
as retrofit exceptions are not serializable which would cause errors in
Spring Security authentication failure error handlers, but the
underlying exception being thrown has since been updated to avoid that
problem.
dependencies{
dependencies {
constraints {
implementation 'org.opensaml:opensaml-core:4.1.0'
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'll note that I tried using a newer release of OpenSAML, but it didn't seem to work with the version of Spring Security I was testing against at the time. With a new enough version of Spring Security, we shouldn't need to override this dependency anymore (this older version of Spring Security supports both OpenSAML 3 and 4).

RelyingPartyRegistrations.fromMetadataLocation(properties.getMetadataUrl())
.registrationId(properties.getRegistrationId())
.entityId(properties.getIssuerId())
.assertionConsumerServiceLocation(properties.getAssertionConsumerServiceLocation());
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but we may want:
.assertingPartyDetails({ party -> party.wantAuthnRequestsSigned(false) })
I'd have to go back check on the requirements on this and whether this was ignored, still digging though. I recall hitting this. OR make it an option. This was comparing fairly low level DSL to look for potential SAML loading changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure how that works; I always use encryption in SAML instead.

@jasonmcintosh jasonmcintosh added the ready to merge Approved and ready for merge label May 8, 2024
@mergify mergify bot added the auto merged label May 8, 2024
@mergify mergify bot merged commit 0c5e4bc into spinnaker:master May 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants