From 6d9710c88d9798727da91545dcd54bc4d6b4c6ef Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 28 May 2021 16:40:46 +0200 Subject: [PATCH 01/21] Add initial support for ACME IP validation --- acme/api/order.go | 31 +++++--- acme/api/order_test.go | 113 ++++++++++++++++++++++++++ acme/order.go | 176 +++++++++++++++++++++++++++++++++-------- acme/order_test.go | 77 ++++++++++++++++++ 4 files changed, 354 insertions(+), 43 deletions(-) diff --git a/acme/api/order.go b/acme/api/order.go index 9d410173..b0b9cb43 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -28,7 +28,7 @@ func (n *NewOrderRequest) Validate() error { return acme.NewError(acme.ErrorMalformedType, "identifiers list cannot be empty") } for _, id := range n.Identifiers { - if id.Type != "dns" { + if !(id.Type == "dns" || id.Type == "ip") { return acme.NewError(acme.ErrorMalformedType, "identifier type unsupported: %s", id.Type) } } @@ -149,15 +149,9 @@ func (h *Handler) newAuthorization(ctx context.Context, az *acme.Authorization) } } - var ( - err error - chTypes = []string{"dns-01"} - ) - // HTTP and TLS challenges can only be used for identifiers without wildcards. - if !az.Wildcard { - chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) - } + chTypes := challengeTypes(az) + var err error az.Token, err = randutil.Alphanumeric(32) if err != nil { return acme.WrapErrorISE(err, "error generating random alphanumeric ID") @@ -275,3 +269,22 @@ func (h *Handler) FinalizeOrder(w http.ResponseWriter, r *http.Request) { w.Header().Set("Location", h.linker.GetLink(ctx, OrderLinkType, o.ID)) api.JSON(w, o) } + +// challengeTypes determines the types of challenges that should be used +// for the ACME authorization request. +func challengeTypes(az *acme.Authorization) []string { + chTypes := []string{} + + // DNS challenge can not be used for identifiers with type IP + if az.Identifier.Type != "ip" { + chTypes = append(chTypes, "dns-01") // TODO: make these types consts/enum? + } + + // HTTP and TLS challenges can only be used for identifiers without wildcards. + if !az.Wildcard { + //chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) + chTypes = append(chTypes, []string{"http-01"}...) // TODO: fix tls-alpn-01 + } + + return chTypes +} diff --git a/acme/api/order_test.go b/acme/api/order_test.go index 300aa61b..6782de75 100644 --- a/acme/api/order_test.go +++ b/acme/api/order_test.go @@ -10,6 +10,7 @@ import ( "io/ioutil" "net/http/httptest" "net/url" + "reflect" "testing" "time" @@ -60,6 +61,68 @@ func TestNewOrderRequest_Validate(t *testing.T) { naf: naf, } }, + "ok/ipv4": func(t *testing.T) test { + nbf := time.Now().UTC().Add(time.Minute) + naf := time.Now().UTC().Add(5 * time.Minute) + return test{ + nor: &NewOrderRequest{ + Identifiers: []acme.Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + }, + NotAfter: naf, + NotBefore: nbf, + }, + nbf: nbf, + naf: naf, + } + }, + "ok/ipv6": func(t *testing.T) test { + nbf := time.Now().UTC().Add(time.Minute) + naf := time.Now().UTC().Add(5 * time.Minute) + return test{ + nor: &NewOrderRequest{ + Identifiers: []acme.Identifier{ + {Type: "ip", Value: "2001:db8::1"}, + }, + NotAfter: naf, + NotBefore: nbf, + }, + nbf: nbf, + naf: naf, + } + }, + "ok/mixed-dns-and-ipv4": func(t *testing.T) test { // TODO: verify that this is allowed and what we want to be possible (in Validate()) + nbf := time.Now().UTC().Add(time.Minute) + naf := time.Now().UTC().Add(5 * time.Minute) + return test{ + nor: &NewOrderRequest{ + Identifiers: []acme.Identifier{ + {Type: "dns", Value: "example.com"}, + {Type: "ip", Value: "192.168.42.42"}, + }, + NotAfter: naf, + NotBefore: nbf, + }, + nbf: nbf, + naf: naf, + } + }, + "ok/mixed-ipv4-and-ipv6": func(t *testing.T) test { + nbf := time.Now().UTC().Add(time.Minute) + naf := time.Now().UTC().Add(5 * time.Minute) + return test{ + nor: &NewOrderRequest{ + Identifiers: []acme.Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "2001:db8::1"}, + }, + NotAfter: naf, + NotBefore: nbf, + }, + nbf: nbf, + naf: naf, + } + }, } for name, run := range tests { tc := run(t) @@ -1581,3 +1644,53 @@ func TestHandler_FinalizeOrder(t *testing.T) { }) } } + +func TestHandler_challengeTypes(t *testing.T) { + type args struct { + az *acme.Authorization + } + tests := []struct { + name string + args args + want []string + }{ + { + name: "ok/dns", + args: args{ + az: &acme.Authorization{ + Identifier: acme.Identifier{Type: "dns", Value: "example.com"}, + Wildcard: false, + }, + }, + want: []string{"dns-01", "http-01", "tls-alpn-01"}, + }, + { + name: "ok/wildcard", + args: args{ + az: &acme.Authorization{ + Identifier: acme.Identifier{Type: "dns", Value: "*.example.com"}, + Wildcard: true, + }, + }, + want: []string{"dns-01"}, + }, + { + name: "ok/ip", + args: args{ + az: &acme.Authorization{ + Identifier: acme.Identifier{Type: "ip", Value: "192.168.42.42"}, + Wildcard: false, + }, + }, + want: []string{"http-01", "tls-alpn-01"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + if got := challengeTypes(tt.args.az); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Handler.challengeTypes() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/acme/order.go b/acme/order.go index a003fe9a..fdab182f 100644 --- a/acme/order.go +++ b/acme/order.go @@ -1,9 +1,11 @@ package acme import ( + "bytes" "context" "crypto/x509" "encoding/json" + "net" "sort" "strings" "time" @@ -131,41 +133,13 @@ func (o *Order) Finalize(ctx context.Context, db DB, csr *x509.CertificateReques return NewErrorISE("unexpected status %s for order %s", o.Status, o.ID) } - // RFC8555: The CSR MUST indicate the exact same set of requested - // 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. - if csr.Subject.CommonName != "" { - csr.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) - } - csr.DNSNames = uniqueSortedLowerNames(csr.DNSNames) - orderNames := make([]string, len(o.Identifiers)) - for i, n := range o.Identifiers { - orderNames[i] = n.Value - } - orderNames = uniqueSortedLowerNames(orderNames) - - // Validate identifier names against CSR alternative names. - // - // Note that with certificate templates we are not going to check for the - // absence of other SANs as they will only be set if the templates allows - // them. - if len(csr.DNSNames) != len(orderNames) { - return NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) - } + // canonicalize the CSR to allow for comparison + csr = canonicalize(csr) - sans := make([]x509util.SubjectAlternativeName, len(csr.DNSNames)) - for i := range csr.DNSNames { - if csr.DNSNames[i] != orderNames[i] { - return NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) - } - sans[i] = x509util.SubjectAlternativeName{ - Type: x509util.DNSType, - Value: csr.DNSNames[i], - } + // retrieve the requested SANs for the Order + sans, err := o.sans(csr) + if err != nil { + return WrapErrorISE(err, "error determining SANs for the CSR") } // Get authorizations from the ACME provisioner. @@ -213,6 +187,120 @@ func (o *Order) Finalize(ctx context.Context, db DB, csr *x509.CertificateReques return nil } +func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativeName, error) { + + var sans []x509util.SubjectAlternativeName + + // order the DNS names and IP addresses, so that they can be compared against the canonicalized CSR + orderNames := make([]string, len(o.Identifiers)) + orderIPs := make([]net.IP, len(o.Identifiers)) + for i, n := range o.Identifiers { + switch n.Type { + case "dns": + orderNames[i] = n.Value + case "ip": + orderIPs[i] = net.ParseIP(n.Value) // NOTE: this assumes are all valid IPs or will result in nil entries + default: + return sans, NewErrorISE("unsupported identifier type in order: %s", n.Type) + } + } + orderNames = uniqueSortedLowerNames(orderNames) + orderIPs = uniqueSortedIPs(orderIPs) + + // TODO: check whether this order was requested with identifier-type IP, + // if so, handle it as an IP order; not as a DNSName order, so the logic + // for verifying the contents MAY not be necessary. + // TODO: limit what IP addresses can be used? Only private? Only certain ranges + // based on configuration? Public vs. private range? That logic should be configurable somewhere. + // TODO: how to handler orders that have DNSNames AND IPs? I guess it could + // happen in cases where there are multiple "identifiers" to order a cert for + // and http or tls-alpn-1 is used (NOT DNS, because that can't be used for IPs). + // TODO: ensure that DNSNames indeed MUST NEVER have an IP + // TODO: only allow IP based identifier based on configuration? + // TODO: validation of the input (if IP; should be valid IPv4/v6) + + // Determine if DNS names or IPs should be processed. + // At this time, orders in which DNS names and IPs are mixed are not supported. // TODO: ensure that's OK and/or should we support more, RFC-wise + shouldProcessIPAddresses := len(csr.DNSNames) == 0 && len(orderIPs) != 0 // TODO: verify that this logic is OK and sufficient + if shouldProcessIPAddresses { + // Validate identifier IPs against CSR alternative names (IPs). + if len(csr.IPAddresses) != len(orderIPs) { + return sans, NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) + } + + sans = make([]x509util.SubjectAlternativeName, len(csr.IPAddresses)) + for i := range csr.IPAddresses { + if !ipsAreEqual(csr.IPAddresses[i], orderIPs[i]) { + return sans, NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) + } + sans[i] = x509util.SubjectAlternativeName{ + Type: x509util.IPType, + Value: csr.IPAddresses[i].String(), + } + } + } else { + // Validate identifier names against CSR alternative names. + // + // Note that with certificate templates we are not going to check for the + // absence of other SANs as they will only be set if the templates allows + // them. + if len(csr.DNSNames) != len(orderNames) { + return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) + } + + sans = make([]x509util.SubjectAlternativeName, len(csr.DNSNames)) + for i := range csr.DNSNames { + if csr.DNSNames[i] != orderNames[i] { + return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) + } + sans[i] = x509util.SubjectAlternativeName{ + Type: x509util.DNSType, + Value: csr.DNSNames[i], + } + } + } + + return sans, nil +} + +func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.CertificateRequest) { + + // for clarity only; we're operating on the same object by pointer + canonicalized = csr + + // RFC8555: The CSR MUST indicate the exact same set of requested + // 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. + if csr.Subject.CommonName != "" { + canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) + } + canonicalized.DNSNames = uniqueSortedLowerNames(csr.DNSNames) + canonicalized.IPAddresses = uniqueSortedIPs(csr.IPAddresses) // TODO: sorting and setting this value MAY result in different values in CSR (and probably also ending up in cert); is that behavior wanted? + + return canonicalized +} + +// ipsAreEqual compares IPs to be equal. IPv6 representations of IPv4 +// adresses are NOT considered equal to the IPv4 address in this case. +// Both IPs should be the same version AND equal to each other. +func ipsAreEqual(x, y net.IP) bool { + if isIPv4(x) && isIPv4(y) { + return x.Equal(y) + } + return x.Equal(y) +} + +// isIPv4 returns if an IP is IPv4 or not. +func isIPv4(ip net.IP) bool { + return ip.To4() != nil +} + // uniqueSortedLowerNames returns the set of all unique names in the input after all // of them are lowercased. The returned names will be in their lowercased form // and sorted alphabetically. @@ -228,3 +316,23 @@ func uniqueSortedLowerNames(names []string) (unique []string) { sort.Strings(unique) return } + +// uniqueSortedIPs returns the set of all unique net.IPs in the input. They +// are sorted by their bytes (octet) representation. +func uniqueSortedIPs(ips []net.IP) (unique []net.IP) { + type entry struct { + ip net.IP + } + ipEntryMap := make(map[string]entry, len(ips)) + for _, ip := range ips { + ipEntryMap[ip.String()] = entry{ip: ip} + } + unique = make([]net.IP, 0, len(ipEntryMap)) + for _, entry := range ipEntryMap { + unique = append(unique, entry.ip) + } + sort.Slice(unique, func(i, j int) bool { + return bytes.Compare(unique[i], unique[j]) < 0 + }) + return +} diff --git a/acme/order_test.go b/acme/order_test.go index 993a92f2..c0461dc6 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -5,6 +5,8 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/json" + "net" + "reflect" "testing" "time" @@ -737,3 +739,78 @@ func TestOrder_Finalize(t *testing.T) { }) } } + +func Test_uniqueSortedIPs(t *testing.T) { + type args struct { + ips []net.IP + } + tests := []struct { + name string + args args + wantUnique []net.IP + }{ + { + name: "ok/empty", + args: args{ + ips: []net.IP{}, + }, + wantUnique: []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")}, + }, + { + 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")}, + }, + wantUnique: []net.IP{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")}, + }, + { + name: "ok/single-ipv6", + args: args{ + ips: []net.IP{net.ParseIP("2001:db8::30")}, + }, + wantUnique: []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")}, + }, + { + 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")}, + }, + { + 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")}, + }, + } + 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) + } + }) + } +} From 3e36522329aeef0009547c50d30e0f54e0081d89 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 29 May 2021 00:19:14 +0200 Subject: [PATCH 02/21] Add preliminary support for TLS-ALPN-01 challenge for IP identifiers --- acme/api/order.go | 7 +++---- acme/challenge.go | 24 +++++++++++++++++++++--- acme/order.go | 9 ++------- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/acme/api/order.go b/acme/api/order.go index b0b9cb43..78a4f9c6 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -275,15 +275,14 @@ func (h *Handler) FinalizeOrder(w http.ResponseWriter, r *http.Request) { func challengeTypes(az *acme.Authorization) []string { chTypes := []string{} - // DNS challenge can not be used for identifiers with type IP - if az.Identifier.Type != "ip" { + // DNS challenge can only be used for identifiers with type dns + if az.Identifier.Type == "dns" { chTypes = append(chTypes, "dns-01") // TODO: make these types consts/enum? } // HTTP and TLS challenges can only be used for identifiers without wildcards. if !az.Wildcard { - //chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) - chTypes = append(chTypes, []string{"http-01"}...) // TODO: fix tls-alpn-01 + chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) } return chTypes diff --git a/acme/challenge.go b/acme/challenge.go index 1059e437..90960fc4 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -119,8 +119,14 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON hostPort := net.JoinHostPort(ch.Value, "443") + fmt.Println(hostPort) + fmt.Println(fmt.Sprintf("%#+v", config)) + + time.Sleep(5 * time.Second) // TODO: remove this; client seems to take a while to start serving; the server does not seem to retry the conn + conn, err := vo.TLSDial("tcp", hostPort, config) if err != nil { + fmt.Println(err) return storeError(ctx, db, ch, false, WrapError(ErrorConnectionType, err, "error doing TLS dial for %s", hostPort)) } @@ -129,6 +135,9 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON cs := conn.ConnectionState() certs := cs.PeerCertificates + fmt.Println(fmt.Sprintf("%#+v", cs)) + fmt.Println(fmt.Sprintf("%#+v", certs)) + if len(certs) == 0 { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "%s challenge for %s resulted in no certificates", ch.Type, ch.Value)) @@ -140,10 +149,19 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON } leafCert := certs[0] + fmt.Println(fmt.Sprintf("%#+v", leafCert)) - if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], ch.Value) { - return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, - "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value)) + // if no DNS names present, look for IP address and verify that exactly one exists + if len(leafCert.DNSNames) == 0 { + if len(leafCert.IPAddresses) != 1 || !strings.EqualFold(leafCert.IPAddresses[0].String(), ch.Value) { + return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, + "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address, %v", ch.Value)) + } + } else { + if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], ch.Value) { + return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, + "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value)) + } } idPeAcmeIdentifier := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 31} diff --git a/acme/order.go b/acme/order.go index fdab182f..e9e161f9 100644 --- a/acme/order.go +++ b/acme/order.go @@ -207,17 +207,12 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ orderNames = uniqueSortedLowerNames(orderNames) orderIPs = uniqueSortedIPs(orderIPs) - // TODO: check whether this order was requested with identifier-type IP, - // if so, handle it as an IP order; not as a DNSName order, so the logic - // for verifying the contents MAY not be necessary. // TODO: limit what IP addresses can be used? Only private? Only certain ranges // based on configuration? Public vs. private range? That logic should be configurable somewhere. - // TODO: how to handler orders that have DNSNames AND IPs? I guess it could - // happen in cases where there are multiple "identifiers" to order a cert for - // and http or tls-alpn-1 is used (NOT DNS, because that can't be used for IPs). // TODO: ensure that DNSNames indeed MUST NEVER have an IP // TODO: only allow IP based identifier based on configuration? - // TODO: validation of the input (if IP; should be valid IPv4/v6) + // TODO: validation of the input (if IP; should be valid IPv4/v6; Incoming request should have Host header set / ALPN IN-ADDR.ARPA) + // TODO: limit the IP address identifier to a single IP address? RFC _can_ be read like that, but there can be multiple identifiers, of course // Determine if DNS names or IPs should be processed. // At this time, orders in which DNS names and IPs are mixed are not supported. // TODO: ensure that's OK and/or should we support more, RFC-wise From 6486e6016bebf596879da34774c7b37e06e41a37 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 29 May 2021 00:37:22 +0200 Subject: [PATCH 03/21] Make logic for which challenge types to use clearer --- acme/api/order.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/acme/api/order.go b/acme/api/order.go index 78a4f9c6..7d43c2d9 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -273,16 +273,19 @@ func (h *Handler) FinalizeOrder(w http.ResponseWriter, r *http.Request) { // challengeTypes determines the types of challenges that should be used // for the ACME authorization request. func challengeTypes(az *acme.Authorization) []string { - chTypes := []string{} + var chTypes []string - // DNS challenge can only be used for identifiers with type dns - if az.Identifier.Type == "dns" { - chTypes = append(chTypes, "dns-01") // TODO: make these types consts/enum? - } - - // HTTP and TLS challenges can only be used for identifiers without wildcards. - if !az.Wildcard { - chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) + switch az.Identifier.Type { + case "ip": // TODO: make these types consts/enum? + chTypes = []string{"http-01", "tls-alpn-01"} + case "dns": + chTypes = []string{"dns-01"} + // HTTP and TLS challenges can only be used for identifiers without wildcards. + if !az.Wildcard { + chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) + } + default: + chTypes = []string{} } return chTypes From a0e92f8e99268fcb26f919fae973ae9af3c3b4c8 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 3 Jun 2021 22:02:13 +0200 Subject: [PATCH 04/21] Verify IP identifier contains valid IP --- acme/api/order.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/acme/api/order.go b/acme/api/order.go index 7d43c2d9..e1ac1aa1 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "encoding/base64" "encoding/json" + "net" "net/http" "strings" "time" @@ -31,6 +32,9 @@ func (n *NewOrderRequest) Validate() error { if !(id.Type == "dns" || id.Type == "ip") { return acme.NewError(acme.ErrorMalformedType, "identifier type unsupported: %s", id.Type) } + if id.Type == "ip" && net.ParseIP(id.Value) == nil { + return acme.NewError(acme.ErrorMalformedType, "%s is not a valid IP address", id.Value) + } } return nil } @@ -85,6 +89,7 @@ func (h *Handler) NewOrder(w http.ResponseWriter, r *http.Request) { "failed to unmarshal new-order request payload")) return } + if err := nor.Validate(); err != nil { api.WriteError(w, err) return From af615db6b51f4fd912e55957b82ae1b02addd3c6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 3 Jun 2021 22:03:21 +0200 Subject: [PATCH 05/21] Support DNS and IPs as SANs in single Order --- acme/order.go | 91 ++++++++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/acme/order.go b/acme/order.go index e9e161f9..e1d77ece 100644 --- a/acme/order.go +++ b/acme/order.go @@ -199,7 +199,7 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ case "dns": orderNames[i] = n.Value case "ip": - orderIPs[i] = net.ParseIP(n.Value) // NOTE: this assumes are all valid IPs or will result in nil entries + orderIPs[i] = net.ParseIP(n.Value) // NOTE: this assumes are all valid IPs at this time; or will result in nil entries default: return sans, NewErrorISE("unsupported identifier type in order: %s", n.Type) } @@ -207,55 +207,49 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ orderNames = uniqueSortedLowerNames(orderNames) orderIPs = uniqueSortedIPs(orderIPs) - // TODO: limit what IP addresses can be used? Only private? Only certain ranges + totalNumberOfSANs := len(csr.DNSNames) + len(csr.IPAddresses) + sans = make([]x509util.SubjectAlternativeName, totalNumberOfSANs) + + // TODO: limit what IP addresses can be used? Only private? Only certain ranges (i.e. only allow the specific ranges by default, configuration for all?) + // TODO: can DNS already be limited to a certain domain? That would probably be nice to have too, but maybe not as part of this PR + // TODO: if it seems not too big of a change, make consts/enums out of the stringly typed identifiers (challenge types, identifier types) // based on configuration? Public vs. private range? That logic should be configurable somewhere. - // TODO: ensure that DNSNames indeed MUST NEVER have an IP - // TODO: only allow IP based identifier based on configuration? - // TODO: validation of the input (if IP; should be valid IPv4/v6; Incoming request should have Host header set / ALPN IN-ADDR.ARPA) - // TODO: limit the IP address identifier to a single IP address? RFC _can_ be read like that, but there can be multiple identifiers, of course - - // Determine if DNS names or IPs should be processed. - // At this time, orders in which DNS names and IPs are mixed are not supported. // TODO: ensure that's OK and/or should we support more, RFC-wise - shouldProcessIPAddresses := len(csr.DNSNames) == 0 && len(orderIPs) != 0 // TODO: verify that this logic is OK and sufficient - if shouldProcessIPAddresses { - // Validate identifier IPs against CSR alternative names (IPs). - if len(csr.IPAddresses) != len(orderIPs) { + // TODO: only allow IP based identifier based on configuration? Some additional configuration and validation on the provisioner for this case. + + // Validate identifier names against CSR alternative names. + // + // Note that with certificate templates we are not going to check for the + // absence of other SANs as they will only be set if the templates allows + // them. + if len(csr.IPAddresses) != len(orderIPs) { + return sans, NewError(ErrorBadCSRType, "number of CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) + } + + for i := range csr.IPAddresses { + if !ipsAreEqual(csr.IPAddresses[i], orderIPs[i]) { return sans, NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) } - - sans = make([]x509util.SubjectAlternativeName, len(csr.IPAddresses)) - for i := range csr.IPAddresses { - if !ipsAreEqual(csr.IPAddresses[i], orderIPs[i]) { - return sans, NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ - "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) - } - sans[i] = x509util.SubjectAlternativeName{ - Type: x509util.IPType, - Value: csr.IPAddresses[i].String(), - } + sans[i] = x509util.SubjectAlternativeName{ + Type: x509util.IPType, + Value: csr.IPAddresses[i].String(), } - } else { - // Validate identifier names against CSR alternative names. - // - // Note that with certificate templates we are not going to check for the - // absence of other SANs as they will only be set if the templates allows - // them. - if len(csr.DNSNames) != len(orderNames) { + } + + if len(csr.DNSNames) != len(orderNames) { + return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) + } + + for i := range csr.DNSNames { + if csr.DNSNames[i] != orderNames[i] { return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) } - - sans = make([]x509util.SubjectAlternativeName, len(csr.DNSNames)) - for i := range csr.DNSNames { - if csr.DNSNames[i] != orderNames[i] { - return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) - } - sans[i] = x509util.SubjectAlternativeName{ - Type: x509util.DNSType, - Value: csr.DNSNames[i], - } + sans[i] = x509util.SubjectAlternativeName{ + Type: x509util.DNSType, + Value: csr.DNSNames[i], } } @@ -276,7 +270,7 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) } canonicalized.DNSNames = uniqueSortedLowerNames(csr.DNSNames) - canonicalized.IPAddresses = uniqueSortedIPs(csr.IPAddresses) // TODO: sorting and setting this value MAY result in different values in CSR (and probably also ending up in cert); is that behavior wanted? + canonicalized.IPAddresses = uniqueSortedIPs(csr.IPAddresses) return canonicalized } @@ -285,15 +279,16 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate // adresses are NOT considered equal to the IPv4 address in this case. // Both IPs should be the same version AND equal to each other. func ipsAreEqual(x, y net.IP) bool { - if isIPv4(x) && isIPv4(y) { + if matchAddrFamily(x, y) { return x.Equal(y) } - return x.Equal(y) + return false } -// isIPv4 returns if an IP is IPv4 or not. -func isIPv4(ip net.IP) bool { - return ip.To4() != nil +// matchAddrFamily returns if two IPs are both IPv4 OR IPv6 +// Implementation taken and adapted from https://golang.org/src/net/ip.go +func matchAddrFamily(x net.IP, y net.IP) bool { + return x.To4() != nil && y.To4() != nil || x.To16() != nil && x.To4() == nil && y.To16() != nil && y.To4() == nil } // uniqueSortedLowerNames returns the set of all unique names in the input after all From 76dcf542d42f66531aeb1dec13ddb8340a9c52ab Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 3 Jun 2021 22:45:24 +0200 Subject: [PATCH 06/21] Fix mixed DNS and IP SANs in Order --- acme/api/order.go | 2 +- acme/order.go | 61 +++++++++++++++++++++++++++++++---------------- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/acme/api/order.go b/acme/api/order.go index e1ac1aa1..936e9573 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -33,7 +33,7 @@ func (n *NewOrderRequest) Validate() error { return acme.NewError(acme.ErrorMalformedType, "identifier type unsupported: %s", id.Type) } if id.Type == "ip" && net.ParseIP(id.Value) == nil { - return acme.NewError(acme.ErrorMalformedType, "%s is not a valid IP address", id.Value) + return acme.NewError(acme.ErrorMalformedType, "invalid IP address: %s", id.Value) } } return nil diff --git a/acme/order.go b/acme/order.go index e1d77ece..add90e1a 100644 --- a/acme/order.go +++ b/acme/order.go @@ -192,14 +192,17 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ var sans []x509util.SubjectAlternativeName // order the DNS names and IP addresses, so that they can be compared against the canonicalized CSR - orderNames := make([]string, len(o.Identifiers)) - orderIPs := make([]net.IP, len(o.Identifiers)) - for i, n := range o.Identifiers { + orderNames := make([]string, numberOfIdentifierType("dns", o.Identifiers)) + orderIPs := make([]net.IP, numberOfIdentifierType("ip", o.Identifiers)) + indexDNS, indexIP := 0, 0 + for _, n := range o.Identifiers { switch n.Type { case "dns": - orderNames[i] = n.Value + orderNames[indexDNS] = n.Value + indexDNS++ case "ip": - orderIPs[i] = net.ParseIP(n.Value) // NOTE: this assumes are all valid IPs at this time; or will result in nil entries + orderIPs[indexIP] = net.ParseIP(n.Value) // NOTE: this assumes are all valid IPs at this time; or will result in nil entries + indexIP++ default: return sans, NewErrorISE("unsupported identifier type in order: %s", n.Type) } @@ -209,6 +212,7 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ totalNumberOfSANs := len(csr.DNSNames) + len(csr.IPAddresses) sans = make([]x509util.SubjectAlternativeName, totalNumberOfSANs) + index := 0 // TODO: limit what IP addresses can be used? Only private? Only certain ranges (i.e. only allow the specific ranges by default, configuration for all?) // TODO: can DNS already be limited to a certain domain? That would probably be nice to have too, but maybe not as part of this PR @@ -221,6 +225,23 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ // Note that with certificate templates we are not going to check for the // absence of other SANs as they will only be set if the templates allows // them. + if len(csr.DNSNames) != len(orderNames) { + return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) + } + + for i := range csr.DNSNames { + if csr.DNSNames[i] != orderNames[i] { + return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) + } + sans[index] = x509util.SubjectAlternativeName{ + Type: x509util.DNSType, + Value: csr.DNSNames[i], + } + index++ + } + if len(csr.IPAddresses) != len(orderIPs) { return sans, NewError(ErrorBadCSRType, "number of CSR IPs do not match identifiers exactly: "+ "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) @@ -231,31 +252,31 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ return sans, NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) } - sans[i] = x509util.SubjectAlternativeName{ + sans[index] = x509util.SubjectAlternativeName{ Type: x509util.IPType, Value: csr.IPAddresses[i].String(), } + index++ } - if len(csr.DNSNames) != len(orderNames) { - return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) - } + return sans, nil +} - for i := range csr.DNSNames { - if csr.DNSNames[i] != orderNames[i] { - return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", csr.DNSNames, orderNames) - } - sans[i] = x509util.SubjectAlternativeName{ - Type: x509util.DNSType, - Value: csr.DNSNames[i], +// numberOfIdentifierType returns the number of Identifiers that +// are of type typ. +func numberOfIdentifierType(typ string, ids []Identifier) int { + c := 0 + for _, id := range ids { + if id.Type == typ { + c++ } } - - return sans, nil + return c } +// 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. func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.CertificateRequest) { // for clarity only; we're operating on the same object by pointer From 2f40011da87cacdd73a51bd7ae9c915fa40f567d Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 4 Jun 2021 00:01:43 +0200 Subject: [PATCH 07/21] Add support for TLS-ALPN-01 challenge --- acme/challenge.go | 67 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 90960fc4..d541bed2 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -107,23 +107,30 @@ func http01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWeb } func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebKey, vo *ValidateChallengeOptions) error { + + var serverName string + + // RFC8738 states that, if HostName is IP, it should be the ARPA + // address https://datatracker.ietf.org/doc/html/rfc8738#section-6. + // It also references TLS Extensions [RFC6066]. + if ip := net.ParseIP(ch.Value); ip != nil { + serverName = reverseAddr(ip) + } else { + serverName = ch.Value + } + config := &tls.Config{ NextProtos: []string{"acme-tls/1"}, // https://tools.ietf.org/html/rfc8737#section-4 // ACME servers that implement "acme-tls/1" MUST only negotiate TLS 1.2 // [RFC5246] or higher when connecting to clients for validation. MinVersion: tls.VersionTLS12, - ServerName: ch.Value, + ServerName: serverName, InsecureSkipVerify: true, // we expect a self-signed challenge certificate } hostPort := net.JoinHostPort(ch.Value, "443") - fmt.Println(hostPort) - fmt.Println(fmt.Sprintf("%#+v", config)) - - time.Sleep(5 * time.Second) // TODO: remove this; client seems to take a while to start serving; the server does not seem to retry the conn - conn, err := vo.TLSDial("tcp", hostPort, config) if err != nil { fmt.Println(err) @@ -135,9 +142,6 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON cs := conn.ConnectionState() certs := cs.PeerCertificates - fmt.Println(fmt.Sprintf("%#+v", cs)) - fmt.Println(fmt.Sprintf("%#+v", certs)) - if len(certs) == 0 { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "%s challenge for %s resulted in no certificates", ch.Type, ch.Value)) @@ -149,7 +153,6 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON } leafCert := certs[0] - fmt.Println(fmt.Sprintf("%#+v", leafCert)) // if no DNS names present, look for IP address and verify that exactly one exists if len(leafCert.DNSNames) == 0 { @@ -262,6 +265,50 @@ func dns01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebK return nil } +// reverseaddr returns the in-addr.arpa. or ip6.arpa. hostname of the IP +// address addr suitable for rDNS (PTR) record lookup or an error if it fails +// to parse the IP address. +// Implementation taken and adapted from https://golang.org/src/net/dnsclient.go?s=780:834#L20 +func reverseAddr(ip net.IP) (arpa string) { + if ip.To4() != nil { + return uitoa(uint(ip[15])) + "." + uitoa(uint(ip[14])) + "." + uitoa(uint(ip[13])) + "." + uitoa(uint(ip[12])) + ".in-addr.arpa." + } + // Must be IPv6 + buf := make([]byte, 0, len(ip)*4+len("ip6.arpa.")) + // Add it, in reverse, to the buffer + for i := len(ip) - 1; i >= 0; i-- { + v := ip[i] + buf = append(buf, hexit[v&0xF], + '.', + hexit[v>>4], + '.') + } + // Append "ip6.arpa." and return (buf already has the final .) + buf = append(buf, "ip6.arpa."...) + return string(buf) +} + +// Convert unsigned integer to decimal string. +// Implementation taken from https://golang.org/src/net/parse.go +func uitoa(val uint) string { + if val == 0 { // avoid string allocation + return "0" + } + var buf [20]byte // big enough for 64bit value base 10 + i := len(buf) - 1 + for val >= 10 { + q := val / 10 + buf[i] = byte('0' + val - q*10) + i-- + val = q + } + // val < 10 + buf[i] = byte('0' + val) + return string(buf[i:]) +} + +const hexit = "0123456789abcdef" + // KeyAuthorization creates the ACME key authorization value from a token // and a jwk. func KeyAuthorization(token string, jwk *jose.JSONWebKey) (string, error) { From a6405e98a9939519ebc8012d263aed1ff1113b1b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 4 Jun 2021 00:06:15 +0200 Subject: [PATCH 08/21] Remove fmt. --- acme/challenge.go | 1 - acme/order.go | 1 - 2 files changed, 2 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index d541bed2..105fcbc8 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -133,7 +133,6 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON conn, err := vo.TLSDial("tcp", hostPort, config) if err != nil { - fmt.Println(err) return storeError(ctx, db, ch, false, WrapError(ErrorConnectionType, err, "error doing TLS dial for %s", hostPort)) } diff --git a/acme/order.go b/acme/order.go index add90e1a..b11d51c7 100644 --- a/acme/order.go +++ b/acme/order.go @@ -217,7 +217,6 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ // TODO: limit what IP addresses can be used? Only private? Only certain ranges (i.e. only allow the specific ranges by default, configuration for all?) // TODO: can DNS already be limited to a certain domain? That would probably be nice to have too, but maybe not as part of this PR // TODO: if it seems not too big of a change, make consts/enums out of the stringly typed identifiers (challenge types, identifier types) - // based on configuration? Public vs. private range? That logic should be configurable somewhere. // TODO: only allow IP based identifier based on configuration? Some additional configuration and validation on the provisioner for this case. // Validate identifier names against CSR alternative names. From 0c79914d0d417fcc4df97663e2e4acb6162108bf Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 4 Jun 2021 00:12:49 +0200 Subject: [PATCH 09/21] Improve check for single IP in TLS-ALPN-01 challenge --- acme/challenge.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 105fcbc8..eabdaaed 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -108,12 +108,12 @@ func http01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWeb func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebKey, vo *ValidateChallengeOptions) error { - var serverName string - // RFC8738 states that, if HostName is IP, it should be the ARPA // address https://datatracker.ietf.org/doc/html/rfc8738#section-6. // It also references TLS Extensions [RFC6066]. - if ip := net.ParseIP(ch.Value); ip != nil { + var serverName string + ip := net.ParseIP(ch.Value) + if ip != nil { serverName = reverseAddr(ip) } else { serverName = ch.Value @@ -155,7 +155,7 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON // if no DNS names present, look for IP address and verify that exactly one exists if len(leafCert.DNSNames) == 0 { - if len(leafCert.IPAddresses) != 1 || !strings.EqualFold(leafCert.IPAddresses[0].String(), ch.Value) { + if len(leafCert.IPAddresses) != 1 || !leafCert.IPAddresses[0].Equal(ip) { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address, %v", ch.Value)) } From af4803b8b883c39322cc440cfd512de3b45281ab Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 4 Jun 2021 08:42:24 +0200 Subject: [PATCH 10/21] Fix tests --- acme/challenge.go | 4 ++-- acme/challenge_test.go | 10 +++++----- acme/order.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index eabdaaed..cb15a188 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -157,12 +157,12 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON if len(leafCert.DNSNames) == 0 { if len(leafCert.IPAddresses) != 1 || !leafCert.IPAddresses[0].Equal(ip) { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, - "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address, %v", ch.Value)) + "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value)) } } else { if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], ch.Value) { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, - "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value)) + "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value)) } } diff --git a/acme/challenge_test.go b/acme/challenge_test.go index 14287945..8e03a414 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -1544,7 +1544,7 @@ func TestTLSALPN01Validate(t *testing.T) { err: NewErrorISE("failure saving error to acme challenge: force"), } }, - "ok/no-names-error": func(t *testing.T) test { + "ok/no-names-nor-ips-error": func(t *testing.T) test { ch := makeTLSCh() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) @@ -1573,7 +1573,7 @@ func TestTLSALPN01Validate(t *testing.T) { assert.Equals(t, updch.Type, ch.Type) assert.Equals(t, updch.Value, ch.Value) - err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value) + err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value) assert.HasPrefix(t, updch.Error.Err.Error(), err.Err.Error()) assert.Equals(t, updch.Error.Type, err.Type) @@ -1616,7 +1616,7 @@ func TestTLSALPN01Validate(t *testing.T) { assert.Equals(t, updch.Type, ch.Type) assert.Equals(t, updch.Value, ch.Value) - err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value) + err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value) assert.HasPrefix(t, updch.Error.Err.Error(), err.Err.Error()) assert.Equals(t, updch.Error.Type, err.Type) @@ -1660,7 +1660,7 @@ func TestTLSALPN01Validate(t *testing.T) { assert.Equals(t, updch.Type, ch.Type) assert.Equals(t, updch.Value, ch.Value) - err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value) + err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value) assert.HasPrefix(t, updch.Error.Err.Error(), err.Err.Error()) assert.Equals(t, updch.Error.Type, err.Type) @@ -1703,7 +1703,7 @@ func TestTLSALPN01Validate(t *testing.T) { assert.Equals(t, updch.Type, ch.Type) assert.Equals(t, updch.Value, ch.Value) - err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.Value) + err := NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value) assert.HasPrefix(t, updch.Error.Err.Error(), err.Err.Error()) assert.Equals(t, updch.Error.Type, err.Type) diff --git a/acme/order.go b/acme/order.go index b11d51c7..73d5e636 100644 --- a/acme/order.go +++ b/acme/order.go @@ -139,7 +139,7 @@ func (o *Order) Finalize(ctx context.Context, db DB, csr *x509.CertificateReques // retrieve the requested SANs for the Order sans, err := o.sans(csr) if err != nil { - return WrapErrorISE(err, "error determining SANs for the CSR") + return err } // Get authorizations from the ACME provisioner. @@ -242,7 +242,7 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ } if len(csr.IPAddresses) != len(orderIPs) { - return sans, NewError(ErrorBadCSRType, "number of CSR IPs do not match identifiers exactly: "+ + return sans, NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ "CSR IPs = %v, Order IPs = %v", csr.IPAddresses, orderIPs) } From 84ea8bd67aa18909ff6d78b7b46eb926cb584076 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 18 Jun 2021 12:03:46 +0200 Subject: [PATCH 11/21] Fix PR comments --- acme/api/order_test.go | 18 +++++++++++++++++- acme/order.go | 17 ++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/acme/api/order_test.go b/acme/api/order_test.go index 6782de75..a0096419 100644 --- a/acme/api/order_test.go +++ b/acme/api/order_test.go @@ -45,6 +45,22 @@ func TestNewOrderRequest_Validate(t *testing.T) { err: acme.NewError(acme.ErrorMalformedType, "identifier type unsupported: foo"), } }, + "fail/bad-ip": func(t *testing.T) test { + nbf := time.Now().UTC().Add(time.Minute) + naf := time.Now().UTC().Add(5 * time.Minute) + return test{ + nor: &NewOrderRequest{ + Identifiers: []acme.Identifier{ + {Type: "ip", Value: "192.168.42.1000"}, + }, + NotAfter: naf, + NotBefore: nbf, + }, + nbf: nbf, + naf: naf, + err: acme.NewError(acme.ErrorMalformedType, "invalid IP address: %s", "192.168.42.1000"), + } + }, "ok": func(t *testing.T) test { nbf := time.Now().UTC().Add(time.Minute) naf := time.Now().UTC().Add(5 * time.Minute) @@ -91,7 +107,7 @@ func TestNewOrderRequest_Validate(t *testing.T) { naf: naf, } }, - "ok/mixed-dns-and-ipv4": func(t *testing.T) test { // TODO: verify that this is allowed and what we want to be possible (in Validate()) + "ok/mixed-dns-and-ipv4": func(t *testing.T) test { nbf := time.Now().UTC().Add(time.Minute) naf := time.Now().UTC().Add(5 * time.Minute) return test{ diff --git a/acme/order.go b/acme/order.go index 73d5e636..86b3c43a 100644 --- a/acme/order.go +++ b/acme/order.go @@ -14,10 +14,17 @@ import ( "go.step.sm/crypto/x509util" ) +type IdentifierType string + +const ( + IP IdentifierType = "ip" + DNS IdentifierType = "dns" +) + // Identifier encodes the type that an order pertains to. type Identifier struct { - Type string `json:"type"` - Value string `json:"value"` + Type IdentifierType `json:"type"` + Value string `json:"value"` } // Order contains order metadata for the ACME protocol order type. @@ -222,7 +229,7 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ // Validate identifier names against CSR alternative names. // // Note that with certificate templates we are not going to check for the - // absence of other SANs as they will only be set if the templates allows + // absence of other SANs as they will only be set if the template allows // them. if len(csr.DNSNames) != len(orderNames) { return sans, NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ @@ -263,7 +270,7 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ // numberOfIdentifierType returns the number of Identifiers that // are of type typ. -func numberOfIdentifierType(typ string, ids []Identifier) int { +func numberOfIdentifierType(typ IdentifierType, ids []Identifier) int { c := 0 for _, id := range ids { if id.Type == typ { @@ -305,7 +312,7 @@ func ipsAreEqual(x, y net.IP) bool { return false } -// matchAddrFamily returns if two IPs are both IPv4 OR IPv6 +// matchAddrFamily returns true if two IPs are both IPv4 OR IPv6 // Implementation taken and adapted from https://golang.org/src/net/ip.go func matchAddrFamily(x net.IP, y net.IP) bool { return x.To4() != nil && y.To4() != nil || x.To16() != nil && x.To4() == nil && y.To16() != nil && y.To4() == nil From 523ae96749cdd187d6228646d6d7f6397da4a0da Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 18 Jun 2021 12:39:36 +0200 Subject: [PATCH 12/21] Change identifier and challenge types to consts --- acme/api/handler_test.go | 10 +++--- acme/api/order.go | 20 ++++++------ acme/api/order_test.go | 66 +++++++++++++++++++------------------- acme/challenge.go | 34 ++++++++++++-------- acme/db/nosql/challenge.go | 18 +++++------ 5 files changed, 78 insertions(+), 70 deletions(-) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 5501479d..f354bbac 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -574,13 +574,13 @@ func TestHandler_GetChallenge(t *testing.T) { assert.Equals(t, azID, "authzID") return &acme.Challenge{ Status: acme.StatusPending, - Type: "http-01", + Type: acme.HTTP01, AccountID: "accID", }, nil }, MockUpdateChallenge: func(ctx context.Context, ch *acme.Challenge) error { assert.Equals(t, ch.Status, acme.StatusPending) - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) assert.Equals(t, ch.AccountID, "accID") assert.Equals(t, ch.AuthorizationID, "authzID") assert.HasSuffix(t, ch.Error.Type, acme.ErrorConnectionType.String()) @@ -616,13 +616,13 @@ func TestHandler_GetChallenge(t *testing.T) { return &acme.Challenge{ ID: "chID", Status: acme.StatusPending, - Type: "http-01", + Type: acme.HTTP01, AccountID: "accID", }, nil }, MockUpdateChallenge: func(ctx context.Context, ch *acme.Challenge) error { assert.Equals(t, ch.Status, acme.StatusPending) - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) assert.Equals(t, ch.AccountID, "accID") assert.Equals(t, ch.AuthorizationID, "authzID") assert.HasSuffix(t, ch.Error.Type, acme.ErrorConnectionType.String()) @@ -633,7 +633,7 @@ func TestHandler_GetChallenge(t *testing.T) { ID: "chID", Status: acme.StatusPending, AuthorizationID: "authzID", - Type: "http-01", + Type: acme.HTTP01, AccountID: "accID", URL: url, Error: acme.NewError(acme.ErrorConnectionType, "force"), diff --git a/acme/api/order.go b/acme/api/order.go index 936e9573..9cf2c1eb 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -29,10 +29,10 @@ func (n *NewOrderRequest) Validate() error { return acme.NewError(acme.ErrorMalformedType, "identifiers list cannot be empty") } for _, id := range n.Identifiers { - if !(id.Type == "dns" || id.Type == "ip") { + if !(id.Type == acme.DNS || id.Type == acme.IP) { return acme.NewError(acme.ErrorMalformedType, "identifier type unsupported: %s", id.Type) } - if id.Type == "ip" && net.ParseIP(id.Value) == nil { + if id.Type == acme.IP && net.ParseIP(id.Value) == nil { return acme.NewError(acme.ErrorMalformedType, "invalid IP address: %s", id.Value) } } @@ -277,20 +277,20 @@ func (h *Handler) FinalizeOrder(w http.ResponseWriter, r *http.Request) { // challengeTypes determines the types of challenges that should be used // for the ACME authorization request. -func challengeTypes(az *acme.Authorization) []string { - var chTypes []string +func challengeTypes(az *acme.Authorization) []acme.ChallengeType { + var chTypes []acme.ChallengeType switch az.Identifier.Type { - case "ip": // TODO: make these types consts/enum? - chTypes = []string{"http-01", "tls-alpn-01"} - case "dns": - chTypes = []string{"dns-01"} + case acme.IP: + chTypes = []acme.ChallengeType{acme.HTTP01, acme.TLSALPN01} + case acme.DNS: + chTypes = []acme.ChallengeType{acme.DNS01} // HTTP and TLS challenges can only be used for identifiers without wildcards. if !az.Wildcard { - chTypes = append(chTypes, []string{"http-01", "tls-alpn-01"}...) + chTypes = append(chTypes, []acme.ChallengeType{acme.HTTP01, acme.TLSALPN01}...) } default: - chTypes = []string{} + chTypes = []acme.ChallengeType{} } return chTypes diff --git a/acme/api/order_test.go b/acme/api/order_test.go index a0096419..38c4c3f0 100644 --- a/acme/api/order_test.go +++ b/acme/api/order_test.go @@ -474,7 +474,7 @@ func TestHandler_newAuthorization(t *testing.T) { db: &acme.MockDB{ MockCreateChallenge: func(ctx context.Context, ch *acme.Challenge) error { assert.Equals(t, ch.AccountID, az.AccountID) - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) assert.Equals(t, ch.Token, az.Token) assert.Equals(t, ch.Status, acme.StatusPending) assert.Equals(t, ch.Value, az.Identifier.Value) @@ -503,15 +503,15 @@ func TestHandler_newAuthorization(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -557,15 +557,15 @@ func TestHandler_newAuthorization(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -607,7 +607,7 @@ func TestHandler_newAuthorization(t *testing.T) { db: &acme.MockDB{ MockCreateChallenge: func(ctx context.Context, ch *acme.Challenge) error { ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) assert.Equals(t, ch.AccountID, az.AccountID) assert.Equals(t, ch.Token, az.Token) assert.Equals(t, ch.Status, acme.StatusPending) @@ -774,7 +774,7 @@ func TestHandler_NewOrder(t *testing.T) { db: &acme.MockDB{ MockCreateChallenge: func(ctx context.Context, ch *acme.Challenge) error { assert.Equals(t, ch.AccountID, "accID") - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) assert.NotEquals(t, ch.Token, "") assert.Equals(t, ch.Status, acme.StatusPending) assert.Equals(t, ch.Value, "zap.internal") @@ -809,15 +809,15 @@ func TestHandler_NewOrder(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -881,22 +881,22 @@ func TestHandler_NewOrder(t *testing.T) { switch chCount { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) assert.Equals(t, ch.Value, "zap.internal") ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) assert.Equals(t, ch.Value, "zap.internal") ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) assert.Equals(t, ch.Value, "zap.internal") ch3 = &ch case 3: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) assert.Equals(t, ch.Value, "zar.internal") ch4 = &ch default: @@ -921,7 +921,7 @@ func TestHandler_NewOrder(t *testing.T) { az.ID = "az2ID" az2ID = &az.ID assert.Equals(t, az.Identifier, acme.Identifier{ - Type: "dns", + Type: acme.DNS, Value: "zar.internal", }) assert.Equals(t, az.Wildcard, true) @@ -996,15 +996,15 @@ func TestHandler_NewOrder(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -1088,15 +1088,15 @@ func TestHandler_NewOrder(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -1179,15 +1179,15 @@ func TestHandler_NewOrder(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -1271,15 +1271,15 @@ func TestHandler_NewOrder(t *testing.T) { switch count { case 0: ch.ID = "dns" - assert.Equals(t, ch.Type, "dns-01") + assert.Equals(t, ch.Type, acme.DNS01) ch1 = &ch case 1: ch.ID = "http" - assert.Equals(t, ch.Type, "http-01") + assert.Equals(t, ch.Type, acme.HTTP01) ch2 = &ch case 2: ch.ID = "tls" - assert.Equals(t, ch.Type, "tls-alpn-01") + assert.Equals(t, ch.Type, acme.TLSALPN01) ch3 = &ch default: assert.FatalError(t, errors.New("test logic error")) @@ -1668,7 +1668,7 @@ func TestHandler_challengeTypes(t *testing.T) { tests := []struct { name string args args - want []string + want []acme.ChallengeType }{ { name: "ok/dns", @@ -1678,7 +1678,7 @@ func TestHandler_challengeTypes(t *testing.T) { Wildcard: false, }, }, - want: []string{"dns-01", "http-01", "tls-alpn-01"}, + want: []acme.ChallengeType{acme.DNS01, acme.HTTP01, acme.TLSALPN01}, //[]string{"dns-01", "http-01", "tls-alpn-01"}, }, { name: "ok/wildcard", @@ -1688,7 +1688,7 @@ func TestHandler_challengeTypes(t *testing.T) { Wildcard: true, }, }, - want: []string{"dns-01"}, + want: []acme.ChallengeType{acme.DNS01}, }, { name: "ok/ip", @@ -1698,7 +1698,7 @@ func TestHandler_challengeTypes(t *testing.T) { Wildcard: false, }, }, - want: []string{"http-01", "tls-alpn-01"}, + want: []acme.ChallengeType{acme.HTTP01, acme.TLSALPN01}, }, } for _, tt := range tests { diff --git a/acme/challenge.go b/acme/challenge.go index cb15a188..706d12a1 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -21,18 +21,26 @@ import ( "go.step.sm/crypto/jose" ) +type ChallengeType string + +const ( + HTTP01 ChallengeType = "http-01" + DNS01 ChallengeType = "dns-01" + TLSALPN01 ChallengeType = "tls-alpn-01" +) + // Challenge represents an ACME response Challenge type. type Challenge struct { - ID string `json:"-"` - AccountID string `json:"-"` - AuthorizationID string `json:"-"` - Value string `json:"-"` - Type string `json:"type"` - Status Status `json:"status"` - Token string `json:"token"` - ValidatedAt string `json:"validated,omitempty"` - URL string `json:"url"` - Error *Error `json:"error,omitempty"` + ID string `json:"-"` + AccountID string `json:"-"` + AuthorizationID string `json:"-"` + Value string `json:"-"` + Type ChallengeType `json:"type"` + Status Status `json:"status"` + Token string `json:"token"` + ValidatedAt string `json:"validated,omitempty"` + URL string `json:"url"` + Error *Error `json:"error,omitempty"` } // ToLog enables response logging. @@ -54,11 +62,11 @@ func (ch *Challenge) Validate(ctx context.Context, db DB, jwk *jose.JSONWebKey, return nil } switch ch.Type { - case "http-01": + case HTTP01: return http01Validate(ctx, ch, db, jwk, vo) - case "dns-01": + case DNS01: return dns01Validate(ctx, ch, db, jwk, vo) - case "tls-alpn-01": + case TLSALPN01: return tlsalpn01Validate(ctx, ch, db, jwk, vo) default: return NewErrorISE("unexpected challenge type '%s'", ch.Type) diff --git a/acme/db/nosql/challenge.go b/acme/db/nosql/challenge.go index f3a3cfca..f84a6f4e 100644 --- a/acme/db/nosql/challenge.go +++ b/acme/db/nosql/challenge.go @@ -11,15 +11,15 @@ import ( ) type dbChallenge struct { - ID string `json:"id"` - AccountID string `json:"accountID"` - Type string `json:"type"` - Status acme.Status `json:"status"` - Token string `json:"token"` - Value string `json:"value"` - ValidatedAt string `json:"validatedAt"` - CreatedAt time.Time `json:"createdAt"` - Error *acme.Error `json:"error"` + ID string `json:"id"` + AccountID string `json:"accountID"` + Type acme.ChallengeType `json:"type"` + Status acme.Status `json:"status"` + Token string `json:"token"` + Value string `json:"value"` + ValidatedAt string `json:"validatedAt"` + CreatedAt time.Time `json:"createdAt"` + Error *acme.Error `json:"error"` } func (dbc *dbChallenge) clone() *dbChallenge { From f33bdee5e06770fd4b1c1aae0f856789444f1a39 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 18 Jun 2021 12:55:50 +0200 Subject: [PATCH 13/21] Fix linter issue S1025 --- api/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/api.go b/api/api.go index 6a0a7e8f..5cf9f593 100644 --- a/api/api.go +++ b/api/api.go @@ -417,7 +417,7 @@ func LogCertificate(w http.ResponseWriter, cert *x509.Certificate) { if len(val.CredentialID) > 0 { m["provisioner"] = fmt.Sprintf("%s (%s)", val.Name, val.CredentialID) } else { - m["provisioner"] = fmt.Sprintf("%s", val.Name) + m["provisioner"] = string(val.Name) } break } From db416a45aea466a8c7fde7fae4ef71c23e3d4c71 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 8 Jun 2021 18:02:54 -0700 Subject: [PATCH 14/21] Fix path for labeler. --- .github/workflows/labeler.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/labeler.yml b/.github/workflows/labeler.yml index f0011406..72b01a92 100644 --- a/.github/workflows/labeler.yml +++ b/.github/workflows/labeler.yml @@ -11,4 +11,4 @@ jobs: - uses: actions/labeler@v3 with: repo-token: "${{ secrets.GITHUB_TOKEN }}" - configuration-path: .github/needs-triage-labeler.yml + configuration-path: .github/labeler.yml From 218a2adb9fc5778678fff7b323ee4943e2d4d7a7 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 18 Jun 2021 16:09:48 +0200 Subject: [PATCH 15/21] Add tests for IP Order validations --- acme/order.go | 12 +- acme/order_test.go | 375 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 380 insertions(+), 7 deletions(-) diff --git a/acme/order.go b/acme/order.go index 86b3c43a..71884603 100644 --- a/acme/order.go +++ b/acme/order.go @@ -199,15 +199,15 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ var sans []x509util.SubjectAlternativeName // 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)) + orderNames := make([]string, numberOfIdentifierType(DNS, o.Identifiers)) + orderIPs := make([]net.IP, numberOfIdentifierType(IP, o.Identifiers)) indexDNS, indexIP := 0, 0 for _, n := range o.Identifiers { switch n.Type { - case "dns": + case DNS: orderNames[indexDNS] = n.Value indexDNS++ - case "ip": + case IP: orderIPs[indexIP] = net.ParseIP(n.Value) // NOTE: this assumes are all valid IPs at this time; or will result in nil entries indexIP++ default: @@ -303,8 +303,8 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate } // ipsAreEqual compares IPs to be equal. IPv6 representations of IPv4 -// adresses are NOT considered equal to the IPv4 address in this case. -// Both IPs should be the same version AND equal to each other. +// adresses are considered equal to the IPv4 address in this case. +// TODO: is this behavior OK to keep? func ipsAreEqual(x, y net.IP) bool { if matchAddrFamily(x, y) { return x.Equal(y) diff --git a/acme/order_test.go b/acme/order_test.go index c0461dc6..e1d3744b 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -393,6 +393,31 @@ func TestOrder_Finalize(t *testing.T) { "CSR names = %v, Order names = %v", []string{"foo.internal"}, orderNames), } }, + "fail/error-ips-length-mismatch": func(t *testing.T) test { + now := clock.Now() + o := &Order{ + ID: "oID", + AccountID: "accID", + Status: StatusReady, + ExpiresAt: now.Add(5 * time.Minute), + AuthorizationIDs: []string{"a", "b"}, + Identifiers: []Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + } + orderIPs := []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")} + csr := &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, + } + + return test{ + o: o, + csr: csr, + err: NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", []net.IP{net.ParseIP("192.168.42.42")}, orderIPs), + } + }, "fail/error-names-mismatch": func(t *testing.T) test { now := clock.Now() o := &Order{ @@ -421,6 +446,31 @@ func TestOrder_Finalize(t *testing.T) { "CSR names = %v, Order names = %v", []string{"foo.internal", "zap.internal"}, orderNames), } }, + "fail/error-ips-mismatch": func(t *testing.T) test { + now := clock.Now() + o := &Order{ + ID: "oID", + AccountID: "accID", + Status: StatusReady, + ExpiresAt: now.Add(5 * time.Minute), + AuthorizationIDs: []string{"a", "b"}, + Identifiers: []Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + } + orderIPs := []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")} + csr := &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.32")}, + } + + return test{ + o: o, + csr: csr, + err: NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", []net.IP{net.ParseIP("192.168.42.32"), net.ParseIP("192.168.42.42")}, orderIPs), + } + }, "fail/error-provisioner-auth": func(t *testing.T) test { now := clock.Now() o := &Order{ @@ -652,7 +702,7 @@ func TestOrder_Finalize(t *testing.T) { err: NewErrorISE("error updating order oID: force"), } }, - "ok/new-cert": func(t *testing.T) test { + "ok/new-cert-dns": func(t *testing.T) test { now := clock.Now() o := &Order{ ID: "oID", @@ -676,6 +726,131 @@ func TestOrder_Finalize(t *testing.T) { bar := &x509.Certificate{Subject: pkix.Name{CommonName: "bar"}} baz := &x509.Certificate{Subject: pkix.Name{CommonName: "baz"}} + return test{ + o: o, + csr: csr, + prov: &MockProvisioner{ + MauthorizeSign: func(ctx context.Context, token string) ([]provisioner.SignOption, error) { + assert.Equals(t, token, "") + return nil, nil + }, + MgetOptions: func() *provisioner.Options { + return nil + }, + }, + ca: &mockSignAuth{ + sign: func(_csr *x509.CertificateRequest, signOpts provisioner.SignOptions, extraOpts ...provisioner.SignOption) ([]*x509.Certificate, error) { + assert.Equals(t, _csr, csr) + return []*x509.Certificate{foo, bar, baz}, nil + }, + }, + db: &MockDB{ + MockCreateCertificate: func(ctx context.Context, cert *Certificate) error { + cert.ID = "certID" + assert.Equals(t, cert.AccountID, o.AccountID) + assert.Equals(t, cert.OrderID, o.ID) + assert.Equals(t, cert.Leaf, foo) + assert.Equals(t, cert.Intermediates, []*x509.Certificate{bar, baz}) + return nil + }, + MockUpdateOrder: func(ctx context.Context, updo *Order) error { + assert.Equals(t, updo.CertificateID, "certID") + assert.Equals(t, updo.Status, StatusValid) + assert.Equals(t, updo.ID, o.ID) + assert.Equals(t, updo.AccountID, o.AccountID) + assert.Equals(t, updo.ExpiresAt, o.ExpiresAt) + assert.Equals(t, updo.AuthorizationIDs, o.AuthorizationIDs) + assert.Equals(t, updo.Identifiers, o.Identifiers) + return nil + }, + }, + } + }, + "ok/new-cert-ip": func(t *testing.T) test { + now := clock.Now() + o := &Order{ + ID: "oID", + AccountID: "accID", + Status: StatusReady, + ExpiresAt: now.Add(5 * time.Minute), + AuthorizationIDs: []string{"a", "b"}, + Identifiers: []Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + } + csr := &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, // in case of IPs, no Common Name + } + + foo := &x509.Certificate{Subject: pkix.Name{CommonName: "foo"}} + bar := &x509.Certificate{Subject: pkix.Name{CommonName: "bar"}} + baz := &x509.Certificate{Subject: pkix.Name{CommonName: "baz"}} + + return test{ + o: o, + csr: csr, + prov: &MockProvisioner{ + MauthorizeSign: func(ctx context.Context, token string) ([]provisioner.SignOption, error) { + assert.Equals(t, token, "") + return nil, nil + }, + MgetOptions: func() *provisioner.Options { + return nil + }, + }, + ca: &mockSignAuth{ + sign: func(_csr *x509.CertificateRequest, signOpts provisioner.SignOptions, extraOpts ...provisioner.SignOption) ([]*x509.Certificate, error) { + assert.Equals(t, _csr, csr) + return []*x509.Certificate{foo, bar, baz}, nil + }, + }, + db: &MockDB{ + MockCreateCertificate: func(ctx context.Context, cert *Certificate) error { + cert.ID = "certID" + assert.Equals(t, cert.AccountID, o.AccountID) + assert.Equals(t, cert.OrderID, o.ID) + assert.Equals(t, cert.Leaf, foo) + assert.Equals(t, cert.Intermediates, []*x509.Certificate{bar, baz}) + return nil + }, + MockUpdateOrder: func(ctx context.Context, updo *Order) error { + assert.Equals(t, updo.CertificateID, "certID") + assert.Equals(t, updo.Status, StatusValid) + assert.Equals(t, updo.ID, o.ID) + assert.Equals(t, updo.AccountID, o.AccountID) + assert.Equals(t, updo.ExpiresAt, o.ExpiresAt) + assert.Equals(t, updo.AuthorizationIDs, o.AuthorizationIDs) + assert.Equals(t, updo.Identifiers, o.Identifiers) + return nil + }, + }, + } + }, + "ok/new-cert-dns-and-ip": func(t *testing.T) test { + now := clock.Now() + o := &Order{ + ID: "oID", + AccountID: "accID", + Status: StatusReady, + ExpiresAt: now.Add(5 * time.Minute), + AuthorizationIDs: []string{"a", "b"}, + Identifiers: []Identifier{ + {Type: "dns", Value: "foo.internal"}, + {Type: "ip", Value: "192.168.42.42"}, + }, + } + csr := &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "foo.internal", + }, + IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, + } + + foo := &x509.Certificate{Subject: pkix.Name{CommonName: "foo"}} + bar := &x509.Certificate{Subject: pkix.Name{CommonName: "bar"}} + baz := &x509.Certificate{Subject: pkix.Name{CommonName: "baz"}} + return test{ o: o, csr: csr, @@ -814,3 +989,201 @@ func Test_uniqueSortedIPs(t *testing.T) { }) } } + +func Test_numberOfIdentifierType(t *testing.T) { + type args struct { + typ IdentifierType + ids []Identifier + } + tests := []struct { + name string + args args + want int + }{ + { + name: "ok/no-identifiers", + args: args{ + typ: DNS, + ids: []Identifier{}, + }, + want: 0, + }, + { + name: "ok/no-dns", + args: args{ + typ: DNS, + ids: []Identifier{ + { + Type: IP, + Value: "192.168.42.42", + }, + }, + }, + want: 0, + }, + { + name: "ok/no-ips", + args: args{ + typ: IP, + ids: []Identifier{ + { + Type: DNS, + Value: "example.com", + }, + }, + }, + want: 0, + }, + { + name: "ok/one-dns", + args: args{ + typ: DNS, + ids: []Identifier{ + { + Type: DNS, + Value: "example.com", + }, + { + Type: IP, + Value: "192.168.42.42", + }, + }, + }, + want: 1, + }, + { + name: "ok/one-ip", + args: args{ + typ: IP, + ids: []Identifier{ + { + Type: DNS, + Value: "example.com", + }, + { + Type: IP, + Value: "192.168.42.42", + }, + }, + }, + want: 1, + }, + { + name: "ok/more-dns", + args: args{ + typ: DNS, + ids: []Identifier{ + { + Type: DNS, + Value: "example.com", + }, + { + Type: DNS, + Value: "*.example.com", + }, + { + Type: IP, + Value: "192.168.42.42", + }, + }, + }, + want: 2, + }, + { + name: "ok/more-ips", + args: args{ + typ: IP, + ids: []Identifier{ + { + Type: DNS, + Value: "example.com", + }, + { + Type: IP, + Value: "192.168.42.42", + }, + { + Type: IP, + Value: "192.168.42.43", + }, + }, + }, + want: 2, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := numberOfIdentifierType(tt.args.typ, tt.args.ids); got != tt.want { + t.Errorf("numberOfIdentifierType() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_ipsAreEqual(t *testing.T) { + type args struct { + x net.IP + y net.IP + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "ok/ipv4", + args: args{ + x: net.ParseIP("192.168.42.42"), + y: net.ParseIP("192.168.42.42"), + }, + want: true, + }, + { + name: "ok/ipv6", + args: args{ + x: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), + y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), + }, + want: true, + }, + { + name: "ok/ipv4-and-ipv6", + args: args{ + x: net.ParseIP("192.168.42.42"), + y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), + }, + want: false, + }, + { + name: "ok/ipv4-mapped-to-ipv6", + args: args{ + x: net.ParseIP("192.168.42.42"), + y: net.ParseIP("::ffff:192.168.42.42"), // parsed to the same IPv4 by Go + }, + want: true, // TODO: is this behavior OK? + }, + { + name: "ok/invalid-ipv4-and-valid-ipv6", + args: args{ + x: net.ParseIP("192.168.42.1000"), + y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), + }, + want: false, + }, + { + name: "ok/invalid-ipv4-and-invalid-ipv6", + args: args{ + x: net.ParseIP("192.168.42.1000"), + y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:1000000"), + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ipsAreEqual(tt.args.x, tt.args.y); got != tt.want { + t.Errorf("ipsAreEqual() = %v, want %v", got, tt.want) + } + }) + } +} From 135e912ac8fe118fe1cf22dd49777b2813b9e7ba Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 18 Jun 2021 17:27:35 +0200 Subject: [PATCH 16/21] Improve coverage for TLS-ALPN-01 challenge --- acme/challenge.go | 31 ++++++----- acme/challenge_test.go | 117 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 14 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 706d12a1..fe643c85 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -115,25 +115,13 @@ func http01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWeb } func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebKey, vo *ValidateChallengeOptions) error { - - // RFC8738 states that, if HostName is IP, it should be the ARPA - // address https://datatracker.ietf.org/doc/html/rfc8738#section-6. - // It also references TLS Extensions [RFC6066]. - var serverName string - ip := net.ParseIP(ch.Value) - if ip != nil { - serverName = reverseAddr(ip) - } else { - serverName = ch.Value - } - config := &tls.Config{ NextProtos: []string{"acme-tls/1"}, // https://tools.ietf.org/html/rfc8737#section-4 // ACME servers that implement "acme-tls/1" MUST only negotiate TLS 1.2 // [RFC5246] or higher when connecting to clients for validation. MinVersion: tls.VersionTLS12, - ServerName: serverName, + ServerName: serverName(ch), InsecureSkipVerify: true, // we expect a self-signed challenge certificate } @@ -163,7 +151,7 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON // if no DNS names present, look for IP address and verify that exactly one exists if len(leafCert.DNSNames) == 0 { - if len(leafCert.IPAddresses) != 1 || !leafCert.IPAddresses[0].Equal(ip) { + if len(leafCert.IPAddresses) != 1 || !leafCert.IPAddresses[0].Equal(net.ParseIP(ch.Value)) { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single IP address or DNS name, %v", ch.Value)) } @@ -272,6 +260,21 @@ func dns01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebK return nil } +// serverName determines the SNI HostName to set based on an acme.Challenge +// for TLS-ALPN-01 challenges. RFC8738 states that, if HostName is an IP, it +// should be the ARPA address https://datatracker.ietf.org/doc/html/rfc8738#section-6. +// It also references TLS Extensions [RFC6066]. +func serverName(ch *Challenge) string { + var serverName string + ip := net.ParseIP(ch.Value) + if ip != nil { + serverName = reverseAddr(ip) + } else { + serverName = ch.Value + } + return serverName +} + // reverseaddr returns the in-addr.arpa. or ip6.arpa. hostname of the IP // address addr suitable for rDNS (PTR) record lookup or an error if it fails // to parse the IP address. diff --git a/acme/challenge_test.go b/acme/challenge_test.go index 8e03a414..727e9ef3 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -2187,6 +2187,43 @@ func TestTLSALPN01Validate(t *testing.T) { srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() + return test{ + ch: ch, + vo: &ValidateChallengeOptions{ + TLSDial: tlsDial, + }, + db: &MockDB{ + MockUpdateChallenge: func(ctx context.Context, updch *Challenge) error { + assert.Equals(t, updch.ID, ch.ID) + assert.Equals(t, updch.Token, ch.Token) + assert.Equals(t, updch.Status, StatusValid) + assert.Equals(t, updch.Type, ch.Type) + assert.Equals(t, updch.Value, ch.Value) + assert.Equals(t, updch.Error, nil) + return nil + }, + }, + srv: srv, + jwk: jwk, + } + }, + "ok/ip": func(t *testing.T) test { + ch := makeTLSCh() + ch.Value = "127.0.0.1" + + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) + + expKeyAuth, err := KeyAuthorization(ch.Token, jwk) + assert.FatalError(t, err) + expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) + + cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], false, true, ch.Value) + assert.FatalError(t, err) + + srv, tlsDial := newTestTLSALPNServer(cert) + srv.Start() + return test{ ch: ch, vo: &ValidateChallengeOptions{ @@ -2234,4 +2271,84 @@ func TestTLSALPN01Validate(t *testing.T) { } }) } + t.Fail() +} + +func Test_reverseAddr(t *testing.T) { + type args struct { + ip net.IP + } + tests := []struct { + name string + args args + wantArpa string + }{ + { + name: "ok/ipv4", + args: args{ + ip: net.ParseIP("127.0.0.1"), + }, + wantArpa: "1.0.0.127.in-addr.arpa.", + }, + { + name: "ok/ipv6", + args: args{ + ip: net.ParseIP("2001:db8::567:89ab"), + }, + wantArpa: "b.a.9.8.7.6.5.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if gotArpa := reverseAddr(tt.args.ip); gotArpa != tt.wantArpa { + t.Errorf("reverseAddr() = %v, want %v", gotArpa, tt.wantArpa) + } + }) + } +} + +func Test_serverName(t *testing.T) { + type args struct { + ch *Challenge + } + tests := []struct { + name string + args args + want string + }{ + { + name: "ok/dns", + args: args{ + ch: &Challenge{ + Value: "example.com", + }, + }, + want: "example.com", + }, + { + name: "ok/ipv4", + args: args{ + ch: &Challenge{ + Value: "127.0.0.1", + }, + }, + want: "1.0.0.127.in-addr.arpa.", + }, + { + name: "ok/ipv4", + args: args{ + ch: &Challenge{ + Value: "2001:db8::567:89ab", + }, + }, + want: "b.a.9.8.7.6.5.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := serverName(tt.args.ch); got != tt.want { + t.Errorf("serverName() = %v, want %v", got, tt.want) + } + }) + } } From c514a187b287abb3ad94610c671a42b25f78696c Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 18 Jun 2021 17:37:56 +0200 Subject: [PATCH 17/21] Fix Fail() -_-b --- acme/challenge_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/acme/challenge_test.go b/acme/challenge_test.go index 727e9ef3..423951e8 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -2271,7 +2271,6 @@ func TestTLSALPN01Validate(t *testing.T) { } }) } - t.Fail() } func Test_reverseAddr(t *testing.T) { From 64c15fde7eb1358546ecf4f9692042ae65226424 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 25 Jun 2021 14:07:40 +0200 Subject: [PATCH 18/21] Add tests for canonicalize function --- acme/api/order_test.go | 3 +- acme/challenge.go | 2 +- acme/challenge_test.go | 2 +- acme/order.go | 3 - acme/order_test.go | 153 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 156 insertions(+), 7 deletions(-) diff --git a/acme/api/order_test.go b/acme/api/order_test.go index 38c4c3f0..afb23c3f 100644 --- a/acme/api/order_test.go +++ b/acme/api/order_test.go @@ -1678,7 +1678,7 @@ func TestHandler_challengeTypes(t *testing.T) { Wildcard: false, }, }, - want: []acme.ChallengeType{acme.DNS01, acme.HTTP01, acme.TLSALPN01}, //[]string{"dns-01", "http-01", "tls-alpn-01"}, + want: []acme.ChallengeType{acme.DNS01, acme.HTTP01, acme.TLSALPN01}, }, { name: "ok/wildcard", @@ -1703,7 +1703,6 @@ func TestHandler_challengeTypes(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := challengeTypes(tt.args.az); !reflect.DeepEqual(got, tt.want) { t.Errorf("Handler.challengeTypes() = %v, want %v", got, tt.want) } diff --git a/acme/challenge.go b/acme/challenge.go index fe643c85..1d5f0ec9 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -261,7 +261,7 @@ func dns01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebK } // serverName determines the SNI HostName to set based on an acme.Challenge -// for TLS-ALPN-01 challenges. RFC8738 states that, if HostName is an IP, it +// for TLS-ALPN-01 challenges RFC8738 states that, if HostName is an IP, it // should be the ARPA address https://datatracker.ietf.org/doc/html/rfc8738#section-6. // It also references TLS Extensions [RFC6066]. func serverName(ch *Challenge) string { diff --git a/acme/challenge_test.go b/acme/challenge_test.go index 423951e8..bb9a2507 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -2334,7 +2334,7 @@ func Test_serverName(t *testing.T) { want: "1.0.0.127.in-addr.arpa.", }, { - name: "ok/ipv4", + name: "ok/ipv6", args: args{ ch: &Challenge{ Value: "2001:db8::567:89ab", diff --git a/acme/order.go b/acme/order.go index 71884603..2a869e78 100644 --- a/acme/order.go +++ b/acme/order.go @@ -221,9 +221,6 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ sans = make([]x509util.SubjectAlternativeName, totalNumberOfSANs) index := 0 - // TODO: limit what IP addresses can be used? Only private? Only certain ranges (i.e. only allow the specific ranges by default, configuration for all?) - // TODO: can DNS already be limited to a certain domain? That would probably be nice to have too, but maybe not as part of this PR - // TODO: if it seems not too big of a change, make consts/enums out of the stringly typed identifiers (challenge types, identifier types) // TODO: only allow IP based identifier based on configuration? Some additional configuration and validation on the provisioner for this case. // Validate identifier names against CSR alternative names. diff --git a/acme/order_test.go b/acme/order_test.go index e1d3744b..67d01f6d 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -13,6 +13,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/assert" "github.com/smallstep/certificates/authority/provisioner" + "go.step.sm/crypto/x509util" ) func TestOrder_UpdateStatus(t *testing.T) { @@ -1187,3 +1188,155 @@ func Test_ipsAreEqual(t *testing.T) { }) } } + +func Test_canonicalize(t *testing.T) { + type args struct { + csr *x509.CertificateRequest + } + tests := []struct { + name string + args args + wantCanonicalized *x509.CertificateRequest + }{ + { + name: "ok/dns", + args: args{ + csr: &x509.CertificateRequest{ + DNSNames: []string{"www.example.com", "example.com"}, + }, + }, + wantCanonicalized: &x509.CertificateRequest{ + DNSNames: []string{"example.com", "www.example.com"}, + IPAddresses: []net.IP{}, + }, + }, + { + name: "ok/common-name", + args: args{ + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "example.com", + }, + DNSNames: []string{"www.example.com"}, + }, + }, + wantCanonicalized: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "example.com", + }, + DNSNames: []string{"example.com", "www.example.com"}, + IPAddresses: []net.IP{}, + }, + }, + { + name: "ok/ipv4", + args: args{ + csr: &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + }, + }, + wantCanonicalized: &x509.CertificateRequest{ + DNSNames: []string{}, + IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, + }, + }, + { + name: "ok/mixed", + args: args{ + csr: &x509.CertificateRequest{ + DNSNames: []string{"www.example.com", "example.com"}, + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + }, + }, + wantCanonicalized: &x509.CertificateRequest{ + DNSNames: []string{"example.com", "www.example.com"}, + IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}, + }, + }, + { + name: "ok/mixed-common-name", + args: args{ + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "example.com", + }, + DNSNames: []string{"www.example.com"}, + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + }, + }, + wantCanonicalized: &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")}, + }, + }, + } + 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) + } + }) + } +} + +func TestOrder_sans(t *testing.T) { + type fields struct { + ID string + AccountID string + ProvisionerID string + Status Status + ExpiresAt time.Time + Identifiers []Identifier + NotBefore time.Time + NotAfter time.Time + Error *Error + AuthorizationIDs []string + AuthorizationURLs []string + FinalizeURL string + CertificateID string + CertificateURL string + } + type args struct { + csr *x509.CertificateRequest + } + tests := []struct { + name string + fields fields + args args + want []x509util.SubjectAlternativeName + wantErr bool + }{ + // TODO: Add test cases. + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := &Order{ + ID: tt.fields.ID, + AccountID: tt.fields.AccountID, + ProvisionerID: tt.fields.ProvisionerID, + Status: tt.fields.Status, + ExpiresAt: tt.fields.ExpiresAt, + Identifiers: tt.fields.Identifiers, + NotBefore: tt.fields.NotBefore, + NotAfter: tt.fields.NotAfter, + Error: tt.fields.Error, + AuthorizationIDs: tt.fields.AuthorizationIDs, + AuthorizationURLs: tt.fields.AuthorizationURLs, + FinalizeURL: tt.fields.FinalizeURL, + CertificateID: tt.fields.CertificateID, + CertificateURL: tt.fields.CertificateURL, + } + got, err := o.sans(tt.args.csr) + if (err != nil) != tt.wantErr { + t.Errorf("Order.sans() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Order.sans() = %v, want %v", got, tt.want) + } + }) + } +} From a6d33b7d0656b004291f73524df8447fda40004f Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 25 Jun 2021 17:21:22 +0200 Subject: [PATCH 19/21] Add tests for sans() --- acme/order_test.go | 189 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 148 insertions(+), 41 deletions(-) diff --git a/acme/order_test.go b/acme/order_test.go index 67d01f6d..c0682fee 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -1284,54 +1284,161 @@ func Test_canonicalize(t *testing.T) { func TestOrder_sans(t *testing.T) { type fields struct { - ID string - AccountID string - ProvisionerID string - Status Status - ExpiresAt time.Time - Identifiers []Identifier - NotBefore time.Time - NotAfter time.Time - Error *Error - AuthorizationIDs []string - AuthorizationURLs []string - FinalizeURL string - CertificateID string - CertificateURL string - } - type args struct { - csr *x509.CertificateRequest + Identifiers []Identifier } tests := []struct { - name string - fields fields - args args - want []x509util.SubjectAlternativeName - wantErr bool + name string + fields fields + csr *x509.CertificateRequest + want []x509util.SubjectAlternativeName + err error }{ - // TODO: Add test cases. + { + name: "ok/dns", + fields: fields{ + Identifiers: []Identifier{ + {Type: "dns", Value: "example.com"}, + }, + }, + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "example.com", + }, + }, + want: []x509util.SubjectAlternativeName{ + {Type: "dns", Value: "example.com"}, + }, + err: nil, + }, + { + name: "fail/error-names-length-mismatch", + fields: fields{ + Identifiers: []Identifier{ + {Type: "dns", Value: "foo.internal"}, + {Type: "dns", Value: "bar.internal"}, + }, + }, + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "foo.internal", + }, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorBadCSRType, "..."), + }, + { + name: "fail/error-names-mismatch", + fields: fields{ + Identifiers: []Identifier{ + {Type: "dns", Value: "foo.internal"}, + {Type: "dns", Value: "bar.internal"}, + }, + }, + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "foo.internal", + }, + DNSNames: []string{"zap.internal"}, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorBadCSRType, "..."), + }, + { + name: "ok/ipv4", + fields: fields{ + Identifiers: []Identifier{ + {Type: "ip", Value: "192.168.43.42"}, + {Type: "ip", Value: "192.168.42.42"}, + }, + }, + csr: &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + }, + want: []x509util.SubjectAlternativeName{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + err: nil, + }, + { + name: "fail/error-ips-length-mismatch", + fields: fields{ + Identifiers: []Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + }, + csr: &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorBadCSRType, "..."), + }, + { + name: "fail/error-ips-mismatch", + fields: fields{ + Identifiers: []Identifier{ + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + }, + csr: &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.32")}, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorBadCSRType, "..."), + }, + { + name: "ok/mixed", + fields: fields{ + Identifiers: []Identifier{ + {Type: "dns", Value: "foo.internal"}, + {Type: "dns", Value: "bar.internal"}, + {Type: "ip", Value: "192.168.43.42"}, + {Type: "ip", Value: "192.168.42.42"}, + }, + }, + csr: &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "bar.internal", + }, + DNSNames: []string{"foo.internal"}, + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + }, + want: []x509util.SubjectAlternativeName{ + {Type: "dns", Value: "bar.internal"}, + {Type: "dns", Value: "foo.internal"}, + {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "192.168.43.42"}, + }, + err: nil, + }, + { + name: "fail/unsupported-identifier-type", + fields: fields{ + Identifiers: []Identifier{ + {Type: "ipv4", Value: "192.168.42.42"}, + }, + }, + csr: &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, + }, + want: []x509util.SubjectAlternativeName{}, + err: NewError(ErrorServerInternalType, "..."), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { o := &Order{ - ID: tt.fields.ID, - AccountID: tt.fields.AccountID, - ProvisionerID: tt.fields.ProvisionerID, - Status: tt.fields.Status, - ExpiresAt: tt.fields.ExpiresAt, - Identifiers: tt.fields.Identifiers, - NotBefore: tt.fields.NotBefore, - NotAfter: tt.fields.NotAfter, - Error: tt.fields.Error, - AuthorizationIDs: tt.fields.AuthorizationIDs, - AuthorizationURLs: tt.fields.AuthorizationURLs, - FinalizeURL: tt.fields.FinalizeURL, - CertificateID: tt.fields.CertificateID, - CertificateURL: tt.fields.CertificateURL, - } - got, err := o.sans(tt.args.csr) - if (err != nil) != tt.wantErr { - t.Errorf("Order.sans() error = %v, wantErr %v", err, tt.wantErr) + Identifiers: tt.fields.Identifiers, + } + canonicalizedCSR := canonicalize(tt.csr) + got, err := o.sans(canonicalizedCSR) + if err != nil && tt.err != nil { + if tt.err.Error() != err.Error() { + t.Errorf("Order.sans() error = %v, wantErr %v", err, tt.err) + return + } return } if !reflect.DeepEqual(got, tt.want) { From 87b72afa25b88f74a4fc9847f76eae4d2e988250 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 26 Jun 2021 00:13:44 +0200 Subject: [PATCH 20/21] Fix IP equality check and add more tests --- acme/order.go | 21 +++++++++------------ acme/order_test.go | 32 ++++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/acme/order.go b/acme/order.go index 2a869e78..9c823f78 100644 --- a/acme/order.go +++ b/acme/order.go @@ -299,20 +299,17 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate return canonicalized } -// ipsAreEqual compares IPs to be equal. IPv6 representations of IPv4 -// adresses are considered equal to the IPv4 address in this case. -// TODO: is this behavior OK to keep? +// ipsAreEqual compares IPs to be equal. Nil values (i.e. invalid IPs) are +// not considered equal. IPv6 representations of IPv4 addresses are +// considered equal to the IPv4 address in this implementation, which is +// standard Go behavior. An example is "::ffff:192.168.42.42", which +// is equal to "192.168.42.42". This is considered a known issue within +// step and is tracked here too: https://github.com/golang/go/issues/37921. func ipsAreEqual(x, y net.IP) bool { - if matchAddrFamily(x, y) { - return x.Equal(y) + if x == nil || y == nil { + return false } - return false -} - -// matchAddrFamily returns true if two IPs are both IPv4 OR IPv6 -// Implementation taken and adapted from https://golang.org/src/net/ip.go -func matchAddrFamily(x net.IP, y net.IP) bool { - return x.To4() != nil && y.To4() != nil || x.To16() != nil && x.To4() == nil && y.To16() != nil && y.To4() == nil + return x.Equal(y) } // uniqueSortedLowerNames returns the set of all unique names in the input after all diff --git a/acme/order_test.go b/acme/order_test.go index c0682fee..b9609cf9 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -1139,6 +1139,14 @@ func Test_ipsAreEqual(t *testing.T) { }, want: true, }, + { + name: "fail/ipv4", + args: args{ + x: net.ParseIP("192.168.42.42"), + y: net.ParseIP("192.168.42.43"), + }, + want: false, + }, { name: "ok/ipv6", args: args{ @@ -1148,7 +1156,15 @@ func Test_ipsAreEqual(t *testing.T) { want: true, }, { - name: "ok/ipv4-and-ipv6", + name: "fail/ipv6", + args: args{ + x: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), + y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7335"), + }, + want: false, + }, + { + name: "fail/ipv4-and-ipv6", args: args{ x: net.ParseIP("192.168.42.42"), y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), @@ -1161,10 +1177,10 @@ func Test_ipsAreEqual(t *testing.T) { x: net.ParseIP("192.168.42.42"), y: net.ParseIP("::ffff:192.168.42.42"), // parsed to the same IPv4 by Go }, - want: true, // TODO: is this behavior OK? + want: true, // we expect this to happen; a known issue in which ipv4 mapped ipv6 addresses are considered the same as their ipv4 counterpart }, { - name: "ok/invalid-ipv4-and-valid-ipv6", + name: "fail/invalid-ipv4-and-valid-ipv6", args: args{ x: net.ParseIP("192.168.42.1000"), y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334"), @@ -1172,7 +1188,15 @@ func Test_ipsAreEqual(t *testing.T) { want: false, }, { - name: "ok/invalid-ipv4-and-invalid-ipv6", + name: "fail/valid-ipv4-and-invalid-ipv6", + args: args{ + x: net.ParseIP("192.168.42.42"), + y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:733400"), + }, + want: false, + }, + { + name: "fail/invalid-ipv4-and-invalid-ipv6", args: args{ x: net.ParseIP("192.168.42.1000"), y: net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:1000000"), From 8e4a4ecc1f79e4ccfecdbf4528d1599e483afda7 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 26 Jun 2021 00:48:40 +0200 Subject: [PATCH 21/21] Refactor tests for sans --- acme/order.go | 2 - acme/order_test.go | 158 ++++++++++++--------------------------------- 2 files changed, 43 insertions(+), 117 deletions(-) diff --git a/acme/order.go b/acme/order.go index 9c823f78..fd8956f7 100644 --- a/acme/order.go +++ b/acme/order.go @@ -221,8 +221,6 @@ func (o *Order) sans(csr *x509.CertificateRequest) ([]x509util.SubjectAlternativ sans = make([]x509util.SubjectAlternativeName, totalNumberOfSANs) index := 0 - // TODO: only allow IP based identifier based on configuration? Some additional configuration and validation on the provisioner for this case. - // Validate identifier names against CSR alternative names. // // Note that with certificate templates we are not going to check for the diff --git a/acme/order_test.go b/acme/order_test.go index b9609cf9..b9a43e5d 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -367,111 +367,6 @@ func TestOrder_Finalize(t *testing.T) { err: NewErrorISE("unrecognized order status: %s", o.Status), } }, - "fail/error-names-length-mismatch": func(t *testing.T) test { - now := clock.Now() - o := &Order{ - ID: "oID", - AccountID: "accID", - Status: StatusReady, - ExpiresAt: now.Add(5 * time.Minute), - AuthorizationIDs: []string{"a", "b"}, - Identifiers: []Identifier{ - {Type: "dns", Value: "foo.internal"}, - {Type: "dns", Value: "bar.internal"}, - }, - } - orderNames := []string{"bar.internal", "foo.internal"} - csr := &x509.CertificateRequest{ - Subject: pkix.Name{ - CommonName: "foo.internal", - }, - } - - return test{ - o: o, - csr: csr, - err: NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", []string{"foo.internal"}, orderNames), - } - }, - "fail/error-ips-length-mismatch": func(t *testing.T) test { - now := clock.Now() - o := &Order{ - ID: "oID", - AccountID: "accID", - Status: StatusReady, - ExpiresAt: now.Add(5 * time.Minute), - AuthorizationIDs: []string{"a", "b"}, - Identifiers: []Identifier{ - {Type: "ip", Value: "192.168.42.42"}, - {Type: "ip", Value: "192.168.43.42"}, - }, - } - orderIPs := []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")} - csr := &x509.CertificateRequest{ - IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, - } - - return test{ - o: o, - csr: csr, - err: NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ - "CSR IPs = %v, Order IPs = %v", []net.IP{net.ParseIP("192.168.42.42")}, orderIPs), - } - }, - "fail/error-names-mismatch": func(t *testing.T) test { - now := clock.Now() - o := &Order{ - ID: "oID", - AccountID: "accID", - Status: StatusReady, - ExpiresAt: now.Add(5 * time.Minute), - AuthorizationIDs: []string{"a", "b"}, - Identifiers: []Identifier{ - {Type: "dns", Value: "foo.internal"}, - {Type: "dns", Value: "bar.internal"}, - }, - } - orderNames := []string{"bar.internal", "foo.internal"} - csr := &x509.CertificateRequest{ - Subject: pkix.Name{ - CommonName: "foo.internal", - }, - DNSNames: []string{"zap.internal"}, - } - - return test{ - o: o, - csr: csr, - err: NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ - "CSR names = %v, Order names = %v", []string{"foo.internal", "zap.internal"}, orderNames), - } - }, - "fail/error-ips-mismatch": func(t *testing.T) test { - now := clock.Now() - o := &Order{ - ID: "oID", - AccountID: "accID", - Status: StatusReady, - ExpiresAt: now.Add(5 * time.Minute), - AuthorizationIDs: []string{"a", "b"}, - Identifiers: []Identifier{ - {Type: "ip", Value: "192.168.42.42"}, - {Type: "ip", Value: "192.168.43.42"}, - }, - } - orderIPs := []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")} - csr := &x509.CertificateRequest{ - IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.32")}, - } - - return test{ - o: o, - csr: csr, - err: NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ - "CSR IPs = %v, Order IPs = %v", []net.IP{net.ParseIP("192.168.42.32"), net.ParseIP("192.168.42.42")}, orderIPs), - } - }, "fail/error-provisioner-auth": func(t *testing.T) test { now := clock.Now() o := &Order{ @@ -1315,7 +1210,7 @@ func TestOrder_sans(t *testing.T) { fields fields csr *x509.CertificateRequest want []x509util.SubjectAlternativeName - err error + err *Error }{ { name: "ok/dns", @@ -1348,7 +1243,8 @@ func TestOrder_sans(t *testing.T) { }, }, want: []x509util.SubjectAlternativeName{}, - err: NewError(ErrorBadCSRType, "..."), + err: NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", []string{"foo.internal"}, []string{"bar.internal", "foo.internal"}), }, { name: "fail/error-names-mismatch", @@ -1365,7 +1261,8 @@ func TestOrder_sans(t *testing.T) { DNSNames: []string{"zap.internal"}, }, want: []x509util.SubjectAlternativeName{}, - err: NewError(ErrorBadCSRType, "..."), + err: NewError(ErrorBadCSRType, "CSR names do not match identifiers exactly: "+ + "CSR names = %v, Order names = %v", []string{"foo.internal", "zap.internal"}, []string{"bar.internal", "foo.internal"}), }, { name: "ok/ipv4", @@ -1384,6 +1281,23 @@ func TestOrder_sans(t *testing.T) { }, err: nil, }, + { + name: "ok/ipv6", + fields: fields{ + Identifiers: []Identifier{ + {Type: "ip", Value: "2001:0db8:85a3::8a2e:0370:7335"}, + {Type: "ip", Value: "2001:0db8:85a3::8a2e:0370:7334"}, + }, + }, + csr: &x509.CertificateRequest{ + IPAddresses: []net.IP{net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7335"), net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334")}, + }, + want: []x509util.SubjectAlternativeName{ + {Type: "ip", Value: "2001:db8:85a3::8a2e:370:7334"}, + {Type: "ip", Value: "2001:db8:85a3::8a2e:370:7335"}, + }, + err: nil, + }, { name: "fail/error-ips-length-mismatch", fields: fields{ @@ -1396,7 +1310,8 @@ func TestOrder_sans(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, }, want: []x509util.SubjectAlternativeName{}, - err: NewError(ErrorBadCSRType, "..."), + err: NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", []net.IP{net.ParseIP("192.168.42.42")}, []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}), }, { name: "fail/error-ips-mismatch", @@ -1410,7 +1325,8 @@ func TestOrder_sans(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.42.32")}, }, want: []x509util.SubjectAlternativeName{}, - err: NewError(ErrorBadCSRType, "..."), + err: NewError(ErrorBadCSRType, "CSR IPs do not match identifiers exactly: "+ + "CSR IPs = %v, Order IPs = %v", []net.IP{net.ParseIP("192.168.42.32"), net.ParseIP("192.168.42.42")}, []net.IP{net.ParseIP("192.168.42.42"), net.ParseIP("192.168.43.42")}), }, { name: "ok/mixed", @@ -1420,6 +1336,7 @@ func TestOrder_sans(t *testing.T) { {Type: "dns", Value: "bar.internal"}, {Type: "ip", Value: "192.168.43.42"}, {Type: "ip", Value: "192.168.42.42"}, + {Type: "ip", Value: "2001:0db8:85a3:0000:0000:8a2e:0370:7334"}, }, }, csr: &x509.CertificateRequest{ @@ -1427,13 +1344,14 @@ func TestOrder_sans(t *testing.T) { CommonName: "bar.internal", }, DNSNames: []string{"foo.internal"}, - IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42")}, + IPAddresses: []net.IP{net.ParseIP("192.168.43.42"), net.ParseIP("192.168.42.42"), net.ParseIP("2001:0db8:85a3:0000:0000:8a2e:0370:7334")}, }, want: []x509util.SubjectAlternativeName{ {Type: "dns", Value: "bar.internal"}, {Type: "dns", Value: "foo.internal"}, {Type: "ip", Value: "192.168.42.42"}, {Type: "ip", Value: "192.168.43.42"}, + {Type: "ip", Value: "2001:db8:85a3::8a2e:370:7334"}, }, err: nil, }, @@ -1448,7 +1366,7 @@ func TestOrder_sans(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("192.168.42.42")}, }, want: []x509util.SubjectAlternativeName{}, - err: NewError(ErrorServerInternalType, "..."), + err: NewError(ErrorServerInternalType, "unsupported identifier type in order: ipv4"), }, } for _, tt := range tests { @@ -1458,11 +1376,21 @@ func TestOrder_sans(t *testing.T) { } canonicalizedCSR := canonicalize(tt.csr) got, err := o.sans(canonicalizedCSR) - if err != nil && tt.err != nil { - if tt.err.Error() != err.Error() { - t.Errorf("Order.sans() error = %v, wantErr %v", err, tt.err) + if tt.err != nil { + if err == nil { + t.Errorf("Order.sans() = %v, want error; got none", got) return } + switch k := err.(type) { + case *Error: + assert.Equals(t, k.Type, tt.err.Type) + assert.Equals(t, k.Detail, tt.err.Detail) + assert.Equals(t, k.Status, tt.err.Status) + assert.Equals(t, k.Err.Error(), tt.err.Err.Error()) + assert.Equals(t, k.Detail, tt.err.Detail) + default: + assert.FatalError(t, errors.New("unexpected error type")) + } return } if !reflect.DeepEqual(got, tt.want) {