diff --git a/changes/9778-saml-validation-workflow b/changes/9778-saml-validation-workflow new file mode 100644 index 000000000000..d498ac81dc1e --- /dev/null +++ b/changes/9778-saml-validation-workflow @@ -0,0 +1 @@ +* Improved SAML validation workflow. diff --git a/server/sso/validate.go b/server/sso/validate.go index 30b2190f908b..e9060f73bdd1 100644 --- a/server/sso/validate.go +++ b/server/sso/validate.go @@ -15,7 +15,6 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" rtvalidator "github.com/mattermost/xml-roundtrip-validator" dsig "github.com/russellhaering/goxmldsig" - "github.com/russellhaering/goxmldsig/etreeutils" ) type Validator interface { @@ -161,38 +160,50 @@ func (v *validator) validateSignature(elt *etree.Element) (*etree.Element, error // If entire doc is signed, success, we're done. return validated, nil } + // Some IdPs (like Google) do not sign the root, and only sign the Assertion. if err == dsig.ErrMissingSignature { - // If entire document is not signed find signed assertions, remove assertions - // that are not signed. - err = v.validateAssertionSignature(elt) - if err != nil { + if err := v.validateAssertionSignature(elt); err != nil { return nil, err } return elt, nil } - return nil, err } +var ( + errMissingAssertion = errors.New("missing Assertion element under namespace urn:oasis:names:tc:SAML:2.0:assertion") + errMultipleAssertions = errors.New("multiple Assertions elements found") + errAssertionWithInvalidNamespace = errors.New("Assertion with invalid namespace found") +) + +// validateAssertionSignature validates that one "Assertion" child element exists under +// the "urn:oasis:names:tc:SAML:2.0:assertion" namespace and that it's signed by the IdP. +// It returns: +// - errMissingAssertion if there is no "Assertion" child element under the given tree. +// - errMultipleAssertions if there's more than one "Assertion" element under the given tree. +// - errAssertionWithInvalidNamespace if an "Assertion" element has a namespace that's not +// "urn:oasis:names:tc:SAML:2.0:assertion" +// - an error if the signature of the one "Assertion" element is invalid. func (v *validator) validateAssertionSignature(elt *etree.Element) error { - validateAssertion := func(ctx etreeutils.NSContext, unverified *etree.Element) error { - if unverified.Parent() != elt { - return fmt.Errorf("assertion with unexpected parent: %s", unverified.Parent().Tag) - } - // Remove assertions that are not signed. - detached, err := etreeutils.NSDetatch(ctx, unverified) - if err != nil { - return err + var assertion *etree.Element + for _, child := range elt.ChildElements() { + if child.Tag == "Assertion" { + if child.NamespaceURI() != "urn:oasis:names:tc:SAML:2.0:assertion" { + return errAssertionWithInvalidNamespace + } + if assertion != nil { + return errMultipleAssertions + } + assertion = child } - signed, err := v.context.Validate(detached) - if err != nil { - return err - } - elt.RemoveChild(unverified) - elt.AddChild(signed) - return nil } - return etreeutils.NSFindIterate(elt, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion", validateAssertion) + if assertion == nil { + return errMissingAssertion + } + if _, err := v.context.Validate(assertion); err != nil { + return fmt.Errorf("failed to validate assertion signature: %w", err) + } + return nil } const ( @@ -221,7 +232,6 @@ func generateSAMLValidID() (string, error) { func ValidateAudiences(metadata Metadata, auth fleet.Auth, audiences ...string) error { validator, err := NewValidator(metadata, WithExpectedAudience(audiences...)) - if err != nil { return fmt.Errorf("create validator from metadata: %w", err) }