Add Identifiers to Authorization & Order structs #7961

jprenken · 2025-01-21T07:09:22Z

Add identifier fields, which will soon replace the dnsName fields, to:

corepb.Authorization
corepb.Order
rapb.NewOrderRequest
sapb.CountFQDNSetsRequest
sapb.CountInvalidAuthorizationsRequest
sapb.FQDNSetExistsRequest
sapb.GetAuthorizationsRequest
sapb.GetOrderForNamesRequest
sapb.GetValidAuthorizationsRequest
sapb.NewOrderRequest

Populate these identifier fields in every function that creates instances of these structs.

Use these identifier fields instead of dnsName fields (at least preferentially) in every function that uses these structs. When crossing component boundaries, don't assume they'll be present, for deployability's sake.

Deployability note: Mismatched cert-checker and sa versions will be incompatible because of a type change in the arguments to sa.SelectAuthzsMatchingIssuance.

Part of #7311

Add `identifier` fields, which will soon replace the `dnsName` fields, to: - `corepb.Authorization` - `corepb.Order` - `rapb.NewOrderRequest` - `sapb.CountFQDNSetsRequest` - `sapb.CountInvalidAuthorizationsRequest` - `sapb.FQDNSetExistsRequest` - `sapb.GetAuthorizationsRequest` - `sapb.GetOrderForNamesRequest` - `sapb.GetValidAuthorizationsRequest` - `sapb.NewOrderRequest` Populate these `identifier` fields in every function that creates instances of these structs. Preferentially use these `identifier` fields in every function that uses these structs - but when crossing component boundaries, don't assume they'll be present, for deployability's sake. Part of #7311

…iers

…th package name

…iers

…within structs only

…iers

… func in WFE

… defers

…iers

aarongable

I've reviewed everything except for ra_test.go and wfe.go, but thought it best to send these comments ASAP. I think I will have further thoughts on the exact names and APIs you've created inside identifier.go, but what you have there is certainly good enough to be getting on with for now.

aarongable · 2025-01-27T18:39:01Z

core/proto/core.proto

  string dnsName = 2;
  string status = 4;
  google.protobuf.Timestamp expires = 9;
  repeated core.Challenge challenges = 6;
+  core.Identifier identifier = 10;


Interestingly I don't think this needs the core. prefix, because we're in core right here. See for example the use of unqualified ProblemDetails on line 123. I'm not sure why we used core.Challenge on the line above this...

aarongable · 2025-01-27T18:41:05Z

core/proto/core.proto

nit: here and throughout, please put the new identifier(s) field immediately adjacent to the existing dnsName(s) field. This is because the fields (especially in sections prefixed with // Fields specified by RFC 8555) are listed in the same order as the fields are in the RFC.

aarongable · 2025-01-27T18:43:32Z

core/util.go

+// TODO(#7311): Process IP address identifiers appropriately, and
+// consistently with how we stringify IPs elsewhere.


I think this TODO is unnecessary: the .Value field of an ACMEIdentifier is already a string, so no stringification should need to happen inside this function.

aarongable · 2025-01-27T18:45:56Z

core/util_test.go

@@ -340,30 +340,45 @@ func TestRetryBackoff(t *testing.T) {

 }

-func TestHashNames(t *testing.T) {
+func TestHashIdentifiers(t *testing.T) {


Add a test case for all identfier.NewIP(), and one or more test cases for a mixes of IP and DNS identifiers in various orders. Might be a good time to turn this into a table-based test, too.

aarongable · 2025-01-27T18:48:05Z

cmd/cert-checker/main.go

@@ -310,16 +311,16 @@ func (c *certChecker) checkValidations(ctx context.Context, cert core.Certificat

 	// We may get multiple authorizations for the same name, but that's okay.
 	// Any authorization for a given name is sufficient.
-	nameToAuthz := make(map[string]*corepb.Authorization)
+	identToAuthz := make(map[string]*corepb.Authorization)


Identifiers are comparable: they only have two fields, both of which are concrete strings. You should be able to use them directly as map keys, rather than collapsing them into just their values.

aarongable · 2025-01-28T00:33:06Z

test/integration/authz_test.go

@@ -40,7 +41,7 @@ func TestValidAuthzExpires(t *testing.T) {

 	// The authz should be valid and for the correct identifier
 	test.AssertEquals(t, authzOb.Status, "valid")
-	test.AssertEquals(t, authzOb.Identifier.Value, domains[0])
+	test.AssertEquals(t, authzOb.Identifier.Value, idents[0].Value)


Check that the identifier type has round-tripped correctly.

aarongable · 2025-01-28T00:35:49Z

test/integration/common_test.go

@@ -89,7 +91,7 @@ func delHTTP01Response(token string) error {
 	return nil
 }

-func makeClientAndOrder(c *client, csrKey *ecdsa.PrivateKey, domains []string, cn bool, certToReplace *x509.Certificate) (*client, *acme.Order, error) {
+func makeClientAndOrder(c *client, csrKey *ecdsa.PrivateKey, idents []identifier.ACMEIdentifier, cn bool, certToReplace *x509.Certificate) (*client, *acme.Order, error) {


These should be acme.Identifiers, not identifier.ACMEIdentifier, since this is client-side code not server-side code. Same with the other functions you've touched in this file.

aarongable · 2025-01-28T00:39:00Z

test/integration/common_test.go

+	// TODO(#7311): Extend this to support IP address identifiers.
+	names := make([]string, len(idents))
+	for i, ident := range idents {
+		names[i] = ident.Value
+	}


I think you can do this right now:

Suggested change

// TODO(#7311): Extend this to support IP address identifiers.

names := make([]string, len(idents))

for i, ident := range idents {

names[i] = ident.Value

}

names := make([]string, 0)

ips := make([]net.IP, 0)

for _, ident := range idents {

switch ident.Type:

case "dns":

names = append(names, ident.Value)

case "ip":

ips = append(ips, ident.Value)

default:

return nil, fmt.Errorf("unrecognized identifier type %q", ident.Type)

}

aarongable · 2025-01-28T00:43:35Z

identifier/identifier.go

+	for key, ident := range idents {
+		pbIdents[key] = ident.AsProto()


It's not quite idiomatic to refer to array indices as "keys", so:

Suggested change

for key, ident := range idents {

pbIdents[key] = ident.AsProto()

for i, ident := range idents {

pbIdents[i] = ident.AsProto()

aarongable · 2025-01-28T00:45:22Z

identifier/identifier.go

@@ -48,6 +75,16 @@ func NewDNS(domain string) ACMEIdentifier {
 	}
 }

+// SliceNewDNS is a convenience function for creating a slice of ACMEIdentifiers
+// with Type "dns" for a given slice of domain names.
+func SliceNewDNS(domains []string) []ACMEIdentifier {


If you make this

Suggested change

func SliceNewDNS(domains []string) []ACMEIdentifier {

func SliceNewDNS(domains ...string) []ACMEIdentifier {

then everywhere you call it, you can just pass it a bunch of individual strings like identifier.SliceNewDNS("hello", "world") instead of having to package them into a slice like identifier.SliceNewDNS([]string{"hello", "world"}).

From our convo: actually I think this can be almost entirely replaced by the combination of

SliceFromProto(idents []ACMEIdentifier, dnsNames []string), for when extracting from the two fields of a proto message;

SliceFromCert(x509.Certificate); and

csr.IdentsFromCSR(x509.CertificateRequest), upgraded from csr.NamesFromCSR.

jprenken added 28 commits January 20, 2025 23:08

Add TODOs, clarify others, found existing NormalizeIdentifiers

a133fb2

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

9e34394

…iers

Use identifiers in policy functions & part of wfe NewOrder

92b5df2

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

807f44a

…iers

Change HashNames to HashIdentifiers, fix more errors, drink more Glurp

8bddab2

Use 'idents' var name instead of 'identifiers' to reduce confusion wi…

9307f39

…th package name

Use identifiers in RequestEvent, add convenience funcs to identifier pkg

66bddc4

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

f56c456

…iers

Use identifiers in ra.NewOrder, fix test typo, move AsProto calls to …

98538f4

…within structs only

Change some SA model & test functions to use identifiers

ed7e503

Change ra.certificateRequestEvent & test functions to use identifiers

3b659a6

Fix more TODOs in cert-checker, tests & mocks

95e473d

Add helper funcs, with tests, to handle both idents & names

a792197

Add empty/nil test cases

008498a

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

5c1845b

…iers

Use identifiers throughout integration tests

1bcd9fc

Consistent nil/empty handling

013206b

Simplify helper funcs; their callers won't use names

4f4905c

Remove singular helper func, not useful because of defers; use helper…

b5840da

… func in WFE

Actually remove singular helper func, and it's because of derefs, not…

9e70389

… defers

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

e2a3082

…iers

Identifier-ize new code from main

4f9700a

Plumb identifiers through RA

9e3c10f

Plumb identifiers through grpc & sa

8bbe0f3

Remove stale TODOs

d0b7523

Fix missed reference in SA (don't worry, there are still more failures)

b368e44

Fix last tests!

f8844d3

jprenken marked this pull request as ready for review January 26, 2025 01:39

jprenken requested a review from a team as a code owner January 26, 2025 01:39

jprenken requested a review from beautifulentropy January 26, 2025 01:39

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

16a2c10

…iers

jprenken requested a review from aarongable January 27, 2025 18:11

jprenken added 3 commits January 27, 2025 10:11

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

d4c07a5

…iers

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

79bb748

…iers

Merge branch 'main' of github.com:letsencrypt/boulder into pb-identif…

b65c35a

…iers

aarongable reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Identifiers to Authorization & Order structs #7961

Add Identifiers to Authorization & Order structs #7961

jprenken commented Jan 21, 2025 •

edited

Loading

aarongable left a comment

aarongable Jan 27, 2025

aarongable Jan 27, 2025

aarongable Jan 27, 2025

aarongable Jan 27, 2025

aarongable Jan 27, 2025

aarongable Jan 28, 2025

aarongable Jan 28, 2025

aarongable Jan 28, 2025

aarongable Jan 28, 2025

aarongable Jan 28, 2025

aarongable Jan 28, 2025

		// TODO(#7311): Process IP address identifiers appropriately, and
		// consistently with how we stringify IPs elsewhere.

-	// TODO(#7311): Extend this to support IP address identifiers.
-	names := make([]string, len(idents))
-	for i, ident := range idents {
-		names[i] = ident.Value
-	}
+	names := make([]string, 0)
+	ips := make([]net.IP, 0)
+	for _, ident := range idents {
+		switch ident.Type:
+		case "dns":
+			names = append(names, ident.Value)
+		case "ip":
+			ips = append(ips, ident.Value)
+		default:
+			return nil, fmt.Errorf("unrecognized identifier type %q", ident.Type)
+	}

		for key, ident := range idents {
		pbIdents[key] = ident.AsProto()

	func SliceNewDNS(domains []string) []ACMEIdentifier {
	func SliceNewDNS(domains ...string) []ACMEIdentifier {

Add Identifiers to Authorization & Order structs #7961

Are you sure you want to change the base?

Add Identifiers to Authorization & Order structs #7961

Conversation

jprenken commented Jan 21, 2025 • edited Loading

aarongable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jprenken commented Jan 21, 2025 •

edited

Loading