Lay the groundwork for supporting IP identifiers (#7692)

Clean up how we handle identifiers throughout the Boulder codebase by
- moving the Identifier protobuf message definition from sa.proto to
core.proto;
- adding support for IP identifier to the "identifier" package;
- renaming the "identifier" package's exported names to be clearer; and
- ensuring we use the identifier package's helper functions everywhere
we can.

This will make future work to actually respect identifier types (such as
in Authorization and Order protobuf messages) simpler and easier to
review.

Part of #7311

cmd/admin/pause_identifier.go

@@ -12,6 +12,7 @@ import (
 	"sync"
 	"sync/atomic"
 
+	corepb "github.com/letsencrypt/boulder/core/proto"
 	"github.com/letsencrypt/boulder/identifier"
 	sapb "github.com/letsencrypt/boulder/sa/proto"
 )
@@ -59,9 +60,9 @@ func (a *admin) pauseIdentifiers(ctx context.Context, entries []pauseCSVData, pa
 		return nil, errors.New("cannot pause identifiers because no pauseData was sent")
 	}
 
-	accountToIdentifiers := make(map[int64][]*sapb.Identifier)
+	accountToIdentifiers := make(map[int64][]*corepb.Identifier)
 	for _, entry := range entries {
-		accountToIdentifiers[entry.accountID] = append(accountToIdentifiers[entry.accountID], &sapb.Identifier{
+		accountToIdentifiers[entry.accountID] = append(accountToIdentifiers[entry.accountID], &corepb.Identifier{
 			Type:  string(entry.identifierType),
 			Value: entry.identifierValue,
 		})
@@ -71,7 +72,7 @@ func (a *admin) pauseIdentifiers(ctx context.Context, entries []pauseCSVData, pa
 	respChan := make(chan *sapb.PauseIdentifiersResponse, len(accountToIdentifiers))
 	work := make(chan struct {
 		accountID   int64
-		identifiers []*sapb.Identifier
+		identifiers []*corepb.Identifier
 	}, parallelism)
 
 	var wg sync.WaitGroup
@@ -97,7 +98,7 @@ func (a *admin) pauseIdentifiers(ctx context.Context, entries []pauseCSVData, pa
 	for accountID, identifiers := range accountToIdentifiers {
 		work <- struct {
 			accountID   int64
-			identifiers []*sapb.Identifier
+			identifiers []*corepb.Identifier
 		}{accountID, identifiers}
 	}
 	close(work)

core/proto/core.proto

@@ -5,6 +5,11 @@ option go_package = "github.com/letsencrypt/boulder/core/proto";
 
 import "google/protobuf/timestamp.proto";
 
+message Identifier {
+  string type = 1;
+  string value = 2;
+}
+
 message Challenge {
   // Next unused field number: 13
   reserved 4, 5, 8, 11;

errors/errors_test.go

@@ -17,14 +17,14 @@ func TestWithSubErrors(t *testing.T) {
 
 	subErrs := []SubBoulderError{
 		{
-			Identifier: identifier.DNSIdentifier("example.com"),
+			Identifier: identifier.NewDNS("example.com"),
 			BoulderError: &BoulderError{
 				Type:   RateLimit,
 				Detail: "everyone uses this example domain",
 			},
 		},
 		{
-			Identifier: identifier.DNSIdentifier("what about example.com"),
+			Identifier: identifier.NewDNS("what about example.com"),
 			BoulderError: &BoulderError{
 				Type:   RateLimit,
 				Detail: "try a real identifier value next time",
@@ -39,7 +39,7 @@ func TestWithSubErrors(t *testing.T) {
 	test.AssertDeepEquals(t, outResult.SubErrors, subErrs)
 	// Adding another suberr shouldn't squash the original sub errors
 	anotherSubErr := SubBoulderError{
-		Identifier: identifier.DNSIdentifier("another ident"),
+		Identifier: identifier.NewDNS("another ident"),
 		BoulderError: &BoulderError{
 			Type:   RateLimit,
 			Detail: "another rate limit err",

grpc/errors_test.go

@@ -12,6 +12,7 @@ import (
 	"google.golang.org/grpc/credentials/insecure"
 
 	"github.com/jmhodges/clock"
+
 	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/grpc/test_proto"
 	"github.com/letsencrypt/boulder/identifier"
@@ -96,7 +97,7 @@ func TestSubErrorWrapping(t *testing.T) {
 
 	subErrors := []berrors.SubBoulderError{
 		{
-			Identifier: identifier.DNSIdentifier("chillserver.com"),
+			Identifier: identifier.NewDNS("chillserver.com"),
 			BoulderError: &berrors.BoulderError{
 				Type:   berrors.RejectedIdentifier,
 				Detail: "2 ill 2 chill",

grpc/pb-marshalling.go

@@ -338,7 +338,7 @@ func PBToAuthz(pb *corepb.Authorization) (core.Authorization, error) {
 	}
 	authz := core.Authorization{
 		ID:             pb.Id,
-		Identifier:     identifier.ACMEIdentifier{Type: identifier.DNS, Value: pb.DnsName},
+		Identifier:     identifier.NewDNS(pb.DnsName),
 		RegistrationID: pb.RegistrationID,
 		Status:         core.AcmeStatus(pb.Status),
 		Expires:        expires,

grpc/pb-marshalling_test.go

@@ -225,7 +225,7 @@ func TestRegistration(t *testing.T) {
 
 func TestAuthz(t *testing.T) {
 	exp := time.Now().AddDate(0, 0, 1).UTC()
-	identifier := identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"}
+	identifier := identifier.NewDNS("example.com")
 	challA := core.Challenge{
 		Type:   core.ChallengeTypeDNS01,
 		Status: core.StatusPending,

identifier/identifier.go

@@ -1,13 +1,23 @@
 // The identifier package defines types for RFC 8555 ACME identifiers.
+// It exists as a separate package to prevent an import loop between the core
+// and probs packages.
 package identifier
 
+import (
+	"net/netip"
+
+	corepb "github.com/letsencrypt/boulder/core/proto"
+)
+
 // IdentifierType is a named string type for registered ACME identifier types.
 // See https://tools.ietf.org/html/rfc8555#section-9.7.7
 type IdentifierType string
 
 const (
-	// DNS is specified in RFC 8555 for DNS type identifiers.
-	DNS = IdentifierType("dns")
+	// TypeDNS is specified in RFC 8555 for TypeDNS type identifiers.
+	TypeDNS = IdentifierType("dns")
+	// TypeIP is specified in RFC 8738
+	TypeIP = IdentifierType("ip")
 )
 
 // ACMEIdentifier is a struct encoding an identifier that can be validated. The
@@ -22,11 +32,27 @@ type ACMEIdentifier struct {
 	Value string `json:"value"`
 }
 
-// DNSIdentifier is a convenience function for creating an ACMEIdentifier with
-// Type DNS for a given domain name.
-func DNSIdentifier(domain string) ACMEIdentifier {
+func (i ACMEIdentifier) AsProto() *corepb.Identifier {
+	return &corepb.Identifier{
+		Type:  string(i.Type),
+		Value: i.Value,
+	}
+}
+
+// NewDNS is a convenience function for creating an ACMEIdentifier with Type
+// "dns" for a given domain name.
+func NewDNS(domain string) ACMEIdentifier {
 	return ACMEIdentifier{
-		Type:  DNS,
+		Type:  TypeDNS,
 		Value: domain,
 	}
 }
+
+// NewIP is a convenience function for creating an ACMEIdentifier with Type "ip"
+// for a given IP address.
+func NewIP(ip netip.Addr) ACMEIdentifier {
+	return ACMEIdentifier{
+		Type:  TypeIP,
+		Value: ip.StringExpanded(),
+	}
+}

mocks/sa.go

@@ -475,10 +475,7 @@ func (sa *StorageAuthorityReadOnly) GetValidAuthorizations2(ctx context.Context,
 			Status:         core.StatusValid,
 			RegistrationID: req.RegistrationID,
 			Expires:        &exp,
-			Identifier: identifier.ACMEIdentifier{
-				Type:  identifier.DNS,
-				Value: name,
-			},
+			Identifier:     identifier.NewDNS(name),
 			Challenges: []core.Challenge{
 				{
 					Status:    core.StatusValid,

policy/pa.go

@@ -362,12 +362,12 @@ func subError(name string, err error) berrors.SubBoulderError {
 	var bErr *berrors.BoulderError
 	if errors.As(err, &bErr) {
 		return berrors.SubBoulderError{
-			Identifier:   identifier.DNSIdentifier(name),
+			Identifier:   identifier.NewDNS(name),
 			BoulderError: bErr,
 		}
 	} else {
 		return berrors.SubBoulderError{
-			Identifier: identifier.DNSIdentifier(name),
+			Identifier: identifier.NewDNS(name),
 			BoulderError: &berrors.BoulderError{
 				Type:   berrors.RejectedIdentifier,
 				Detail: err.Error(),
@@ -526,12 +526,12 @@ func (pa *AuthorityImpl) ChallengeTypesFor(ident identifier.ACMEIdentifier) ([]c
 	// challenge, to comply with the BRs Sections 3.2.2.4.19 and 3.2.2.4.20
 	// stating that ACME HTTP-01 and TLS-ALPN-01 are not suitable for validating
 	// Wildcard Domains.
-	if ident.Type == identifier.DNS && strings.HasPrefix(ident.Value, "*.") {
+	if ident.Type == identifier.TypeDNS && strings.HasPrefix(ident.Value, "*.") {
 		return []core.AcmeChallenge{core.ChallengeTypeDNS01}, nil
 	}
 
 	// Return all challenge types we support for non-wildcard DNS identifiers.
-	if ident.Type == identifier.DNS {
+	if ident.Type == identifier.TypeDNS {
 		return []core.AcmeChallenge{
 			core.ChallengeTypeHTTP01,
 			core.ChallengeTypeDNS01,

policy/pa_test.go

@@ -2,6 +2,7 @@ package policy
 
 import (
 	"fmt"
+	"net/netip"
 	"os"
 	"testing"
 
@@ -335,14 +336,14 @@ func TestWillingToIssue_SubErrors(t *testing.T) {
 						Type:   berrors.Malformed,
 						Detail: "Domain name contains an invalid character",
 					},
-					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "letsdecrypt_org"},
+					Identifier: identifier.NewDNS("letsdecrypt_org"),
 				},
 				{
 					BoulderError: &berrors.BoulderError{
 						Type:   berrors.Malformed,
 						Detail: "Domain name does not end with a valid public suffix (TLD)",
 					},
-					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.comm"},
+					Identifier: identifier.NewDNS("example.comm"),
 				},
 			},
 		})
@@ -366,14 +367,14 @@ func TestWillingToIssue_SubErrors(t *testing.T) {
 						Type:   berrors.RejectedIdentifier,
 						Detail: "The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy",
 					},
-					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "letsdecrypt.org"},
+					Identifier: identifier.NewDNS("letsdecrypt.org"),
 				},
 				{
 					BoulderError: &berrors.BoulderError{
 						Type:   berrors.RejectedIdentifier,
 						Detail: "The ACME server refuses to issue a certificate for this domain name, because it is forbidden by policy",
 					},
-					Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"},
+					Identifier: identifier.NewDNS("example.com"),
 				},
 			},
 		})
@@ -399,21 +400,21 @@ func TestChallengeTypesFor(t *testing.T) {
 	}{
 		{
 			name:  "dns",
-			ident: identifier.DNSIdentifier("example.com"),
+			ident: identifier.NewDNS("example.com"),
 			wantChalls: []core.AcmeChallenge{
 				core.ChallengeTypeHTTP01, core.ChallengeTypeDNS01, core.ChallengeTypeTLSALPN01,
 			},
 		},
 		{
 			name:  "wildcard",
-			ident: identifier.DNSIdentifier("*.example.com"),
+			ident: identifier.NewDNS("*.example.com"),
 			wantChalls: []core.AcmeChallenge{
 				core.ChallengeTypeDNS01,
 			},
 		},
 		{
 			name:    "other",
-			ident:   identifier.ACMEIdentifier{Type: "ip", Value: "1.2.3.4"},
+			ident:   identifier.NewIP(netip.MustParseAddr("1.2.3.4")),
 			wantErr: "unrecognized identifier type",
 		},
 	}
@@ -504,23 +505,23 @@ func TestCheckAuthzChallenges(t *testing.T) {
 		{
 			name: "no challenges",
 			authz: core.Authorization{
-				Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"},
+				Identifier: identifier.NewDNS("example.com"),
 				Challenges: []core.Challenge{},
 			},
 			wantErr: "has no challenges",
 		},
 		{
 			name: "no valid challenges",
 			authz: core.Authorization{
-				Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"},
+				Identifier: identifier.NewDNS("example.com"),
 				Challenges: []core.Challenge{{Type: core.ChallengeTypeDNS01, Status: core.StatusPending}},
 			},
 			wantErr: "not solved by any challenge",
 		},
 		{
 			name: "solved by disabled challenge",
 			authz: core.Authorization{
-				Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"},
+				Identifier: identifier.NewDNS("example.com"),
 				Challenges: []core.Challenge{{Type: core.ChallengeTypeDNS01, Status: core.StatusValid}},
 			},
 			enabled: map[core.AcmeChallenge]bool{core.ChallengeTypeHTTP01: true},
@@ -529,15 +530,15 @@ func TestCheckAuthzChallenges(t *testing.T) {
 		{
 			name: "solved by wrong kind of challenge",
 			authz: core.Authorization{
-				Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "*.example.com"},
+				Identifier: identifier.NewDNS("*.example.com"),
 				Challenges: []core.Challenge{{Type: core.ChallengeTypeHTTP01, Status: core.StatusValid}},
 			},
 			wantErr: "inapplicable challenge type",
 		},
 		{
 			name: "valid authz",
 			authz: core.Authorization{
-				Identifier: identifier.ACMEIdentifier{Type: identifier.DNS, Value: "example.com"},
+				Identifier: identifier.NewDNS("example.com"),
 				Challenges: []core.Challenge{{Type: core.ChallengeTypeTLSALPN01, Status: core.StatusValid}},
 			},
 		},

probs/probs_test.go

@@ -67,15 +67,15 @@ func TestWithSubProblems(t *testing.T) {
 	}
 	subProbs := []SubProblemDetails{
 		{
-			Identifier: identifier.DNSIdentifier("example.com"),
+			Identifier: identifier.NewDNS("example.com"),
 			ProblemDetails: ProblemDetails{
 				Type:       RateLimitedProblem,
 				Detail:     "don't you think you have enough certificates already?",
 				HTTPStatus: http.StatusTooManyRequests,
 			},
 		},
 		{
-			Identifier: identifier.DNSIdentifier("what about example.com"),
+			Identifier: identifier.NewDNS("what about example.com"),
 			ProblemDetails: ProblemDetails{
 				Type:       MalformedProblem,
 				Detail:     "try a real identifier value next time",
@@ -92,7 +92,7 @@ func TestWithSubProblems(t *testing.T) {
 	test.AssertDeepEquals(t, outResult.SubProblems, subProbs)
 	// Adding another sub problem shouldn't squash the original sub problems
 	anotherSubProb := SubProblemDetails{
-		Identifier: identifier.DNSIdentifier("another ident"),
+		Identifier: identifier.NewDNS("another ident"),
 		ProblemDetails: ProblemDetails{
 			Type:       RateLimitedProblem,
 			Detail:     "yet another rate limit err",