use SAN when comparing certs during path building, #55

smx509/verify.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto"
 	"crypto/x509"
+	"crypto/x509/pkix"
 	"errors"
 	"fmt"
 	"net"
@@ -712,6 +713,50 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate
 	return n
 }
 
+// alreadyInChain checks whether a candidate certificate is present in a chain.
+// Rather than doing a direct byte for byte equivalency check, we check if the
+// subject, public key, and SAN, if present, are equal. This prevents loops that
+// are created by mutual cross-signatures, or other cross-signature bridge
+// oddities.
+func alreadyInChain(candidate *Certificate, chain []*Certificate) bool {
+	type pubKeyEqual interface {
+		Equal(crypto.PublicKey) bool
+	}
+
+	var candidateSAN *pkix.Extension
+	for _, ext := range candidate.Extensions {
+		if ext.Id.Equal(oidExtensionSubjectAltName) {
+			candidateSAN = &ext
+			break
+		}
+	}
+
+	for _, cert := range chain {
+		if !bytes.Equal(candidate.RawSubject, cert.RawSubject) {
+			continue
+		}
+		if !candidate.PublicKey.(pubKeyEqual).Equal(cert.PublicKey) {
+			continue
+		}
+		var certSAN *pkix.Extension
+		for _, ext := range cert.Extensions {
+			if ext.Id.Equal(oidExtensionSubjectAltName) {
+				certSAN = &ext
+				break
+			}
+		}
+		if candidateSAN == nil && certSAN == nil {
+			return true
+		} else if candidateSAN == nil || certSAN == nil {
+			return false
+		}
+		if bytes.Equal(candidateSAN.Value, certSAN.Value) {
+			return true
+		}
+	}
+	return false
+}
+
 // maxChainSignatureChecks is the maximum number of CheckSignatureFrom calls
 // that an invocation of buildChains will (transitively) make. Most chains are
 // less than 15 certificates long, so this leaves space for multiple chains and
@@ -723,18 +768,10 @@ func (c *Certificate) buildChains(currentChain []*Certificate, sigChecks *int, o
 		hintErr  error
 		hintCert *Certificate
 	)
-	type pubKeyEqual interface {
-		Equal(crypto.PublicKey) bool
-	}
 
 	considerCandidate := func(certType int, candidate *Certificate) {
-		for _, cert := range currentChain {
+		if alreadyInChain(candidate, currentChain) {
-			// If a certificate already appeared in the chain we've built, don't
+			return
-			// reconsider it. This prevents loops, for isntance those created by
-			// mutual cross-signatures, or other cross-signature bridges oddities.
-			if bytes.Equal(cert.RawSubject, candidate.RawSubject) && cert.PublicKey.(pubKeyEqual).Equal(candidate.PublicKey) {
-				return
-			}
 		}
 
 		if sigChecks == nil {

smx509/verify_test.go

@@ -2324,6 +2324,29 @@ func TestPathBuilding(t *testing.T) {
 				"CN=leaf -> CN=inter b -> CN=inter c -> CN=root",
 			},
 		},
+		{
+			// Build a simple two node graph, where the leaf is directly issued from
+			// the root and both certificates have matching subject and public key, but
+			// the leaf has SANs.
+			name: "leaf with same subject, key, as parent but with SAN",
+			graph: trustGraphDescription{
+				Roots: []string{"root"},
+				Leaf:  "root",
+				Graph: []trustGraphEdge{
+					{
+						Issuer:  "root",
+						Subject: "root",
+						Type:    leafCertificate,
+						MutateTemplate: func(c *Certificate) {
+							c.DNSNames = []string{"localhost"}
+						},
+					},
+				},
+			},
+			expectedChains: []string{
+				"CN=root -> CN=root",
+			},
+		},
 		{
 			// Build a basic graph with two paths from leaf to root, but the path passing
 			// through C should be ignored, because it has invalid EKU nesting.