From f6c7d7b002cb23376da83cdabbcd75c124b84daa Mon Sep 17 00:00:00 2001 From: Hafsa Imran Date: Fri, 6 Dec 2024 16:37:02 -0500 Subject: [PATCH] changes to inlcude more validateSignature options for SAML --- saml/response.go | 76 +++++++++++++++++++++++++++++-------------- saml/response_test.go | 42 ++++++++++++++++++++---- 2 files changed, 87 insertions(+), 31 deletions(-) diff --git a/saml/response.go b/saml/response.go index 4592db7..984e593 100644 --- a/saml/response.go +++ b/saml/response.go @@ -19,21 +19,25 @@ import ( ) type parseResponseOptions struct { - clock clockwork.Clock - skipRequestIDValidation bool - skipAssertionConditionValidation bool - skipSignatureValidation bool - assertionConsumerServiceURL string - requireSignatureForResponseAndAssertion bool + clock clockwork.Clock + skipRequestIDValidation bool + skipAssertionConditionValidation bool + skipSignatureValidation bool + assertionConsumerServiceURL string + validateResponseAndAssertionSignatures bool + validateResponseSignature bool + validateAssertionSignature bool } func parseResponseOptionsDefault() parseResponseOptions { return parseResponseOptions{ - clock: clockwork.NewRealClock(), - skipRequestIDValidation: false, - skipAssertionConditionValidation: false, - skipSignatureValidation: false, - requireSignatureForResponseAndAssertion: false, + clock: clockwork.NewRealClock(), + skipRequestIDValidation: false, + skipAssertionConditionValidation: false, + skipSignatureValidation: false, + validateResponseAndAssertionSignatures: false, + validateResponseSignature: false, + validateAssertionSignature: false, } } @@ -75,11 +79,29 @@ func InsecureSkipSignatureValidation() Option { } } -// RequireSignatureForBothResponseAndAssertion enables validation of both the SAML Response and its assertions. -func RequireSignatureForBothResponseAndAssertion() Option { +// ValidateResponseAndAssertionSignatures enables validation of both the SAML response and its assertions. +func ValidateResponseAndAssertionSignatures() Option { return func(o interface{}) { if o, ok := o.(*parseResponseOptions); ok { - o.requireSignatureForResponseAndAssertion = true + o.validateResponseAndAssertionSignatures = true + } + } +} + +// ValidateAssertionSignature enables validation of just the SAML response. +func ValidateResponseSignature() Option { + return func(o interface{}) { + if o, ok := o.(*parseResponseOptions); ok { + o.validateResponseSignature = true + } + } +} + +// ValidateAssertionSignature enables validation of just the SAML assertion. +func ValidateAssertionSignature() Option { + return func(o interface{}) { + if o, ok := o.(*parseResponseOptions); ok { + o.validateAssertionSignature = true } } } @@ -100,6 +122,7 @@ func (sp *ServiceProvider) ParseResponse( const op = "saml.(ServiceProvider).ParseResponse" opts := getParseResponseOptions(opt...) + callValidateSignature := opts.validateResponseAndAssertionSignatures || opts.validateResponseSignature || opts.validateAssertionSignature switch { case sp == nil: return nil, fmt.Errorf("%s: missing service provider %w", op, ErrInternal) @@ -107,9 +130,8 @@ func (sp *ServiceProvider) ParseResponse( return nil, fmt.Errorf("%s: missing saml response: %w", op, ErrInvalidParameter) case requestID == "": return nil, fmt.Errorf("%s: missing request ID: %w", op, ErrInvalidParameter) - case opts.skipSignatureValidation && opts.requireSignatureForResponseAndAssertion: - return nil, fmt.Errorf("%s: option `skip signature validation` cannot be true with `require signature"+ - " for response and assertion` : %w", op, ErrInvalidParameter) + case opts.skipSignatureValidation && callValidateSignature: + return nil, fmt.Errorf("%s: option `skip signature validation` cannot be true with any validate signature option : %w", op, ErrInvalidParameter) } // We use github.com/russellhaering/gosaml2 for SAMLResponse signature and condition validation. @@ -167,12 +189,11 @@ func (sp *ServiceProvider) ParseResponse( } samlResponse := core.Response{Response: *response} - if opts.requireSignatureForResponseAndAssertion { + if callValidateSignature { // func ip.ValidateEncodedResponse(...) above only requires either `response or all its `assertions` are signed, - // but does not require both. - // If option requireSignatureForResponseAndAssertion is true, adding another check to validate that both of - // these are signed always. - if err := validateSignature(&samlResponse, op); err != nil { + // but does not require both. The validateSignature function will validate either response or assertion is signeed + // or both depending on the the parse response options given. + if err := validateSignature(&samlResponse, op, opts); err != nil { return nil, err } } @@ -272,20 +293,25 @@ func parsePEMCertificate(cert []byte) (*x509.Certificate, error) { return x509.ParseCertificate(block.Bytes) } -func validateSignature(response *core.Response, op string) error { +func validateSignature(response *core.Response, op string, opts parseResponseOptions) error { // validate child object assertions for _, assert := range response.Assertions() { if !assert.SignatureValidated { // 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 value for one assertion, so we do not depend on dependency implementation. - return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + if opts.validateAssertionSignature || opts.validateResponseAndAssertionSignatures { + return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + } } } // validate root object response if !response.SignatureValidated { - return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + if opts.validateResponseSignature || opts.validateResponseAndAssertionSignatures { + return fmt.Errorf("%s: %w", op, ErrInvalidSignature) + } } + return nil } diff --git a/saml/response_test.go b/saml/response_test.go index 28efa65..d1c43c2 100644 --- a/saml/response_test.go +++ b/saml/response_test.go @@ -80,10 +80,24 @@ func TestServiceProvider_ParseResponse(t *testing.T) { requestID: testRequestId, }, { - name: "success - with both response and assertion signed and both signature required", + name: "success for option validate both signatures - with both response and assertion signed", sp: testSp, samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithResponseAndAssertionSigned()))), - opts: []saml.Option{saml.RequireSignatureForBothResponseAndAssertion()}, + opts: []saml.Option{saml.ValidateResponseAndAssertionSignatures()}, + requestID: testRequestId, + }, + { + name: "success for option validate response signature - with only response signed", + sp: testSp, + samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseElemSigned()))), + opts: []saml.Option{saml.ValidateResponseSignature()}, + requestID: testRequestId, + }, + { + name: "success for option validate assertion signature - with only assertion signed", + sp: testSp, + samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustAssertionElemSigned()))), + opts: []saml.Option{saml.ValidateAssertionSignature()}, requestID: testRequestId, }, { @@ -95,18 +109,34 @@ func TestServiceProvider_ParseResponse(t *testing.T) { wantErrContains: "response and/or assertions must be signed", }, { - name: "error-invalid-signature - with just response signed", + name: "error-invalid-signature for option validate both signatures - with just response signed", sp: testSp, samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseElemSigned()))), - opts: []saml.Option{saml.RequireSignatureForBothResponseAndAssertion()}, + opts: []saml.Option{saml.ValidateResponseAndAssertionSignatures()}, + requestID: testRequestId, + wantErrContains: "invalid signature", + }, + { + name: "error-invalid-signature for option validate both signatures - with just assertion signed", + sp: testSp, + samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustAssertionElemSigned()))), + opts: []saml.Option{saml.ValidateResponseAndAssertionSignatures()}, requestID: testRequestId, wantErrContains: "invalid signature", }, { - name: "error-invalid-signature - with just assertion signed", + name: "error-invalid-signature for option validate response signature - with just assertion signed", sp: testSp, samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustAssertionElemSigned()))), - opts: []saml.Option{saml.RequireSignatureForBothResponseAndAssertion()}, + opts: []saml.Option{saml.ValidateResponseSignature()}, + requestID: testRequestId, + wantErrContains: "invalid signature", + }, + { + name: "error-invalid-signature for option validate assertion signature - with just response signed", + sp: testSp, + samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseElemSigned()))), + opts: []saml.Option{saml.ValidateAssertionSignature()}, requestID: testRequestId, wantErrContains: "invalid signature", },