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

@@ -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
+		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 {

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"
@@ -818,69 +819,90 @@ func Test_uniqueSortedIPs(t *testing.T) {
 	tests := []struct {
 		name string
 		args args
-		wantUnique []net.IP
+		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) {
+			got := uniqueSortedIPs(tt.args.ips)
-				t.Errorf("uniqueSortedIPs() = %v, want %v", gotUnique, tt.wantUnique)
+			if !cmp.Equal(tt.want, got) {
+				t.Errorf("uniqueSortedIPs() diff =\n%s", cmp.Diff(tt.want, got))
 			}
 		})
 	}
@@ -1115,7 +1137,7 @@ func Test_canonicalize(t *testing.T) {
 	tests := []struct {
 		name string
 		args args
-		wantCanonicalized *x509.CertificateRequest
+		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) {
+			got := canonicalize(tt.args.csr)
-				t.Errorf("canonicalize() = %v, want %v", gotCanonicalized, tt.wantCanonicalized)
+			if !cmp.Equal(tt.want, got) {
+				t.Errorf("canonicalize() diff =\n%s", cmp.Diff(tt.want, got))
 			}
 		})
 	}