Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancing signature validation in SAML Response #144 #145

Merged
merged 14 commits into from
Dec 30, 2024
Merged

Enhancing signature validation in SAML Response #144 #145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c0e0eb0
adding signature for assertion in saml provider, enhancing validation…
himran92 Nov 28, 2024
91cd02b
remove duplicate tests
himran92 Nov 28, 2024
33fc312
linting done
himran92 Nov 28, 2024
340d6e4
small cleanup + small fix to test
himran92 Nov 28, 2024
9b4f45e
add helping comment
himran92 Nov 28, 2024
3748d22
use cap/saml response types
himran92 Dec 2, 2024
5f2819d
making validation of signature of both fields as optional and adding …
himran92 Dec 3, 2024
f6c7d7b
changes to inlcude more validateSignature options for SAML
himran92 Dec 6, 2024
c9cf0d9
improving / fixing comments and var names
himran92 Dec 6, 2024
c4af8fc
improving / fixing comments and var names
himran92 Dec 6, 2024
d857fba
improving variable name
himran92 Dec 6, 2024
cd6c290
add validation to only allow one validateSignature option at one time
himran92 Dec 16, 2024
011ef0c
removing validateBothSignature option
himran92 Dec 30, 2024
8fe0cbf
Merge branch 'main' into VAULT-27387/himran92-enhance-signature-valid…
himran92 Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
small cleanup + small fix to test
  • Loading branch information
@himran92
himran92 committed Nov 28, 2024
commit 340d6e408b05ed0b9ae2d26e97db9876df8d6dcd
11 changes: 5 additions & 6 deletions saml/response.go
View file
Open in desktop
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this fully optional? Something like opts.ResponseSigned or opts.ResponseAndAssertionsSigned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agree that we need to make this optional.
I had a discussion internally and moved the PR to draft again to bring the optional field change + test the e2e flow with vault.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made all settings optional.

// 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 {
Copy link
Collaborator

@hcjulz hcjulz Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gosaml2 requires either the response or the assertions to be signed. It doesn't require both.

My qustion would be if we need to sign both (response and assertions)? Because signature inheritance applies (if the parent object is signed, the child object is included).
Having both objects signed makes sense in some instances where the service provider could retrieve the assertion outside of a SAML Response, but that's not the case here.

Copy link
Collaborator

@hcjulz hcjulz Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some further digging on this. There were many threads on the internet were customers required to have both, response and assertions to be signed. So there is a customer need for that.

Most SAML IDPs (e.g. Entra ID and Okta) do provide the option to sign both. However, it looks like there are indeed some IDPs that do not provide the option, such as Auth0. This is from their application settings:

signResponse (bool): Whether or not the SAML Response should be signed. By default the SAML Assertion will be signed, but not the SAML Response. If true, SAML Response will be signed instead of SAML Assertion.

On your edit above in the description:

After few major version, the vault variable will be removed and user will always have to sign both.

Making both a requirement might lead to incompatibilities with some IDPs (like Auth0 for example). So I wonder if supporting all 3 options would be the ideal case:

  • Either Response or Assertion has to be signed
  • Response has to be signed
  • Both have to be signed.

Copy link
Collaborator Author

@himran92 himran92 Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @hcjulz for researching this point.
I do agree that with this point it does not make sense to add just a strict validation option today and make the setting permanent in future version , rather what makes sense is to give user the control to opt for right validation.
I still would need to check with team on thoughts here as it was called out as a gap from trail of bits. Will update shortly if team aligns with these thoughts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes done to add separate options for all signature validation. Really appreciate for you finding the info on Auth0 as IDP.

Copy link
Collaborator Author

@himran92 himran92 Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also btw the options i kept are:

  • assertion is at least signed
  • response is at least signed
  • both are signed

Did not include the either option as i thought user should makes a known decision relative to their IDP. Also either is the default case today when you do not opt for any option.
Let me know if you think otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @himran92 sorry for the late response and thx a lot for addressing my comments 🙇

Sounds great. The options makes totally sense.

The default would still be either, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am catching up after vacations :).

yes correct.
The default where neither of these options is provided is still either.
This is given by these unit tests.

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
View file
Open in desktop
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
View file
Open in desktop
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
Loading