Fix mixed DNS and IP SANs in Order

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

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