Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wfe/sa/features: Deprecate TrackReplacementCertificatesARI #7766

Merged
merged 2 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions features/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type Config struct {
CertCheckerRequiresCorrespondence bool
ECDSAForAll bool
CheckRenewalExemptionAtWFE bool
TrackReplacementCertificatesARI bool
UseKvLimitsForNewAccount bool

// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
Expand Down Expand Up @@ -73,16 +74,6 @@ type Config struct {
// maxRemoteValidationFailures. Only used when EnforceMultiCAA is true.
MultiCAAFullResults bool

// TrackReplacementCertificatesARI, when enabled, triggers the following
// behavior:
// - SA.NewOrderAndAuthzs: upon receiving a NewOrderRequest with a
// 'replacesSerial' value, will create a new entry in the 'replacement
// Orders' table. This will occur inside of the new order transaction.
// - SA.FinalizeOrder will update the 'replaced' column of any row with
// a 'orderID' matching the finalized order to true. This will occur
// inside of the finalize (order) transaction.
TrackReplacementCertificatesARI bool

// MultipleCertificateProfiles, when enabled, triggers the following
// behavior:
// - SA.NewOrderAndAuthzs: upon receiving a NewOrderRequest with a
Expand Down
7 changes: 0 additions & 7 deletions sa/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"google.golang.org/protobuf/types/known/timestamppb"

"github.com/letsencrypt/boulder/db"
"github.com/letsencrypt/boulder/features"
"github.com/letsencrypt/boulder/grpc"
"github.com/letsencrypt/boulder/probs"
"github.com/letsencrypt/boulder/test/vars"
Expand Down Expand Up @@ -461,9 +460,6 @@ func TestAddReplacementOrder(t *testing.T) {
sa, _, cleanUp := initSA(t)
defer cleanUp()

features.Set(features.Config{TrackReplacementCertificatesARI: true})
defer features.Reset()

oldCertSerial := "1234567890"
orderId := int64(1337)
orderExpires := time.Now().Add(24 * time.Hour).UTC().Truncate(time.Second)
Expand Down Expand Up @@ -513,9 +509,6 @@ func TestSetReplacementOrderFinalized(t *testing.T) {
sa, _, cleanUp := initSA(t)
defer cleanUp()

features.Set(features.Config{TrackReplacementCertificatesARI: true})
defer features.Reset()

oldCertSerial := "1234567890"
orderId := int64(1337)
orderExpires := time.Now().Add(24 * time.Hour).UTC().Truncate(time.Second)
Expand Down
8 changes: 3 additions & 5 deletions sa/sa.go
Original file line number Diff line number Diff line change
Expand Up @@ -739,11 +739,9 @@ func (ssa *SQLStorageAuthority) FinalizeOrder(ctx context.Context, req *sapb.Fin
return nil, err
}

if features.Get().TrackReplacementCertificatesARI {
err = setReplacementOrderFinalized(ctx, tx, req.Id)
if err != nil {
return nil, err
}
err = setReplacementOrderFinalized(ctx, tx, req.Id)
if err != nil {
return nil, err
}

return nil, nil
Expand Down
3 changes: 0 additions & 3 deletions sa/sa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4059,9 +4059,6 @@ func TestReplacementOrderExists(t *testing.T) {
sa, fc, cleanUp := initSA(t)
defer cleanUp()

features.Set(features.Config{TrackReplacementCertificatesARI: true})
defer features.Reset()

oldCertSerial := "1234567890"

// Check that a non-existent replacement order does not exist.
Expand Down
1 change: 0 additions & 1 deletion test/config-next/sa.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
"healthCheckInterval": "4s",
"features": {
"MultipleCertificateProfiles": true,
"TrackReplacementCertificatesARI": true,
"DisableLegacyLimitWrites": true,
"InsertAuthzsIndividually": true
}
Expand Down
1 change: 0 additions & 1 deletion test/config-next/wfe2.json
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@
},
"features": {
"ServeRenewalInfo": true,
"TrackReplacementCertificatesARI": true,
"CheckIdentifiersPaused": true,
"UseKvLimitsForNewOrder": true
},
Expand Down
4 changes: 3 additions & 1 deletion test/config/sa.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@
}
}
},
"features": {}
"features": {
"TrackReplacementCertificatesARI": true
}
},
"syslog": {
"stdoutlevel": 6,
Expand Down
1 change: 1 addition & 0 deletions test/config/wfe2.json
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
"pendingAuthorizationLifetimeDays": 7,
"features": {
"ServeRenewalInfo": true,
"TrackReplacementCertificatesARI": true,
"CheckRenewalExemptionAtWFE": true,
"UseKvLimitsForNewAccount": true
}
Expand Down
33 changes: 11 additions & 22 deletions test/integration/ari_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"crypto/rand"
"crypto/x509/pkix"
"math/big"
"os"
"testing"
"time"

Expand Down Expand Up @@ -50,27 +49,17 @@ func TestARI(t *testing.T) {
test.AssertEquals(t, ari.SuggestedWindow.End.Sub(time.Now()).Round(time.Hour), 1463*time.Hour)
test.AssertEquals(t, ari.RetryAfter.Sub(time.Now()).Round(time.Hour), 6*time.Hour)

// TODO(@pgporada): Clean this up when 'test/config/{sa,wfe2}.json' sets
// TrackReplacementCertificatesARI=true.
if os.Getenv("BOULDER_CONFIG_DIR") == "test/config-next" {
// Make a new order which indicates that it replaces the cert issued above.
_, order, err := makeClientAndOrder(client, key, []string{name}, true, cert)
test.AssertNotError(t, err, "failed to issue test cert")
replaceID, err := acme.GenerateARICertID(cert)
test.AssertNotError(t, err, "failed to generate ARI certID")
test.AssertEquals(t, order.Replaces, replaceID)
test.AssertNotEquals(t, order.Replaces, "")

// Try it again and verify it fails
_, order, err = makeClientAndOrder(client, key, []string{name}, true, cert)
test.AssertError(t, err, "subsequent ARI replacements for a replaced cert should fail, but didn't")
} else {
// ARI is disabled so we only use the client to POST the replacement
// order, but we never finalize it.
replacementOrder, err := client.ReplacementOrder(client.Account, cert, []acme.Identifier{{Type: "dns", Value: name}})
test.AssertNotError(t, err, "ARI replacement request should have succeeded")
test.AssertNotEquals(t, replacementOrder.Replaces, "")
}
// Make a new order which indicates that it replaces the cert issued above.
_, order, err := makeClientAndOrder(client, key, []string{name}, true, cert)
test.AssertNotError(t, err, "failed to issue test cert")
replaceID, err := acme.GenerateARICertID(cert)
test.AssertNotError(t, err, "failed to generate ARI certID")
test.AssertEquals(t, order.Replaces, replaceID)
test.AssertNotEquals(t, order.Replaces, "")

// Try it again and verify it fails
_, order, err = makeClientAndOrder(client, key, []string{name}, true, cert)
test.AssertError(t, err, "subsequent ARI replacements for a replaced cert should fail, but didn't")

// Revoke the cert and re-request ARI. The renewal window should now be in
// the past indicating to the client that a renewal should happen
Expand Down
20 changes: 8 additions & 12 deletions wfe2/wfe.go
Original file line number Diff line number Diff line change
Expand Up @@ -2348,12 +2348,10 @@ func (wfe *WebFrontEndImpl) NewOrder(

var replaces string
var isARIRenewal bool
if features.Get().TrackReplacementCertificatesARI {
replaces, isARIRenewal, err = wfe.validateReplacementOrder(ctx, acct, names, newOrderRequest.Replaces)
if err != nil {
wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While validating order as a replacement an error occurred"), err)
return
}
replaces, isARIRenewal, err = wfe.validateReplacementOrder(ctx, acct, names, newOrderRequest.Replaces)
if err != nil {
wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While validating order as a replacement an error occurred"), err)
return
}

var isRenewal bool
Expand Down Expand Up @@ -2391,12 +2389,10 @@ func (wfe *WebFrontEndImpl) NewOrder(
var newOrderSuccessful bool
var errIsRateLimit bool
defer func() {
if features.Get().TrackReplacementCertificatesARI {
wfe.stats.ariReplacementOrders.With(prometheus.Labels{
"isReplacement": fmt.Sprintf("%t", replaces != ""),
"limitsExempt": fmt.Sprintf("%t", isARIRenewal),
}).Inc()
}
wfe.stats.ariReplacementOrders.With(prometheus.Labels{
"isReplacement": fmt.Sprintf("%t", replaces != ""),
"limitsExempt": fmt.Sprintf("%t", isARIRenewal),
}).Inc()

if !newOrderSuccessful && !errIsRateLimit && refundLimits != nil {
go refundLimits()
Expand Down
1 change: 0 additions & 1 deletion wfe2/wfe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4007,7 +4007,6 @@ func makeARICertID(leaf *x509.Certificate) (string, error) {

func TestCountNewOrderWithReplaces(t *testing.T) {
wfe, _, signer := setupWFE(t)
features.Set(features.Config{TrackReplacementCertificatesARI: true})

expectExpiry := time.Now().AddDate(0, 0, 1)
var expectAKID []byte
Expand Down
Loading