From 59420b28754545b2ab377d36d7d43a41190ef053 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 --- service_provider.go | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) 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())