sp: support for IDP initiated

[crewjam]

[crewjam]

It looks like IDP initiated just makes the same SAMLResponse POST to /saml/acs, so this should be fairly direct. We may need to relax the requirement for RelayState to be signed (need to reason about if this is okay or not), or allow RelayState to be signed by some other key that we could provide, or not allow RelayState at all.

[crewjam]

Also, I couldn't find a way to do IDP-initiated with testshib, but maybe I didn't look hard enough, so we'd need some other way to do the initial interop test

[chrsmith]

IDP-initiated logins generally somewhat work today, however there are some remaining issues that are blocking a good experience for users. This is covered briefly in the description of #183 by @praneetloke, but I've written a more detailed explanation here. Along with some proposals for how to fix the remaining issue with IDP-initiated logins at the end.

Support for IDP-initiated login was added in 72821a4, via the Options.AllowIDPInitiated field. If set, everything works like you'd expect. The flag just allows the assertion validation code to account for an empty InReplyTo field in the SAML response from the IDP. (Since the IDP isn't replying to an SP-initiated request, it's just empty.)

Problem Description

Like mentioned earlier, the problem remaining for IDP-initiated logins has to do with the built-in assumptions around the RelayState query parameter. (This is also called out in the README.md file too, here.)

For SP-initiated logins, the URL to redirect to post-login is stored via the ClientState abstraction, and the RelayState passed to the IDP is just a nonce. The code for this is here:

saml/samlsp/middleware.go

Lines 133 to 147 in ebc5f78

	// relayState is limited to 80 bytes but also must be integrety protected.
	// this means that we cannot use a JWT because it is way to long. Instead
	// we set a cookie that corresponds to the state
	relayState := base64.URLEncoding.EncodeToString(randomBytes(42))
	secretBlock := x509.MarshalPKCS1PrivateKey(m.ServiceProvider.Key)
	state := jwt.New(jwtSigningMethod)
	claims := state.Claims.(jwt.MapClaims)
	claims["id"] = req.ID
	claims["uri"] = r.URL.String()
	signedState, err := state.SignedString(secretBlock)
	if err != nil {
	http.Error(w, err.Error(), http.StatusInternalServerError)
	return
	}

When the IDP returns with the SAML assertions doc, the Authorize method will take the RelayState returned by the IDP, use that to crack open the stored URI in the ClientState, and complete the login.

saml/samlsp/middleware.go

Lines 205 to 229 in ebc5f78

	if relayState := r.Form.Get("RelayState"); relayState != "" {
	stateValue := m.ClientState.GetState(r, relayState)
	if stateValue == "" {
	m.ServiceProvider.Logger.Printf("cannot find corresponding state: %s", relayState)
	http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
	return
	}
	jwtParser := jwt.Parser{
	ValidMethods: []string{jwtSigningMethod.Name},
	}
	state, err := jwtParser.Parse(stateValue, func(t *jwt.Token) (interface{}, error) {
	return secretBlock, nil
	})
	if err != nil || !state.Valid {
	m.ServiceProvider.Logger.Printf("Cannot decode state JWT: %s (%s)", err, stateValue)
	http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
	return
	}
	claims := state.Claims.(jwt.MapClaims)
	redirectURI = claims["uri"].(string)
	// delete the cookie
	m.ClientState.DeleteState(w, r, relayState)
	}

While this works fine for SP-initiated login, for IDP-initiated there are two problems:

1. The IDP may be configured to provide a custom RelayState . For example, when using GSuite a "Start URL" can be provided, which will set the RelayState value. The samlsp package assumes the RelayState maps to a nonce value, fails with "cannot find corresponding state".
2. Even if the IDP does not provide a custom RelayState value, the default of redirecting to "/" may not be ideal. And it would be nice to allow customizing that default redirect in the Options struct.

Proposals

To support IDP-initiated logins, we need to change the interpretation of the RelayState parameter for determining where to redirect to after a valid SAML assertions doc has been provided by the IDP.

(1) Just trust the IDP

The simplest to explain option is to blindly follow the RelayState provided by the IDP. It follows that if the SAML middleware was configured to explicitly set AllowIDPInitiated = true, it is reasonable to trust the RelayState parameter it provided by the IDP.

The counter argument is that while the SAML assertions integrity can be verified, the request URL can not. So perhaps the request URL could be modified, and the redirect changed to something malicious.

For reference, this is generally the change involved for this option:

	if relayState := r.Form.Get("RelayState"); relayState != "" {
		stateValue := m.ClientState.GetState(r, relayState)
		if stateValue != "" {
			... use stored redirect URI from ClientState
		} else if m.AllowIDPInitiated {
			... trust the IDP RelayState
			redirectURl = relayState
		} else {
			... error. We don't know what to make of the relay state
		}
	...

(2) Only honor specific RelayState values

Instead of blindly trusting any RelayState value from the IDP, perhaps that could be pared down to some set of whitelisted values in the middleware's configuration?

OAuth has a similar mechanism. The OAuth provider may be passed a redirect_uri from a client, and after the OAuth provider confirms the identity of the request, they redirect to that URI. The restriction is that the OAuth client provides that redirect_uri to the OAuth provider as part of the application configuration.

e.g. when creating a GitHub OAuth app, you specify a "callback URL" that the GitHub will redirect to after any OAuth authorization request completes.

So if the SAML library were to use an approach like this, the code could be something like:

... := samlsp.New(samlsp.Options{
	...
	AllowIDPInitiated: true,
  	// Only honor IDP-supplied RelayStates if prefixed with one of these strings. 
	AllowedIDPRedirectPrefixes: []string {
		"https://example.com/",
		"https://beta.example.com/",
	},
})

This approach seems reasonable, in that the developer essentially specifies a domain or URL route restriction for any IDP RelayState values. That avoids the most common sort of malicious URL redirect type attacks.

The drawback here is that the RelayState is assumed to be a redirect URL. When it (to my very poor understanding of SAML) is intended to be a more general way to pass state between parties.

(3) Ignore the RelayState if AllowIDPInitiated is set

This boils down to "don't trust the IDP" and ignore its RelayState. This means you cannot customize things via the IDP-side configuration, but at least doesn't lead to a 403.

Something like:

	if relayState := r.Form.Get("RelayState"); relayState != "" {
		stateValue := m.ClientState.GetState(r, relayState)
		if stateValue != "" {
			... use stored redirect URI from ClientState
		} else if m.AllowIDPInitiated {
			// Don't treat this as an error. If we have a valid set of SAML assertions just
			// ignore the RelayState and carry on.
			redirectURl = m.DefaultRelayState
		} else {
			... error. We don't know what to make of the relay state
		}
	...

@crewjam , I'm happy to send out a PR for any of these. Just let me know what approach (or something I haven't mentioned) seems the most reasonable. (3) seems like the best option given the constraint of not making a breaking change to the existing library surface area.

Testing

On a somewhat related note, I looked into adding some unit tests for IDP-initiated login flows. I was able to write some tests (gist of the WIP).

However, I wasn't able to complete the tests because the test.SamlResponse includes a saml2:EncryptedAssertion element which needs to be modified to simulate the IDP-initiated login scenario. (Specifically, the SubjectConfirmation embedded in that assertions doc has the InReplyTo ID of the SP-initiated login; whereas it will be "" for the IDP-initated case.)

The reason I mention this, is that it isn't clear how to write new tests for this scenario. I assume the checked-in SamlResponse value is from some shibboleth.net response from several years ago? (i.e. this comment.)

Would it be possible for you to log into that same shibboleth configuration/application and see if you can generate an IDP-initiated SAML response to support a new set of unit tests? Or would it be better to just start fresh with a new test configuration/pre-canned response, etc.?