From 624f63dbdc200687e97d80d354667bde44e0bae5 Mon Sep 17 00:00:00 2001 From: Jan Szumiec Date: Fri, 22 Sep 2017 10:43:38 +0100 Subject: [PATCH 1/4] Destination is checked only if this is a signed SAML request, as that is the only case in which a Destination attribute is required. --- service_provider.go | 38 ++++++++++++++++- service_provider_test.go | 88 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/service_provider.go b/service_provider.go index caae479a..4ced3d4a 100644 --- a/service_provider.go +++ b/service_provider.go @@ -378,6 +378,39 @@ func (ivr *InvalidResponseError) Error() string { return fmt.Sprintf("Authentication failed") } +func responseContainsSignature(response *etree.Document) (bool, error) { + signatureElement, err := findChild(response.Root(), "http://www.w3.org/2000/09/xmldsig#", "Signature") + if err != nil { + return false, err + } + if signatureElement == nil { + return false, nil + } + return true, nil +} + +func (sp *ServiceProvider) validateDestination(response []byte, responseDom *Response) error { + // A response MUST contain a Destination attribute if the SAML request is signed. + responseXml := etree.NewDocument() + err := responseXml.ReadFromBytes(response) + if err != nil { + return err + } + + present, err := responseContainsSignature(responseXml) + if err != nil { + return err + } + + if present { + if responseDom.Destination != sp.AcsURL.String() { + return fmt.Errorf("`Destination` does not match AcsURL (expected %q, actual %q)", sp.AcsURL.String(), responseDom.Destination) + } + } + + return nil +} + // ParseResponse extracts the SAML IDP response received in req, validates // it, and returns the verified attributes of the request. // @@ -409,8 +442,9 @@ func (sp *ServiceProvider) ParseResponse(req *http.Request, possibleRequestIDs [ retErr.PrivateErr = fmt.Errorf("cannot unmarshal response: %s", err) return nil, retErr } - if resp.Destination != sp.AcsURL.String() { - retErr.PrivateErr = fmt.Errorf("`Destination` does not match AcsURL (expected %q)", sp.AcsURL.String()) + + if err := sp.validateDestination(rawResponseBuf, &resp); err != nil { + retErr.PrivateErr = err return nil, retErr } diff --git a/service_provider_test.go b/service_provider_test.go index bb0e0e1e..1bb6a53e 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -17,7 +17,9 @@ import ( "crypto/x509" + "github.com/beevik/etree" . "gopkg.in/check.v1" + "strings" ) // Hook up gocheck into the "go test" runner. @@ -642,6 +644,86 @@ func (test *ServiceProviderTest) TestCanParseResponse(c *C) { }) } +func (test *ServiceProviderTest) TestCanProcessResponseWithoutDestination(c *C) { + s := ServiceProvider{ + Key: test.Key, + Certificate: test.Certificate, + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + IDPMetadata: &EntityDescriptor{}, + } + err := xml.Unmarshal([]byte(test.IDPMetadata), &s.IDPMetadata) + c.Assert(err, IsNil) + + stripDestination := func(source string) (output string) { + return strings.Replace(source, "Destination=\"https://15661444.ngrok.io/saml2/acs\"", "", 1) + } + + req := http.Request{PostForm: url.Values{}} + samlResponseWithoutDestination := stripDestination(test.SamlResponse) + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString([]byte(samlResponseWithoutDestination))) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + c.Assert(err, Equals, nil) +} + +func (test *ServiceProviderTest) responseDom() (doc *etree.Document) { + doc = etree.NewDocument() + doc.ReadFromString(test.SamlResponse) + return doc +} + +func addSignatureToDocument(doc *etree.Document) *etree.Document { + responseEl := doc.FindElement("//Response") + signatureEl := doc.CreateElement("xmldsig:Signature") + signatureEl.CreateAttr("xmlns:xmldsig", "http://www.w3.org/2000/09/xmldsig#") + responseEl.AddChild(signatureEl) + return doc +} + +func removeDestinationFromDocument(doc *etree.Document) *etree.Document { + responseEl := doc.FindElement("//Response") + responseEl.RemoveAttr("Destination") + return doc +} + +func (test *ServiceProviderTest) TestMismatchedDestinationsWithSignaturePresent(c *C) { + s := ServiceProvider{ + Key: test.Key, + Certificate: test.Certificate, + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + IDPMetadata: &EntityDescriptor{}, + } + err := xml.Unmarshal([]byte(test.IDPMetadata), &s.IDPMetadata) + c.Assert(err, IsNil) + + req := http.Request{PostForm: url.Values{}} + s.AcsURL = mustParseURL("https://wrong/saml2/acs") + bytes, _ := addSignatureToDocument(test.responseDom()).WriteToBytes() + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://wrong/saml2/acs\", actual \"https://15661444.ngrok.io/saml2/acs\")") + s.AcsURL = mustParseURL("https://15661444.ngrok.io/saml2/acs") +} + +func (test *ServiceProviderTest) TestMissingDestinationWithSignaturePresent(c *C) { + s := ServiceProvider{ + Key: test.Key, + Certificate: test.Certificate, + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + IDPMetadata: &EntityDescriptor{}, + } + err := xml.Unmarshal([]byte(test.IDPMetadata), &s.IDPMetadata) + c.Assert(err, IsNil) + + req := http.Request{PostForm: url.Values{}} + bytes, _ := removeDestinationFromDocument(addSignatureToDocument(test.responseDom())).WriteToBytes() + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://15661444.ngrok.io/saml2/acs\", actual \"\")") +} + func (test *ServiceProviderTest) TestInvalidResponses(c *C) { s := ServiceProvider{ Key: test.Key, @@ -662,12 +744,6 @@ func (test *ServiceProviderTest) TestInvalidResponses(c *C) { _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) c.Assert(err.(*InvalidResponseError).PrivateErr, ErrorMatches, "cannot unmarshal response: expected element type but have ") - s.AcsURL = mustParseURL("https://wrong/saml2/acs") - req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString([]byte(test.SamlResponse))) - _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) - c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://wrong/saml2/acs\")") - s.AcsURL = mustParseURL("https://15661444.ngrok.io/saml2/acs") - req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString([]byte(test.SamlResponse))) _, err = s.ParseResponse(&req, []string{"wrongRequestID"}) c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`InResponseTo` does not match any of the possible request IDs (expected [wrongRequestID])") From 17cd2ec2e90e8f93673b9bfe8b1af9040221409a Mon Sep 17 00:00:00 2001 From: Christopher Vermilion Date: Thu, 11 Jul 2019 15:56:52 -0400 Subject: [PATCH 2/4] clean up a bit --- service_provider.go | 13 +++++-------- service_provider_test.go | 1 - 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/service_provider.go b/service_provider.go index 4ced3d4a..6677e1b0 100644 --- a/service_provider.go +++ b/service_provider.go @@ -378,31 +378,28 @@ func (ivr *InvalidResponseError) Error() string { return fmt.Sprintf("Authentication failed") } -func responseContainsSignature(response *etree.Document) (bool, error) { +func responseIsSigned(response *etree.Document) (bool, error) { signatureElement, err := findChild(response.Root(), "http://www.w3.org/2000/09/xmldsig#", "Signature") if err != nil { return false, err } - if signatureElement == nil { - return false, nil - } - return true, nil + return signatureElement != nil, nil } +// validateDestination validates the Destination attribute iff the response is signed. func (sp *ServiceProvider) validateDestination(response []byte, responseDom *Response) error { - // A response MUST contain a Destination attribute if the SAML request is signed. responseXml := etree.NewDocument() err := responseXml.ReadFromBytes(response) if err != nil { return err } - present, err := responseContainsSignature(responseXml) + signed, err := responseIsSigned(responseXml) if err != nil { return err } - if present { + if signed { if responseDom.Destination != sp.AcsURL.String() { return fmt.Errorf("`Destination` does not match AcsURL (expected %q, actual %q)", sp.AcsURL.String(), responseDom.Destination) } diff --git a/service_provider_test.go b/service_provider_test.go index 1bb6a53e..c31db9ce 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -19,7 +19,6 @@ import ( "github.com/beevik/etree" . "gopkg.in/check.v1" - "strings" ) // Hook up gocheck into the "go test" runner. From f64703af6bc23b700e0868a9473f4b4893a9a46f Mon Sep 17 00:00:00 2001 From: Christopher Vermilion Date: Thu, 11 Jul 2019 16:54:00 -0400 Subject: [PATCH 3/4] some test cleanup --- service_provider_test.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/service_provider_test.go b/service_provider_test.go index c31db9ce..16bd7ca1 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -643,6 +643,14 @@ func (test *ServiceProviderTest) TestCanParseResponse(c *C) { }) } +func (test *ServiceProviderTest) replaceDestination(newDestination string) { + newStr := "" + if newDestination != "" { + newStr = `Destination="` + newDestination + `"` + } + test.SamlResponse = strings.Replace(test.SamlResponse, `Destination="https://15661444.ngrok.io/saml2/acs"`, newStr, 1) +} + func (test *ServiceProviderTest) TestCanProcessResponseWithoutDestination(c *C) { s := ServiceProvider{ Key: test.Key, @@ -654,13 +662,9 @@ func (test *ServiceProviderTest) TestCanProcessResponseWithoutDestination(c *C) err := xml.Unmarshal([]byte(test.IDPMetadata), &s.IDPMetadata) c.Assert(err, IsNil) - stripDestination := func(source string) (output string) { - return strings.Replace(source, "Destination=\"https://15661444.ngrok.io/saml2/acs\"", "", 1) - } - req := http.Request{PostForm: url.Values{}} - samlResponseWithoutDestination := stripDestination(test.SamlResponse) - req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString([]byte(samlResponseWithoutDestination))) + test.replaceDestination("") + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString([]byte(test.SamlResponse))) _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) c.Assert(err, Equals, nil) } @@ -697,12 +701,11 @@ func (test *ServiceProviderTest) TestMismatchedDestinationsWithSignaturePresent( c.Assert(err, IsNil) req := http.Request{PostForm: url.Values{}} - s.AcsURL = mustParseURL("https://wrong/saml2/acs") + test.replaceDestination("https://wrong/saml2/acs") bytes, _ := addSignatureToDocument(test.responseDom()).WriteToBytes() req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) - c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://wrong/saml2/acs\", actual \"https://15661444.ngrok.io/saml2/acs\")") - s.AcsURL = mustParseURL("https://15661444.ngrok.io/saml2/acs") + c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://15661444.ngrok.io/saml2/acs\", actual \"https://wrong/saml2/acs\")") } func (test *ServiceProviderTest) TestMissingDestinationWithSignaturePresent(c *C) { @@ -717,7 +720,8 @@ func (test *ServiceProviderTest) TestMissingDestinationWithSignaturePresent(c *C c.Assert(err, IsNil) req := http.Request{PostForm: url.Values{}} - bytes, _ := removeDestinationFromDocument(addSignatureToDocument(test.responseDom())).WriteToBytes() + test.replaceDestination("") + bytes, _ := addSignatureToDocument(test.responseDom()).WriteToBytes() req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://15661444.ngrok.io/saml2/acs\", actual \"\")") From efa98f99d02785748eb141a88b49a45aefc2fe6b Mon Sep 17 00:00:00 2001 From: Christopher Vermilion Date: Fri, 12 Jul 2019 11:00:37 -0400 Subject: [PATCH 4/4] Check Destination when present, even if response unsigned --- service_provider.go | 8 ++++++-- service_provider_test.go | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/service_provider.go b/service_provider.go index 6677e1b0..ac40e888 100644 --- a/service_provider.go +++ b/service_provider.go @@ -386,7 +386,8 @@ func responseIsSigned(response *etree.Document) (bool, error) { return signatureElement != nil, nil } -// validateDestination validates the Destination attribute iff the response is signed. +// validateDestination validates the Destination attribute. +// If the response is signed, the Destination is required to be present. func (sp *ServiceProvider) validateDestination(response []byte, responseDom *Response) error { responseXml := etree.NewDocument() err := responseXml.ReadFromBytes(response) @@ -399,7 +400,10 @@ func (sp *ServiceProvider) validateDestination(response []byte, responseDom *Res return err } - if signed { + + // Compare if the response is signed OR the Destination is provided. + // (Even if the response is not signed, if the Destination is set it must match.) + if signed || responseDom.Destination != "" { if responseDom.Destination != sp.AcsURL.String() { return fmt.Errorf("`Destination` does not match AcsURL (expected %q, actual %q)", sp.AcsURL.String(), responseDom.Destination) } diff --git a/service_provider_test.go b/service_provider_test.go index 16bd7ca1..f18ba5cc 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -708,6 +708,25 @@ func (test *ServiceProviderTest) TestMismatchedDestinationsWithSignaturePresent( c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://15661444.ngrok.io/saml2/acs\", actual \"https://wrong/saml2/acs\")") } +func (test *ServiceProviderTest) TestMismatchedDestinationsWithNoSignaturePresent(c *C) { + s := ServiceProvider{ + Key: test.Key, + Certificate: test.Certificate, + MetadataURL: mustParseURL("https://15661444.ngrok.io/saml2/metadata"), + AcsURL: mustParseURL("https://15661444.ngrok.io/saml2/acs"), + IDPMetadata: &EntityDescriptor{}, + } + err := xml.Unmarshal([]byte(test.IDPMetadata), &s.IDPMetadata) + c.Assert(err, IsNil) + + req := http.Request{PostForm: url.Values{}} + test.replaceDestination("https://wrong/saml2/acs") + bytes, _ := test.responseDom().WriteToBytes() + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(bytes)) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + c.Assert(err.(*InvalidResponseError).PrivateErr.Error(), Equals, "`Destination` does not match AcsURL (expected \"https://15661444.ngrok.io/saml2/acs\", actual \"https://wrong/saml2/acs\")") +} + func (test *ServiceProviderTest) TestMissingDestinationWithSignaturePresent(c *C) { s := ServiceProvider{ Key: test.Key,