Merge faee7bf69e into 12339f2cef

connector/saml/saml.go

@@ -341,22 +341,30 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
 	}
 
 	// Root element is allowed to not be signed if the Assertion element is.
-	rootElementSigned := true
+	var rawSignedAssertion []byte
+	var rawSignedResponse []byte
+
 	if p.validator != nil {
-		rawResp, rootElementSigned, err = verifyResponseSig(p.validator, rawResp)
+		// if raw assertion must be
+		rawSignedAssertion, rawSignedResponse, err = verifyResponseSig(p.validator, rawResp)
 		if err != nil {
 			return ident, fmt.Errorf("verify signature: %v", err)
 		}
-	}
+	} else {
+		// no validator? I'm rejecting it for now, because it's unsafe.
 
-	var resp response
-	if err := xml.Unmarshal(rawResp, &resp); err != nil {
-		return ident, fmt.Errorf("unmarshal response: %v", err)
 	}
 
 	// If the root element isn't signed, there's no reason to inspect these
 	// elements. They're not verified.
-	if rootElementSigned {
+
+	// we got a signed response, let's carry about business logic checks on it
+	if rawSignedResponse != nil {
+		var resp response
+		if err := xml.Unmarshal(rawSignedResponse, &resp); err != nil {
+			return ident, fmt.Errorf("unmarshal response: %v", err)
+		}
+
 		if p.ssoIssuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.ssoIssuer {
 			return ident, fmt.Errorf("expected Issuer value %s, got %s", p.ssoIssuer, resp.Issuer.Issuer)
 		}
@@ -382,11 +390,17 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
 		}
 	}
 
-	assertion := resp.Assertion
-	if assertion == nil {
+	if rawSignedAssertion == nil {
 		return ident, fmt.Errorf("response did not contain an assertion")
 	}
 
+	// assertion, use the rawSignedAssertion
+
+	var assertion assertion
+	if err := xml.Unmarshal(rawSignedAssertion, &assertion); err != nil {
+		return ident, fmt.Errorf("unmarshal response: %v", err)
+	}
+
 	// Subject is usually optional, but we need it for the user ID, so complain
 	// if it's not present.
 	subject := assertion.Subject
@@ -646,44 +660,68 @@ func (p *provider) validateConditions(conditions *conditions) error {
 
 // verifyResponseSig attempts to verify the signature of a SAML response or
 // the assertion.
+// Using the signed contents from the signature, it attempts to obtain both an assertion and if the response is signed
+// the response
 //
-// If the root element is properly signed, this method returns it.
+// If the root response is signed: returns the first assertion, along with the root response
 //
 // The SAML spec requires supporting responses where the root element is
-// unverified, but the sub <Assertion> elements are signed. In these cases,
-// this method returns rootVerified=false to indicate that the <Assertion>
-// elements should be trusted, but all other elements MUST be ignored.
+// unverified, but the sub <Assertion> elements are signed.
+// In these cases, the method returns the assertion, however, the signedResponse will be nil
 //
 // Note: we still don't support multiple <Assertion> tags. If there are
 // multiple present this code will only process the first.
-func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed []byte, rootVerified bool, err error) {
+func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signedAssertion []byte, signedResponse []byte, err error) {
 	doc := etree.NewDocument()
 	if err = doc.ReadFromBytes(data); err != nil {
-		return nil, false, fmt.Errorf("parse document: %v", err)
+		return nil, nil, fmt.Errorf("parse document: %v", err)
 	}
 
 	response := doc.Root()
 	if response == nil {
-		return nil, false, fmt.Errorf("parse document: empty root")
+		return nil, nil, fmt.Errorf("parse document: empty root")
 	}
+	// transformedResponse is signed, show return value is parsed solely from transformedResponse
 	transformedResponse, err := validator.Validate(response)
 	if err == nil {
-		// Root element is verified, return it.
-		doc.SetRoot(transformedResponse)
-		signed, err = doc.WriteToBytes()
-		return signed, true, err
+		// signedResponse is the serialization of ONLY transformedResponse
+		isolatedDoc := etree.NewDocument()
+		isolatedDoc.SetRoot(transformedResponse)
+		signedResponse, err := isolatedDoc.WriteToBytes()
+		if err != nil || signedResponse == nil {
+			return nil, nil, fmt.Errorf("serialize response document: %v", err)
+		}
+
+		// signedAssertionElement part of transformedResponse
+		signedAssertionElement, err := etreeutils.NSSelectOne(transformedResponse, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
+		if err != nil || signedAssertionElement == nil {
+			return nil, nil, fmt.Errorf("response does not contain an Assertion element")
+		}
+
+		assertionIsolatedDoc := etree.NewDocument()
+		assertionIsolatedDoc.SetRoot(signedAssertionElement)
+		signedAssertion, err := assertionIsolatedDoc.WriteToBytes()
+		if err != nil || signedAssertion == nil {
+			return nil, nil, fmt.Errorf("serialize assertion node: %v", err)
+		}
+		return signedAssertion, signedResponse, nil
 	}
 
+	// Case 2: Assertion signed
+
 	// Ensures xmlns are copied down to the assertion element when they are defined in the root
 	//
 	// TODO: Only select from child elements of the root.
 	assertion, err := etreeutils.NSSelectOne(response, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
 	if err != nil || assertion == nil {
-		return nil, false, fmt.Errorf("response does not contain an Assertion element")
+		return nil, nil, fmt.Errorf("response does not contain an Assertion element")
 	}
+
+	// transformedAssertion is signed
+
 	transformedAssertion, err := validator.Validate(assertion)
 	if err != nil {
-		return nil, false, fmt.Errorf("response does not contain a valid signature element: %v", err)
+		return nil, nil, fmt.Errorf("response does not contain a valid signature element: %v", err)
 	}
 
 	// Verified an assertion but not the response. Can't trust any child elements,
@@ -692,12 +730,13 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed [
 		response.RemoveChild(el)
 	}
 
-	// We still return the full <Response> element, even though it's unverified
-	// because the <Assertion> element is not a valid XML document on its own.
-	// It still requires the root element to define things like namespaces.
-	response.AddChild(transformedAssertion)
-	signed, err = doc.WriteToBytes()
-	return signed, false, err
+	newDoc := etree.NewDocument()
+	newDoc.SetRoot(transformedAssertion)
+	signedAssertion, err = newDoc.WriteToBytes()
+	if err != nil || signedAssertion == nil {
+		return nil, nil, fmt.Errorf("serialize signed assertion: %v", err)
+	}
+	return signedAssertion, nil, nil
 }
 
 // before determines if a given time is before the current time, with an