mvp/minimal Review

[rwinch]

• Saml2AuthenticationToken

  • Add another constructor for when we create the Authentication that is to be authenticated. This constructor should only accept the properties that are required to perform authentication
  • getRelayState is not being used currently. If we do want to use it, I don't think it needs to be on the Authentication object as it isn't intrinsic to authentication so much as the controller (i.e. it might use the RelayState to determine what to do after authentication success).
  • The way it is currently setup, the destinationUrl is a bit confusing to me. Is it the URL that the SAML response was received or is it the destinationUrl that was in the SAML response. This leads me to question a bit the structure of this bit of code, but I'm not sure if I have an answer that I like here yet. I'm bringing it up in case you have thoughts on what to do.

• Saml2AuthenticationProvider

  • We should ensure that the SAMLResponse passed into the Saml2AuthenticationProvider is (if necessary) already deflated and decoded. That logic makes more sense in the Filters which can make a more informed decision as to what needs to happen. You can remove decodeAndInflate method. Remove any references to HttpServletRequest HttpServletResponse
  • It should not be in the servlet package as it does not need any servlet dependencies
  • Do not throw PreAuthenticatedCredentialsNotFoundException This is not a preauthentication scenario. There should be no need to do the supports as the contract for AuthenticationProvider is to call supports first, so you can just cast the token without checking
  • I'm somewhat conflicted about passing the destinationUrl in and having the AuthenticationProvider validate it vs having the Authentication have the expected destination URL (from the SAMLResponse) on it and then having the Filter validate. The biggest thing is my comment in Saml2AuthentionToken about destinationUrl (i.e. it isn't clear which destination url it is).

• Saml2EncodingUtils

  • Do not make this class/methods public

• Saml2IdentityProviderRegistration

  • It is a bit strange that we have Saml2IdentityProviderRegistration inside the serviceprovider package. I'm not sure if we should rename the registration or move it to a new package
  • The registration uses Saml2KeyData which uses String for the keys. This means that the Saml2AuthenticationProvider must decode the certificate every time it validates. Instead use Java crypto types (i.e. X509Certificate).
  • We can provide utilities for converting from a String to the keys that we need to support. You can see https://github.com/spring-projects/spring-security/blob/5aa50500cf3f8266d85713ac20f5d99231956143/core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java#L41 as a reference for how we can do that. For now let's keep all of the code in the sample, but we can later extract it out into Spring Security core as is done in the above sample

[fhanik]

Saml2AuthenticationToken

Add another constructor for when we create the Authentication that is to be authenticated. This constructor should only accept the properties that are required to perform authentication

Completed.

getRelayState is not being used currently. If we do want to use it, I don't think it needs to be on the Authentication object as it isn't intrinsic to authentication so much as the controller (i.e. it might use the RelayState to determine what to do after authentication success).

Removed. We can add it back when the need arises.

The way it is currently setup, the destinationUrl is a bit confusing to me. Is it the URL that the SAML response was received or is it the destinationUrl that was in the SAML response. This leads me to question a bit the structure of this bit of code, but I'm not sure if I have an answer that I like here yet. I'm bringing it up in case you have thoughts on what to do.

SAML 2 - Core Spec. 3.2.1 recommends that assertions are only considered valid if they are processed on the endpoint that they were intended for.

Destination [Optional] A URI reference indicating the address to 
which this request has been sent. This is useful to prevent malicious
forwarding of requests to unintended recipients, a protection that is 
required by some protocol bindings. If it is present, the 
actual recipient MUST check that the URI reference identifies the location
at which the message was received. If it does not, the request MUST be 
discarded. Some protocol bindings may require the 
use of this attribute (see [SAMLBind]). 

This is used to ensure that the assertions are not forwarded to other SPs. It's part of the validation process.

Saml2AuthenticationProvider

We should ensure that the SAMLResponse passed into the Saml2AuthenticationProvider is (if necessary) already deflated and decoded. That logic makes more sense in the Filters which can make a more informed decision as to what needs to happen. You can remove decodeAndInflate method. Remove any references to HttpServletRequest HttpServletResponse

This was already completed. Removed unused code.

It should not be in the servlet package as it does not need any servlet dependencies

Moved.

Do not throw PreAuthenticatedCredentialsNotFoundException This is not a preauthentication scenario. There should be no need to do the supports as the contract for AuthenticationProvider is to call supports first, so you can just cast the token without checking

I'm somewhat conflicted about passing the destinationUrl in and having the AuthenticationProvider validate it vs having the Authentication have the expected destination URL (from the SAMLResponse) on it and then having the Filter validate. The biggest thing is my comment in Saml2AuthentionToken about destinationUrl (i.e. it isn't clear which destination url it is).

See above. I will rename this to recipientURL so that it aligns better with the spec. All validation happens in the AuthenticationProvider.

Saml2EncodingUtils

Do not make this class/methods public

Completed.

Saml2IdentityProviderRegistration

It is a bit strange that we have Saml2IdentityProviderRegistration inside the serviceprovider package. I'm not sure if we should rename the registration or move it to a new package

A single Service Provider, will connect to 1 or more Identity Providers. Thus, when you have a SP, you register IDPs (multiple) with the service provider. In this case of "login flow", the "app" is a service provider, because it receives assertions from identity providers. We can rename the package service provider to what ever we want, that won't change the fact that it is a service provider. The Saml2IdentityProviderRegistration can also be renamed, but it does reflect a remote and paired Identity Provider (not service provider).

The registration uses Saml2KeyData which uses String for the keys. This means that the Saml2AuthenticationProvider must decode the certificate every time it validates. Instead use Java crypto types (i.e. X509Certificate). We can provide utilities for converting from a String to the keys that we need to support. You can see https://github.com/spring-projects/spring-security/blob/5aa50500cf3f8266d85713ac20f5d99231956143/core/src/main/java/org/springframework/security/converter/RsaKeyConverters.java#L41 as a reference for how we can do that. For now let's keep all of the code in the sample, but we can later extract it out into Spring Security core as is done in the above sample

Completed. Using only Java object representations of keys.

[rwinch]

This is used to ensure that the assertions are not forwarded to other SPs. It's part of the validation process.

Right this is more of a semantic discussion of where it should go. I think you are right we should ensure it is done in the AuthenticationProvider otherwise it is misleading to call state it is authenticated.

We can rename the package service provider to what ever we want, that won't change the fact that it is a service provider. The Saml2IdentityProviderRegistration can also be renamed, but it does reflect a remote and paired Identity Provider (not service provider).

Understood, but the arrangement still doesn't make sense to have identity provider information within the service provider package. I imagine at some point we will have information for the Service Provider (i.e. it's key pair?). If that is the case, perhaps then we can move the Saml2IdentityProviderRegistration to be an inner class of Saml2ServiceProviderRegistration. I would like that arrangement. Would that make sense?

[fhanik]

If that is the case, perhaps then we can move the Saml2IdentityProviderRegistration to be an inner class of Saml2ServiceProviderRegistration. I would like that arrangement. Would that make sense?

Yes, at a first glance, that does make sense. Until you decide to implement an Identity Provider that

• receives authentication requests from a service provider
• sends authentication assertions to a service provider

At that point, you'd have a Saml2IdentityProviderRegistration that contains a list of Saml2ServiceProviderRegistration and it becomes cyclical.

The previous implementation attempted to disambiguate this by differentiating between a local/hosted provider and a remote provider.

In the MVP case, the disambiguation comes from the fact that a service provider is what the app already is, there isn't a data structure for it. And when configuring the SP, you must configure one or more IDPs.

[fhanik]

Saml2IdentityProviderRegistration to be an inner class of Saml2ServiceProviderRegistration. I would like that arrangement. Would that make sense?

That encapsulation makes it a lot simpler. https://github.com/spring-projects/spring-security-saml/commit/5e69b2208fafc60f238da5b7456aa21e997f5250