SA: Stop storing and retrieving contacts (#8198)

Add a feature flag "IgnoreAccountContacts" which has two effects in the
SA:
- When a new account is created, don't insert any contacts provided; and
- When an account is retrieved, ignore any contacts already present.

This causes boulder to act as though all accounts have no associated
contacts, and is the first step towards being able to drop the contacts
from the database entirely.

Part of https://github.com/letsencrypt/boulder/issues/8176

features/features.go

@@ -85,6 +85,10 @@ 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)

sa/model.go

@@ -24,6 +24,7 @@ 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"
@@ -298,7 +299,7 @@ func registrationPbToModel(reg *corepb.Registration) (*regModel, error) {
 	// 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 {
+	if len(reg.Contact) != 0 && !features.Get().IgnoreAccountContacts {
 		jsonContact, err = json.Marshal(reg.Contact)
 		if err != nil {
 			return nil, err
@@ -327,7 +328,7 @@ func registrationModelToPb(reg *regModel) (*corepb.Registration, error) {
 	}
 
 	contact := []string{}
-	if len(reg.Contact) > 0 {
+	if len(reg.Contact) > 0 && !features.Get().IgnoreAccountContacts {
 		err := json.Unmarshal([]byte(reg.Contact), &contact)
 		if err != nil {
 			return nil, err

test/config-next/sa.json

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

test/integration/account_test.go

@@ -6,6 +6,9 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
+	"os"
+	"slices"
+	"strings"
 	"testing"
 
 	"github.com/eggsampler/acme/v3"
@@ -13,6 +16,138 @@ import (
 	"github.com/letsencrypt/boulder/core"
 )
 
+// TestNewAccount tests that various new-account requests are handled correctly.
+// It does not test malform account contacts, as those are covered by
+// TestAccountEmailError in errors_test.go.
+func TestNewAccount(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)
+	}
+
+	for _, tc := range []struct {
+		name    string
+		tos     bool
+		contact []string
+		wantErr string
+	}{
+		{
+			name:    "No TOS agreement",
+			tos:     false,
+			contact: nil,
+			wantErr: "must agree to terms of service",
+		},
+		{
+			name:    "No contacts",
+			tos:     true,
+			contact: nil,
+		},
+		{
+			name:    "One contact",
+			tos:     true,
+			contact: []string{"mailto:single@chisel.com"},
+		},
+		{
+			name:    "Many contacts",
+			tos:     true,
+			contact: []string{"mailto:one@chisel.com", "mailto:two@chisel.com", "mailto:three@chisel.com"},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+			if err != nil {
+				t.Fatalf("failed to generate account key: %s", err)
+			}
+
+			acct, err := c.NewAccount(key, false, tc.tos, tc.contact...)
+
+			if tc.wantErr == "" {
+				if err != nil {
+					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)
+					}
+				}
+
+			} 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)
+				}
+
+				if !strings.Contains(err.Error(), tc.wantErr) {
+					t.Errorf("NewAccount(tos: %t, contact: %#v) = %q, but want error %q", tc.tos, tc.contact, err, tc.wantErr)
+				}
+			}
+		})
+	}
+}
+
+func TestNewAccount_DuplicateKey(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)
+	}
+
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("failed to generate account key: %s", err)
+	}
+
+	// OnlyReturnExisting: true with a never-before-used key should result in an error.
+	acct, err := c.NewAccount(key, true, true)
+	if err == nil {
+		t.Fatalf("NewAccount(key: 1, ore: true) = %#v, but want error notFound", acct)
+	}
+
+	// Create an account.
+	acct, err = c.NewAccount(key, false, true)
+	if err != nil {
+		t.Fatalf("NewAccount(key: 1, ore: false) = %#v, but want success", err)
+	}
+
+	// A duplicate request should just return the same account.
+	acct, err = c.NewAccount(key, false, true)
+	if err != nil {
+		t.Fatalf("NewAccount(key: 1, ore: false) = %#v, but want success", err)
+	}
+
+	// Specifying OnlyReturnExisting should do the same.
+	acct, err = c.NewAccount(key, true, true)
+	if err != nil {
+		t.Fatalf("NewAccount(key: 1, ore: true) = %#v, but want success", err)
+	}
+
+	// Deactivate the account.
+	acct, err = c.DeactivateAccount(acct)
+	if err != nil {
+		t.Fatalf("DeactivateAccount(acct: 1) = %#v, but want success", err)
+	}
+
+	// Now a new account request should return an error.
+	acct, err = c.NewAccount(key, false, true)
+	if err == nil {
+		t.Fatalf("NewAccount(key: 1, ore: false) = %#v, but want error deactivated", acct)
+	}
+
+	// Specifying OnlyReturnExisting should do the same.
+	acct, err = c.NewAccount(key, true, true)
+	if err == nil {
+		t.Fatalf("NewAccount(key: 1, ore: true) = %#v, but want error deactivated", acct)
+	}
+}
+
 // TestAccountDeactivate tests that account deactivation works. It does not test
 // that we reject requests for other account statuses, because eggsampler/acme
 // wisely does not allow us to construct such malformed requests.
@@ -73,7 +208,13 @@ func TestAccountUpdate_UnspecifiedContacts(t *testing.T) {
 	if err != nil {
 		t.Errorf("failed to no-op update account: %s", err)
 	}
-	if len(acct.Contact) != 1 {
-		t.Errorf("unexpected number of contacts: want 1, got %d", len(acct.Contact))
+	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))
+		}
 	}
 }

test/integration/admin_test.go

@@ -16,6 +16,12 @@ import (
 
 func TestAdminClearEmail(t *testing.T) {
 	t.Parallel()
+	if os.Getenv("BOULDER_CONFIG_DIR") == "test/config-next" {
+		// This test relies on contact addresses being persisted, which we no longer
+		// do in config-next.
+		t.Skip()
+	}
+
 	os.Setenv("DIRECTORY", "http://boulder.service.consul:4001/directory")
 
 	// Note that `example@mail.example.letsencrypt.org` is a substring of `long-example@mail.example.letsencrypt.org`.

test/integration/revocation_test.go

@@ -622,6 +622,12 @@ func TestRevokeWithKeyCompromiseBlocksKey(t *testing.T) {
 }
 
 func TestBadKeyRevoker(t *testing.T) {
+	if os.Getenv("BOULDER_CONFIG_DIR") == "test/config-next" {
+		// This test relies on contact addresses being persisted, and cares about
+		// emails being sent, which we no longer do in config-next.
+		t.Skip()
+	}
+
 	// Both accounts have two email addresses, one of which is shared between
 	// them. All three addresses should receive mail, because the revocation
 	// request is signed by the certificate key, not an account key, so we don't

test/v2_integration.py

@@ -1147,6 +1147,11 @@ def test_ocsp_exp_unauth():
         raise(Exception("timed out waiting for unauthorized OCSP response for expired certificate. Last error: {}".format(last_error)))
 
 def test_expiration_mailer():
+    # This test relies on contact addresses being persisted, which we no longer
+    # do in config-next.
+    if 'config-next' in os.environ['BOULDER_CONFIG_DIR']:
+        return
+
     email_addr = "integration.%x@letsencrypt.org" % random.randrange(2**16)
     order = chisel2.auth_and_issue([random_domain()], email=email_addr)
     cert = parse_cert(order)
@@ -1209,38 +1214,6 @@ def test_caa_extensions():
     chisel2.expect_problem("urn:ietf:params:acme:error:caa", lambda: chisel2.auth_and_issue(["accounturi.good-caa-reserved.com"]))
     chisel2.auth_and_issue(["accounturi.good-caa-reserved.com"], client=client)
 
-def test_new_account():
-    """
-    Test creating new accounts with no email, empty email, one email, and a
-    tuple of multiple emails.
-    """
-    for contact in (None, (), ("mailto:single@chisel.com",), ("mailto:one@chisel.com", "mailto:two@chisel.com")):
-        # We don't use `chisel2.make_client` or `messages.NewRegistration.from_data`
-        # here because they do too much client-side processing to make the
-        # contact addresses look "nice".
-        client = chisel2.uninitialized_client()
-        result = client.new_account(messages.NewRegistration(contact=contact, terms_of_service_agreed=True))
-        actual = result.body.contact
-        if contact is not None and contact != actual:
-            raise(Exception("New Account failed: expected contact %s, got %s" % (contact, actual)))
-
-def test_account_update():
-    """
-    Create a new ACME client/account with one contact email. Then update the
-    account to a different contact emails.
-    """
-    for contact in (None, (), ("mailto:single@chisel.com",), ("mailto:one@chisel.com", "mailto:two@chisel.com")):
-        # We don't use `chisel2.update_email` or `messages.NewRegistration.from_data`
-        # here because they do too much client-side processing to make the
-        # contact addresses look "nice".
-        print()
-        client = chisel2.make_client()
-        update = client.net.account.update(body=client.net.account.body.update(contact=contact))
-        result = client.update_registration(update)
-        actual = result.body.contact
-        if contact is not None and contact != actual:
-            raise(Exception("New Account failed: expected contact %s, got %s" % (contact, actual)))
-
 def test_renewal_exemption():
     """
     Under a single domain, issue two certificates for different subdomains of