Skip to content

Commit

Permalink
small cleanup + small fix to test
Browse files Browse the repository at this point in the history
  • Loading branch information
himran92 committed Nov 28, 2024
1 parent 33fc312 commit 340d6e4
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 10 deletions.
11 changes: 5 additions & 6 deletions saml/response.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ func (sp *ServiceProvider) ParseResponse(

// This will validate the response and all assertions.
response, err := ip.ValidateEncodedResponse(samlResp)

switch {
case err != nil:
return nil, fmt.Errorf("%s: unable to validate encoded response: %w", op, err)
Expand Down Expand Up @@ -154,7 +153,7 @@ func (sp *ServiceProvider) ParseResponse(
}

if !opts.skipSignatureValidation {
// func ip.ValidateEncodedResponse(...) above only requires either response or all its assertions are signed,
// func ip.ValidateEncodedResponse(...) above only requires either `response or all its `assertions` are signed,
// but does not require both. Adding another check to validate that both of these are signed always.
if err := validateSignature(response, op); err != nil {
return nil, err
Expand Down Expand Up @@ -257,17 +256,17 @@ func parsePEMCertificate(cert []byte) (*x509.Certificate, error) {
}

func validateSignature(response *types.Response, op string) error {
// validate child attr assertions
// 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 are both. We are still looping on all assertions instead of
// retrieving value for one assertion, so we do not depend on dependency implementation.
// 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)
}
}

// validate root response attr
// validate root object response
if !response.SignatureValidated {
return fmt.Errorf("%s: %w", op, ErrInvalidSignature)
}
Expand Down
2 changes: 1 addition & 1 deletion saml/response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func TestServiceProvider_ParseResponse(t *testing.T) {
{
name: "error-invalid-signature",
sp: testSp,
samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustResponseElemSigned()))),
samlResp: base64.StdEncoding.EncodeToString([]byte(tp.SamlResponse(t, testprovider.WithJustAssertionElemSigned()))),
opts: []saml.Option{},
requestID: testRequestId,
wantErrContains: "invalid signature",
Expand Down
6 changes: 3 additions & 3 deletions saml/test/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,20 +561,20 @@ func (p *TestProvider) SamlResponse(t *testing.T, opts ...ResponseOption) string
if opt.signResponseElem || opt.signAssertionElem {
signCtx := dsig.NewDefaultSigningContext(p.keystore)

// sign child attr assertions
// sign child object assertions
if opt.signAssertionElem {
responseEl := doc.SelectElement("Response")
for _, assert := range responseEl.FindElements("Assertion") {
signedAssert, err := signCtx.SignEnveloped(assert)
r.NoError(err)

// replace signed assert element
// replace signed assert object
responseEl.RemoveChildAt(assert.Index())
responseEl.AddChild(signedAssert)
}
}

// sign root attr response
// sign root object response
if opt.signResponseElem {
signed, err := signCtx.SignEnveloped(doc.Root())
r.NoError(err)
Expand Down

0 comments on commit 340d6e4

Please sign in to comment.