Sample mvp/minimal Review

[rwinch]

= Saml2AuthenticationProvider

• validate the constructor injected is not null and rename the parameter name
• validate authoritiesMapper is not null
• AuthenticationCredentialsNotFoundException is not appropriate for incorrect type or null value. No need to do null or instance of check in authenticate. Just cast it and proceed
• Change clockSkew to a Duration

= OpenSaml2Implementation

• Having an init method can be useful if you need to do additional setup on the object before initialization is done, but otherwise it just makes using it error prone. Is there a need for init or can that be done in the constructor?

= Saml2AuthenticationToken

• Does it make sense to have a null value for recipientUrl when the user authenticates or should we inject that when authentication has completed too?

= Saml2KeyPair

• No need for this class. Instead use java.security.KeyPair

= Saml2ServiceProviderRegistration

• Replace public constructor with a builder so we ensure it is built correctly.
• Make it immutable (no setters and no addIdentityProvider) as it will be used across threads
• Saml2IdentityProviderRegistration
  • Rename Saml2IdentityProviderRegistration to Saml2IdentityProviderDetails to align with OAuth naming
  • Add builder and make constructor private
  • Make it immutable (no setters and no addIdentityProvider) as it will be used across threads

= build.grade

Update the syntax to align with spring security for build.gradle

Fr example, in https://github.com/spring-projects/spring-security-saml/blob/2a0bbb837cb949ef999d44d30cb8d134fa3abe79/build.gradle it should use something like:

compile "commons-logging:commons-logging:$commonsLoggingVersion"

Also for dependencies like Spring Security that have a bom, the bom should be used with the dependency management plugin to avoid needing to specify the version for all the dependencies. For example https://docs.spring.io/spring-security/site/docs/5.0.x/reference/htmlsingle/#gradle-without-spring-boot

[fhanik]

All feedback incorporated minus the Maven "bom" stuff.
Let's talk more about that. Not sure how that's accomplished in Gradle. The fragment #gradle-without-spring-boot did not resolve properly.

[rwinch]

I reopened this because it appears a few things were missed. I'm adding additional comments and responses too.

Saml2AuthenticationProvider

• Change clockSkew to a Duration

OpenSaml2Implementation

• Is there a reason for hasInitCompleted? Currently initialization must be done in the constructor so we know it has been invoked. Perhaps hasInitCompleted should be static since it invoking static methods?
• I think it would be best to make a static createDefaultParserPool method that initializes all the parser pool settings rather than operating on the injected ParserPool. We can also remove OpenSaml2Implementation(BasicParserPool parserPool) since we are not using it.
• I do not think there is a need for parserPool to be a member variable

Saml2X509Credential

• Can you provide an explanation as to why you chose this name? At the moment it seems pretty generic. The name also doesn't convey anything about the private key. What makes it part of serviceprovider vs some other package? Have you looked at other options? If so what options did you look at? Did you compare against the SAML spec language?

Saml2ServiceProviderRegistration

We mentioned a builder, but with the other feedback (simplifying Saml2ServiceProviderRegistration to not include the IdP info), we could easily do this validation in the constructor. I'd say go back to using a constructor now.

• We should validate the members too

Saml2IdentityProviderDetails

• I think we can remove linktext member. This is very UI specific. Instead we can display the alias by default and if users want custom text they can provide some mapping from the alias to custom text (i.e. it might be i18n keys of something like idp.{alias}.linktext)
• Validate the member variables
• What is alias used for? It appears to be null right now. Should we just remove it?
• should entityId be called issuer?

Saml2AuthenticationToken

• Does it make sense to have a null value for recipientUrl when the user authenticates or should we inject that when authentication has completed too?

build.gradle

You are right. I somehow botched the link to the bom. Please try https://docs.spring.io/spring-security/site/docs/5.1.x/reference/htmlsingle/#gradle-without-spring-boot

Sample

• Update reading the PrivateKey and X509Certificate to be something like Add RSA Key Converters spring-projects/spring-security#6494 For now just place in the sample, but we can eventually extract out to move to appropriate Spring Security modules
• I get the following error (it appears to be related to the fact that OpenSaml2Implementation has not been initialized yet). We need to ensure that initialization is done before Bouncy castle is used.

May 21, 2019 2:34:35 PM org.apache.catalina.core.StandardService stopInternal
INFO: Stopping service [Tomcat]
Exception in thread "main" org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'springSecurityFilterChain' defined in class path resource [org/springframework/security/config/annotation/web/configuration/WebSecurityConfiguration.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [javax.servlet.Filter]: Factory method 'springSecurityFilterChain' threw exception; nested exception is java.lang.IllegalArgumentException: org.bouncycastle.openssl.PEMException: Unable to create OpenSSL PBDKF: PBKDF-OpenSSL SecretKeyFactory not available
	at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:627)
	at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:456)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1321)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1160)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:555)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:515)
	at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:320)
	at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:222)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:318)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
	at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:307)
	at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:199)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:849)
	at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:877)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:549)
	at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:142)
	at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:775)
	at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:397)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:316)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1260)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1248)
	at sample.Saml2ServiceProviderStarterApplication.main(Saml2ServiceProviderStarterApplication.java:25)
Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [javax.servlet.Filter]: Factory method 'springSecurityFilterChain' threw exception; nested exception is java.lang.IllegalArgumentException: org.bouncycastle.openssl.PEMException: Unable to create OpenSSL PBDKF: PBKDF-OpenSSL SecretKeyFactory not available
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:185)
	at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:622)
	... 21 more
Caused by: java.lang.IllegalArgumentException: org.bouncycastle.openssl.PEMException: Unable to create OpenSSL PBDKF: PBKDF-OpenSSL SecretKeyFactory not available
	at sample.SecurityConfig.readPrivateKey(SecurityConfig.java:112)
	at sample.SecurityConfig.getLocalSpKey(SecurityConfig.java:73)
	at sample.SecurityConfig.configure(SecurityConfig.java:54)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter.getHttp(WebSecurityConfigurerAdapter.java:231)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter.init(WebSecurityConfigurerAdapter.java:322)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter.init(WebSecurityConfigurerAdapter.java:92)
	at sample.SecurityConfig$$EnhancerBySpringCGLIB$$5ade8708.init(<generated>)
	at org.springframework.security.config.annotation.AbstractConfiguredSecurityBuilder.init(AbstractConfiguredSecurityBuilder.java:371)
	at org.springframework.security.config.annotation.AbstractConfiguredSecurityBuilder.doBuild(AbstractConfiguredSecurityBuilder.java:325)
	at org.springframework.security.config.annotation.AbstractSecurityBuilder.build(AbstractSecurityBuilder.java:41)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfiguration.springSecurityFilterChain(WebSecurityConfiguration.java:104)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfiguration$$EnhancerBySpringCGLIB$$84e24650.CGLIB$springSecurityFilterChain$2(<generated>)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfiguration$$EnhancerBySpringCGLIB$$84e24650$$FastClassBySpringCGLIB$$f5e024b1.invoke(<generated>)
	at org.springframework.cglib.proxy.MethodProxy.invokeSuper(MethodProxy.java:244)
	at org.springframework.context.annotation.ConfigurationClassEnhancer$BeanMethodInterceptor.intercept(ConfigurationClassEnhancer.java:363)
	at org.springframework.security.config.annotation.web.configuration.WebSecurityConfiguration$$EnhancerBySpringCGLIB$$84e24650.springSecurityFilterChain(<generated>)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:154)
	... 22 more
Caused by: org.bouncycastle.openssl.PEMException: Unable to create OpenSSL PBDKF: PBKDF-OpenSSL SecretKeyFactory not available
	at org.bouncycastle.openssl.jcajce.PEMUtilities.getKey(Unknown Source)
	at org.bouncycastle.openssl.jcajce.PEMUtilities.crypt(Unknown Source)
	at org.bouncycastle.openssl.jcajce.JcePEMDecryptorProviderBuilder$1$1.decrypt(Unknown Source)
	at org.bouncycastle.openssl.PEMEncryptedKeyPair.decryptKeyPair(Unknown Source)
	at sample.SecurityConfig.readPrivateKey(SecurityConfig.java:102)
	... 42 more
Caused by: java.security.NoSuchAlgorithmException: PBKDF-OpenSSL SecretKeyFactory not available
	at javax.crypto.SecretKeyFactory.<init>(SecretKeyFactory.java:122)
	at javax.crypto.SecretKeyFactory.getInstance(SecretKeyFactory.java:160)
	at org.bouncycastle.jcajce.util.DefaultJcaJceHelper.createSecretKeyFactory(Unknown Source)
	... 47 more

[fhanik]

I reopened this because it appears a few things were missed. I'm adding additional comments and responses too.

Saml2AuthenticationProvider

• Change clockSkew to a Duration

Duration would be wrong. It's a skew in two different directions. Forward and backwards.
The name was changed to responseTimeToleranceMillis in a previous commit

OpenSaml2Implementation

• Is there a reason for hasInitCompleted? Currently initialization must be done in the constructor so we know it has been invoked. Perhaps hasInitCompleted should be static since it invoking static methods?
• I think it would be best to make a static createDefaultParserPool method that initializes all the parser pool settings rather than operating on the injected ParserPool. We can also remove OpenSaml2Implementation(BasicParserPool parserPool) since we are not using it.
• I do not think there is a need for parserPool to be a member variable

OpenSaml2Implementation initialization is static.

Saml2X509Credential

• Can you provide an explanation as to why you chose this name? At the moment it seems pretty generic. The name also doesn't convey anything about the private key. What makes it part of serviceprovider vs some other package? Have you looked at other options? If so what options did you look at? Did you compare against the SAML spec language?

SAML Implementation Guidelines Section 4.3

Credentials are used in a number of ways in a single sign-on system and are often the basis for
establishing trust with the credential bearer. Credentials may represent security-related attributes of the
bearer, including the owner’s identity. Sensitive credentials that require special protection, such as private
cryptographic keys, must be protected from unauthorized exposure. Some credentials are intended to beshared, such as public-key certificates

SAML supports credentials in many forms, DSA, RSA, X509. We have chosen to start with X509.
The name is generic, but recognized in OpenSAML for example (org.opensaml.security.credential.Credential). We can use a name that narrows it down.
The object will fit into both IDP and SP implementation. I have it moved it out of the sp package.

Saml2ServiceProviderRegistration

We mentioned a builder, but with the other feedback (simplifying Saml2ServiceProviderRegistration to not include the IdP info), we could easily do this validation in the constructor. I'd say go back to using a constructor now.

• We should validate the members too

completed.

Saml2IdentityProviderDetails

• I think we can remove linktext member. This is very UI specific. Instead we can display the alias by default and if users want custom text they can provide some mapping from the alias to custom text (i.e. it might be i18n keys of something like idp.{alias}.linktext)
• Validate the member variables

completed

• What is alias used for? It appears to be null right now. Should we just remove it?

typically, you would configure a remote IDP (Saml2IdentityProviderDetails) by specifying a URL to metadata.
Since we are not including that functionality, the user must manually set the entityId.
We can remove it for now. It will be required when the entityId is fetched dynamically, as at that time there needs to be another unique identifier for the IDP.

• should entityId be called issuer?

no, it should remain entityId. the spec refers to it that way. It just happens that an attribute called issuer, is set to the value of the entityId of the sending entity. There are other attributes set to this value too.

Saml2AuthenticationToken

• Does it make sense to have a null value for recipientUrl when the user authenticates or should we inject that when authentication has completed too?

not really. the validation has already taken place at this time.

build.gradle

You are right. I somehow botched the link to the bom. Please try https://docs.spring.io/spring-security/site/docs/5.1.x/reference/htmlsingle/#gradle-without-spring-boot

thank you, I will review this separate. Right now we don't have a build file without boot, but we will once we separate the sample out.

Sample

• Update reading the PrivateKey and X509Certificate to be something like spring-projects/spring-security#6494 For now just place in the sample, but we can eventually extract out to move to appropriate Spring Security modules

ok. Created Saml2KeyConverters in the sample.

• I get the following error (it appears to be related to the fact that OpenSaml2Implementation has not been initialized yet). We need to ensure that initialization is done before Bouncy castle is used.

fixed. I separated the initialization of BC completely.

[rwinch]

Duration would be wrong. It's a skew in two different directions. Forward and backwards.

Why is that wrong? Duration doesn't imply a direction. It is just an amount time (a number plus a unit).

The name was changed to responseTimeToleranceMillis in a previous commit

Personally, I think clockSkew is a fine name and conveys the intent much more concisely.

typically, you would configure a remote IDP (Saml2IdentityProviderDetails) by specifying a URL to metadata.
Since we are not including that functionality, the user must manually set the entityId.
We can remove it for now. It will be required when the entityId is fetched dynamically, as at that time there needs to be another unique identifier for the IDP.

What is the difference between the alias and the entityId? If we don't need it now, let's remove it please.

not really. the validation has already taken place at this time.

If it is null, I think we should consider using two separate Authentication objects rather than having an unnecessary and null attribute.

ok. Created Saml2KeyConverters in the sample

Please externalize it so that users can @Autowire the concrete types from a resource. See the tests in the commit I referenced for what I mean.

Note this will take some thought as you will need to find a good way to represent the passphrase for the key. Perhaps it will be a new annotation that can provide the passphrase on it.

SAML supports credentials in many forms, DSA, RSA, X509. We have chosen to start with X509.
The name is generic, but recognized in OpenSAML for example (org.opensaml.security.credential.Credential).

Thanks. Looking at X509Credential I'm wondering if we will need any of these other attributes. Thoughts?

Sample

Look for ways to simplify this configuration. A few ideas:

• Can we default the serviceProviderEntityId? Perhaps it uses a default value with a template (i.e. ${baseUrl}) as is done with OAuth?
• Can we simplify the configuration to just exposing a (or a few) Bean(s) and invocation of saml2Login()? This will be critical to allow Boot and Spring Security SAML to work together.

[fhanik]

I gave up on the key converters for now. Unlikely will they be injected using @value with a passphrase.
The work around is to just remove the passphrase from the key (openssl) and use the existing converters.

I have allowed serviceProviderEntityId to be null, but not removed the configuration. For a complete validation of the incoming message, this should be set, and not derived.

I have added the bean definitions as an example.

I removed the key converter, because the beans already provide the configured objects. ie, conversion will not be done using @Value or direct injection. It happens when the bean is provided.

saml2Login() is now standalone, and the sample boot config has been added.

[rwinch]

First, I think we need to provide something for the keys that works with a passphrase. Perhaps this is just a BeanFactory that takes an InputStream and a passphrase and can output a key.

We should move Saml2SampleConfiguration to be autoconfiguration that will eventually be implemented by Spring Boot. I'd move this to a seperate package to make it clear that this is not part of the sample code, but what will be provided by Spring.

I still have concerns about Saml2X509Credential. It seems like it does the immediate job of what we want now. However, I have concerns about the naming and how it will work in the future. First, the credential doesn't state what type of key it is used for. Is it signing or encryption? Should that be baked into the class name or should their be a type enum?

[fhanik]

First, I think we need to provide something for the keys that works with a passphrase. Perhaps this is just a BeanFactory that takes an InputStream and a passphrase and can output a key.

Let's think about that for a second. The keys are no different than OAuth, does our OAuth implementation support pass phrases, or do we simply tell the user remove the pass phrase from the key? The path of least resistance is simply to do that.
Sample currently supports the pass phrase, but not as a single value.

I'm going to rework the sample to see how this would look.

We should move Saml2SampleConfiguration to be autoconfiguration that will eventually be implemented by Spring Boot. I'd move this to a seperate package to make it clear that this is not part of the sample code, but what will be provided by Spring.

Will do

I still have concerns about Saml2X509Credential. It seems like it does the immediate job of what we want now. However, I have concerns about the naming and how it will work in the future. First, the credential doesn't state what type of key it is used for. Is it signing or encryption? Should that be baked into the class name or should their be a type enum?

The type doesn't matter until we introduce metadata. Then there can be a type. It's very common that the same key/certificate pair is used for both signing and decryption.
Currently SamlCredential works for

1. Private Key - SP signs messages it sends
2. Private Key - SP decrypts messages it receives
3. Certificate - shared with IDPs for signature verification
4. Certificate - shared with IDPs for message encryption

[rwinch]

Let's think about that for a second. The keys are no different than OAuth, does our OAuth implementation support pass phrases, or do we simply tell the user remove the pass phrase from the key? The path of least resistance is simply to do that.
Sample currently supports the pass phrase, but not as a single value.

We don't use private keys for OAuth yet. It is all just public keys, so there are no pass phrases.

The type doesn't matter until we introduce metadata.

So how would we shape this passively once we get there? While it is common that the same key pair is used for signing and encryption, this is generally bad practice so we need to ensure we think about this out of the box.

[fhanik]

We don't use private keys for OAuth yet. It is all just public keys, so there are no pass phrases.

I worked with the boot team a little bit on this. There isn't an easy way to map two properties into a single object via a converter. their suggestion was to create an intermediary object, a solution we already have in place

While it is common that the same key pair is used for signing and encryption, this is generally bad practice so we need to ensure we think about this out of the box.

I have added a KeyUsage enum to the credentials type. The two prior implementations always had this. It's a place holder right now so we can differentiate the keys in the future.

[rwinch]

• While we want to keep it simple, we shouldn't hard code the links. These links should be dynamic. Perhaps we just remove this and use Saml2LoginPageGeneratingFilter
• It does not seem that Saml2AuthenticationRequestFilter is being used yet. Perhaps you just haven't done this yet?
• If OpenSaml2Implementation is instantiated twice bootstrap configures the the static parserPool again unnecessarily. We should change bootstrap to be a createDefaultParserPool method instead which creates an instance of parserPool that is configured correctly.
• The public static methods on OpenSaml2Implementation should be removed. The static references in these methods should be extracted to default values for member variables in OpenSaml2Implementation

[fhanik]

Thanks!

I do not think there is a need for parserPool to be a member variable

followed by

The static references in these methods should be extracted to default values for member variables in OpenSaml2Implementation

I'll defer this a little bit. and continue to focus on API and configuration portions of the API.

[fhanik]

1. The static sample page has been removed (it was required because of lack of AuthNRequest, so there wasn't a way to initiate a logout before.

2. AuthNRequest - we can now initiate a logout from the SP. We currently on support one binding.
   and that is HTTP-Redirect (a deflated and encoded 302 redirect). This is accomplished using the Saml2AuthenticationRequestFilter

3. An authentication request resolver has been added.
   I would like some feedback here. Should we create a domain object for the request itself? (currently returns the XML data). A domain object for an AuthenticationRequest can become elaborate if the goal is to encompass the common spec use cases. The main goal with the mvp/minimal branch was to get away from creating a domain model, which previous branches already had.

4. OpenSaml2Implementation has been made a singleton

5. I considered an authentication entry point that would redirect automatically if there was only one IDP configured. That's very easy to do, however, it often leads to a redirect loop upon logout, that is difficult for developers to trace down. The loop is because we don't yet have distributed logout, so the IDP session is still alive.

[rwinch]

I considered an authentication entry point that would redirect automatically if there was only one IDP configured. That's very easy to do, however, it often leads to a redirect loop upon logout, that is difficult for developers to trace down. The loop is because we don't yet have distributed logout, so the IDP session is still alive.

I think we should implement this as it is consistent with other modules (i.e. CAS, OAuth, etc). The logout success would be /login?logout which is rendered by DefaultLoginPageGeneratingFilter. Since DefaultLoginPageGeneratingFilter is before the authorization, it will render just fine.

Is there a reason we need Saml2AuthenticationFailureHandler? It seems that we could use something more generic? Perhaps SimpleUrlAuthenticationFailureHandler?

Do we need RequestUtils or can we use UrlUtils?

I know we have gone back and forth on this, but I'm afraid we may have an issue at the moment for Saml2ServiceProviderRegistration having the getIdentityProviders on it. The problem is that this will not scale if there are very large number of IdPs. We need a lookup based on the alias on a repository interface for the IdP as well.

Saml2AuthenticationRequestFilter

• The implementation for getIdpAlias is brittle. If there is a an empty context root, then this breaks. Furthermore, manually parsing is not something we want to do. Instead I propose we use something like RequestVariablesExtractor (i.e. AntPathRequestMatcher) to extract the alias.
• Debug statements should not use string concatenation without a guard around it (i.e. logger.isDebugEnabled()).
• Can we simplify sendAuthenticationRequest so that manual encoding isn't done but instead UriComponentsBuilder encodes the parameters?

[fhanik]

I considered an authentication entry point that would redirect automatically if there was only one IDP configured.
I think we should implement this as it is consistent with other modules (i.e. CAS, OAuth, etc).

Implemented. Modeled after oauth2Login. 44ad0f6

Is there a reason we need Saml2AuthenticationFailureHandler? It seems that we could use something more generic? Perhaps SimpleUrlAuthenticationFailureHandler?

Completed. b2c67f5

Do we need RequestUtils or can we use UrlUtils?

Completed. 20e5188

I know we have gone back and forth on this, but I'm afraid we may have an issue at the moment for Saml2ServiceProviderRegistration having the getIdentityProviders on it. The problem is that this will not scale if there are very large number of IdPs. We need a lookup based on the alias on a repository interface for the IdP as well.

Completed. e5d43ac

The implementation for getIdpAlias is brittle.

Completed. d26ffba

Debug statements should not use string concatenation without a guard around it (i.e. logger.isDebugEnabled()).

Completed. da94a09

Can we simplify sendAuthenticationRequest so that manual encoding isn't done but instead UriComponentsBuilder encodes the parameters?

Completed. 689f9ed

[rwinch]

Having Saml2ServiceProviderRepository look up Saml2IdentityProviderDetailsRepository is not a typical pattern of a repository. Instead, I'd suggest Saml2IdentityProviderDetailsRepository accept a request object that contains the serviceProviderEntityId along with the IdP's id or alias.

[rwinch]

• For now let's keep new features like metadata in a separate branch. We don't want to delay getting the MVP getting in and putting too much in will just delay things.
• OpenSaml2Implementation cannot be public because there is no interface. For now, we should just try and keep this as a package private class as was done before adding the additional functionality. Later we can extract an appropriate API if necessary.
• Saml2IdentityProviderDetailsRepository
  • Should not need to input the applicationUri as this will be the same value for the application.
  • Rename methods to match Spring Data conventions (i.e. findByEntityId and findByAlias)

[fhanik]

Saml2IdentityProviderDetailsRepository has been simplified

public interface Saml2IdentityProviderDetailsRepository {
	Saml2IdentityProviderDetails findByEntityId(String idpEntityId);
	Saml2IdentityProviderDetails findByAlias(String alias);
}

and as a result, we've added a POJO to convey the intention of creating a SAML2 AuthNRequest

public interface Saml2AuthenticationRequestResolver {
	String resolveAuthenticationRequest(Saml2AuthenticationRequest request);
}

[rwinch]

• We do not want to require BouncyCastleProvider. It should be an optional dependency since the API allows users to provide the keys in anyway they want (it might not be parsed via BC).
• Logging seems to be messed up. I don't see a log stating the app is started. Perhaps this is caused by having commons logging on the classpath which should not be there when using slf4j over jars.
• Saml2AuthenticationRequestFilter
  • We should try to align the names of Saml2AuthenticationRequestFilter and Saml2WebSsoAuthenticationFilter. Right now the first makes no reference to the profile while the second does.
  • I think that the alias could be allowed to be null. In that findByAlias would return a default instance of Saml2IdentityProviderDetails
• DefaultSaml2AuthenticationRequestResolver
  • Rename to OpenSamlAuthenticationRequestResolver since it is very clearly coupled to OpenSaml
• Saml2IdentityProviderDetails
  • Unless there is good reason, I'd prefer this be named more similarly to OAuth. Something like RelyingPartyRegistration

[fhanik]

Ready for review.

https://github.com/spring-projects/spring-security-saml/commits/mvp/minimal

We do not want to require BouncyCastleProvider

Fixed. 0ff0e6a

Logging seems to be messed up

Fixed. 688acac

We should try to align the names
Rename to OpenSamlAuthenticationRequestResolver

Fixed . cd058a9
Also started aligning names better across the module,

Rename Saml2IdentityProviderDetails

Fixed. 709a1ac and d078488

I think that the alias could be allowed to be null.

I implemented an interface, Saml2RequestMatcher in commit 300da8e that allows the matching for the filter to trigger as well as extraction of the alias. The alias doesn't have to be URL based, thus a resolver makes sense.

[rwinch]

I don't like having a specialized Saml2RequestMatcher interface as it is very specific for something that is quite general. I think you should look into working on spring-projects/spring-security#7148 and leveraging it.

I also think things are ready for @jzheaux to put another set of eyes on it.

After your changes you can also start preparing the changes to be added to Spring Security proper in a new module within saml2/service-provider

[jzheaux]

This was merged into Spring Security via spring-projects/spring-security@e9a44bc, so I'm closing this issue.