From a2c9b5cd7e66c06a4999b8a04acd30e87cebdee1 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 13 Dec 2021 15:30:20 +0100 Subject: [PATCH 1/6] Allow IP identifiers in subject, including authorization enforcement To support IPs in the subject using `step-cli`, this PR ensures that Subject Common Names that can be parsed as an IP are also checked to have been authorized before. The PR for `step-cli` is here: github.com/smallstep/cli/pull/576. --- acme/order.go | 24 ++++++++++--- acme/order_test.go | 90 +++++++++++++++++++++++++++++++++------------- 2 files changed, 85 insertions(+), 29 deletions(-) diff --git a/acme/order.go b/acme/order.go index 237c6979..9cd683c1 100644 --- a/acme/order.go +++ b/acme/order.go @@ -277,7 +277,9 @@ func numberOfIdentifierType(typ IdentifierType, ids []Identifier) int { // canonicalize canonicalizes a CSR so that it can be compared against an Order // NOTE: this effectively changes the order of SANs in the CSR, which may be OK, -// but may not be expected. +// but may not be expected. It also adds a Subject Common Name to either the IP +// addresses or DNS names slice, depending on whether it can be parsed as an IP +// or not. This might result in an additional SAN in the final certificate. func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.CertificateRequest) { // for clarity only; we're operating on the same object by pointer @@ -287,11 +289,20 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate // identifiers as the initial newOrder request. Identifiers of type "dns" // MUST appear either in the commonName portion of the requested subject // name or in an extensionRequest attribute [RFC2985] requesting a - // subjectAltName extension, or both. + // subjectAltName extension, or both. Subject Common Names that can be + // parsed as an IP are included as an IP address for the equality check. + // If these were excluded, a certificate could contain an IP as the + // common name without having been challenged. if csr.Subject.CommonName != "" { - // nolint:gocritic - canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) + ip := net.ParseIP(csr.Subject.CommonName) + subjectIsIP := ip != nil + if subjectIsIP { + canonicalized.IPAddresses = append(csr.IPAddresses, ip) + } else { + canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) + } } + canonicalized.DNSNames = uniqueSortedLowerNames(csr.DNSNames) canonicalized.IPAddresses = uniqueSortedIPs(csr.IPAddresses) @@ -335,7 +346,10 @@ func uniqueSortedIPs(ips []net.IP) (unique []net.IP) { } ipEntryMap := make(map[string]entry, len(ips)) for _, ip := range ips { - ipEntryMap[ip.String()] = entry{ip: ip} + // reparsing the IP results in the IP being represented using 16 bytes + // for both IPv4 as well as IPv6, even when the ips slice contains IPs that + // are represented by 4 bytes. This ensures a fair comparison and thus ordering. + ipEntryMap[ip.String()] = entry{ip: net.ParseIP(ip.String())} } unique = make([]net.IP, 0, len(ipEntryMap)) for _, entry := range ipEntryMap { diff --git a/acme/order_test.go b/acme/order_test.go index 83488c8c..45c18085 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/pkg/errors" "github.com/smallstep/assert" "github.com/smallstep/certificates/authority/provisioner" @@ -816,71 +817,92 @@ func Test_uniqueSortedIPs(t *testing.T) { ips []net.IP } tests := []struct { - name string - args args - wantUnique []net.IP + name string + args args + want []net.IP }{ { name: "ok/empty", args: args{ ips: []net.IP{}, }, - wantUnique: []net.IP{}, + want: []net.IP{}, }, { name: "ok/single-ipv4", args: args{ ips: []net.IP{net.ParseIP("192.168.42.42")}, }, - wantUnique: []net.IP{net.ParseIP("192.168.42.42")}, + want: []net.IP{net.ParseIP("192.168.42.42")}, }, { name: "ok/multiple-ipv4", args: args{ - ips: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.10"), net.ParseIP("192.168.42.1")}, + ips: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.10"), net.ParseIP("192.168.42.1"), net.ParseIP("127.0.0.1")}, }, - wantUnique: []net.IP{net.ParseIP("192.168.42.1"), net.ParseIP("192.168.42.10"), net.ParseIP("192.168.42.42")}, + want: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("192.168.42.1"), net.ParseIP("192.168.42.10"), net.ParseIP("192.168.42.42")}, + }, { + name: "ok/multiple-ipv4-with-varying-byte-representations", + args: args{ + ips: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.10"), net.ParseIP("192.168.42.1"), []byte{0x7f, 0x0, 0x0, 0x1}}, + }, + want: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("192.168.42.1"), net.ParseIP("192.168.42.10"), net.ParseIP("192.168.42.42")}, }, { name: "ok/unique-ipv4", args: args{ ips: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.42")}, }, - wantUnique: []net.IP{net.ParseIP("192.168.42.42")}, + want: []net.IP{net.ParseIP("192.168.42.42")}, }, { name: "ok/single-ipv6", args: args{ ips: []net.IP{net.ParseIP("2001:db8::30")}, }, - wantUnique: []net.IP{net.ParseIP("2001:db8::30")}, + want: []net.IP{net.ParseIP("2001:db8::30")}, }, { name: "ok/multiple-ipv6", args: args{ ips: []net.IP{net.ParseIP("2001:db8::30"), net.ParseIP("2001:db8::20"), net.ParseIP("2001:db8::10")}, }, - wantUnique: []net.IP{net.ParseIP("2001:db8::10"), net.ParseIP("2001:db8::20"), net.ParseIP("2001:db8::30")}, + want: []net.IP{net.ParseIP("2001:db8::10"), net.ParseIP("2001:db8::20"), net.ParseIP("2001:db8::30")}, }, { name: "ok/unique-ipv6", args: args{ ips: []net.IP{net.ParseIP("2001:db8::1"), net.ParseIP("2001:db8::1")}, }, - wantUnique: []net.IP{net.ParseIP("2001:db8::1")}, + want: []net.IP{net.ParseIP("2001:db8::1")}, }, { name: "ok/mixed-ipv4-and-ipv6", args: args{ ips: []net.IP{net.ParseIP("2001:db8::1"), net.ParseIP("2001:db8::1"), net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.42")}, }, - wantUnique: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("2001:db8::1")}, + want: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("2001:db8::1")}, + }, + { + name: "ok/mixed-ipv4-and-ipv6-and-varying-byte-representations", + args: args{ + ips: []net.IP{net.ParseIP("2001:db8::1"), net.ParseIP("2001:db8::1"), net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.42"), []byte{0x7f, 0x0, 0x0, 0x1}}, + }, + want: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("192.168.42.42"), net.ParseIP("2001:db8::1")}, + }, + { + name: "ok/mixed-ipv4-and-ipv6-and-more-varying-byte-representations", + args: args{ + ips: []net.IP{net.ParseIP("2001:db8::1"), net.ParseIP("2001:db8::1"), net.ParseIP("192.168.42.42"), net.ParseIP("2001:db8::2"), net.ParseIP("192.168.42.42"), []byte{0x7f, 0x0, 0x0, 0x1}, []byte{0x7f, 0x0, 0x0, 0x1}, []byte{0x7f, 0x0, 0x0, 0x2}}, + }, + want: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2"), net.ParseIP("192.168.42.42"), net.ParseIP("2001:db8::1"), net.ParseIP("2001:db8::2")}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if gotUnique := uniqueSortedIPs(tt.args.ips); !reflect.DeepEqual(gotUnique, tt.wantUnique) { - t.Errorf("uniqueSortedIPs() = %v, want %v", gotUnique, tt.wantUnique) + got := uniqueSortedIPs(tt.args.ips) + if !cmp.Equal(tt.want, got) { + t.Errorf("uniqueSortedIPs() diff =\n%s", cmp.Diff(tt.want, got)) } }) } @@ -1113,9 +1135,9 @@ func Test_canonicalize(t *testing.T) { csr *x509.CertificateRequest } tests := []struct { - name string - args args - wantCanonicalized *x509.CertificateRequest + name string + args args + want *x509.CertificateRequest }{ { name: "ok/dns", @@ -1124,7 +1146,7 @@ func Test_canonicalize(t *testing.T) { DNSNames: []string{"www.example.com", "example.com"}, }, }, - wantCanonicalized: &x509.CertificateRequest{ + want: &x509.CertificateRequest{ DNSNames: []string{"example.com", "www.example.com"}, IPAddresses: []net.IP{}, }, @@ -1139,7 +1161,7 @@ func Test_canonicalize(t *testing.T) { DNSNames: []string{"www.example.com"}, }, }, - wantCanonicalized: &x509.CertificateRequest{ + want: &x509.CertificateRequest{ Subject: pkix.Name{ CommonName: "example.com", }, @@ -1154,7 +1176,7 @@ func Test_canonicalize(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, }, }, - wantCanonicalized: &x509.CertificateRequest{ + want: &x509.CertificateRequest{ DNSNames: []string{}, IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, }, @@ -1167,7 +1189,7 @@ func Test_canonicalize(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, }, }, - wantCanonicalized: &x509.CertificateRequest{ + want: &x509.CertificateRequest{ DNSNames: []string{"example.com", "www.example.com"}, IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, }, @@ -1183,7 +1205,7 @@ func Test_canonicalize(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, }, }, - wantCanonicalized: &x509.CertificateRequest{ + want: &x509.CertificateRequest{ Subject: pkix.Name{ CommonName: "example.com", }, @@ -1191,11 +1213,31 @@ func Test_canonicalize(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, }, }, + { + name: "ok/exclude-ip-from-common-name", + args: args{ + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + DNSNames: []string{"example.com"}, + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + }, + }, + want: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + DNSNames: []string{"example.com"}, + IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if gotCanonicalized := canonicalize(tt.args.csr); !reflect.DeepEqual(gotCanonicalized, tt.wantCanonicalized) { - t.Errorf("canonicalize() = %v, want %v", gotCanonicalized, tt.wantCanonicalized) + got := canonicalize(tt.args.csr) + if !cmp.Equal(tt.want, got) { + t.Errorf("canonicalize() diff =\n%s", cmp.Diff(tt.want, got)) } }) } From a5d33512fe8da94a20e3b854b21cb0df7a673a6e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 13 Dec 2021 15:59:01 +0100 Subject: [PATCH 2/6] Fix test --- acme/order_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/order_test.go b/acme/order_test.go index 45c18085..dee828f7 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -1229,7 +1229,7 @@ func Test_canonicalize(t *testing.T) { CommonName: "127.0.0.1", }, DNSNames: []string{"example.com"}, - IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, }, }, } From ca707cbe05783f9029ce3f6958d56cc64136bee6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 13 Dec 2021 16:01:40 +0100 Subject: [PATCH 3/6] Fix linting --- acme/order.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/acme/order.go b/acme/order.go index 9cd683c1..7e65b5d7 100644 --- a/acme/order.go +++ b/acme/order.go @@ -297,8 +297,10 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate ip := net.ParseIP(csr.Subject.CommonName) subjectIsIP := ip != nil if subjectIsIP { + // nolint:gocritic canonicalized.IPAddresses = append(csr.IPAddresses, ip) } else { + // nolint:gocritic canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) } } From bc0875bd7bf908078f91ad67e97d9fa20c93ae54 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 13 Dec 2021 16:14:39 +0100 Subject: [PATCH 4/6] Disallow email address and URLs in the CSR Before this commit `step` would allow email addresses and URLs in the CSR. This doesn't fit nicely with the rest of ACME, in which identifiers need to be authorized before a certificate is issued. --- acme/order.go | 4 ++++ acme/order_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/acme/order.go b/acme/order.go index 366d1a5e..1ef0409c 100644 --- a/acme/order.go +++ b/acme/order.go @@ -200,6 +200,10 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ var sans []x509util.SubjectAlternativeName + if len(csr.EmailAddresses) > 0 || len(csr.URIs) > 0 { + return sans, NewError(ErrorBadCSRType, "Only DNS names and IP addresses are allowed") + } + // order the DNS names and IP addresses, so that they can be compared against the canonicalized CSR orderNames := make([]string, numberOfIdentifierType(DNS, o.Identifiers)) orderIPs := make([]net.IP, numberOfIdentifierType(IP, o.Identifiers)) diff --git a/acme/order_test.go b/acme/order_test.go index 73f72065..cb57fff9 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -6,6 +6,7 @@ import ( "crypto/x509/pkix" "encoding/json" "net" + "net/url" "reflect" "testing" "time" @@ -1280,6 +1281,39 @@ func TestOrder_sans(t *testing.T) { }, err: nil, }, + { + name: "fail/invalid-alternative-name-email", + fields: fields{ + Identifiers: []Identifier{}, + }, + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "foo.internal", + }, + EmailAddresses: []string{"test@example.com"}, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorBadCSRType, "Only DNS names and IP addresses are allowed"), + }, + { + name: "fail/invalid-alternative-name-uri", + fields: fields{ + Identifiers: []Identifier{}, + }, + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "foo.internal", + }, + URIs: []*url.URL{ + { + Scheme: "https://", + Host: "smallstep.com", + }, + }, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorBadCSRType, "Only DNS names and IP addresses are allowed"), + }, { name: "fail/error-names-length-mismatch", fields: fields{ From 80bebda69c8300c40dfd08d6555ac589d51c0b8f Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 20 Dec 2021 13:40:17 +0100 Subject: [PATCH 5/6] Fix code style issue --- acme/order.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/acme/order.go b/acme/order.go index 1ef0409c..1fa0809e 100644 --- a/acme/order.go +++ b/acme/order.go @@ -300,19 +300,15 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate // If these were excluded, a certificate could contain an IP as the // common name without having been challenged. if csr.Subject.CommonName != "" { - ip := net.ParseIP(csr.Subject.CommonName) - subjectIsIP := ip != nil - if subjectIsIP { - // nolint:gocritic - canonicalized.IPAddresses = append(csr.IPAddresses, ip) + if ip := net.ParseIP(csr.Subject.CommonName); ip != nil { + canonicalized.IPAddresses = append(canonicalized.IPAddresses, ip) } else { - // nolint:gocritic - canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) + canonicalized.DNSNames = append(canonicalized.DNSNames, csr.Subject.CommonName) } } - canonicalized.DNSNames = uniqueSortedLowerNames(csr.DNSNames) - canonicalized.IPAddresses = uniqueSortedIPs(csr.IPAddresses) + canonicalized.DNSNames = uniqueSortedLowerNames(canonicalized.DNSNames) + canonicalized.IPAddresses = uniqueSortedIPs(canonicalized.IPAddresses) return canonicalized } From a5f2f004e3bc9480865a68bf91e12fbe8934a6ed Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 20 Dec 2021 18:55:23 +0100 Subject: [PATCH 6/6] Change name of IP Common Name test for clarity --- acme/order_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/order_test.go b/acme/order_test.go index cb57fff9..493b40b7 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -1224,7 +1224,7 @@ func Test_canonicalize(t *testing.T) { }, }, { - name: "ok/exclude-ip-from-common-name", + name: "ok/ip-common-name", args: args{ csr: &x509.CertificateRequest{ Subject: pkix.Name{