diff --git a/saml/README.md b/saml/README.md index 6ff4c298..0847e940 100644 --- a/saml/README.md +++ b/saml/README.md @@ -45,3 +45,16 @@ the requirements of the interoperable SAML You can find the full demo code in the [`saml/demo`](./saml/demo/main.go) package. + +## ParseResponse signature validation policy + +`ParseResponse` always validates any signatures that are present, but the +required signature policy is controlled by options: + +- `ValidateResponseSignature()`: requires a valid signature on the top-level + SAML `Response`. +- `ValidateAssertionSignature()`: if the response is signed and valid, + assertions are accepted as covered by that signature; if the response is not + signed, each assertion must be signed and valid. +- Using both options requires a valid response signature and validates any + assertion signatures that are present. diff --git a/saml/go.mod b/saml/go.mod index 6f3c5d0c..4edcbefa 100644 --- a/saml/go.mod +++ b/saml/go.mod @@ -1,16 +1,16 @@ module github.com/hashicorp/cap/saml -go 1.23.0 +go 1.25.0 require ( - github.com/beevik/etree v1.2.0 + github.com/beevik/etree v1.6.0 github.com/crewjam/go-xmlsec v0.0.0-20200414151428-d2b1a58f7262 github.com/crewjam/saml v0.4.14 github.com/hashicorp/go-uuid v1.0.3 - github.com/jonboulle/clockwork v0.4.0 - github.com/russellhaering/gosaml2 v0.9.1 - github.com/russellhaering/goxmldsig v1.4.0 - github.com/stretchr/testify v1.8.4 + github.com/jonboulle/clockwork v0.5.0 + github.com/russellhaering/gosaml2 v0.11.0 + github.com/russellhaering/goxmldsig v1.6.0 + github.com/stretchr/testify v1.11.1 ) require ( diff --git a/saml/go.sum b/saml/go.sum index 66a4ce64..01516fb7 100644 --- a/saml/go.sum +++ b/saml/go.sum @@ -1,7 +1,5 @@ -github.com/beevik/etree v1.1.0/go.mod h1:r8Aw8JqVegEf0w2fDnATrX9VpkMcyFeM0FhwO62wh+A= -github.com/beevik/etree v1.2.0 h1:l7WETslUG/T+xOPs47dtd6jov2Ii/8/OjCldk5fYfQw= -github.com/beevik/etree v1.2.0/go.mod h1:aiPf89g/1k3AShMVAzriilpcE4R/Vuor90y83zVZWFc= -github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= +github.com/beevik/etree v1.6.0 h1:u8Kwy8pp9D9XeITj2Z0XtA5qqZEmtJtuXZRQi+j03eE= +github.com/beevik/etree v1.6.0/go.mod h1:bh4zJxiIr62SOf9pRzN7UUYaEDa9HEKafK25+sLc0Gc= github.com/crewjam/errset v0.0.0-20160219153700-f78d65de925c h1:dCJ9oZ0VgnzJHR5BjkSrwkXA1USu483qlxBd0u29P8s= github.com/crewjam/errset v0.0.0-20160219153700-f78d65de925c/go.mod h1:XhiWL7J86xoqJ8+x2OA+AM2l9skQP2DZ0UOXQYVg7uI= github.com/crewjam/go-xmlsec v0.0.0-20200414151428-d2b1a58f7262 h1:3V8RSsB1mxeAfxMb7lGSd0HlCHhc/ElJj1peaJMAkyk= @@ -17,50 +15,36 @@ github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/hashicorp/go-uuid v1.0.3 h1:2gKiV6YVmrJ1i2CKKa9obLvRieoRGviZFL26PcT/Co8= github.com/hashicorp/go-uuid v1.0.3/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= -github.com/jonboulle/clockwork v0.2.2/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= -github.com/jonboulle/clockwork v0.3.0/go.mod h1:Pkfl5aHPm1nk2H9h0bjmnJD/BcgbGXUBGnn1kMkgxc8= -github.com/jonboulle/clockwork v0.4.0 h1:p4Cf1aMWXnXAUh8lVfewRBx1zaTSYKrKMF2g3ST4RZ4= -github.com/jonboulle/clockwork v0.4.0/go.mod h1:xgRqUGwRcjKCO1vbZUEtSLrqKoPSsUpK7fnezOII0kc= -github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= -github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= -github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= +github.com/jonboulle/clockwork v0.5.0 h1:Hyh9A8u51kptdkR+cqRpT1EebBwTn1oK9YfGYbdFz6I= +github.com/jonboulle/clockwork v0.5.0/go.mod h1:3mZlmanh0g2NDKO5TWZVJAfofYk64M7XN3SzBPjZF60= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= -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.1.0 h1:RXbVD2UAl7A7nOTR4u7E3ILa4IbtvKBHw64LDsmu9hU= github.com/mattermost/xml-roundtrip-validator v0.1.0/go.mod h1:qccnGMcpgwcNaBnxqpJpWWUiPNr5H3O8eDgGV9gT5To= github.com/moov-io/signedxml v1.1.1 h1:TQ2fK4DRCYv7agH+z6RjtnBTmEyYMAztFzuHIPtUJpg= github.com/moov-io/signedxml v1.1.1/go.mod h1:p+b4f/Wo/qKyew8fHW8VZOgsILWylyvvjdE68egzbwc= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= -github.com/rogpeppe/go-internal v1.8.0/go.mod h1:WmiCO8CzOY8rg0OYDC4/i/2WRWAB6poM+XZ2dLUbcbE= github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= -github.com/russellhaering/gosaml2 v0.9.1 h1:H/whrl8NuSoxyW46Ww5lKPskm+5K+qYLw9afqJ/Zef0= -github.com/russellhaering/gosaml2 v0.9.1/go.mod h1:ja+qgbayxm+0mxBRLMSUuX3COqy+sb0RRhIGun/W2kc= -github.com/russellhaering/goxmldsig v1.3.0/go.mod h1:gM4MDENBQf7M+V824SGfyIUVFWydB7n0KkEubVJl+Tw= -github.com/russellhaering/goxmldsig v1.4.0 h1:8UcDh/xGyQiyrW+Fq5t8f+l2DLB1+zlhYzkPUJ7Qhys= -github.com/russellhaering/goxmldsig v1.4.0/go.mod h1:gM4MDENBQf7M+V824SGfyIUVFWydB7n0KkEubVJl+Tw= +github.com/russellhaering/gosaml2 v0.11.0 h1:wlWm7dWMrpJBzh0xEOZof70nVen4f/2BEF8ZXaidJ9o= +github.com/russellhaering/gosaml2 v0.11.0/go.mod h1:GmL5LeCP7PBYzSkkFxtmHuRzC2eUZ/6JSLYQd5fzKK4= +github.com/russellhaering/goxmldsig v1.6.0 h1:8fdWXEPh2k/NZNQBPFNoVfS3JmzS4ZprY/sAOpKQLks= +github.com/russellhaering/goxmldsig v1.6.0/go.mod h1:TrnaquDcYxWXfJrOjeMBTX4mLBeYAqaHEyUeWPxZlBM= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= -github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= golang.org/x/crypto v0.36.0 h1:AnAEvhDddvBdpY+uR+MyHmuZzzNqXSe/GvuDeob5L34= golang.org/x/crypto v0.36.0/go.mod h1:Y4J0ReaxCR1IMaabaSMugxJES1EpwhBHhv2bDHklZvc= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= -gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo= diff --git a/saml/response.go b/saml/response.go index a1d70d71..3c078020 100644 --- a/saml/response.go +++ b/saml/response.go @@ -9,6 +9,7 @@ import ( "encoding/pem" "fmt" "regexp" + "slices" "github.com/jonboulle/clockwork" saml2 "github.com/russellhaering/gosaml2" @@ -77,7 +78,8 @@ func InsecureSkipSignatureValidation() Option { } } -// ValidateResponseSignature enables signature validation to ensure the response is at least signed +// ValidateResponseSignature requires a valid signature on the top-level SAML +// response envelope. func ValidateResponseSignature() Option { return func(o interface{}) { if o, ok := o.(*parseResponseOptions); ok { @@ -86,7 +88,14 @@ func ValidateResponseSignature() Option { } } -// ValidateAssertionSignature enables signature validation to ensure the assertion is at least signed +// ValidateAssertionSignature enables assertion-signature policy checks. +// +// Behavior: +// - If the response envelope is signed and valid, assertions are accepted as +// covered by that signature. Any assertion signatures that are present must +// still validate. +// - If the response envelope is not signed, each assertion must be signed and +// valid. func ValidateAssertionSignature() Option { return func(o interface{}) { if o, ok := o.(*parseResponseOptions); ok { @@ -101,6 +110,8 @@ func ValidateAssertionSignature() Option { // - InsecureSkipRequestIDValidation // - InsecureSkipAssertionConditionValidation // - InsecureSkipSignatureValidation +// - ValidateResponseSignature +// - ValidateAssertionSignature // - WithAssertionConsumerServiceURL // - WithClock func (sp *ServiceProvider) ParseResponse( @@ -282,20 +293,34 @@ func parsePEMCertificate(cert []byte) (*x509.Certificate, error) { } func validateSignature(response *core.Response, op string, opts parseResponseOptions) error { - // validate child object assertions - for _, assert := range response.Assertions() { - // note: at one time func ip.ValidateEncodedResponse(...) above allows all signed or all unsigned - // assertions, and will give error if there is a mix of both. We are still looping on all assertions - // instead of retrieving signature for one assertion, so we do not depend on dependency implementation. - if !assert.SignatureValidated && opts.validateAssertionSignature { - return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + switch { + case response.SignatureValidated: + // When decoding a signed response, the internal decoder will automatically + // verify the validaty of both the response signature and any assertions + // that are signed. Any unsigned assertions will be handled by the response + // envelope signature. Only when all of this is true will the internal + // decoder set the SignatureValidated field be set to true. + return nil + case !response.SignatureValidated && opts.validateResponseSignature: + // Our response envelope is not signed but we require it + return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + case !response.SignatureValidated && opts.validateAssertionSignature: + // Our response envelope is not signed but we require that each assertion + // is signed. The internal decoder will have set SignatureValidated on + // each assertion in this code path. We can use to determine whether or not + // it is valid. + for assert := range slices.Values(response.Assertions()) { + // note: at one time func ip.ValidateEncodedResponse(...) above allows all signed or all unsigned + // assertions, and will give error if there is a mix of both. We are still looping on all assertions + // instead of retrieving signature for one assertion, so we do not depend on dependency implementation. + if !assert.SignatureValidated { + return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + } } - } - // validate root object response - if !response.SignatureValidated && opts.validateResponseSignature { + return nil + default: + // This ought to be unreachable but we'll catch here. return fmt.Errorf("%s: %w", op, ErrInvalidSignature) } - - return nil } diff --git a/saml/response_test.go b/saml/response_test.go index f5c83972..2e2d84ec 100644 --- a/saml/response_test.go +++ b/saml/response_test.go @@ -100,21 +100,28 @@ func TestServiceProvider_ParseResponse(t *testing.T) { opts: []saml.Option{saml.ValidateAssertionSignature()}, requestID: testRequestId, }, + { + name: "success - with both options enabled of validating both signatures & with only response signed with no assertions signed", + sp: testSp, + samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseSigned()))), + opts: []saml.Option{saml.ValidateResponseSignature(), saml.ValidateAssertionSignature()}, + requestID: testRequestId, + }, { name: "missing signature", sp: testSp, samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t))), opts: []saml.Option{}, requestID: testRequestId, - wantErrContains: "response and/or assertions must be signed", + wantErrContains: "Missing signature referencing the top-level element", }, { - name: "error-invalid-signature - with both options enabled of validating both signatures & with only response signed", + name: "error-invalid-signature - with both options enabled of validating both signatures & with only response signed and wrap attacked assertion", sp: testSp, - samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseSigned()))), + samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithWrapAttackedAssertion()))), opts: []saml.Option{saml.ValidateResponseSignature(), saml.ValidateAssertionSignature()}, requestID: testRequestId, - wantErrContains: "invalid signature", + wantErrContains: "Signature could not be verified", }, { name: "error-invalid-signature - with both options enabled of validating both signatures & with only assertion signed", @@ -132,14 +139,6 @@ func TestServiceProvider_ParseResponse(t *testing.T) { requestID: testRequestId, wantErrContains: "invalid signature", }, - { - name: "error-invalid-signature - with option of validate assertion signature & with just response signed", - sp: testSp, - samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseSigned()))), - opts: []saml.Option{saml.ValidateAssertionSignature()}, - requestID: testRequestId, - wantErrContains: "invalid signature", - }, { name: "err-assertion-missing-attribute-stmt", sp: testSp, @@ -498,7 +497,7 @@ NBp9UZome44qZAYH1iqrpmmjsfI9pJItsgWu3kXPjhSfj1AJGR1l9JGvJrHki1iHTA== - . + . http://saml.julz/invalid-audience diff --git a/saml/test/provider.go b/saml/test/provider.go index 5a7ea7f1..18a63478 100644 --- a/saml/test/provider.go +++ b/saml/test/provider.go @@ -431,9 +431,10 @@ func (p *TestProvider) parseRequestPost(request string) *core.AuthnRequest { } type responseOptions struct { - signResponseElem bool - signAssertionElem bool - expired bool + signResponseElem bool + signAssertionElem bool + wrapAttackAssertionElem bool + expired bool } type ResponseOption func(*responseOptions) @@ -458,6 +459,13 @@ func WithResponseAndAssertionSigned() ResponseOption { } } +func WithWrapAttackedAssertion() ResponseOption { + return func(o *responseOptions) { + o.signResponseElem = true + o.wrapAttackAssertionElem = true + } +} + func WithJustAssertionSigned() ResponseOption { return func(o *responseOptions) { o.signAssertionElem = true @@ -584,6 +592,27 @@ func (p *TestProvider) SamlResponse(t *testing.T, opts ...ResponseOption) string } } + if opt.wrapAttackAssertionElem { + signCtx := dsig.NewDefaultSigningContext(p.keystore) + wrapCtx := dsig.NewDefaultSigningContext(dsig.RandomKeyStoreForTest()) + + // Sign the signature with a mismatched context but wrap it with the correct + // signature. + responseEl := doc.SelectElement("Response") + for _, assert := range responseEl.FindElements("Assertion") { + signedAssert, err := wrapCtx.SignEnveloped(assert) + r.NoError(err) + + // replace signed assert object + responseEl.RemoveChildAt(assert.Index()) + responseEl.AddChild(signedAssert) + } + + signed, err := signCtx.SignEnveloped(doc.Root()) + r.NoError(err) + doc.SetRoot(signed) + } + result, err := doc.WriteToString() r.NoError(err)