Skip to content

Commit 52615d9

Browse files
jprenkenjprenken
authored
ra: Fully support identifiers in NewOrder, PerformValidation & RevokeCertByApplicant (#8139)
In `ra.NewOrder`, improve safety of authz reuse logic by making it explicit that only DNS identifiers might be wildcards. Also, now that the conditional statements need to be more complicated, collapse them for brevity. In `vapb.PerformValidationRequest`, remove `DnsName`. In `ra.PerformValidation`, pass an `Identifier` instead of a `DnsName`. In `ra.RevokeCertByApplicant`, check that the requester controls identifiers of all types (not just DNS). Fixes #7995 (the RA now fully supports IP address identifiers, except for rate limits) Fixes #7647 Part of #8023
1 parent b26b116 commit 52615d9

File tree

7 files changed

+97
-172
lines changed

7 files changed

+97
-172
lines changed

‎ra/ra.go‎

Lines changed: 32 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1457,7 +1457,8 @@ func (ra *RegistrationAuthorityImpl) recordValidation(ctx context.Context, authI
14571457
// countFailedValidations increments the FailedAuthorizationsPerDomainPerAccount limit.
14581458
// and the FailedAuthorizationsForPausingPerDomainPerAccountTransaction limit.
14591459
//
1460-
// TODO(#7311): Handle IP address identifiers.
1460+
// TODO(#7311): Handle IP address identifiers properly; don't just trust that
1461+
// the value will always make sense in context.
14611462
func (ra *RegistrationAuthorityImpl) countFailedValidations(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) error {
14621463
txn, err := ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(regId, ident.Value)
14631464
if err != nil {
@@ -1506,7 +1507,8 @@ func (ra *RegistrationAuthorityImpl) countFailedValidations(ctx context.Context,
15061507
// resetAccountPausingLimit resets bucket to maximum capacity for given account.
15071508
// There is no reason to surface errors from this function to the Subscriber.
15081509
//
1509-
// TODO(#7311): Handle IP address identifiers.
1510+
// TODO(#7311): Handle IP address identifiers properly; don't just trust that
1511+
// the value will always make sense in context.
15101512
func (ra *RegistrationAuthorityImpl) resetAccountPausingLimit(ctx context.Context, regId int64, ident identifier.ACMEIdentifier) {
15111513
bucketKey, err := ratelimits.NewRegIdDomainBucketKey(ratelimits.FailedAuthorizationsForPausingPerDomainPerAccount, regId, ident.Value)
15121514
if err != nil {
@@ -1628,7 +1630,7 @@ func (ra *RegistrationAuthorityImpl) PerformValidation(
16281630
checkProb, checkRecords, err := ra.checkDCVAndCAA(
16291631
vaCtx,
16301632
&vapb.PerformValidationRequest{
1631-
DnsName: authz.Identifier.Value,
1633+
Identifier: authz.Identifier.ToProto(),
16321634
Challenge: chall,
16331635
Authz: &vapb.AuthzMeta{Id: authz.ID, RegID: authz.RegistrationID},
16341636
ExpectedKeyAuthorization: expectedKeyAuthorization,
@@ -1863,11 +1865,11 @@ func (ra *RegistrationAuthorityImpl) RevokeCertByApplicant(ctx context.Context,
18631865
// authorizations for all names in the cert.
18641866
logEvent.Method = "control"
18651867

1866-
// TODO(#7311): Support other kinds of SANs/identifiers here.
1868+
idents := identifier.FromCert(cert)
18671869
var authzPB *sapb.Authorizations
18681870
authzPB, err = ra.SA.GetValidAuthorizations2(ctx, &sapb.GetValidAuthorizationsRequest{
18691871
RegistrationID: req.RegID,
1870-
Identifiers: identifier.NewDNSSlice(cert.DNSNames).ToProtoSlice(),
1872+
Identifiers: idents.ToProtoSlice(),
18711873
ValidUntil: timestamppb.New(ra.clk.Now()),
18721874
})
18731875
if err != nil {
@@ -1880,10 +1882,9 @@ func (ra *RegistrationAuthorityImpl) RevokeCertByApplicant(ctx context.Context,
18801882
return nil, err
18811883
}
18821884

1883-
// TODO(#7311): TODO(#7647): Support other kinds of SANs/identifiers here.
1884-
for _, name := range cert.DNSNames {
1885-
if _, present := authzMap[identifier.NewDNS(name)]; !present {
1886-
return nil, berrors.UnauthorizedError("requester does not control all names in cert with serial %q", serialString)
1885+
for _, ident := range idents {
1886+
if _, present := authzMap[ident]; !present {
1887+
return nil, berrors.UnauthorizedError("requester does not control all identifiers in cert with serial %q", serialString)
18871888
}
18881889
}
18891890

@@ -2392,22 +2393,23 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
23922393
// For each of the identifiers in the order, if there is an acceptable
23932394
// existing authz, append it to the order to reuse it. Otherwise track that
23942395
// there is a missing authz for that identifier.
2395-
//
2396-
// TODO(#7311): TODO(#7647): Support non-dnsName identifier types here.
23972396
var newOrderAuthzs []int64
23982397
var missingAuthzIdents identifier.ACMEIdentifiers
23992398
for _, ident := range idents {
2400-
name := ident.Value
24012399
// If there isn't an existing authz, note that its missing and continue
24022400
authz, exists := identToExistingAuthz[ident]
24032401
if !exists {
2402+
// The existing authz was not acceptable for reuse, and we need to
2403+
// mark the name as requiring a new pending authz.
24042404
missingAuthzIdents = append(missingAuthzIdents, ident)
24052405
continue
24062406
}
24072407

24082408
// If the authz is associated with the wrong profile, don't reuse it.
24092409
if authz.CertificateProfileName != req.CertificateProfileName {
24102410
missingAuthzIdents = append(missingAuthzIdents, ident)
2411+
// Delete the authz from the identToExistingAuthz map since we are not reusing it.
2412+
delete(identToExistingAuthz, ident)
24112413
continue
24122414
}
24132415

@@ -2417,40 +2419,28 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New
24172419
authzAge = (profile.pendingAuthzLifetime - authz.Expires.Sub(ra.clk.Now())).Seconds()
24182420
}
24192421

2420-
// If the identifier is a wildcard and the existing authz only has one
2421-
// DNS-01 type challenge we can reuse it. In theory we will
2422-
// never get back an authorization for a domain with a wildcard prefix
2423-
// that doesn't meet this criteria from SA.GetAuthorizations but we verify
2424-
// again to be safe.
2425-
if strings.HasPrefix(name, "*.") &&
2426-
len(authz.Challenges) == 1 && authz.Challenges[0].Type == core.ChallengeTypeDNS01 {
2427-
authzID, err := strconv.ParseInt(authz.ID, 10, 64)
2428-
if err != nil {
2429-
return nil, err
2430-
}
2431-
newOrderAuthzs = append(newOrderAuthzs, authzID)
2432-
ra.authzAges.WithLabelValues("NewOrder", string(authz.Status)).Observe(authzAge)
2433-
continue
2434-
} else if !strings.HasPrefix(name, "*.") {
2435-
// If the identifier isn't a wildcard, we can reuse any authz
2436-
authzID, err := strconv.ParseInt(authz.ID, 10, 64)
2437-
if err != nil {
2438-
return nil, err
2439-
}
2440-
newOrderAuthzs = append(newOrderAuthzs, authzID)
2441-
ra.authzAges.WithLabelValues("NewOrder", string(authz.Status)).Observe(authzAge)
2442-
continue
2422+
// If the identifier is a wildcard DNS name, it must have exactly one
2423+
// DNS-01 type challenge. The PA guarantees this at order creation time,
2424+
// but we verify again to be safe.
2425+
if ident.Type == identifier.TypeDNS && strings.HasPrefix(ident.Value, "*.") &&
2426+
(len(authz.Challenges) != 1 || authz.Challenges[0].Type != core.ChallengeTypeDNS01) {
2427+
return nil, berrors.InternalServerError(
2428+
"SA.GetAuthorizations returned a DNS wildcard authz (%s) with invalid challenge(s)",
2429+
authz.ID)
24432430
}
24442431

2445-
// Delete the authz from the identToExistingAuthz map since we are not reusing it.
2446-
delete(identToExistingAuthz, ident)
2447-
// If we reached this point then the existing authz was not acceptable for
2448-
// reuse and we need to mark the name as requiring a new pending authz
2449-
missingAuthzIdents = append(missingAuthzIdents, ident)
2432+
// If we reached this point then the existing authz was acceptable for
2433+
// reuse.
2434+
authzID, err := strconv.ParseInt(authz.ID, 10, 64)
2435+
if err != nil {
2436+
return nil, err
2437+
}
2438+
newOrderAuthzs = append(newOrderAuthzs, authzID)
2439+
ra.authzAges.WithLabelValues("NewOrder", string(authz.Status)).Observe(authzAge)
24502440
}
24512441

2452-
// Loop through each of the names missing authzs and create a new pending
2453-
// authorization for each.
2442+
// Loop through each of the identifiers missing authzs and create a new
2443+
// pending authorization for each.
24542444
var newAuthzs []*sapb.NewAuthzRequest
24552445
for _, ident := range missingAuthzIdents {
24562446
challTypes, err := ra.PA.ChallengeTypesFor(ident)

‎ra/ra_test.go‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ func (dva *DummyValidationAuthority) PerformValidation(ctx context.Context, req
178178
return dcvRes, nil
179179
}
180180
caaResp, err := dva.DoCAA(ctx, &vapb.IsCAAValidRequest{
181-
Domain: req.DnsName,
181+
Domain: req.Identifier.Value,
182182
ValidationMethod: req.Challenge.Type,
183183
AccountURIID: req.Authz.RegID,
184184
AuthzID: req.Authz.Id,
@@ -1992,12 +1992,10 @@ func TestNewOrderAuthzReuseSafety(t *testing.T) {
19921992
}
19931993

19941994
// Create an order for that request
1995-
order, err := ra.NewOrder(ctx, orderReq)
1996-
// It shouldn't fail
1997-
test.AssertNotError(t, err, "Adding an initial order for regA failed")
1998-
test.AssertEquals(t, numAuthorizations(order), 1)
1999-
// It should *not* be the bad authorization!
2000-
test.AssertNotEquals(t, order.V2Authorizations[0], int64(1))
1995+
_, err := ra.NewOrder(ctx, orderReq)
1996+
// It should fail
1997+
test.AssertError(t, err, "Added an initial order for regA with invalid challenge(s)")
1998+
test.AssertContains(t, err.Error(), "SA.GetAuthorizations returned a DNS wildcard authz (1) with invalid challenge(s)")
20011999
}
20022000

20032001
func TestNewOrderWildcard(t *testing.T) {
@@ -3727,7 +3725,7 @@ func TestRevokeCertByApplicant_Controller(t *testing.T) {
37273725
RegID: 2,
37283726
})
37293727
test.AssertError(t, err, "should have failed with wrong RegID")
3730-
test.AssertContains(t, err.Error(), "requester does not control all names")
3728+
test.AssertContains(t, err.Error(), "requester does not control all identifiers")
37313729

37323730
// Revoking when the account does have valid authzs for the name should succeed,
37333731
// but override the revocation reason to cessationOfOperation.

‎test/certs.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package test
22

33
import (
4+
"bytes"
45
"crypto"
56
"crypto/ecdsa"
67
"crypto/elliptic"
@@ -12,6 +13,7 @@ import (
1213
"errors"
1314
"fmt"
1415
"math/big"
16+
"net"
1517
"os"
1618
"testing"
1719
"time"
@@ -71,6 +73,13 @@ func ThrowAwayCert(t *testing.T, clk clock.Clock) (string, *x509.Certificate) {
7173
_, _ = rand.Read(nameBytes[:])
7274
name := fmt.Sprintf("%s.example.com", hex.EncodeToString(nameBytes[:]))
7375

76+
// Generate a random IPv6 address under the RFC 3849 space.
77+
// https://www.rfc-editor.org/rfc/rfc3849.txt
78+
var ipBytes [12]byte
79+
_, _ = rand.Read(ipBytes[:])
80+
ipPrefix, _ := hex.DecodeString("20010db8")
81+
ip := net.IP(bytes.Join([][]byte{ipPrefix, ipBytes[:]}, nil))
82+
7483
var serialBytes [16]byte
7584
_, _ = rand.Read(serialBytes[:])
7685
serial := big.NewInt(0).SetBytes(serialBytes[:])
@@ -81,6 +90,7 @@ func ThrowAwayCert(t *testing.T, clk clock.Clock) (string, *x509.Certificate) {
8190
template := &x509.Certificate{
8291
SerialNumber: serial,
8392
DNSNames: []string{name},
93+
IPAddresses: []net.IP{ip},
8494
NotBefore: clk.Now(),
8595
NotAfter: clk.Now().Add(6 * 24 * time.Hour),
8696
IssuingCertificateURL: []string{"http://localhost:4001/acme/issuer-cert/1234"},

‎va/proto/va.pb.go‎

Lines changed: 45 additions & 56 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)