Merge commit from fork

Author: lucasmrod

changes/9778-saml-validation-workflow

@@ -0,0 +1 @@
+* Improved SAML validation workflow.

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)
 	}