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.

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

Add SAML Service Provider Support #7260

Merged
merged 1 commit into from
Sep 6, 2019
Merged

Add SAML Service Provider Support #7260

merged 1 commit into from
Sep 6, 2019

Conversation

fhanik
Copy link
Contributor

@fhanik fhanik commented Aug 15, 2019

Simple SAML 2 authentication.

jzheaux
jzheaux requested changes Aug 15, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @fhanik, for all your hard work to put this PR together! I've left some feedback inline. I'd also recommend before merging that we add unit tests and java doc.

...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...org/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRepository.java Outdated Show resolved Hide resolved
...ngframework/security/saml2/serviceprovider/provider/InMemorySaml2RelyingPartyRepository.java Outdated Show resolved Hide resolved
...org/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRepository.java Outdated Show resolved Hide resolved
...org/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRepository.java Outdated Show resolved Hide resolved
samples/boot/saml2login/src/main/java/boot/saml2/config/Saml2SampleBootConfiguration.java Outdated Show resolved Hide resolved
samples/boot/saml2login/src/main/java/sample/SecurityConfig.java Outdated Show resolved Hide resolved
...gframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationProvider.java Outdated Show resolved Hide resolved
...ngframework/security/saml2/serviceprovider/provider/InMemorySaml2RelyingPartyRepository.java Outdated Show resolved Hide resolved
...org/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRepository.java Outdated Show resolved Hide resolved
rwinch
rwinch requested changes Aug 19, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch 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 @fhanik! I provided feedback inline.

...g/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRegistration.java Outdated Show resolved Hide resolved
...g/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRegistration.java Outdated Show resolved Hide resolved
...g/springframework/security/saml2/serviceprovider/provider/Saml2RelyingPartyRegistration.java Outdated Show resolved Hide resolved
...gframework/security/saml2/serviceprovider/servlet/filter/Saml2LoginPageGeneratingFilter.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
@fhanik fhanik self-assigned this Aug 20, 2019
rwinch
rwinch requested changes Aug 22, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

I provided some feedback inline

samples/boot/saml2login/src/main/java/sample/Saml2ServiceProviderStarterApplication.java Outdated Show resolved Hide resolved
samples/boot/saml2login/src/main/java/sample/Saml2ServiceProviderStarterApplication.java Outdated
public static void main(String[] args) {
Log log = LogFactory.getLog(Saml2ServiceProviderStarterApplication.class);
log.info("Starting SAML 2 Sample Application");
SpringApplication.run(
Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure we log a ticket to Boot to ensure it doesn't automatically create a user

2019-08-22 10:25:49.114  INFO 27629 --- [           main] .s.s.UserDetailsServiceAutoConfiguration : 

Using generated security password: 801c42c2-5167-4761-80f4-4fd7b3103ba0

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 believe this is my fault. I left in a test configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO

samples/boot/saml2login/src/main/resources/application-multipleidps.yml Outdated Show resolved Hide resolved
samples/boot/saml2login/src/main/resources/application-multipleidps.yml Outdated Show resolved Hide resolved
samples/boot/saml2login/src/main/java/sample/web/SampleSaml2AppController.java Outdated Show resolved Hide resolved
...va/org/springframework/security/saml2/serviceprovider/provider/RelyingPartyRegistration.java Outdated Show resolved Hide resolved
...va/org/springframework/security/saml2/serviceprovider/provider/RelyingPartyRegistration.java Outdated Show resolved Hide resolved
...ingframework/security/saml2/serviceprovider/provider/RelyingPartyRegistrationRepository.java Outdated
* limitations under the License.
*/

package org.springframework.security.saml2.serviceprovider.provider;
Copy link
Member

Choose a reason for hiding this comment

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

Rename the package to align with OAuth codebase by changing provider to registration

Copy link
Contributor

@jzheaux jzheaux Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The package for oauth resource server is org.springframework.security.oauth2.server.resource with an anticipated org.springframework.security.oauth2.server.authorization.

Would it be better to call this org.springframework.security.saml2.provider.service with an anticipated org.springframework.saml2.provider.identity? (And subsequently, org.springframework.saml2.provider.service.registration)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

...gframework/security/saml2/serviceprovider/servlet/filter/Saml2LoginPageGeneratingFilter.java Outdated Show resolved Hide resolved
...framework/security/saml2/serviceprovider/servlet/filter/Saml2WebSsoAuthenticationFilter.java Outdated Show resolved Hide resolved
jzheaux
jzheaux requested changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux 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 last update, @fhanik! I've left some additional feedback inline.

...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...amework/security/config/annotation/web/configurers/saml2/Saml2ServiceProviderConfigurer.java Outdated Show resolved Hide resolved
...gframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationProvider.java Outdated Show resolved Hide resolved
...gframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationProvider.java Outdated Show resolved Hide resolved
...work/security/saml2/serviceprovider/provider/InMemoryRelyingPartyRegistrationRepository.java Outdated Show resolved Hide resolved
...gframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationProvider.java Outdated Show resolved Hide resolved
...va/org/springframework/security/saml2/serviceprovider/provider/RelyingPartyRegistration.java Outdated Show resolved Hide resolved
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 27, 2019
@fhanik
Implement feedback
20172d6 
spring-projects#7260 (comment)
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 27, 2019
@fhanik
Remove context path /sample-sp
eb1e6ce 
Remove default port

spring-projects#7260 (comment)
spring-projects#7260 (comment)
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 27, 2019
@fhanik
Convert URI to String
72acb48 
spring-projects#7260 (comment)
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 27, 2019
@fhanik
Remove millis from method, the type Duration is agnostic
c9b5ff9 
spring-projects#7260 (comment)
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 27, 2019
@fhanik
typo spring-projects#7260 (comment)
1686591
@fhanik fhanik force-pushed the feature/saml2-sp-mvp branch from 5ce7c97 to 0b30c14 Compare August 27, 2019 22:00
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 29, 2019
@fhanik
Allow configuration of custom authentication manager (not provider)
21bc513 
spring-projects#7260 (comment)
jzheaux
jzheaux requested changes Aug 29, 2019
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @fhanik, I've left a bit more feedback inline.

...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java Outdated Show resolved Hide resolved
...g/springframework/security/config/annotation/web/configurers/saml2/Saml2LoginConfigurer.java Outdated Show resolved Hide resolved
...gframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationProvider.java Outdated Show resolved Hide resolved
...ngframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationManager.java Outdated Show resolved Hide resolved
...ngframework/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationManager.java Outdated Show resolved Hide resolved
...ork/security/saml2/serviceprovider/authentication/OpenSamlAuthenticationRequestResolver.java Outdated Show resolved Hide resolved
...framework/security/saml2/serviceprovider/servlet/filter/Saml2WebSsoAuthenticationFilter.java Outdated Show resolved Hide resolved
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 29, 2019
@fhanik
Make clock configurable
739cf90 
spring-projects#7260 (comment)
fhanik added a commit to fhanik/spring-security that referenced this pull request Aug 29, 2019
@fhanik
Correctly try all decryption keys
a6944e3 
spring-projects#7260 (comment)
@fhanik fhanik force-pushed the feature/saml2-sp-mvp branch from 4d51872 to 0f69330 Compare September 4, 2019 03:19
@fhanik fhanik force-pushed the feature/saml2-sp-mvp branch 2 times, most recently from dc3043c to 6ed71c8 Compare September 5, 2019 18:23
This was referenced Sep 5, 2019
Closed
Open
Closed
@fhanik fhanik force-pushed the feature/saml2-sp-mvp branch 3 times, most recently from 2b5ff9c to c5df8d8 Compare September 5, 2019 21:28
@fhanik
HttpSecurity.saml2login() - MVP Core Code
e9a44bc 
Implements minimal SAML 2.0 login/authentication functionality with the
following feature set:

  - Supports IDP initiated login at the default url of /login/saml2/sso/{registrationId}
  - Supports SP initiated login at the default url of /saml2/authenticate/{registrationId}
  - Supports basic java-configuration via DSL
  - Provides an integration sample using Spring Boot

Not implemented with this MVP

  - Single Logout
  - Dynamic Service Provider Metadata

Fixes spring-projectsgh-6019
@fhanik fhanik force-pushed the feature/saml2-sp-mvp branch from c5df8d8 to e9a44bc Compare September 5, 2019 21:40
@fhanik fhanik merged commit 08d5086 into spring-projects:master Sep 6, 2019
@fhanik fhanik deleted the feature/saml2-sp-mvp branch September 6, 2019 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch requested changes

@jzheaux jzheaux jzheaux approved these changes

Assignees

@fhanik fhanik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fhanik @rwinch @jzheaux