Skip to content

Commit

Permalink
Merge pull request #773 from smallstep/herman/ip-sans-improvements
Browse files Browse the repository at this point in the history 
Improve IP SANS support
  • Loading branch information
@hslatman
hslatman authored Dec 20, 2021
2 parents f7c1a32 + a5f2f00 commit 8473164
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 34 deletions.
33 changes: 23 additions & 10 deletions acme/order.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -279,7 +283,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
Expand All @@ -289,16 +295,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.
// TODO(hs): we might want to check if the CommonName is in fact a DNS (and cannot
// be parsed as IP). This is related to https://github.com/smallstep/cli/pull/576
// (ACME IP SANS)
// 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)
if ip := net.ParseIP(csr.Subject.CommonName); ip != nil {
canonicalized.IPAddresses = append(canonicalized.IPAddresses, ip)
} else {
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
}
Expand Down Expand Up @@ -340,7 +350,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 {
Expand Down
124 changes: 100 additions & 24 deletions acme/order_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import (
"crypto/x509/pkix"
"encoding/json"
"net"
"net/url"
"reflect"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"github.com/smallstep/assert"
"github.com/smallstep/certificates/authority"
Expand Down Expand Up @@ -825,71 +827,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")},
},
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}},
},
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/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))
}
})
}
Expand Down Expand Up @@ -1122,9 +1145,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",
Expand All @@ -1133,7 +1156,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{},
},
Expand All @@ -1148,7 +1171,7 @@ func Test_canonicalize(t *testing.T) {
DNSNames: []string{"www.example.com"},
},
},
wantCanonicalized: &x509.CertificateRequest{
want: &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: "example.com",
},
Expand All @@ -1163,7 +1186,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")},
},
Expand All @@ -1176,7 +1199,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")},
},
Expand All @@ -1192,19 +1215,39 @@ 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",
},
DNSNames: []string{"example.com", "www.example.com"},
IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")},
},
},
{
name: "ok/ip-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("127.0.0.1"), 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))
}
})
}
Expand Down Expand Up @@ -1238,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{"[email protected]"},
},
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{
Expand Down

0 comments on commit 8473164

Please sign in to comment.