From 2df7b0429e6a943c321552a3022bd04e35f930d9 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Thu, 24 Aug 2023 13:58:14 +0100 Subject: [PATCH] fix invalid handling of ip addresses in comparisons Signed-off-by: Ashley Davis --- cmd/cainjector/go.mod | 1 + cmd/cainjector/go.sum | 2 + pkg/util/pki/csr.go | 2 + pkg/util/pki/match.go | 21 ++++++++- pkg/util/util.go | 37 ++++++++------- pkg/util/util_test.go | 107 ++++++++++++++++++++++++++++++++++++++---- 6 files changed, 143 insertions(+), 27 deletions(-) diff --git a/cmd/cainjector/go.mod b/cmd/cainjector/go.mod index 7f88e3d62ec..e17b2ce1d10 100644 --- a/cmd/cainjector/go.mod +++ b/cmd/cainjector/go.mod @@ -57,6 +57,7 @@ require ( go.uber.org/atomic v1.9.0 // indirect go.uber.org/multierr v1.6.0 // indirect go.uber.org/zap v1.24.0 // indirect + golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect golang.org/x/net v0.10.0 // indirect golang.org/x/oauth2 v0.5.0 // indirect golang.org/x/sys v0.8.0 // indirect diff --git a/cmd/cainjector/go.sum b/cmd/cainjector/go.sum index 56332f42307..e3561ec2753 100644 --- a/cmd/cainjector/go.sum +++ b/cmd/cainjector/go.sum @@ -176,6 +176,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= +golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 h1:k/i9J1pBpvlfR+9QsetwPyERsqu1GIbi967PQMq3Ivc= +golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1/go.mod h1:V1LtkGg67GoY2N1AnLN78QLrzxkLyJw7RJb1gzOOz9w= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU= golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 80732e9ab1c..43653af3621 100644 --- a/pkg/util/pki/csr.go +++ b/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 { diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index 8572749358f..41485cfb75f 100644 --- a/pkg/util/pki/match.go +++ b/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") } diff --git a/pkg/util/util.go b/pkg/util/util.go index 94b9b56f34c..9591edbbe61 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -28,6 +28,7 @@ import ( "time" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "golang.org/x/exp/slices" ) func OnlyOneNotNil(items ...interface{}) (any bool, one bool) { @@ -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 diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index 64ef624cccd..72d36fb7058 100644 --- a/pkg/util/util_test.go +++ b/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