Remove path map manipulations into types/signature.go & fix ValidateEx()

etreeutils/unmarshal.go

@@ -9,7 +9,7 @@ import (
 // NSUnmarshalElement unmarshals the passed etree Element into the value pointed to by
 // v using encoding/xml in the context of the passed NSContext. If v implements
 // ElementKeeper, SetUnderlyingElement will be called on v with a reference to el.
-func NSUnmarshalElement(ctx NSContext, el *etree.Element, v interface{}) error {
+func NSUnmarshalElement(ctx NSContext, root, el *etree.Element, v interface{}) error {
 	detatched, err := NSDetatch(ctx, el)
 	if err != nil {
 		return err
@@ -29,7 +29,7 @@ func NSUnmarshalElement(ctx NSContext, el *etree.Element, v interface{}) error {
 
 	switch v := v.(type) {
 	case ElementKeeper:
-		v.SetUnderlyingElement(el)
+		v.SetUnderlyingElement(root, el)
 	}
 
 	return nil
@@ -38,6 +38,6 @@ func NSUnmarshalElement(ctx NSContext, el *etree.Element, v interface{}) error {
 // ElementKeeper should be implemented by types which will be passed to
 // UnmarshalElement, but wish to keep a reference
 type ElementKeeper interface {
-	SetUnderlyingElement(*etree.Element)
+	SetUnderlyingElement(root, el *etree.Element)
 	UnderlyingElement() *etree.Element
 }

main_test.go

@@ -81,3 +81,53 @@ func TestManifestExample(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEmpty(t, manifest)
 }
+
+func TestRecursiveSigning(t *testing.T) {
+
+	// Generate a key and self-signed certificate for signing
+	randomKeyStore := RandomKeyStoreForTest()
+	ctx := NewDefaultSigningContext(randomKeyStore)
+
+	test := []byte{0x45, 0xf1, 0xab, 0xd7, 0x8a, 0x6f, 0x92, 0xe6, 0xa4, 0xb6, 0x8e, 0xba, 0x8f, 0xe7, 0x91, 0x96, 0xe0, 0xb2, 0x16, 0xd6, 0x0b, 0x82, 0x1b, 0x00, 0x45, 0xfa, 0xb8, 0xad, 0xd4, 0xfa, 0xff, 0xf9}
+
+	sig := ctx.CreateSignature("id1234")
+	err := ctx.AddManifestRef(sig, "package", crypto.SHA256, test)
+	require.NoError(t, err)
+
+	// Sign the signature
+	signed, err := ctx.Sign(sig)
+	require.NoError(t, err)
+
+	list := &etree.Element{Tag: "Signatures"}
+	list.AddChild(signed)
+
+	// create second layer
+	signed, err = ctx.SignEnveloped(list)
+	require.NoError(t, err)
+
+	// Validate
+	_, certData, err := ctx.KeyStore.GetKeyPair()
+	require.NoError(t, err)
+
+	cert, err := x509.ParseCertificate(certData)
+	require.NoError(t, err)
+
+	// Construct a signing context with one or more roots of trust.
+	vctx := NewDefaultValidationContext(&MemoryX509CertificateStore{
+		Roots: []*x509.Certificate{cert},
+	})
+
+	// It is important to only use the returned validated element.
+	// See: https://www.w3.org/TR/xmldsig-bestpractices/#check-what-is-signed
+	manifest, err := vctx.ValidateManifest(signed)
+
+	require.NoError(t, err)
+	require.NotEmpty(t, manifest)
+	require.Equal(t, len(manifest.References), 1)
+
+	hash, digest, err := vctx.DecodeRef(&manifest.References[0])
+
+	require.NoError(t, err)
+	require.Equal(t, digest, test)
+	require.Equal(t, hash, crypto.SHA256)
+}

types/signature.go

@@ -84,17 +84,65 @@ type Signature struct {
 	SignatureValue *SignatureValue `xml:"SignatureValue"`
 	KeyInfo        *KeyInfo        `xml:"KeyInfo"`
 	el             *etree.Element
+	path           []int
 	RootRef        *Reference
 }
 
 // SetUnderlyingElement will be called with a reference to the Element this Signature
 // was unmarshaled from.
-func (s *Signature) SetUnderlyingElement(el *etree.Element) {
+func (s *Signature) SetUnderlyingElement(root, el *etree.Element) {
 	s.el = el
+	// map the path to the passed signature relative to the passed root, in order
+	// to enable removal of the signature by an enveloped signature transform
+	s.path = mapPathToElement(root, el)
 }
 
 // UnderlyingElement returns a reference to the Element this signature was unmarshaled
 // from, where applicable.
 func (s *Signature) UnderlyingElement() *etree.Element {
 	return s.el
 }
+
+func (s *Signature) RemoveUnderlyingElement(el *etree.Element) bool {
+	return removeElementAtPath(el, s.path)
+}
+
+func mapPathToElement(tree, el *etree.Element) []int {
+	for i, child := range tree.Child {
+		if child == el {
+			return []int{i}
+		}
+	}
+
+	for i, child := range tree.Child {
+		if childElement, ok := child.(*etree.Element); ok {
+			childPath := mapPathToElement(childElement, el)
+			if childPath != nil {
+				return append([]int{i}, childPath...)
+			}
+		}
+	}
+
+	return nil
+}
+
+func removeElementAtPath(el *etree.Element, path []int) bool {
+
+	if len(path) == 0 {
+		return false
+	}
+
+	if path[0] < len(el.Child) {
+
+		if len(path) == 1 {
+			el.RemoveChildAt(path[0])
+			return true
+		}
+
+		childElement, ok := el.Child[path[0]].(*etree.Element)
+		if ok {
+			return removeElementAtPath(childElement, path[1:])
+		}
+	}
+	return false
+}

types/signature_test.go

@@ -0,0 +1,28 @@
+package types
+
+import (
+	"testing"
+
+	"github.com/austdev/goxmldsig/etreeutils"
+
+	"github.com/beevik/etree"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMapPathAndRemove(t *testing.T) {
+	doc := etree.NewDocument()
+	err := doc.ReadFromString(`<X><Y/><Y><RemoveMe xmlns="x"/></Y></X>`)
+	require.NoError(t, err)
+
+	el, err := etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe")
+	require.NoError(t, err)
+	require.NotNil(t, el)
+
+	path := mapPathToElement(doc.Root(), el)
+	removed := removeElementAtPath(doc.Root(), path)
+	require.True(t, removed)
+
+	el, err = etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe")
+	require.NoError(t, err)
+	require.Nil(t, el)
+}

validate.go

@@ -79,46 +79,6 @@ func childPath(space, tag string) string {
 	}
 }
 
-func mapPathToElement(tree, el *etree.Element) []int {
-	for i, child := range tree.Child {
-		if child == el {
-			return []int{i}
-		}
-	}
-
-	for i, child := range tree.Child {
-		if childElement, ok := child.(*etree.Element); ok {
-			childPath := mapPathToElement(childElement, el)
-			if childPath != nil {
-				return append([]int{i}, childPath...)
-			}
-		}
-	}
-
-	return nil
-}
-
-func removeElementAtPath(el *etree.Element, path []int) bool {
-
-	if len(path) == 0 {
-		return false
-	}
-
-	if path[0] < len(el.Child) {
-
-		if len(path) == 1 {
-			el.RemoveChildAt(path[0])
-			return true
-		}
-
-		childElement, ok := el.Child[path[0]].(*etree.Element)
-		if ok {
-			return removeElementAtPath(childElement, path[1:])
-		}
-	}
-	return false
-}
-
 // Transform returns a new element equivalent to the passed root el, but with
 // the set of transformations described by the ref applied.
 //
@@ -133,11 +93,6 @@ func (ctx *ValidationContext) transform(
 	}
 	transforms := ref.Transforms.Transforms
 
-	// map the path to the passed signature relative to the passed root, in
-	// order to enable removal of the signature by an enveloped signature
-	// transform
-	signaturePath := mapPathToElement(el, sig.UnderlyingElement())
-
 	var canonicalizer Canonicalizer
 
 	for _, transform := range transforms {
@@ -146,7 +101,7 @@ func (ctx *ValidationContext) transform(
 		switch AlgorithmID(algo) {
 		case EnvelopedSignatureAltorithmId:
 			el = el.Copy() // make a copy of the passed root
-			if !removeElementAtPath(el, signaturePath) {
+			if !sig.RemoveUnderlyingElement(el) {
 				return nil, nil, fmt.Errorf("%w: error applying canonicalization transform: Signature not found", ErrInvalidSignature)
 			}
 
@@ -407,7 +362,7 @@ func (ctx *ValidationContext) findSignature(root *etree.Element) (*types.Signatu
 
 		// Unmarshal the signature into a structured Signature type
 		_sig := &types.Signature{}
-		err = etreeutils.NSUnmarshalElement(ctx, signatureEl, _sig)
+		err = etreeutils.NSUnmarshalElement(ctx, root, signatureEl, _sig)
 		if err != nil {
 			return err
 		}
@@ -591,12 +546,10 @@ func (ctx *ValidationContext) Validate(el *etree.Element) (*etree.Element, error
 // Validate verifies that the passed element contains a valid signatures
 // matching a currently-valid certificate in the context's CertificateStore.
 func (ctx *ValidationContext) ValidateManifest(el *etree.Element) (*types.Manifest, error) {
-	// Make a copy of the element to avoid mutating the one we were passed.
-	el = el.Copy()
 
 	for {
-
-		sig, err := ctx.findSignature(el)
+		// Make a copy of the element to avoid mutating the one we were passed.
+		sig, err := ctx.findSignature(el.Copy())
 		if err != nil {
 			return nil, err
 		}

validate_test.go

@@ -286,24 +286,6 @@ func TestValidateWithModifiedAndSignatureEdited(t *testing.T) {
 	require.Error(t, err)
 }
 
-func TestMapPathAndRemove(t *testing.T) {
-	doc := etree.NewDocument()
-	err := doc.ReadFromString(`<X><Y/><Y><RemoveMe xmlns="x"/></Y></X>`)
-	require.NoError(t, err)
-
-	el, err := etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe")
-	require.NoError(t, err)
-	require.NotNil(t, el)
-
-	path := mapPathToElement(doc.Root(), el)
-	removed := removeElementAtPath(doc.Root(), path)
-	require.True(t, removed)
-
-	el, err = etreeutils.NSFindOne(doc.Root(), "x", "RemoveMe")
-	require.NoError(t, err)
-	require.Nil(t, el)
-}
-
 const (
 	validExample = `<?xml version="1.0" encoding="UTF-8"?><saml2p:Response Destination="https://dev.sudo.wtf:8443/v1/_saml_callback" ID="id149481635007085371203272055" InResponseTo="_ffea96b1-44a2-4a86-9683-45807984ab5b" IssueInstant="2020-09-01T17:51:12.176Z" Version="2.0" xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:xs="http://www.w3.org/2001/XMLSchema"><saml2:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity" xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">http://www.okta.com/exkrfkzzb7NyB3UeP0h7</saml2:Issuer><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><ds:Reference URI="#id149481635007085371203272055"><ds:Transforms><ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"><ec:InclusiveNamespaces PrefixList="xs" xmlns:ec="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:Transform></ds:Transforms><ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><ds:DigestValue>LwRDkrPmsTcUa++BIS5VJIANUlZN7zzdtjLfxfLAWds=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>UyjNRj9ZFbhApPhWEuVG26yACVqd25uyRKalSpp6XCdjrqKjI8Fmx7Q/IFkk5M755cxyFCQGttxThR6IPBk4Kp5OG2qGKXNHt7OQ8mumSLqWZpBJbmzNIKyG3nWlFoLVCoWPtBTd2gZM0aHOQp1JKa1birFBp2NofkEXbLeghZQ2YfCc4m8qgpZW5k/Itc0P/TVIkvPInjdSMyjm/ql4FUDO8cMkExJNR/i+GElW8cfnniWGcDPSiOqfIjLEDvZouXC7F1v5Wa0SmIxg7NJUTB+g6yrDN15VDq3KbHHTMlZXOZTXON2mBZOj5cwyyd4uX3aGSmYQiy/CGqBdqxrW2A==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIIDnjCCAoagAwIBAgIGAXHxS90vMA0GCSqGSIb3DQEBCwUAMIGPMQswCQYDVQQGEwJVUzETMBEG
 A1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwET2t0YTEU