sa: Encode IP identifiers for issuedNames (#8210)

Move usage of `sa.ReverseName` to a new `sa.EncodeIssuedName`, which
detects IP addresses and exempts them from being reversed. Retain
`reverseName` as an internal helper function.

Update `id-exporter`, `reversed-hostname-checker`, and tests to use the
new function and handle IP addresses.

Part of #7311

Author: jprenken

cmd/id-exporter/main.go

@@ -38,10 +38,10 @@ type resultEntry struct {
 	Hostname string `json:"hostname,omitempty"`
 }
 
-// reverseHostname converts (reversed) names sourced from the
-// registrations table to standard hostnames.
-func (r *resultEntry) reverseHostname() {
-	r.Hostname = sa.ReverseName(r.Hostname)
+// encodeIssuedName converts FQDNs and IP addresses to/from their format in the
+// issuedNames table.
+func (r *resultEntry) encodeIssuedName() {
+	r.Hostname = sa.EncodeIssuedName(r.Hostname)
 }
 
 // idExporterResults is passed as a selectable 'holder' for the results
@@ -112,7 +112,7 @@ func (c idExporter) findIDsWithExampleHostnames(ctx context.Context) (idExporter
 	}
 
 	for _, result := range holder {
-		result.reverseHostname()
+		result.encodeIssuedName()
 	}
 	return holder, nil
 }
@@ -135,7 +135,7 @@ func (c idExporter) findIDsForHostnames(ctx context.Context, hostnames []string)
 				AND n.reversedName = :reversedName;`,
 			map[string]interface{}{
 				"expireCutoff": c.clk.Now().Add(-c.grace),
-				"reversedName": sa.ReverseName(hostname),
+				"reversedName": sa.EncodeIssuedName(hostname),
 			},
 		)
 		if err != nil {

cmd/reversed-hostname-checker/main.go

@@ -1,5 +1,5 @@
-// Read a list of reversed hostnames, separated by newlines. Print only those
-// that are rejected by the current policy.
+// Read a list of reversed FQDNs and/or normal IP addresses, separated by
+// newlines. Print only those that are rejected by the current policy.
 
 package notmain
 
@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"net/netip"
 	"os"
 
 	"github.com/letsencrypt/boulder/cmd"
@@ -50,8 +51,15 @@ func main() {
 	}
 	var errors bool
 	for scanner.Scan() {
-		n := sa.ReverseName(scanner.Text())
-		err := pa.WillingToIssue(identifier.ACMEIdentifiers{identifier.NewDNS(n)})
+		n := sa.EncodeIssuedName(scanner.Text())
+		var ident identifier.ACMEIdentifier
+		ip, err := netip.ParseAddr(n)
+		if err == nil {
+			ident = identifier.NewIP(ip)
+		} else {
+			ident = identifier.NewDNS(n)
+		}
+		err = pa.WillingToIssue(identifier.ACMEIdentifiers{ident})
 		if err != nil {
 			errors = true
 			fmt.Printf("%s: %s\n", n, err)

sa/model.go

@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"math"
+	"net/netip"
 	"net/url"
 	"slices"
 	"strconv"
@@ -1056,8 +1057,6 @@ func deleteOrderFQDNSet(
 }
 
 func addIssuedNames(ctx context.Context, queryer db.Execer, cert *x509.Certificate, isRenewal bool) error {
-	// TODO(#7311): Determine & explicitly document whether to place IP address
-	// identifiers in issuedNames. We currently skip them.
 	if len(cert.DNSNames) == 0 && len(cert.IPAddresses) == 0 {
 		return berrors.InternalServerError("certificate has no DNSNames or IPAddresses")
 	}
@@ -1068,7 +1067,18 @@ func addIssuedNames(ctx context.Context, queryer db.Execer, cert *x509.Certifica
 	}
 	for _, name := range cert.DNSNames {
 		err = multiInserter.Add([]interface{}{
-			ReverseName(name),
+			reverseFQDN(name),
+			core.SerialToString(cert.SerialNumber),
+			cert.NotBefore.Truncate(24 * time.Hour),
+			isRenewal,
+		})
+		if err != nil {
+			return err
+		}
+	}
+	for _, ip := range cert.IPAddresses {
+		err = multiInserter.Add([]interface{}{
+			ip.String(),
 			core.SerialToString(cert.SerialNumber),
 			cert.NotBefore.Truncate(24 * time.Hour),
 			isRenewal,
@@ -1080,6 +1090,32 @@ func addIssuedNames(ctx context.Context, queryer db.Execer, cert *x509.Certifica
 	return multiInserter.Insert(ctx, queryer)
 }
 
+// EncodeIssuedName translates a FQDN to/from the issuedNames table by reversing
+// its dot-separated elements, and translates an IP address by returning its
+// normal string form.
+//
+// This is for strings of ambiguous identifier values. If you know your string
+// is a FQDN, use reverseFQDN(). If you have an IP address, use
+// netip.Addr.String() or net.IP.String().
+func EncodeIssuedName(name string) string {
+	netIP, err := netip.ParseAddr(name)
+	if err == nil {
+		return netIP.String()
+	}
+	return reverseFQDN(name)
+}
+
+// reverseFQDN reverses the elements of a dot-separated FQDN.
+//
+// If your string might be an IP address, use EncodeIssuedName() instead.
+func reverseFQDN(fqdn string) string {
+	labels := strings.Split(fqdn, ".")
+	for i, j := 0, len(labels)-1; i < j; i, j = i+1, j-1 {
+		labels[i], labels[j] = labels[j], labels[i]
+	}
+	return strings.Join(labels, ".")
+}
+
 func addKeyHash(ctx context.Context, db db.Inserter, cert *x509.Certificate) error {
 	if cert.RawSubjectPublicKeyInfo == nil {
 		return errors.New("certificate has a nil RawSubjectPublicKeyInfo")

sa/sa_test.go

@@ -322,7 +322,7 @@ func TestReplicationLagRetries(t *testing.T) {
 
 // findIssuedName is a small helper test function to directly query the
 // issuedNames table for a given name to find a serial (or return an err).
-func findIssuedName(ctx context.Context, dbMap db.OneSelector, name string) (string, error) {
+func findIssuedName(ctx context.Context, dbMap db.OneSelector, issuedName string) (string, error) {
 	var issuedNamesSerial string
 	err := dbMap.SelectOne(
 		ctx,
@@ -331,7 +331,7 @@ func findIssuedName(ctx context.Context, dbMap db.OneSelector, name string) (str
 		WHERE reversedName = ?
 		ORDER BY notBefore DESC
 		LIMIT 1`,
-		ReverseName(name))
+		issuedName)
 	return issuedNamesSerial, err
 }
 
@@ -438,7 +438,7 @@ func TestAddPrecertificate(t *testing.T) {
 	test.AssertEquals(t, now, certStatus.OcspLastUpdated.AsTime())
 
 	// It should show up in the issued names table
-	issuedNamesSerial, err := findIssuedName(ctx, sa.dbMap, testCert.DNSNames[0])
+	issuedNamesSerial, err := findIssuedName(ctx, sa.dbMap, reverseFQDN(testCert.DNSNames[0]))
 	test.AssertNotError(t, err, "expected no err querying issuedNames for precert")
 	test.AssertEquals(t, issuedNamesSerial, serial)
 
@@ -912,10 +912,10 @@ func TestDeactivateAccount(t *testing.T) {
 	test.AssertError(t, err, "Deactivating an already-deactivated account should fail")
 }
 
-func TestReverseName(t *testing.T) {
+func TestReverseFQDN(t *testing.T) {
 	testCases := []struct {
-		inputDomain   string
-		inputReversed string
+		fqdn     string
+		reversed string
 	}{
 		{"", ""},
 		{"...", "..."},
@@ -926,8 +926,46 @@ func TestReverseName(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		output := ReverseName(tc.inputDomain)
-		test.AssertEquals(t, output, tc.inputReversed)
+		output := reverseFQDN(tc.fqdn)
+		test.AssertEquals(t, output, tc.reversed)
+
+		output = reverseFQDN(tc.reversed)
+		test.AssertEquals(t, output, tc.fqdn)
+	}
+}
+
+func TestEncodeIssuedName(t *testing.T) {
+	testCases := []struct {
+		issuedName string
+		reversed   string
+		oneWay     bool
+	}{
+		// Empty strings and bare separators/TLDs should be unchanged.
+		{"", "", false},
+		{"...", "...", false},
+		{"com", "com", false},
+		// FQDNs should be reversed.
+		{"example.com", "com.example", false},
+		{"www.example.com", "com.example.www", false},
+		{"world.wide.web.example.com", "com.example.web.wide.world", false},
+		// IP addresses should stay the same.
+		{"1.2.3.4", "1.2.3.4", false},
+		{"2602:ff3a:1:abad:c0f:fee:abad:cafe", "2602:ff3a:1:abad:c0f:fee:abad:cafe", false},
+		// Tricksy FQDNs that look like IPv6 addresses should be parsed as FQDNs.
+		{"2602.ff3a.1.abad.c0f.fee.abad.cafe", "cafe.abad.fee.c0f.abad.1.ff3a.2602", false},
+		{"2602.ff3a.0001.abad.0c0f.0fee.abad.cafe", "cafe.abad.0fee.0c0f.abad.0001.ff3a.2602", false},
+		// IPv6 addresses should be returned in RFC 5952 format.
+		{"2602:ff3a:0001:abad:0c0f:0fee:abad:cafe", "2602:ff3a:1:abad:c0f:fee:abad:cafe", true},
+	}
+
+	for _, tc := range testCases {
+		output := EncodeIssuedName(tc.issuedName)
+		test.AssertEquals(t, output, tc.reversed)
+
+		if !tc.oneWay {
+			output = EncodeIssuedName(tc.reversed)
+			test.AssertEquals(t, output, tc.issuedName)
+		}
 	}
 }
 
@@ -2106,7 +2144,7 @@ func TestAddCertificateRenewalBit(t *testing.T) {
 
 	reg := createWorkingRegistration(t, sa)
 
-	assertIsRenewal := func(t *testing.T, name string, expected bool) {
+	assertIsRenewal := func(t *testing.T, issuedName string, expected bool) {
 		t.Helper()
 		var count int
 		err := sa.dbMap.SelectOne(
@@ -2115,14 +2153,14 @@ func TestAddCertificateRenewalBit(t *testing.T) {
 			`SELECT COUNT(*) FROM issuedNames
 		WHERE reversedName = ?
 		AND renewal = ?`,
-			ReverseName(name),
+			issuedName,
 			expected,
 		)
 		test.AssertNotError(t, err, "Unexpected error from SelectOne on issuedNames")
 		test.AssertEquals(t, count, 1)
 	}
 
-	// Add a certificate with a never-before-seen name.
+	// Add a certificate with never-before-seen identifiers.
 	_, testCert := test.ThrowAwayCert(t, fc)
 	_, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
@@ -2138,16 +2176,19 @@ func TestAddCertificateRenewalBit(t *testing.T) {
 	})
 	test.AssertNotError(t, err, "Failed to add certificate")
 
-	// None of the names should have a issuedNames row marking it as a renewal.
+	// No identifier should have an issuedNames row marking it as a renewal.
 	for _, name := range testCert.DNSNames {
-		assertIsRenewal(t, name, false)
+		assertIsRenewal(t, reverseFQDN(name), false)
+	}
+	for _, ip := range testCert.IPAddresses {
+		assertIsRenewal(t, ip.String(), false)
 	}
 
 	// Make a new cert and add its FQDN set to the db so it will be considered a
 	// renewal
 	serial, testCert := test.ThrowAwayCert(t, fc)
 	err = addFQDNSet(ctx, sa.dbMap, identifier.FromCert(testCert), serial, testCert.NotBefore, testCert.NotAfter)
-	test.AssertNotError(t, err, "Failed to add name set")
+	test.AssertNotError(t, err, "Failed to add identifier set")
 	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
 		Issued:       timestamppb.New(testCert.NotBefore),
@@ -2162,9 +2203,12 @@ func TestAddCertificateRenewalBit(t *testing.T) {
 	})
 	test.AssertNotError(t, err, "Failed to add certificate")
 
-	// All of the names should have a issuedNames row marking it as a renewal.
+	// Each identifier should have an issuedNames row marking it as a renewal.
 	for _, name := range testCert.DNSNames {
-		assertIsRenewal(t, name, true)
+		assertIsRenewal(t, reverseFQDN(name), true)
+	}
+	for _, ip := range testCert.IPAddresses {
+		assertIsRenewal(t, ip.String(), true)
 	}
 }
 

sa/saro.go

@@ -153,14 +153,6 @@ func (ssa *SQLStorageAuthorityRO) GetRegistrationByKey(ctx context.Context, req
 	return registrationModelToPb(model)
 }
 
-func ReverseName(domain string) string {
-	labels := strings.Split(domain, ".")
-	for i, j := 0, len(labels)-1; i < j; i, j = i+1, j-1 {
-		labels[i], labels[j] = labels[j], labels[i]
-	}
-	return strings.Join(labels, ".")
-}
-
 // GetSerialMetadata returns metadata stored alongside the serial number,
 // such as the RegID whose certificate request created that serial, and when
 // the certificate with that serial will expire.

test/hostname-policy.yaml

@@ -14,7 +14,7 @@ ExactBlockedNames:
 # all subdomains/wildcards.
 HighRiskBlockedNames:
   # See RFC 3152
-  - "ipv6.arpa"
+  - "ip6.arpa"
   # See RFC 2317
   - "in-addr.arpa"
   # Etc etc etc

test/integration/cert_storage_failed_test.go

@@ -31,8 +31,8 @@ import (
 
 // getPrecertByName finds and parses a precertificate using the given hostname.
 // It returns the most recent one.
-func getPrecertByName(db *sql.DB, name string) (*x509.Certificate, error) {
-	name = sa.ReverseName(name)
+func getPrecertByName(db *sql.DB, reversedName string) (*x509.Certificate, error) {
+	reversedName = sa.EncodeIssuedName(reversedName)
 	// Find the certificate from the precertificates table. We don't know the serial so
 	// we have to look it up by name.
 	var der []byte
@@ -43,15 +43,15 @@ func getPrecertByName(db *sql.DB, name string) (*x509.Certificate, error) {
 		WHERE reversedName = ?
 		ORDER BY issuedNames.id DESC
 		LIMIT 1
-	`, name)
+	`, reversedName)
 	for rows.Next() {
 		err = rows.Scan(&der)
 		if err != nil {
 			return nil, err
 		}
 	}
 	if der == nil {
-		return nil, fmt.Errorf("no precertificate found for %q", name)
+		return nil, fmt.Errorf("no precertificate found for %q", reversedName)
 	}
 
 	cert, err := x509.ParseCertificate(der)