diff --git a/Makefile b/Makefile new file mode 100644 index 00000000..e8b4c3a1 --- /dev/null +++ b/Makefile @@ -0,0 +1,10 @@ +.PHONY: help +.DEFAULT_GOAL := help + +test: ## Run package tests + @GO111MODULE=auto go test + +help: ## Display this help message + @awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_\/-]+:.*?## / {printf "\033[34m%-30s\033[0m %s\n", $$1, $$2}' $(MAKEFILE_LIST) | \ + sort | \ + grep -v '#' diff --git a/go.mod b/go.mod index dceabc4f..eb68f9be 100644 --- a/go.mod +++ b/go.mod @@ -1,10 +1,11 @@ -module github.com/crewjam/saml +module github.com/InVisionApp/saml go 1.13 require ( github.com/beevik/etree v1.1.0 github.com/crewjam/httperr v0.2.0 + github.com/crewjam/saml v0.4.5 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/dchest/uniuri v0.0.0-20200228104902-7aecb25e1fe5 github.com/form3tech-oss/jwt-go v3.2.2+incompatible @@ -14,7 +15,7 @@ require ( github.com/kr/text v0.2.0 // indirect github.com/mattermost/xml-roundtrip-validator v0.1.0 github.com/pkg/errors v0.9.1 // indirect - github.com/russellhaering/goxmldsig v1.1.0 + github.com/russellhaering/goxmldsig v1.1.1-0.20201210191726-3541f5e554ee github.com/zenazn/goji v1.0.1 golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect diff --git a/go.sum b/go.sum index c46f4a4f..cb05cb88 100644 --- a/go.sum +++ b/go.sum @@ -1,18 +1,24 @@ github.com/beevik/etree v1.1.0 h1:T0xke/WvNtMoCqgzPhkX2r4rjY3GDZFi+FjpRZY2Jbs= github.com/beevik/etree v1.1.0/go.mod h1:r8Aw8JqVegEf0w2fDnATrX9VpkMcyFeM0FhwO62wh+A= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/crewjam/httperr v0.0.0-20190612203328-a946449404da/go.mod h1:+rmNIXRvYMqLQeR4DHyTvs6y0MEMymTz4vyFpFkKTPs= github.com/crewjam/httperr v0.2.0 h1:b2BfXR8U3AlIHwNeFFvZ+BV1LFvKLlzMjzaTnZMybNo= github.com/crewjam/httperr v0.2.0/go.mod h1:Jlz+Sg/XqBQhyMjdDiC+GNNRzZTD7x39Gu3pglZ5oH4= +github.com/crewjam/saml v0.4.5 h1:H9u+6CZAESUKHxMyxUbVn0IawYvKZn4nt3d4ccV4O/M= +github.com/crewjam/saml v0.4.5/go.mod h1:qCJQpUtZte9R1ZjUBcW8qtCNlinbO363ooNl02S68bk= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dchest/uniuri v0.0.0-20160212164326-8902c56451e9/go.mod h1:GgB8SF9nRG+GqaDtLcwJZsQFhcogVCJ79j4EdT0c2V4= github.com/dchest/uniuri v0.0.0-20200228104902-7aecb25e1fe5 h1:RAV05c0xOkJ3dZGS0JFybxFKZ2WMLabgx3uXnd7rpGs= github.com/dchest/uniuri v0.0.0-20200228104902-7aecb25e1fe5/go.mod h1:GgB8SF9nRG+GqaDtLcwJZsQFhcogVCJ79j4EdT0c2V4= +github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/form3tech-oss/jwt-go v3.2.2+incompatible h1:TcekIExNqud5crz4xD2pavyTgWiPvpYe4Xau31I0PRk= github.com/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k= github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/jonboulle/clockwork v0.2.0/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= +github.com/jonboulle/clockwork v0.2.1/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= github.com/jonboulle/clockwork v0.2.2 h1:UOGuzwb1PwsrDAObMuhUnj0p5ULPj8V/xJ7Kx9qUBdQ= github.com/jonboulle/clockwork v0.2.2/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI= @@ -21,6 +27,7 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/mattermost/xml-roundtrip-validator v0.0.0-20201213122252-bcd7e1b9601e/go.mod h1:qccnGMcpgwcNaBnxqpJpWWUiPNr5H3O8eDgGV9gT5To= github.com/mattermost/xml-roundtrip-validator v0.1.0 h1:RXbVD2UAl7A7nOTR4u7E3ILa4IbtvKBHw64LDsmu9hU= github.com/mattermost/xml-roundtrip-validator v0.1.0/go.mod h1:qccnGMcpgwcNaBnxqpJpWWUiPNr5H3O8eDgGV9gT5To= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= @@ -30,17 +37,26 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russellhaering/goxmldsig v1.1.0 h1:lK/zeJie2sqG52ZAlPNn1oBBqsIsEKypUUBGpYYF6lk= github.com/russellhaering/goxmldsig v1.1.0/go.mod h1:QK8GhXPB3+AfuCrfo0oRISa9NfzeCpWmxeGnqEpDF9o= +github.com/russellhaering/goxmldsig v1.1.1-0.20201210191726-3541f5e554ee h1:crOrBljowvmyKZv2tCRyngWUPHyMHeKRaycSJuEw5/Q= +github.com/russellhaering/goxmldsig v1.1.1-0.20201210191726-3541f5e554ee/go.mod h1:QK8GhXPB3+AfuCrfo0oRISa9NfzeCpWmxeGnqEpDF9o= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/zenazn/goji v0.9.1-0.20160507202103-64eb34159fe5/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q= github.com/zenazn/goji v1.0.1 h1:4lbD8Mx2h7IvloP7r2C0D6ltZP6Ufip8Hn0wmSK5LR8= github.com/zenazn/goji v1.0.1/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q= +golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/crypto v0.0.0-20190923035154-9ee001bba392/go.mod h1:/lpIB1dKB+9EgE3H3cr1v9wB50oz8l4C4h62xy7jSTY= golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2 h1:It14KIkyBFYkHkwZ7k45minvA9aorojkyjGk9KJ5B/w= golang.org/x/crypto v0.0.0-20210322153248-0c34fe9e7dc2/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4= +golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= +golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190922100055-0a153f010e69/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/service_provider.go b/service_provider.go index f76140ab..94c11412 100644 --- a/service_provider.go +++ b/service_provider.go @@ -108,6 +108,37 @@ type ServiceProvider struct { // SignatureMethod, if non-empty, authentication requests will be signed SignatureMethod string + + // + // INVISION CHANGES BELOW + // + + // SkipDestinationCheck, if true, will skip the Destination validation when parsing the response. + // + // AUTH-2414: If the message is signed, the Destination XML attribute in the root SAML element of the protocol + // message MUST contain the URL to which the sender has instructed the user agent to deliver the + // message. The recipient MUST then verify that the value matches the location at which the message has + // been received. Ref: http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf @ 3.4.5.2 + // + // Unfortunately, PingFederate does not send Destination, but Okta and OneLogin do. + // This will check Destination only if provided. The Recipient is checked to the ACSURL in validateAssertion() + // so this particular check is not terribly important. + SkipDestinationCheck bool + + // SkipIssuerCheck, if true, will skip Issuer validation when parsing the response. + // + // Most IdP's allow anything as value for the Issuer, even empty string, which makes checking + // the Issuer rather pointless. + SkipIssuerCheck bool + + // EaseAudienceRestrictions, if true, will only check that the value has the same host as the metadata URL. + // + // This has been changed from the upstream which forces the AudienceRestriction + // value to equal the metadata URL. That is not a requirement in the SAML spec + // and does not meet work with conversions. V6 was implemented such that the + // AudienceRestriction value is the unique company URL (with subdomain). + // In order to maintain backwards compatibility, allow the URL without path. + EaseAudienceRestrictions bool } // MaxIssueDelay is the longest allowed time between when a SAML assertion is @@ -529,6 +560,10 @@ func (sp *ServiceProvider) validateDestination(response []byte, responseDom *Res return err } + if sp.SkipDestinationCheck && responseDom.Destination == "" { + return nil + } + signed, err := responseIsSigned(responseXML) if err != nil { return err @@ -608,16 +643,16 @@ func (sp *ServiceProvider) ParseXMLResponse(decodedResponseXML []byte, possibleR requestIDvalid := false - if sp.AllowIDPInitiated { - requestIDvalid = true - } else { - for _, possibleRequestID := range possibleRequestIDs { - if resp.InResponseTo == possibleRequestID { - requestIDvalid = true - } + for _, possibleRequestID := range possibleRequestIDs { + if resp.InResponseTo == possibleRequestID { + requestIDvalid = true } } + if sp.AllowIDPInitiated && !requestIDvalid { + requestIDvalid = true + } + if !requestIDvalid { retErr.PrivateErr = fmt.Errorf("`InResponseTo` does not match any of the possible request IDs (expected %v)", possibleRequestIDs) return nil, retErr @@ -627,7 +662,7 @@ func (sp *ServiceProvider) ParseXMLResponse(decodedResponseXML []byte, possibleR retErr.PrivateErr = fmt.Errorf("response IssueInstant expired at %s", resp.IssueInstant.Add(MaxIssueDelay)) return nil, retErr } - if resp.Issuer != nil && resp.Issuer.Value != sp.IDPMetadata.EntityID { + if !sp.SkipIssuerCheck && resp.Issuer != nil && resp.Issuer.Value != sp.IDPMetadata.EntityID { retErr.PrivateErr = fmt.Errorf("response Issuer does not match the IDP metadata (expected %q)", sp.IDPMetadata.EntityID) return nil, retErr } @@ -675,12 +710,16 @@ func (sp *ServiceProvider) ParseXMLResponse(decodedResponseXML []byte, possibleR retErr.PrivateErr = err return nil, retErr } + /* BUG(gus): Disabling this validation because the transforms in goxmldsig v1.1.0 are broken. + So even if you have a correct digest it will fail. Once this is fixed, there are PRs, + we need to reenable this. if responseSigned { if err := sp.validateSigned(doc.Root()); err != nil { retErr.PrivateErr = err return nil, retErr } } + */ var key interface{} = sp.Key keyEl := doc.FindElement("//EncryptedAssertion/EncryptedKey") @@ -744,12 +783,19 @@ func (sp *ServiceProvider) validateAssertion(assertion *Assertion, possibleReque if assertion.IssueInstant.Add(MaxIssueDelay).Before(now) { return fmt.Errorf("expired on %s", assertion.IssueInstant.Add(MaxIssueDelay)) } - if assertion.Issuer.Value != sp.IDPMetadata.EntityID { + if !sp.SkipIssuerCheck && assertion.Issuer.Value != sp.IDPMetadata.EntityID { return fmt.Errorf("issuer is not %q", sp.IDPMetadata.EntityID) } 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 @@ -767,16 +813,12 @@ func (sp *ServiceProvider) validateAssertion(assertion *Assertion, possibleReque // // 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 sp.AllowIDPInitiated && !requestIDvalid { + requestIDvalid = true + } + + 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()) @@ -798,6 +840,9 @@ func (sp *ServiceProvider) validateAssertion(assertion *Assertion, possibleReque if audienceRestriction.Audience.Value == audience { audienceRestrictionsValid = true } + if !audienceRestrictionsValid && sp.EaseAudienceRestrictions { + audienceRestrictionsValid = IsSameBase(audience, audienceRestriction.Audience.Value) + } } if !audienceRestrictionsValid { return fmt.Errorf("assertion Conditions AudienceRestriction does not contain %q", audience) diff --git a/service_provider_test.go b/service_provider_test.go index 72d95070..fe075061 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -1648,3 +1648,68 @@ func TestSPResponseWithNoIssuer(t *testing.T) { _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) assert.Check(t, err) } + +func TestSkipDestinationCheck(t *testing.T) { + test := NewServiceProviderTest(t) + + 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{}, + SkipDestinationCheck: true, + } + err := xml.Unmarshal([]byte(test.IDPMetadata), &s.IDPMetadata) + assert.Check(t, err) + + req := http.Request{PostForm: url.Values{}} + test.replaceDestination("") + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(test.SamlResponse)) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + assert.Check(t, err) +} + +func TestSkipIssuerCheck(t *testing.T) { + test := NewServiceProviderTest(t) + + 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{}, + SkipIssuerCheck: true, + } + err := xml.Unmarshal(test.IDPMetadata, &s.IDPMetadata) + assert.Check(t, err) + + req := http.Request{PostForm: url.Values{}} + test.SamlResponse = bytes.Replace(test.SamlResponse, + []byte(`https://idp.testshib.org/idp/shibboleth`), []byte("PINEAPPLE"), 1) + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(test.SamlResponse)) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + assert.Check(t, err) +} + +func TestEaseAudienceRestrictions(t *testing.T) { + test := NewServiceProviderTest(t) + + 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{}, + SkipIssuerCheck: true, + } + err := xml.Unmarshal(test.IDPMetadata, &s.IDPMetadata) + assert.Check(t, err) + + req := http.Request{PostForm: url.Values{}} + test.SamlResponse = bytes.Replace(test.SamlResponse, + []byte(`https://idp.testshib.org/idp/shibboleth`), []byte("PINEAPPLE"), 1) + req.PostForm.Set("SAMLResponse", base64.StdEncoding.EncodeToString(test.SamlResponse)) + _, err = s.ParseResponse(&req, []string{"id-9e61753d64e928af5a7a341a97f420c9"}) + assert.Check(t, err) +} diff --git a/util.go b/util.go index c9731b1b..579d24dc 100644 --- a/util.go +++ b/util.go @@ -3,6 +3,8 @@ package saml import ( "crypto/rand" "io" + "net/url" + "strings" "time" dsig "github.com/russellhaering/goxmldsig" @@ -29,3 +31,28 @@ func randomBytes(n int) []byte { } return rv } + +// IsSameBase returns true if both urls have the same base URI, false otherwise. +func IsSameBase(refURL, someURL string) bool { + if refURL == someURL { + return true + } + + ref, err := url.Parse(refURL) + if err != nil { + return false + } + + if ref.Host == "" { + return false + } + + base := ref.ResolveReference(&url.URL{Path: "/"}) + base.Path = "" // Strip path "/" + + if strings.HasPrefix(someURL, base.String()) { + return true + } + + return false +} diff --git a/util_test.go b/util_test.go new file mode 100644 index 00000000..105f97b1 --- /dev/null +++ b/util_test.go @@ -0,0 +1,50 @@ +package saml + +import "testing" + +func TestIsSameBase(t *testing.T) { + type args struct { + refURL string + someURL string + } + tests := []struct { + name string + args args + want bool + }{ + {name: "both empty", + args: args{}, + want: true}, + {name: "refURL empty", + args: args{refURL: "", someURL: "https://some.work"}, + want: false}, + {name: "someURL empty", + args: args{refURL: "https://some.work", someURL: ""}, + want: false}, + {name: "different schemes", + args: args{refURL: "https://some.work", someURL: "http://some.work"}, + want: false}, + {name: "should match 1", + args: args{refURL: "https://some.work", someURL: "https://some.work/a/b/c"}, + want: true}, + {name: "should match 2", + args: args{refURL: "https://some.work/a/b/c", someURL: "https://some.work"}, + want: true}, + {name: "should match 3", + args: args{refURL: "https://some.work/a/b/c", someURL: "https://some.work/1/2/3/4/5"}, + want: true}, + {name: "similar match", + args: args{refURL: "https://some.work/", someURL: "https://some.work"}, + want: true}, + {name: "exact match", + args: args{refURL: "https://some.work", someURL: "https://some.work"}, + want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsSameBase(tt.args.refURL, tt.args.someURL); got != tt.want { + t.Errorf("IsSameBase() = %v, want %v", got, tt.want) + } + }) + } +}