Skip to content

Commit

Permalink
changes to inlcude more validateSignature options for SAML
Browse files Browse the repository at this point in the history
  • Loading branch information
himran92 committed Dec 6, 2024
1 parent 5f2819d commit f6c7d7b
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 31 deletions.
76 changes: 51 additions & 25 deletions saml/response.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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
}
}
}
Expand All @@ -100,16 +122,16 @@ 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)
case samlResp == "":
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.
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
42 changes: 36 additions & 6 deletions saml/response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
{
Expand All @@ -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",
},
Expand Down

0 comments on commit f6c7d7b

Please sign in to comment.