Remove logging of contact email addresses (#7833)

Fixes #7801

Author: aarongable

cmd/bad-key-revoker/main.go

@@ -284,7 +284,7 @@ func (bkr *badKeyRevoker) revokeCerts(revokerEmails []string, emailToCerts map[s
 		err := bkr.sendMessage(email, revokedSerials)
 		if err != nil {
 			mailErrors.Inc()
-			bkr.logger.Errf("failed to send message to %q: %s", email, err)
+			bkr.logger.Errf("failed to send message: %s", err)
 			continue
 		}
 	}
@@ -370,9 +370,11 @@ func (bkr *badKeyRevoker) invoke(ctx context.Context) (bool, error) {
 		}
 	}
 
-	revokerEmails := idToEmails[unchecked.RevokedBy]
-	bkr.logger.AuditInfo(fmt.Sprintf("revoking certs. revoked emails=%v, emailsToCerts=%s",
-		revokerEmails, emailsToCerts))
+	var serials []string
+	for _, cert := range unrevokedCerts {
+		serials = append(serials, cert.Serial)
+	}
+	bkr.logger.AuditInfo(fmt.Sprintf("revoking serials %v for key with hash %s", serials, unchecked.KeyHash))
 
 	// revoke each certificate and send emails to their owners
 	err = bkr.revokeCerts(idToEmails[unchecked.RevokedBy], emailsToCerts)

cmd/expiration-mailer/main.go

@@ -106,7 +106,7 @@ func (lim *limiter) check(address string) error {
 
 	lim.maybeBumpDay()
 	if lim.counts[address] >= lim.limit {
-		return fmt.Errorf("daily mail limit exceeded for %q", address)
+		return errors.New("daily mail limit exceeded for this email address")
 	}
 	return nil
 }
@@ -155,7 +155,7 @@ func (m *mailer) sendNags(conn bmail.Conn, contacts []string, certs []*x509.Cert
 	for _, contact := range contacts {
 		parsed, err := url.Parse(contact)
 		if err != nil {
-			m.log.Errf("parsing contact email %s: %s", contact, err)
+			m.log.Errf("parsing contact email: %s", err)
 			continue
 		}
 		if parsed.Scheme != "mailto" {
@@ -164,7 +164,7 @@ func (m *mailer) sendNags(conn bmail.Conn, contacts []string, certs []*x509.Cert
 		address := parsed.Opaque
 		err = policy.ValidEmail(address)
 		if err != nil {
-			m.log.Debugf("skipping invalid email %q: %s", address, err)
+			m.log.Debugf("skipping invalid email: %s", err)
 			continue
 		}
 		err = m.addressLimiter.check(address)
@@ -249,28 +249,24 @@ func (m *mailer) sendNags(conn bmail.Conn, contacts []string, certs []*x509.Cert
 	}
 
 	logItem := struct {
-		Rcpt              []string
 		DaysToExpiration  int
 		TruncatedDNSNames []string
 		TruncatedSerials  []string
 	}{
-		Rcpt:              emails,
 		DaysToExpiration:  email.DaysToExpiration,
 		TruncatedDNSNames: truncatedDomains,
 		TruncatedSerials:  truncatedSerials,
 	}
 	logStr, err := json.Marshal(logItem)
 	if err != nil {
-		m.log.Errf("logItem could not be serialized to JSON. Raw: %+v", logItem)
-		return err
+		return fmt.Errorf("failed to serialize log line: %w", err)
 	}
-	m.log.Infof("attempting send JSON=%s", string(logStr))
+	m.log.Infof("attempting send for JSON=%s", string(logStr))
 
 	startSending := m.clk.Now()
 	err = conn.SendMail(emails, subjBuf.String(), msgBuf.String())
 	if err != nil {
-		m.log.Errf("failed send JSON=%s err=%s", string(logStr), err)
-		return err
+		return fmt.Errorf("failed send for %s: %w", string(logStr), err)
 	}
 	finishSending := m.clk.Now()
 	elapsed := finishSending.Sub(startSending)

cmd/expiration-mailer/main_test.go

@@ -180,13 +180,10 @@ func TestSendNags(t *testing.T) {
 	test.AssertErrorIs(t, err, errNoValidEmail)
 	test.AssertEquals(t, len(mc.Messages), 0)
 
-	sendLogs := log.GetAllMatching("INFO: attempting send JSON=.*")
+	sendLogs := log.GetAllMatching("INFO: attempting send for JSON=.*")
 	if len(sendLogs) != 2 {
 		t.Errorf("expected 2 'attempting send' log line, got %d: %s", len(sendLogs), strings.Join(sendLogs, "\n"))
 	}
-	if !strings.Contains(sendLogs[0], `"Rcpt":["[email protected]"]`) {
-		t.Errorf("expected first 'attempting send' log line to have one address, got %q", sendLogs[0])
-	}
 	if !strings.Contains(sendLogs[0], `"TruncatedSerials":["000000000000000000000000000000000304"]`) {
 		t.Errorf("expected first 'attempting send' log line to have one serial, got %q", sendLogs[0])
 	}
@@ -196,6 +193,9 @@ func TestSendNags(t *testing.T) {
 	if !strings.Contains(sendLogs[0], `"TruncatedDNSNames":["example.com"]`) {
 		t.Errorf("expected first 'attempting send' log line to have 1 domain, 'example.com', got %q", sendLogs[0])
 	}
+	if strings.Contains(sendLogs[0], `"@gmail.com"`) {
+		t.Errorf("log line should not contain email address, got %q", sendLogs[0])
+	}
 }
 
 func TestSendNagsAddressLimited(t *testing.T) {

policy/pa.go

@@ -336,23 +336,18 @@ var forbiddenMailDomains = map[string]bool{
 func ValidEmail(address string) error {
 	email, err := mail.ParseAddress(address)
 	if err != nil {
-		if len(address) > 254 {
-			address = address[:254] + "..."
-		}
-		return berrors.InvalidEmailError("%q is not a valid e-mail address", address)
+		return berrors.InvalidEmailError("unable to parse email address")
 	}
 	splitEmail := strings.SplitN(email.Address, "@", -1)
 	domain := strings.ToLower(splitEmail[len(splitEmail)-1])
 	err = validNonWildcardDomain(domain)
 	if err != nil {
-		return berrors.InvalidEmailError(
-			"contact email %q has invalid domain : %s",
-			email.Address, err)
+		return berrors.InvalidEmailError("contact email has invalid domain: %s", err)
 	}
 	if forbiddenMailDomains[domain] {
-		return berrors.InvalidEmailError(
-			"invalid contact domain. Contact emails @%s are forbidden",
-			domain)
+		// We're okay including the domain in the error message here because this
+		// case occurs only for a small block-list of domains listed above.
+		return berrors.InvalidEmailError("contact email has forbidden domain %q", domain)
 	}
 	return nil
 }

policy/pa_test.go

@@ -473,16 +473,16 @@ func TestMalformedExactBlocklist(t *testing.T) {
 
 func TestValidEmailError(t *testing.T) {
 	err := ValidEmail("(๑•́ ω •̀๑)")
-	test.AssertEquals(t, err.Error(), "\"(๑•́ ω •̀๑)\" is not a valid e-mail address")
+	test.AssertEquals(t, err.Error(), "unable to parse email address")
 
 	err = ValidEmail("[email protected] #replace with real email")
-	test.AssertEquals(t, err.Error(), "\"[email protected] #replace with real email\" is not a valid e-mail address")
+	test.AssertEquals(t, err.Error(), "unable to parse email address")
 
 	err = ValidEmail("[email protected]")
-	test.AssertEquals(t, err.Error(), "invalid contact domain. Contact emails @example.com are forbidden")
+	test.AssertEquals(t, err.Error(), "contact email has forbidden domain \"example.com\"")
 
 	err = ValidEmail("[email protected]")
-	test.AssertEquals(t, err.Error(), "contact email \"[email protected]\" has invalid domain : Domain name contains an invalid character")
+	test.AssertEquals(t, err.Error(), "contact email has invalid domain: Domain name contains an invalid character")
 }
 
 func TestCheckAuthzChallenges(t *testing.T) {

ra/ra.go

@@ -464,19 +464,16 @@ func (ra *RegistrationAuthorityImpl) validateContacts(contacts []string) error {
 			return berrors.InvalidEmailError("invalid contact")
 		}
 		if parsed.Scheme != "mailto" {
-			return berrors.UnsupportedContactError("contact method %q is not supported", parsed.Scheme)
+			return berrors.UnsupportedContactError("only contact scheme 'mailto:' is supported")
 		}
 		if parsed.RawQuery != "" || contact[len(contact)-1] == '?' {
-			return berrors.InvalidEmailError("contact email %q contains a question mark", contact)
+			return berrors.InvalidEmailError("contact email contains a question mark")
 		}
 		if parsed.Fragment != "" || contact[len(contact)-1] == '#' {
-			return berrors.InvalidEmailError("contact email %q contains a '#'", contact)
+			return berrors.InvalidEmailError("contact email contains a '#'")
 		}
 		if !core.IsASCII(contact) {
-			return berrors.InvalidEmailError(
-				"contact email [%q] contains non-ASCII characters",
-				contact,
-			)
+			return berrors.InvalidEmailError("contact email contains non-ASCII characters")
 		}
 		err = policy.ValidEmail(parsed.Opaque)
 		if err != nil {
@@ -490,10 +487,7 @@ func (ra *RegistrationAuthorityImpl) validateContacts(contacts []string) error {
 	// That means the largest marshalled JSON value we can store is 191 bytes.
 	const maxContactBytes = 191
 	if jsonBytes, err := json.Marshal(contacts); err != nil {
-		// This shouldn't happen with a simple []string but if it does we want the
-		// error to be logged internally but served as a 500 to the user so we
-		// return a bare error and not a berror here.
-		return fmt.Errorf("failed to marshal reg.Contact to JSON: %#v", contacts)
+		return fmt.Errorf("failed to marshal reg.Contact to JSON: %w", err)
 	} else if len(jsonBytes) >= maxContactBytes {
 		return berrors.InvalidEmailError(
 			"too many/too long contact(s). Please use shorter or fewer email addresses")

test/integration/errors_test.go

@@ -66,19 +66,19 @@ func TestAccountEmailError(t *testing.T) {
 			name:               "empty proto",
 			contacts:           []string{"mailto:[email protected]", " "},
 			expectedProbType:   "urn:ietf:params:acme:error:unsupportedContact",
-			expectedProbDetail: `contact method "" is not supported`,
+			expectedProbDetail: `only contact scheme 'mailto:' is supported`,
 		},
 		{
 			name:               "empty mailto",
 			contacts:           []string{"mailto:[email protected]", "mailto:"},
 			expectedProbType:   "urn:ietf:params:acme:error:invalidContact",
-			expectedProbDetail: `"" is not a valid e-mail address`,
+			expectedProbDetail: `unable to parse email address`,
 		},
 		{
 			name:               "non-ascii mailto",
 			contacts:           []string{"mailto:[email protected]", "mailto:cpu@l̴etsencrypt.org"},
 			expectedProbType:   "urn:ietf:params:acme:error:invalidContact",
-			expectedProbDetail: `contact email ["mailto:cpu@l̴etsencrypt.org"] contains non-ASCII characters`,
+			expectedProbDetail: `contact email contains non-ASCII characters`,
 		},
 		{
 			name:               "too many contacts",
@@ -90,25 +90,25 @@ func TestAccountEmailError(t *testing.T) {
 			name:               "invalid contact",
 			contacts:           []string{"mailto:[email protected]", "mailto:a@"},
 			expectedProbType:   "urn:ietf:params:acme:error:invalidContact",
-			expectedProbDetail: `"a@" is not a valid e-mail address`,
+			expectedProbDetail: `unable to parse email address`,
 		},
 		{
 			name:               "forbidden contact domain",
 			contacts:           []string{"mailto:[email protected]", "mailto:[email protected]"},
 			expectedProbType:   "urn:ietf:params:acme:error:invalidContact",
-			expectedProbDetail: "invalid contact domain. Contact emails @example.com are forbidden",
+			expectedProbDetail: "contact email has forbidden domain \"example.com\"",
 		},
 		{
 			name:               "contact domain invalid TLD",
 			contacts:           []string{"mailto:[email protected]", "mailto:[email protected]"},
 			expectedProbType:   "urn:ietf:params:acme:error:invalidContact",
-			expectedProbDetail: `contact email "[email protected]" has invalid domain : Domain name does not end with a valid public suffix (TLD)`,
+			expectedProbDetail: `contact email has invalid domain: Domain name does not end with a valid public suffix (TLD)`,
 		},
 		{
 			name:               "contact domain invalid",
 			contacts:           []string{"mailto:[email protected]", "mailto:a@example./.com"},
 			expectedProbType:   "urn:ietf:params:acme:error:invalidContact",
-			expectedProbDetail: "contact email \"a@example./.com\" has invalid domain : Domain name contains an invalid character",
+			expectedProbDetail: "contact email has invalid domain: Domain name contains an invalid character",
 		},
 		{
 			name: "too long contact",