fix invalid handling of ip addresses in comparisons

Signed-off-by: Ashley Davis <ashley.davis@venafi.com>

pkg/util/pki/csr.go

@@ -85,6 +85,8 @@ func URLsFromStrings(urlStrs []string) ([]*url.URL, error) {
 	return urls, nil
 }
 
+// IPAddressesToString converts a slice of IP addresses to strings, which can be useful for
+// printing a list of addresses but MUST NOT be used for comparing two slices of IP addresses.
 func IPAddressesToString(ipAddresses []net.IP) []string {
 	var ipNames []string
 	for _, ip := range ipAddresses {

pkg/util/pki/match.go

@@ -21,6 +21,7 @@ import (
 	"crypto/ecdsa"
 	"crypto/ed25519"
 	"crypto/rsa"
+	"net"
 
 	"fmt"
 	"reflect"
@@ -103,6 +104,16 @@ func ed25519PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSp
 	return nil, nil
 }
 
+func ipSlicesMatch(parsedIPs []net.IP, stringIPs []string) bool {
+	parsedStringIPs := make([]net.IP, len(stringIPs))
+
+	for i, s := range stringIPs {
+		parsedStringIPs[i] = net.ParseIP(s)
+	}
+
+	return util.EqualIPsUnsorted(parsedStringIPs, parsedIPs)
+}
+
 // RequestMatchesSpec compares a CertificateRequest with a CertificateSpec
 // and returns a list of field names on the Certificate that do not match their
 // counterpart fields on the CertificateRequest.
@@ -121,15 +132,19 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe
 
 	var violations []string
 
-	if !util.EqualUnsorted(IPAddressesToString(x509req.IPAddresses), spec.IPAddresses) {
+	if !ipSlicesMatch(x509req.IPAddresses, spec.IPAddresses) {
+
 		violations = append(violations, "spec.ipAddresses")
 	}
+
 	if !util.EqualUnsorted(URLsToString(x509req.URIs), spec.URIs) {
 		violations = append(violations, "spec.uris")
 	}
+
 	if !util.EqualUnsorted(x509req.EmailAddresses, spec.EmailAddresses) {
 		violations = append(violations, "spec.emailAddresses")
 	}
+
 	if !util.EqualUnsorted(x509req.DNSNames, spec.DNSNames) {
 		violations = append(violations, "spec.dnsNames")
 	}
@@ -239,12 +254,14 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp
 		}
 	}
 
-	if !util.EqualUnsorted(IPAddressesToString(x509cert.IPAddresses), spec.IPAddresses) {
+	if !ipSlicesMatch(x509cert.IPAddresses, spec.IPAddresses) {
 		violations = append(violations, "spec.ipAddresses")
 	}
+
 	if !util.EqualUnsorted(URLsToString(x509cert.URIs), spec.URIs) {
 		violations = append(violations, "spec.uris")
 	}
+
 	if !util.EqualUnsorted(x509cert.EmailAddresses, spec.EmailAddresses) {
 		violations = append(violations, "spec.emailAddresses")
 	}

pkg/util/util.go

@@ -23,6 +23,7 @@ import (
 	"math/rand"
 	"net"
 	"net/url"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -98,31 +99,35 @@ func EqualURLsUnsorted(s1, s2 []*url.URL) bool {
 	return true
 }
 
-// Test for equal IP slices even if unsorted
+// EqualIPsUnsorted checks if the given slices of IP addresses contain the same elements, even if in a different order
 func EqualIPsUnsorted(s1, s2 []net.IP) bool {
 	if len(s1) != len(s2) {
 		return false
 	}
-	s1_2, s2_2 := make([]string, len(s1)), make([]string, len(s2))
-	// we may want to implement a sort interface here instead of []byte conversion
-	for i := range s1 {
-		s1_2[i] = string(s1[i])
-		s2_2[i] = string(s2[i])
+
+	// Two IPv4 addresses can compare unequal with bytes.Equal which is why net.IP.Equal exists.
+	// We still want to sort the lists, though, and we don't want different representations of IPv4 addresses
+	// to be sorted differently. That can happen if one is stored as a 4-byte address while
+	// the other is stored as a 16-byte representation
+
+	// To avoid ambiguity, we ensure that only the 16-byte form is used for all addresses we work with.
+
+	for i := 0; i < len(s1); i++ {
+		s1[i] = s1[i].To16()
+		s2[i] = s2[i].To16()
 	}
 
-	sort.SliceStable(s1_2, func(i, j int) bool {
-		return s1_2[i] < s1_2[j]
+	slices.SortFunc(s1, func(a net.IP, b net.IP) int {
+		return bytes.Compare([]byte(a), []byte(b))
 	})
-	sort.SliceStable(s2_2, func(i, j int) bool {
-		return s2_2[i] < s2_2[j]
+
+	slices.SortFunc(s2, func(a net.IP, b net.IP) int {
+		return bytes.Compare([]byte(a), []byte(b))
 	})
 
-	for i, s := range s1_2 {
-		if s != s2_2[i] {
-			return false
-		}
-	}
-	return true
+	return slices.EqualFunc(s1, s2, func(a net.IP, b net.IP) bool {
+		return a.Equal(b)
+	})
 }
 
 // Test for equal KeyUsage slices even if unsorted

pkg/util/util_test.go

@@ -88,15 +88,104 @@ func TestEqualURLsUnsorted(t *testing.T) {
 }
 
 func TestEqualIPsUnsorted(t *testing.T) {
-	for _, test := range stringSliceTestData {
-		s1, s2 := parseIPs(t, test.s1), parseIPs(t, test.s2)
-		t.Run(test.desc, func(test testT) func(*testing.T) {
-			return func(t *testing.T) {
-				if actual := EqualIPsUnsorted(s1, s2); actual != test.equal {
-					t.Errorf("equalIpsUnsorted(%+v, %+v) = %t, but expected %t", s1, s2, actual, test.equal)
-				}
+	// This test uses string representations of IP addresses because it's much more convenient to
+	// represent different types of IPv6 address as strings. This implicitly relies on the behavior
+	// of net.ParseIP under the hood when it comes to parsing IPv4 addresses, though - it could return
+	// either a 4 or 16 byte slice to represent an IPv4 address. As such, we have a separate test
+	// which uses raw net.IP byte slices below, which checks that we're not relying on underlying
+	// behavior of ParseIP when comparing.
+	specs := map[string]struct {
+		s1 []string
+		s2 []string
+
+		expEqual bool
+	}{
+		"simple ipv4 comparison": {
+			s1:       []string{"8.8.8.8", "1.1.1.1"},
+			s2:       []string{"1.1.1.1", "8.8.8.8"},
+			expEqual: true,
+		},
+		"simple ipv6 comparison": {
+			s1:       []string{"2a00:1450:4009:822::200e", "2a03:2880:f166:81:face:b00c:0:25de"},
+			s2:       []string{"2a03:2880:f166:81:face:b00c:0:25de", "2a00:1450:4009:822::200e"},
+			expEqual: true,
+		},
+		"mixed ipv4 and ipv6": {
+			s1:       []string{"2a00:1450:4009:822::200e", "2a03:2880:f166:81:face:b00c:0:25de", "1.1.1.1"},
+			s2:       []string{"2a03:2880:f166:81:face:b00c:0:25de", "1.1.1.1", "2a00:1450:4009:822::200e"},
+			expEqual: true,
+		},
+		"mixed ipv6 specificity": {
+			s1:       []string{"2a03:2880:f166:0081:face:b00c:0000:25de"},
+			s2:       []string{"2a03:2880:f166:81:face:b00c:0:25de"},
+			expEqual: true,
+		},
+		"unequal addresses ipv6": {
+			s1:       []string{"2a03:2880:f166:0081:face::25de"},
+			s2:       []string{"2a03:2880:f166:81:face:b00c:1:25de"},
+			expEqual: false,
+		},
+	}
+
+	for name, spec := range specs {
+		s1 := parseIPs(t, spec.s1)
+		s2 := parseIPs(t, spec.s2)
+
+		t.Run(name, func(t *testing.T) {
+			got := EqualIPsUnsorted(s1, s2)
+
+			if got != spec.expEqual {
+				t.Errorf("EqualIPsUnsorted(%+v, %+v) = %t, but expected %t", s1, s2, got, spec.expEqual)
 			}
-		}(test))
+		})
+	}
+}
+
+func TestEqualIPsUnsorted_RawIPs(t *testing.T) {
+	// See description in  TestEqualIPsUnsorted for motivation here
+	specs := map[string]struct {
+		s1 []net.IP
+		s2 []net.IP
+
+		expEqual bool
+	}{
+		"simple ipv4 comparison": {
+			s1:       []net.IP{net.IP([]byte{0x1, 0x1, 0x1, 0x1}), net.IP([]byte{0x8, 0x8, 0x8, 0x8})},
+			s2:       []net.IP{net.IP([]byte{0x8, 0x8, 0x8, 0x8}), net.IP([]byte{0x1, 0x1, 0x1, 0x1})},
+			expEqual: true,
+		},
+		"simple ipv6 comparison": {
+			s1: []net.IP{
+				net.IP([]byte{0x2a, 0xe, 0x23, 0x45, 0x67, 0x89, 0x0, 0x1, 0x0, 0x1, 0x0, 0x2, 0x0, 0x0, 0x0, 0x6}),
+				net.IP([]byte{0x2a, 0x03, 0x28, 0x80, 0xf1, 0x66, 0x00, 0x81, 0xfa, 0xce, 0xb0, 0x0c, 0x00, 0x00, 0x25, 0xde}),
+			},
+			s2: []net.IP{
+				net.IP([]byte{0x2a, 0x03, 0x28, 0x80, 0xf1, 0x66, 0x00, 0x81, 0xfa, 0xce, 0xb0, 0x0c, 0x00, 0x00, 0x25, 0xde}),
+				net.IP([]byte{0x2a, 0xe, 0x23, 0x45, 0x67, 0x89, 0x0, 0x1, 0x0, 0x1, 0x0, 0x2, 0x0, 0x0, 0x0, 0x6}),
+			},
+			expEqual: true,
+		},
+		"mixed ipv4 lengths": {
+			// This is the most important test in this test function!
+			// IPv4 addresses have two valid representations as `net.IP`s and we shouldn't miss the case where they're equal
+			s1: []net.IP{
+				net.IP([]byte{0xa, 0x0, 0x0, 0xce}),
+			},
+			s2: []net.IP{
+				net.IP([]byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xa, 0x0, 0x0, 0xce}),
+			},
+			expEqual: true,
+		},
+	}
+
+	for name, spec := range specs {
+		t.Run(name, func(t *testing.T) {
+			got := EqualIPsUnsorted(spec.s1, spec.s2)
+
+			if got != spec.expEqual {
+				t.Errorf("EqualIPsUnsorted(%+v, %+v) = %t, but expected %t", spec.s1, spec.s2, got, spec.expEqual)
+			}
+		})
 	}
 }
 
@@ -158,7 +247,7 @@ func parseIPs(t *testing.T, ipStrs []string) []net.IP {
 	var ips []net.IP
 
 	for _, i := range ipStrs {
-		ips = append(ips, []byte(i))
+		ips = append(ips, net.ParseIP(i))
 	}
 
 	return ips