SA: Deprecate IgnoreAccountContacts feature flag

Author: aarongable

cmd/contact-auditor/main_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/jmhodges/clock"
+
 	corepb "github.com/letsencrypt/boulder/core/proto"
 	"github.com/letsencrypt/boulder/db"
 	blog "github.com/letsencrypt/boulder/log"
@@ -33,6 +34,11 @@ const (
 )
 
 func TestContactAuditor(t *testing.T) {
+	// Leave this technically in place until contact-auditor is fully deleted, but
+	// this test cannot function now that the SA never stores nor retrieves
+	// contacts.
+	t.Skip()
+
 	testCtx := setup(t)
 	defer testCtx.cleanUp()
 

cmd/expiration-mailer/main_test.go

@@ -417,6 +417,11 @@ func TestProcessCertsConnectError(t *testing.T) {
 }
 
 func TestFindExpiringCertificates(t *testing.T) {
+	// Leave this technically in place until expiration-mailer is fully deleted,
+	// but this test cannot function now that the SA never stores nor retrieves
+	// contacts.
+	t.Skip()
+
 	testCtx := setup(t, []time.Duration{time.Hour * 24, time.Hour * 24 * 4, time.Hour * 24 * 7})
 
 	addExpiringCerts(t, testCtx)
@@ -620,6 +625,11 @@ func countGroupsAtCapacity(group string, counter *prometheus.GaugeVec) int {
 }
 
 func TestFindCertsAtCapacity(t *testing.T) {
+	// Leave this technically in place until expiration-mailer is fully deleted,
+	// but this test cannot function now that the SA never stores nor retrieves
+	// contacts.
+	t.Skip()
+
 	testCtx := setup(t, []time.Duration{time.Hour * 24})
 
 	addExpiringCerts(t, testCtx)
@@ -776,6 +786,11 @@ func TestCertIsRenewed(t *testing.T) {
 }
 
 func TestLifetimeOfACert(t *testing.T) {
+	// Leave this technically in place until expiration-mailer is fully deleted,
+	// but this test cannot function now that the SA never stores nor retrieves
+	// contacts.
+	t.Skip()
+
 	testCtx := setup(t, []time.Duration{time.Hour * 24, time.Hour * 24 * 4, time.Hour * 24 * 7})
 	defer testCtx.cleanUp()
 
@@ -882,6 +897,11 @@ func TestDontFindRevokedCert(t *testing.T) {
 }
 
 func TestDedupOnRegistration(t *testing.T) {
+	// Leave this technically in place until expiration-mailer is fully deleted,
+	// but this test cannot function now that the SA never stores nor retrieves
+	// contacts.
+	t.Skip()
+
 	expiresIn := 96 * time.Hour
 	testCtx := setup(t, []time.Duration{expiresIn})
 

cmd/id-exporter/main.go

@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/jmhodges/clock"
+
 	"github.com/letsencrypt/boulder/cmd"
 	"github.com/letsencrypt/boulder/db"
 	"github.com/letsencrypt/boulder/features"
@@ -77,8 +78,7 @@ func (c idExporter) findIDs(ctx context.Context) (idExporterResults, error) {
 		`SELECT DISTINCT r.id
 		FROM registrations AS r
 			INNER JOIN certificates AS c on c.registrationID = r.id
-		WHERE r.contact NOT IN ('[]', 'null')
-			AND c.expires >= :expireCutoff;`,
+		WHERE c.expires >= :expireCutoff;`,
 		map[string]interface{}{
 			"expireCutoff": c.clk.Now().Add(-c.grace),
 		})

features/features.go

@@ -25,6 +25,7 @@ type Config struct {
 	EnforceMPIC                 bool
 	MPICFullResults             bool
 	UnsplitIssuance             bool
+	IgnoreAccountContacts       bool
 
 	// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
 	// GET requests. WARNING: This feature is a draft and highly unstable.
@@ -85,10 +86,6 @@ type Config struct {
 	// StoreARIReplacesInOrders causes the SA to store and retrieve the optional
 	// ARI replaces field in the orders table.
 	StoreARIReplacesInOrders bool
-
-	// IgnoreAccountContacts causes the SA to omit the contacts column when
-	// creating new account rows, and when retrieving existing account rows.
-	IgnoreAccountContacts bool
 }
 
 var fMu = new(sync.RWMutex)

ra/ra_test.go

@@ -485,8 +485,7 @@ func TestNewRegistration(t *testing.T) {
 		t.Fatalf("could not create new registration: %s", err)
 	}
 	test.AssertByteEquals(t, result.Key, acctKeyB)
-	test.Assert(t, len(result.Contact) == 1, "Wrong number of contacts")
-	test.Assert(t, mailto == (result.Contact)[0], "Contact didn't match")
+	test.Assert(t, len(result.Contact) == 0, "Wrong number of contacts")
 	test.Assert(t, result.Agreement == "", "Agreement didn't default empty")
 
 	reg, err := sa.GetRegistration(ctx, &sapb.RegistrationID{Id: result.Id})

sa/model.go

@@ -24,7 +24,6 @@ import (
 	corepb "github.com/letsencrypt/boulder/core/proto"
 	"github.com/letsencrypt/boulder/db"
 	berrors "github.com/letsencrypt/boulder/errors"
-	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/identifier"
 	"github.com/letsencrypt/boulder/probs"
@@ -61,7 +60,7 @@ func badJSONError(msg string, jsonData []byte, err error) error {
 	}
 }
 
-const regFields = "id, jwk, jwk_sha256, contact, agreement, createdAt, LockCol, status"
+const regFields = "id, jwk, jwk_sha256, agreement, createdAt, LockCol, status"
 
 // ClearEmail removes the provided email address from one specified registration. If
 // there are multiple email addresses present, it does not modify other ones. If the email
@@ -273,11 +272,13 @@ type regModel struct {
 	ID        int64     `db:"id"`
 	Key       []byte    `db:"jwk"`
 	KeySHA256 string    `db:"jwk_sha256"`
-	Contact   string    `db:"contact"`
 	Agreement string    `db:"agreement"`
 	CreatedAt time.Time `db:"createdAt"`
 	LockCol   int64
 	Status    string `db:"status"`
+	// Deprecated: never populated from the database, and always written to the
+	// database as just "[]".
+	Contact string `db:"contact"`
 }
 
 func registrationPbToModel(reg *corepb.Registration) (*regModel, error) {
@@ -294,18 +295,6 @@ func registrationPbToModel(reg *corepb.Registration) (*regModel, error) {
 		return nil, err
 	}
 
-	// We don't want to write literal JSON "null" strings into the database if the
-	// list of contact addresses is empty. Replace any possibly-`nil` slice with
-	// an empty JSON array. We don't need to check reg.ContactPresent, because
-	// we're going to write the whole object to the database anyway.
-	jsonContact := []byte("[]")
-	if len(reg.Contact) != 0 && !features.Get().IgnoreAccountContacts {
-		jsonContact, err = json.Marshal(reg.Contact)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	var createdAt time.Time
 	if !core.IsAnyNilOrZero(reg.CreatedAt) {
 		createdAt = reg.CreatedAt.AsTime()
@@ -315,7 +304,7 @@ func registrationPbToModel(reg *corepb.Registration) (*regModel, error) {
 		ID:        reg.Id,
 		Key:       reg.Key,
 		KeySHA256: sha,
-		Contact:   string(jsonContact),
+		Contact:   "[]",
 		Agreement: reg.Agreement,
 		CreatedAt: createdAt,
 		Status:    reg.Status,
@@ -327,18 +316,9 @@ func registrationModelToPb(reg *regModel) (*corepb.Registration, error) {
 		return nil, errors.New("incomplete Registration retrieved from DB")
 	}
 
-	contact := []string{}
-	if len(reg.Contact) > 0 && !features.Get().IgnoreAccountContacts {
-		err := json.Unmarshal([]byte(reg.Contact), &contact)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	return &corepb.Registration{
 		Id:        reg.ID,
 		Key:       reg.Key,
-		Contact:   contact,
 		Agreement: reg.Agreement,
 		CreatedAt: timestamppb.New(reg.CreatedAt.UTC()),
 		Status:    reg.Status,

sa/model_test.go

@@ -53,8 +53,6 @@ func TestRegistrationModelToPb(t *testing.T) {
 	test.AssertNotError(t, err, "Should pass")
 }
 
-func TestRegistrationPbToModel(t *testing.T) {}
-
 func TestAuthzModel(t *testing.T) {
 	// newTestAuthzPB returns a new *corepb.Authorization for `example.com` that
 	// is valid, and contains a single valid HTTP-01 challenge. These are the

sa/sa_test.go

@@ -201,7 +201,7 @@ func TestAddRegistration(t *testing.T) {
 		t.Fatalf("Couldn't create new registration: %s", err)
 	}
 	test.Assert(t, reg.Id != 0, "ID shouldn't be 0")
-	test.AssertDeepEquals(t, reg.Contact, contacts)
+	test.AssertEquals(t, len(reg.Contact), 0)
 
 	_, err = sa.GetRegistration(ctx, &sapb.RegistrationID{Id: 0})
 	test.AssertError(t, err, "Registration object for ID 0 was returned")
@@ -4461,6 +4461,7 @@ func newAcctKey(t *testing.T) []byte {
 }
 
 func TestUpdateRegistrationContact(t *testing.T) {
+	// TODO(#8199): Delete this.
 	sa, _, cleanUp := initSA(t)
 	defer cleanUp()
 
@@ -4517,13 +4518,13 @@ func TestUpdateRegistrationContact(t *testing.T) {
 			})
 			test.AssertNotError(t, err, "unexpected error for UpdateRegistrationContact()")
 			test.AssertEquals(t, updatedReg.Id, reg.Id)
-			test.AssertDeepEquals(t, updatedReg.Contact, tt.newContacts)
+			test.AssertEquals(t, len(updatedReg.Contact), 0)
 
 			refetchedReg, err := sa.GetRegistration(ctx, &sapb.RegistrationID{
 				Id: reg.Id,
 			})
 			test.AssertNotError(t, err, "retrieving registration")
-			test.AssertDeepEquals(t, refetchedReg.Contact, tt.newContacts)
+			test.AssertEquals(t, len(refetchedReg.Contact), 0)
 		})
 	}
 }

test/config-next/sa.json

@@ -49,8 +49,7 @@
 		},
 		"healthCheckInterval": "4s",
 		"features": {
-			"StoreARIReplacesInOrders": true,
-			"IgnoreAccountContacts": true
+			"StoreARIReplacesInOrders": true
 		}
 	},
 	"syslog": {

test/config/sa.json

@@ -51,7 +51,8 @@
 		},
 		"features": {
 			"MultipleCertificateProfiles": true,
-			"InsertAuthzsIndividually": true
+			"InsertAuthzsIndividually": true,
+			"IgnoreAccountContacts": true
 		}
 	},
 	"syslog": {

test/integration/account_test.go

@@ -6,8 +6,6 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
-	"os"
-	"slices"
 	"strings"
 	"testing"
 
@@ -68,17 +66,9 @@ func TestNewAccount(t *testing.T) {
 					t.Fatalf("NewAccount(tos: %t, contact: %#v) = %s, but want no err", tc.tos, tc.contact, err)
 				}
 
-				// In config-next, we're wholly ignoring account contacts
-				if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
-					if len(acct.Contact) != 0 {
-						t.Errorf("NewAccount(tos: %t, contact: %#v) = %#v, but want empty contacts", tc.tos, tc.contact, acct)
-					}
-				} else {
-					if !slices.Equal(acct.Contact, tc.contact) {
-						t.Errorf("NewAccount(tos: %t, contact: %#v) = %#v, but want contacts %q", tc.tos, tc.contact, acct, tc.contact)
-					}
+				if len(acct.Contact) != 0 {
+					t.Errorf("NewAccount(tos: %t, contact: %#v) = %#v, but want empty contacts", tc.tos, tc.contact, acct)
 				}
-
 			} else if tc.wantErr != "" {
 				if err == nil {
 					t.Fatalf("NewAccount(tos: %t, contact: %#v) = %#v, but want error %q", tc.tos, tc.contact, acct, tc.wantErr)
@@ -177,44 +167,4 @@ func TestAccountDeactivate(t *testing.T) {
 	if got.Status != string(core.StatusDeactivated) {
 		t.Errorf("account deactivation should have set status to %q, instead got %q", core.StatusDeactivated, got.Status)
 	}
-
-	// TODO(#5554): Check that the contacts have been cleared. We can't do this
-	// today because eggsampler/acme unmarshals the WFE's response into the same
-	// account object as it used to make the request, and a wholly missing
-	// contacts field doesn't overwrite whatever eggsampler was holding in memory.
-}
-
-func TestAccountUpdate_UnspecifiedContacts(t *testing.T) {
-	t.Parallel()
-
-	c, err := acme.NewClient("http://boulder.service.consul:4001/directory")
-	if err != nil {
-		t.Fatalf("failed to connect to acme directory: %s", err)
-	}
-
-	acctKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	if err != nil {
-		t.Fatalf("failed to generate account key: %s", err)
-	}
-
-	acct, err := c.NewAccount(acctKey, false, true, "mailto:example@"+random_domain())
-	if err != nil {
-		t.Fatalf("failed to create initial account: %s", err)
-	}
-
-	// This request does not include the Contact field, meaning that the contacts
-	// should remain unchanged (i.e. not be removed).
-	acct, err = c.UpdateAccount(acct)
-	if err != nil {
-		t.Errorf("failed to no-op update account: %s", err)
-	}
-	if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
-		if len(acct.Contact) != 0 {
-			t.Errorf("unexpected number of contacts: want 0, got %d", len(acct.Contact))
-		}
-	} else {
-		if len(acct.Contact) != 1 {
-			t.Errorf("unexpected number of contacts: want 1, got %d", len(acct.Contact))
-		}
-	}
 }