Skip to content

Commit 3ad889f

Browse files
jprenkenjprenken
committed
Address feedback
1 parent 7fe5841 commit 3ad889f

File tree

3 files changed

+133
-54
lines changed

3 files changed

+133
-54
lines changed

‎csr/csr.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,9 @@ type names struct {
101101
// compatibility with prior Let's Encrypt behaviour. The resulting SANs will
102102
// always include the original CN, if any.
103103
//
104-
// Deprecated: TODO(#7311): Use identifier.FromCSR instead.
104+
// TODO(#7311): For callers that don't care about CNs, use identifier.FromCSR.
105+
// For the rest, either revise the names struct to hold identifiers instead of
106+
// strings, or add an ipSANs field (and rename SANs to dnsSANs).
105107
func NamesFromCSR(csr *x509.CertificateRequest) names {
106108
// Produce a new "sans" slice with the same memory address as csr.DNSNames
107109
// but force a new allocation if an append happens so that we don't

‎identifier/identifier.go

+29-11
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@ package identifier
55

66
import (
77
"crypto/x509"
8-
"fmt"
98
"net/netip"
109
"slices"
11-
"sort"
1210
"strings"
1311

1412
corepb "github.com/letsencrypt/boulder/core/proto"
@@ -92,16 +90,23 @@ func NewIP(ip netip.Addr) ACMEIdentifier {
9290
}
9391

9492
// FromCert extracts the Subject Alternative Names from a certificate, and
95-
// returns a slice of ACMEIdentifiers and an error. It does not extract the
96-
// Subject Common Name.
93+
// returns a slice of ACMEIdentifiers and an error.
9794
//
98-
// FromCSR is similar but handles CSRs, and is kept separate so that it's always
99-
// clear we are handling an untrusted CSR.
95+
// FromCSR is similar, but handles CSRs.
10096
func FromCert(cert *x509.Certificate) []ACMEIdentifier {
10197
var sans []ACMEIdentifier
10298
for _, name := range cert.DNSNames {
10399
sans = append(sans, NewDNS(name))
104100
}
101+
if cert.Subject.CommonName != "" {
102+
// Boulder won't generate certificates with a CN that's not also present
103+
// in the SANs, but such a certificate is possible. If appended, this is
104+
// deduplicated later with Normalize(). We assume the CN is a DNSName,
105+
// because CNs are untyped strings without metadata, and we will never
106+
// configure a Boulder profile to issue a certificate that contains both
107+
// an IP address identifier and a CN.
108+
sans = append(sans, NewDNS(cert.Subject.CommonName))
109+
}
105110

106111
for _, ip := range cert.IPAddresses {
107112
sans = append(sans, ACMEIdentifier{
@@ -143,16 +148,29 @@ func FromCSR(csr *x509.CertificateRequest) []ACMEIdentifier {
143148
return Normalize(sans)
144149
}
145150

146-
// Normalize returns the set of all unique ACME identifiers in the
147-
// input after all of them are lowercased. The returned identifier values will
148-
// be in their lowercased form and sorted alphabetically by value.
151+
// Normalize returns the set of all unique ACME identifiers in the input after
152+
// all of them are lowercased. The returned identifier values will be in their
153+
// lowercased form and sorted alphabetically by value. DNS identifiers will
154+
// precede IP address identifiers.
149155
func Normalize(idents []ACMEIdentifier) []ACMEIdentifier {
150156
for i := range idents {
151157
idents[i].Value = strings.ToLower(idents[i].Value)
152158
}
153159

154-
sort.Slice(idents, func(i, j int) bool {
155-
return fmt.Sprintf("%s:%s", idents[i].Type, idents[i].Value) < fmt.Sprintf("%s:%s", idents[j].Type, idents[j].Value)
160+
slices.SortFunc(idents, func(a, b ACMEIdentifier) int {
161+
if a.Type == b.Type {
162+
if a.Value == b.Value {
163+
return 0
164+
}
165+
if a.Value < b.Value {
166+
return -1
167+
}
168+
return 1
169+
}
170+
if a.Type == "dns" && b.Type == "ip" {
171+
return -1
172+
}
173+
return 1
156174
})
157175

158176
return slices.Compact(idents)

‎identifier/identifier_test.go

+101-42
Original file line numberDiff line numberDiff line change
@@ -10,67 +10,126 @@ import (
1010
"github.com/letsencrypt/boulder/test"
1111
)
1212

13-
func TestNormalize(t *testing.T) {
14-
idents := []ACMEIdentifier{
15-
{Type: "DNS", Value: "foobar.com"},
16-
{Type: "DNS", Value: "fooBAR.com"},
17-
{Type: "DNS", Value: "baz.com"},
18-
{Type: "DNS", Value: "foobar.com"},
19-
{Type: "DNS", Value: "bar.com"},
20-
{Type: "DNS", Value: "bar.com"},
21-
{Type: "DNS", Value: "a.com"},
22-
}
23-
expected := []ACMEIdentifier{
24-
{Type: "DNS", Value: "a.com"},
25-
{Type: "DNS", Value: "bar.com"},
26-
{Type: "DNS", Value: "baz.com"},
27-
{Type: "DNS", Value: "foobar.com"},
28-
}
29-
u := Normalize(idents)
30-
test.AssertDeepEquals(t, expected, u)
31-
}
32-
33-
// TestFromCSR covers TestFromCert as well, because their logic is exactly the same.
34-
func TestFromCSR(t *testing.T) {
13+
// TestFromCertAndCSR covers both FromCert and FromCSR; their logic is exactly the same.
14+
func TestFromCertAndCSR(t *testing.T) {
3515
cases := []struct {
3616
name string
37-
csr *x509.CertificateRequest
17+
subject pkix.Name
18+
dnsNames []string
19+
ipAddresses []net.IP
3820
expectedIdents []ACMEIdentifier
3921
}{
4022
{
41-
"no explicit CN",
42-
&x509.CertificateRequest{DNSNames: []string{"a.com"}},
43-
[]ACMEIdentifier{NewDNS("a.com")},
23+
name: "no explicit CN",
24+
dnsNames: []string{"a.com"},
25+
expectedIdents: []ACMEIdentifier{NewDNS("a.com")},
26+
},
27+
{
28+
name: "explicit uppercase CN",
29+
subject: pkix.Name{CommonName: "A.com"},
30+
dnsNames: []string{"a.com"},
31+
expectedIdents: []ACMEIdentifier{NewDNS("a.com")},
32+
},
33+
{
34+
name: "no explicit CN, uppercase SAN",
35+
dnsNames: []string{"A.com"},
36+
expectedIdents: []ACMEIdentifier{NewDNS("a.com")},
4437
},
4538
{
46-
"explicit uppercase CN",
47-
&x509.CertificateRequest{Subject: pkix.Name{CommonName: "A.com"}, DNSNames: []string{"a.com"}},
48-
[]ACMEIdentifier{NewDNS("a.com")},
39+
name: "duplicate SANs",
40+
dnsNames: []string{"b.com", "b.com", "a.com", "a.com"},
41+
expectedIdents: []ACMEIdentifier{NewDNS("a.com"), NewDNS("b.com")},
4942
},
5043
{
51-
"no explicit CN, uppercase SAN",
52-
&x509.CertificateRequest{DNSNames: []string{"A.com"}},
53-
[]ACMEIdentifier{NewDNS("a.com")},
44+
name: "explicit CN not found in SANs",
45+
subject: pkix.Name{CommonName: "a.com"},
46+
dnsNames: []string{"b.com"},
47+
expectedIdents: []ACMEIdentifier{NewDNS("a.com"), NewDNS("b.com")},
48+
},
49+
{
50+
name: "mix of DNSNames and IPAddresses",
51+
dnsNames: []string{"a.com"},
52+
ipAddresses: []net.IP{{192, 168, 1, 1}},
53+
expectedIdents: []ACMEIdentifier{NewDNS("a.com"), NewIP(netip.MustParseAddr("192.168.1.1"))},
54+
},
55+
}
56+
for _, tc := range cases {
57+
t.Run(tc.name, func(t *testing.T) {
58+
cert := &x509.Certificate{Subject: tc.subject, DNSNames: tc.dnsNames, IPAddresses: tc.ipAddresses}
59+
csr := &x509.CertificateRequest{Subject: tc.subject, DNSNames: tc.dnsNames, IPAddresses: tc.ipAddresses}
60+
test.AssertDeepEquals(t, FromCert(cert), tc.expectedIdents)
61+
test.AssertDeepEquals(t, FromCSR(csr), tc.expectedIdents)
62+
})
63+
}
64+
}
65+
66+
func TestNormalize(t *testing.T) {
67+
cases := []struct {
68+
name string
69+
idents []ACMEIdentifier
70+
expected []ACMEIdentifier
71+
}{
72+
{
73+
name: "convert to lowercase",
74+
idents: []ACMEIdentifier{
75+
{Type: TypeDNS, Value: "AlPha.example.coM"},
76+
{Type: TypeIP, Value: "fe80::CAFE"},
77+
},
78+
expected: []ACMEIdentifier{
79+
{Type: TypeDNS, Value: "alpha.example.com"},
80+
{Type: TypeIP, Value: "fe80::cafe"},
81+
},
5482
},
5583
{
56-
"duplicate SANs",
57-
&x509.CertificateRequest{DNSNames: []string{"b.com", "b.com", "a.com", "a.com"}},
58-
[]ACMEIdentifier{NewDNS("a.com"), NewDNS("b.com")},
84+
name: "sort",
85+
idents: []ACMEIdentifier{
86+
{Type: TypeDNS, Value: "foobar.com"},
87+
{Type: TypeDNS, Value: "bar.com"},
88+
{Type: TypeDNS, Value: "baz.com"},
89+
{Type: TypeDNS, Value: "a.com"},
90+
{Type: TypeIP, Value: "fe80::cafe"},
91+
{Type: TypeIP, Value: "2001:db8::1dea"},
92+
{Type: TypeIP, Value: "192.168.1.1"},
93+
},
94+
expected: []ACMEIdentifier{
95+
{Type: TypeDNS, Value: "a.com"},
96+
{Type: TypeDNS, Value: "bar.com"},
97+
{Type: TypeDNS, Value: "baz.com"},
98+
{Type: TypeDNS, Value: "foobar.com"},
99+
{Type: TypeIP, Value: "192.168.1.1"},
100+
{Type: TypeIP, Value: "2001:db8::1dea"},
101+
{Type: TypeIP, Value: "fe80::cafe"},
102+
},
59103
},
60104
{
61-
"explicit CN not found in SANs",
62-
&x509.CertificateRequest{Subject: pkix.Name{CommonName: "a.com"}, DNSNames: []string{"b.com"}},
63-
[]ACMEIdentifier{NewDNS("a.com"), NewDNS("b.com")},
105+
name: "de-duplicate",
106+
idents: []ACMEIdentifier{
107+
{Type: TypeDNS, Value: "AlPha.example.coM"},
108+
{Type: TypeIP, Value: "fe80::CAFE"},
109+
{Type: TypeDNS, Value: "alpha.example.com"},
110+
{Type: TypeIP, Value: "fe80::cafe"},
111+
NewIP(netip.MustParseAddr("fe80:0000:0000:0000:0000:0000:0000:cafe")),
112+
},
113+
expected: []ACMEIdentifier{
114+
{Type: TypeDNS, Value: "alpha.example.com"},
115+
{Type: TypeIP, Value: "fe80::cafe"},
116+
},
64117
},
65118
{
66-
"mix of DNSNames and IPAddresses",
67-
&x509.CertificateRequest{DNSNames: []string{"a.com"}, IPAddresses: []net.IP{{192, 168, 1, 1}}},
68-
[]ACMEIdentifier{NewDNS("a.com"), NewIP(netip.MustParseAddr("192.168.1.1"))},
119+
name: "DNS before IP",
120+
idents: []ACMEIdentifier{
121+
{Type: TypeIP, Value: "fe80::cafe"},
122+
{Type: TypeDNS, Value: "alpha.example.com"},
123+
},
124+
expected: []ACMEIdentifier{
125+
{Type: TypeDNS, Value: "alpha.example.com"},
126+
{Type: TypeIP, Value: "fe80::cafe"},
127+
},
69128
},
70129
}
71130
for _, tc := range cases {
72131
t.Run(tc.name, func(t *testing.T) {
73-
test.AssertDeepEquals(t, FromCSR(tc.csr), tc.expectedIdents)
132+
test.AssertDeepEquals(t, tc.expected, Normalize(tc.idents))
74133
})
75134
}
76135
}

0 commit comments

Comments
 (0)