Skip to content

Commit

Permalink
Stop validating InResponseTo when AllowIDPInitiated is set
Browse files Browse the repository at this point in the history
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
  • Loading branch information
crewjam committed Dec 14, 2020
1 parent 6ab128f commit 59420b2
Showing 1 changed file with 27 additions and 7 deletions.
34 changes: 27 additions & 7 deletions service_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down

0 comments on commit 59420b2

Please sign in to comment.