From cc66a76d00734199b1607d2f1d26aea5e826bce3 Mon Sep 17 00:00:00 2001 From: Ross Kinder Date: Mon, 14 Dec 2020 10:26:40 -0500 Subject: [PATCH] Stop validating InResponseTo when AllowIDPInitiated is set With this change we no longer validate InResponseTo when AllowIDPInitiated is set. Here's why: The SAML specification does not provide clear guidance for handling InResponseTo for IDP-initiated requests where there is no request to be in response to. The specification says: InResponseTo [Optional] The ID of a SAML protocol message in response to which an attesting entity can present the assertion. For example, this attribute might be used to correlate the assertion to a SAML request that resulted in its presentation. The initial thought was that we should specify a single empty string in possibleRequestIDs for IDP-initiated requests so that we would ensure that an InResponseTo was *not* provided in those cases where it wasn't expected. Even that turns out to be frustrating for users. And in practice some IDPs (e.g. Rippling) set a specific non-empty value for InResponseTo in IDP-initiated requests. Finally, it is unclear that there is significant security value in checking InResponseTo when we allow IDP initiated assertions. This issue has been reported quite a few times, including: Fixes #291 #286 #300 #301 #286 --- samlidp/session_test.go | 4 ++-- service_provider.go | 34 +++++++++++++++++++++++++++------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/samlidp/session_test.go b/samlidp/session_test.go index 2e437390..9e03e4af 100644 --- a/samlidp/session_test.go +++ b/samlidp/session_test.go @@ -36,7 +36,7 @@ func TestSessionsCrud(t *testing.T) { w.Header().Get("Set-Cookie")) assert.Equal(t, "{\"ID\":\"AAIEBggKDA4QEhQWGBocHiAiJCYoKiwuMDI0Njg6PD4=\",\"CreateTime\":\"2015-12-01T01:57:09Z\",\"ExpireTime\":\"2015-12-01T02:57:09Z\",\"Index\":\"40424446484a4c4e50525456585a5c5e60626466686a6c6e70727476787a7c7e\",\"NameID\":\"\",\"Groups\":null,\"UserName\":\"alice\",\"UserEmail\":\"\",\"UserCommonName\":\"\",\"UserSurname\":\"\",\"UserGivenName\":\"\",\"UserScopedAffiliation\":\"\",\"CustomAttributes\":null}\n", - string(w.Body.Bytes())) + string(w.Body.Bytes())) w = httptest.NewRecorder() r, _ = http.NewRequest("GET", "https://idp.example.com/login", nil) @@ -53,7 +53,7 @@ func TestSessionsCrud(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code) assert.Equal(t, "{\"ID\":\"AAIEBggKDA4QEhQWGBocHiAiJCYoKiwuMDI0Njg6PD4=\",\"CreateTime\":\"2015-12-01T01:57:09Z\",\"ExpireTime\":\"2015-12-01T02:57:09Z\",\"Index\":\"40424446484a4c4e50525456585a5c5e60626466686a6c6e70727476787a7c7e\",\"NameID\":\"\",\"Groups\":null,\"UserName\":\"alice\",\"UserEmail\":\"\",\"UserCommonName\":\"\",\"UserSurname\":\"\",\"UserGivenName\":\"\",\"UserScopedAffiliation\":\"\",\"CustomAttributes\":null}\n", - string(w.Body.Bytes())) + string(w.Body.Bytes())) w = httptest.NewRecorder() r, _ = http.NewRequest("DELETE", "https://idp.example.com/sessions/AAIEBggKDA4QEhQWGBocHiAiJCYoKiwuMDI0Njg6PD4=", nil) diff --git a/service_provider.go b/service_provider.go index 230b3c72..8c41d43c 100644 --- a/service_provider.go +++ b/service_provider.go @@ -717,14 +717,34 @@ func (sp *ServiceProvider) validateAssertion(assertion *Assertion, possibleReque } for _, subjectConfirmation := range assertion.Subject.SubjectConfirmations { requestIDvalid := false - for _, possibleRequestID := range possibleRequestIDs { - if subjectConfirmation.SubjectConfirmationData.InResponseTo == possibleRequestID { - requestIDvalid = true - break + + // We *DO NOT* validate InResponseTo when AllowIDPInitiated is set. Here's why: + // + // The SAML specification does not provide clear guidance for handling InResponseTo for IDP-initiated + // requests where there is no request to be in response to. The specification says: + // + // InResponseTo [Optional] + // The ID of a SAML protocol message in response to which an attesting entity can present the + // assertion. For example, this attribute might be used to correlate the assertion to a SAML + // request that resulted in its presentation. + // + // The initial thought was that we should specify a single empty string in possibleRequestIDs for IDP-initiated + // requests so that we would ensure that an InResponseTo was *not* provided in those cases where it wasn't + // expected. Even that turns out to be frustrating for users. And in practice some IDPs (e.g. Rippling) + // set a specific non-empty value for InResponseTo in IDP-initiated requests. + // + // Finally, it is unclear that there is significant security value in checking InResponseTo when we allow + // IDP initiated assertions. + if !sp.AllowIDPInitiated { + for _, possibleRequestID := range possibleRequestIDs { + if subjectConfirmation.SubjectConfirmationData.InResponseTo == possibleRequestID { + requestIDvalid = true + break + } + } + if !requestIDvalid { + return fmt.Errorf("assertion SubjectConfirmation one of the possible request IDs (%v)", possibleRequestIDs) } - } - if !requestIDvalid { - return fmt.Errorf("assertion SubjectConfirmation one of the possible request IDs (%v)", possibleRequestIDs) } if subjectConfirmation.SubjectConfirmationData.Recipient != sp.AcsURL.String() { return fmt.Errorf("assertion SubjectConfirmation Recipient is not %s", sp.AcsURL.String())