Enhancing signature validation in SAML Response #144 #145

himran92 · 2024-11-29T17:12:33Z

The SAML protocol allows signing of a Response, its Assertion(s), neither, or both. Since Assertion(s) are sub-elements of a Response, they are signed if the Response is signed. Today we are depending on the gosaml2 for signature validation. It only checks only one or the other be signed.
For enhancing security, we would like to give user the options to sign just assign response or assertion or both.

Changes in PR include:

adding two new options for parse response
enhancing SAML response to validate either response or assertion or both according to the given options
extending provider to allow signing response or assertion as controlled by parameters
extending tests to cover these cases

Points before/after merging:

We will only merge once the affiliated PR for vault SAML auth plugin is also approved.
Once approved, we will work with repo owners on cutting a new release. Since the last release was cut, i see this current PR for SAML and this other PR for oidc as the impacting change. As the other PR is for oidc, it seems it will only impact vault-plugin-auth-jwt when its updated to use the latest cap release. Checked with oidc PR author that it is a backward compatible change.
EDIT: there is another PR merged in cap related to Adds an option to enable sAMAccountname logins when upndomain is set

… on signature for saml response & extending tests

hashicorp-cla-app · 2024-11-29T17:12:47Z

All committers have signed the CLA.

hashicorp-cla-app · 2024-11-29T17:12:47Z

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

hcjulz

Hey @himran92 👋 Thx a lot for opening this PR.

A general comment:
I think gosaml2 follows the SAML spec and it is not a requirement to sign both Response and Assertions. If the Reponse is signed signature inheritance applies, and if the assertions are signed it leaves only the Response unsigned. The fields in the Response object are not really an issue if tempered. The reason is mainly because relevant fields like the InResponseTo field is (should be) checked on the service provider. The cap/saml implementation by default requires the InResponseTo field to be checked and explicitly prefixes the option to leave it out with Insecure. If additional protection is required.

If we, however, still want to have the entire response to be signed, I'd suggest to make this optional. If this is forced it could lead to be incompatible with identity providers that do not provide the option to signed the full response.

I left a few comments/questions.

hcjulz · 2024-12-02T13:17:30Z

saml/response.go

@@ -12,6 +12,7 @@ import (

 	"github.com/jonboulle/clockwork"
 	saml2 "github.com/russellhaering/gosaml2"
+	"github.com/russellhaering/gosaml2/types"


Can we use the cap/saml types instead? https://github.com/hashicorp/cap/blob/main/saml/models/core/response.go#L10-L14

hcjulz · 2024-12-02T13:32:16Z

saml/response.go

+
+func validateSignature(response *types.Response, op string) error {
+	// validate child object assertions
+	for _, assert := range response.Assertions {


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.

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.

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.

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

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.

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?

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.

hcjulz · 2024-12-02T13:33:36Z

saml/response.go

@@ -151,6 +152,14 @@ func (sp *ServiceProvider) ParseResponse(
 		}
 	}

+	if !opts.skipSignatureValidation {


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

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.

Made all settings optional.

…unit tests to cover

himran92 · 2024-12-04T16:42:15Z

saml/response.go

+	skipAssertionConditionValidation        bool
+	skipSignatureValidation                 bool
+	assertionConsumerServiceURL             string
+	requireSignatureForResponseAndAssertion bool


For requireSignatureForResponseAndAssertion :
On the vault usage side i opted for a name EnableStrictResponseSignatureValidation as that sounded more user friendly for a user exposed setting. The description will share the impact of the setting. Whereas kept it requireSignatureForResponseAndAssertion on cap side thinking of it as more of an internal lib so can have any name.

Any preference of using EnableStrictResponseSignatureValidation here?

I don't have a strong preference. Based on my comment about having all 3 options signature validation, it would just be important to think about the general naming pattern:

Validate Response And Assertion Signatures = EnableStrictSignatureValidation

Validate Response Signature = EnableResponseSignatureValidation?

Alternatively, we could use a more explicit naming pattern:

ValidateResponseSignature

ValidateResponseAndAssertionSignatures

I'm ok either way.

Updated the name of the setting.

kpcraig · 2024-12-16T17:02:45Z

saml/response.go

 	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 && callValidateSignature:
+		return nil, fmt.Errorf("%s: option `skip signature validation` cannot be true with any validate signature option : %w", op, ErrInvalidParameter)
+	case multipleSignatureOptionEnabled(opts.validateResponseAndAssertionSignatures, opts.validateResponseSignature, opts.validateAssertionSignature):


I think I see discussion on these options already in the discussion, so I'll defer to cap people on the ergonomics/patterns, but if we need to check if the options are sensible, would it be better to reshape the options so that any pattern is valid? (for example, can we remove the "both" boolean and let having both validateResponse and validateAssertion be true imply it instead?

If there is a need/desire to have an explicit "both" option mutually exclusive with the other two, would it be worth turning it into a single field set to const values, e.g.

const ( NoSignatureValidation = iota ValidateAssertionSignature ValidateResponseSignature ValidateResponseAndAssertionSignature )

I understand we wouldn't want to remove the InsecureSkipValidation functions, for compatibility reasons.

@hcjulz Do you have any recommendation for above w.r.t cap?

I like Kay point to remove the both option and ask user to set individual option to achieve signatures-validation-on-both for cap level and then similarly at Vault Plugin level too

@mgaffney just randomly tagging you as you are mentioned as the repo owner.
Would you be able to assist in the review as Julian is out till end of the year, thanks

Let me know if you prefer/recommend someone else.

Mike was not familiar with this piece of code..
tagging @jimlambrt for review for this Pr.

I think, removing ValidateResponseAndAssertionSignatures makes for a better
dev experience.

PR is updated now with only keeping two options

jimlambrt

Ty for the tests!

jimlambrt · 2024-12-20T15:00:48Z

saml/response.go

 	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 && callValidateSignature:
+		return nil, fmt.Errorf("%s: option `skip signature validation` cannot be true with any validate signature option : %w", op, ErrInvalidParameter)
+	case multipleSignatureOptionEnabled(opts.validateResponseAndAssertionSignatures, opts.validateResponseSignature, opts.validateAssertionSignature):


I think, removing ValidateResponseAndAssertionSignatures makes for a better
dev experience.

…ation-for-saml-response

jimlambrt

Ty!

himran92 added 5 commits November 28, 2024 11:33

adding signature for assertion in saml provider, enhancing validation…

c0e0eb0

… on signature for saml response & extending tests

remove duplicate tests

91cd02b

linting done

33fc312

small cleanup + small fix to test

340d6e4

add helping comment

9b4f45e

himran92 marked this pull request as ready for review November 29, 2024 17:14

himran92 requested a review from jimlambrt as a code owner November 29, 2024 17:14

himran92 marked this pull request as draft November 29, 2024 19:06

hcjulz reviewed Dec 2, 2024

View reviewed changes

himran92 added 2 commits December 2, 2024 14:21

use cap/saml response types

3748d22

making validation of signature of both fields as optional and adding …

5f2819d

…unit tests to cover

himran92 commented Dec 4, 2024

View reviewed changes

himran92 added 3 commits December 6, 2024 16:37

changes to inlcude more validateSignature options for SAML

f6c7d7b

improving / fixing comments and var names

c9cf0d9

improving / fixing comments and var names

c4af8fc

himran92 marked this pull request as ready for review December 6, 2024 22:33

himran92 requested a review from hcjulz December 6, 2024 22:34

himran92 added 2 commits December 6, 2024 17:53

improving variable name

d857fba

add validation to only allow one validateSignature option at one time

cd6c290

kpcraig reviewed Dec 16, 2024

View reviewed changes

jimlambrt requested changes Dec 20, 2024

View reviewed changes

himran92 added 2 commits December 30, 2024 06:10

removing validateBothSignature option

011ef0c

Merge branch 'main' into VAULT-27387/himran92-enhance-signature-valid…

8fe0cbf

…ation-for-saml-response

himran92 requested review from jimlambrt and kpcraig December 30, 2024 18:06

jimlambrt approved these changes Dec 30, 2024

View reviewed changes

jimlambrt merged commit 52eae09 into main Dec 30, 2024
6 checks passed

jimlambrt deleted the VAULT-27387/himran92-enhance-signature-validation-for-saml-response branch December 30, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancing signature validation in SAML Response #144 #145

Enhancing signature validation in SAML Response #144 #145

himran92 commented Nov 29, 2024 •

edited

Loading

hashicorp-cla-app bot commented Nov 29, 2024 •

edited

Loading

hashicorp-cla-app bot commented Nov 29, 2024

hcjulz left a comment

hcjulz Dec 2, 2024

himran92 Dec 6, 2024

hcjulz Dec 2, 2024 •

edited

Loading

hcjulz Dec 3, 2024 •

edited

Loading

himran92 Dec 6, 2024 •

edited

Loading

himran92 Dec 6, 2024

himran92 Dec 6, 2024 •

edited

Loading

hcjulz Dec 11, 2024

himran92 Dec 16, 2024

hcjulz Dec 2, 2024

himran92 Dec 2, 2024

himran92 Dec 6, 2024

himran92 Dec 4, 2024

hcjulz Dec 5, 2024

himran92 Dec 6, 2024

kpcraig Dec 16, 2024 •

edited

Loading

himran92 Dec 18, 2024 •

edited

Loading

himran92 Dec 18, 2024

himran92 Dec 19, 2024 •

edited

Loading

jimlambrt Dec 20, 2024

himran92 Dec 30, 2024

jimlambrt left a comment

jimlambrt Dec 20, 2024

jimlambrt left a comment

Enhancing signature validation in SAML Response #144 #145

Enhancing signature validation in SAML Response #144 #145

Conversation

himran92 commented Nov 29, 2024 • edited Loading

hashicorp-cla-app bot commented Nov 29, 2024 • edited Loading

hashicorp-cla-app bot commented Nov 29, 2024

hcjulz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcjulz Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

hcjulz Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

himran92 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himran92 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpcraig Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

himran92 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himran92 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimlambrt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimlambrt left a comment

Choose a reason for hiding this comment

himran92 commented Nov 29, 2024 •

edited

Loading

hashicorp-cla-app bot commented Nov 29, 2024 •

edited

Loading

hcjulz Dec 2, 2024 •

edited

Loading

hcjulz Dec 3, 2024 •

edited

Loading

himran92 Dec 6, 2024 •

edited

Loading

himran92 Dec 6, 2024 •

edited

Loading

kpcraig Dec 16, 2024 •

edited

Loading

himran92 Dec 18, 2024 •

edited

Loading

himran92 Dec 19, 2024 •

edited

Loading