search

spring-attic / spring-security-saml

SAML extension for the Spring Security project
Other
419 stars 479 forks source link

Sample mvp/minimal Review #436

Closed rwinch closed 4 years ago

rwinch commented 5 years ago

= Saml2AuthenticationProvider

= OpenSaml2Implementation

= Saml2AuthenticationToken

= Saml2KeyPair

= Saml2ServiceProviderRegistration

= 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 commented 5 years ago

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 commented 5 years ago

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

Saml2AuthenticationProvider

OpenSaml2Implementation

Saml2X509Credential

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.

Saml2IdentityProviderDetails

Saml2AuthenticationToken

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

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 commented 5 years ago

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 commented 5 years ago

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:

fhanik commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago
fhanik commented 5 years ago

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 commented 5 years ago

  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 commented 5 years ago

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

fhanik commented 5 years ago

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. 44ad0f695f5ef348105276db7ae64bf055b9cebf

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

Completed. b2c67f5bf83a6596474a43933728f4af06c05414

Do we need RequestUtils or can we use UrlUtils?

Completed. 20e5188820df0a9af7c999666c2ab6c2b5840c69

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. e5d43aca76685e73ba8e8dbd08127256c7b72ca3

The implementation for getIdpAlias is brittle.

Completed. d26ffbaf97195470cb9401ee81f012ab26790da3

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

Completed. da94a095ed7c4ab53ad581db289bd6c2aa0aa2e1

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

Completed. 689f9ed6a1c8346375999da1ccf7531734003cb8

rwinch commented 5 years ago

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 commented 5 years ago
fhanik commented 5 years ago

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 commented 5 years ago
fhanik commented 5 years ago

Ready for review.

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

We do not want to require BouncyCastleProvider

Fixed. 0ff0e6a026884c9de02582a2a033f0f60e0b728a

Logging seems to be messed up

Fixed. 688acacde835c90f884fc7fafc9b17b6d0d987f3

We should try to align the names Rename to OpenSamlAuthenticationRequestResolver

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

Rename Saml2IdentityProviderDetails

Fixed. 709a1ac9b90faacc94706cd31a76aa7bae595390 and d078488fc85cf43297b81d894ef460cc3115c4ac

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

I implemented an interface, Saml2RequestMatcher in commit 300da8e2a5aea54e91291e0d41cc4ab628316c11 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 commented 5 years ago

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 https://github.com/spring-projects/spring-security/issues/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 commented 4 years ago

This was merged into Spring Security via https://github.com/spring-projects/spring-security/commit/e9a44bc0ce98db5a758c373f87dd2346d0c2aff5, so I'm closing this issue.