From 640c8a7fd50ca7e5ce5c0c07b433f64af5056092 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 20 Nov 2024 13:26:03 -0800 Subject: [PATCH 1/4] Remove logging of contact email addresses --- cmd/bad-key-revoker/main.go | 10 ++++++---- cmd/expiration-mailer/main.go | 16 ++++++---------- policy/pa.go | 13 +++---------- policy/pa_test.go | 8 ++++---- ra/ra.go | 16 +++++----------- 5 files changed, 24 insertions(+), 39 deletions(-) diff --git a/cmd/bad-key-revoker/main.go b/cmd/bad-key-revoker/main.go index e7015e0c848..530b6be420d 100644 --- a/cmd/bad-key-revoker/main.go +++ b/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)) + serials := make([]string, 0, len(unrevokedCerts)) + 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) diff --git a/cmd/expiration-mailer/main.go b/cmd/expiration-mailer/main.go index 46fa939a61b..664a58067ae 100644 --- a/cmd/expiration-mailer/main.go +++ b/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") } 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) diff --git a/policy/pa.go b/policy/pa.go index 592d26a0757..fa266e00737 100644 --- a/policy/pa.go +++ b/policy/pa.go @@ -336,23 +336,16 @@ 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 e-mail 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) + return berrors.InvalidEmailError("contact email has forbidden domain") } return nil } diff --git a/policy/pa_test.go b/policy/pa_test.go index 3c994a81db4..f13878281cc 100644 --- a/policy/pa_test.go +++ b/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 e-mail address") err = ValidEmail("john.smith@gmail.com #replace with real email") - test.AssertEquals(t, err.Error(), "\"john.smith@gmail.com #replace with real email\" is not a valid e-mail address") + test.AssertEquals(t, err.Error(), "unable to parse e-mail address") err = ValidEmail("example@example.com") - test.AssertEquals(t, err.Error(), "invalid contact domain. Contact emails @example.com are forbidden") + test.AssertEquals(t, err.Error(), "contact email has forbidden domain") err = ValidEmail("example@-foobar.com") - test.AssertEquals(t, err.Error(), "contact email \"example@-foobar.com\" 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) { diff --git a/ra/ra.go b/ra/ra.go index 7564a1395ae..149239cc986 100644 --- a/ra/ra.go +++ b/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") From f9608a8f418e2571e4b8d473581a47fdb028ebb0 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 21 Nov 2024 14:09:22 -0800 Subject: [PATCH 2/4] Update unit and integration tests --- cmd/expiration-mailer/main_test.go | 8 ++++---- policy/pa.go | 2 +- policy/pa_test.go | 4 ++-- test/integration/errors_test.go | 14 +++++++------- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/cmd/expiration-mailer/main_test.go b/cmd/expiration-mailer/main_test.go index 407c76b866a..cc9bef768e7 100644 --- a/cmd/expiration-mailer/main_test.go +++ b/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":["rolandshoemaker@gmail.com"]`) { - 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) { diff --git a/policy/pa.go b/policy/pa.go index fa266e00737..dab7e80c200 100644 --- a/policy/pa.go +++ b/policy/pa.go @@ -336,7 +336,7 @@ var forbiddenMailDomains = map[string]bool{ func ValidEmail(address string) error { email, err := mail.ParseAddress(address) if err != nil { - return berrors.InvalidEmailError("unable to parse e-mail address") + return berrors.InvalidEmailError("unable to parse email address") } splitEmail := strings.SplitN(email.Address, "@", -1) domain := strings.ToLower(splitEmail[len(splitEmail)-1]) diff --git a/policy/pa_test.go b/policy/pa_test.go index f13878281cc..853c9870555 100644 --- a/policy/pa_test.go +++ b/policy/pa_test.go @@ -473,10 +473,10 @@ func TestMalformedExactBlocklist(t *testing.T) { func TestValidEmailError(t *testing.T) { err := ValidEmail("(๑•́ ω •̀๑)") - test.AssertEquals(t, err.Error(), "unable to parse e-mail address") + test.AssertEquals(t, err.Error(), "unable to parse email address") err = ValidEmail("john.smith@gmail.com #replace with real email") - test.AssertEquals(t, err.Error(), "unable to parse e-mail address") + test.AssertEquals(t, err.Error(), "unable to parse email address") err = ValidEmail("example@example.com") test.AssertEquals(t, err.Error(), "contact email has forbidden domain") diff --git a/test/integration/errors_test.go b/test/integration/errors_test.go index 0c71bdb7269..0822f21b566 100644 --- a/test/integration/errors_test.go +++ b/test/integration/errors_test.go @@ -66,19 +66,19 @@ func TestAccountEmailError(t *testing.T) { name: "empty proto", contacts: []string{"mailto:valid@valid.com", " "}, 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:valid@valid.com", "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:valid@valid.com", "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:valid@valid.com", "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:valid@valid.com", "mailto:a@example.com"}, expectedProbType: "urn:ietf:params:acme:error:invalidContact", - expectedProbDetail: "invalid contact domain. Contact emails @example.com are forbidden", + expectedProbDetail: "contact email has forbidden domain", }, { name: "contact domain invalid TLD", contacts: []string{"mailto:valid@valid.com", "mailto:a@example.cpu"}, expectedProbType: "urn:ietf:params:acme:error:invalidContact", - expectedProbDetail: `contact email "a@example.cpu" 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:valid@valid.com", "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", From 49437f8384dd5ce688caaad26847d845a993a191 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 21 Nov 2024 17:16:17 -0800 Subject: [PATCH 3/4] Comments --- cmd/bad-key-revoker/main.go | 2 +- cmd/expiration-mailer/main.go | 2 +- policy/pa.go | 4 +++- policy/pa_test.go | 2 +- test/integration/errors_test.go | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cmd/bad-key-revoker/main.go b/cmd/bad-key-revoker/main.go index 530b6be420d..43d5dcf3807 100644 --- a/cmd/bad-key-revoker/main.go +++ b/cmd/bad-key-revoker/main.go @@ -370,7 +370,7 @@ func (bkr *badKeyRevoker) invoke(ctx context.Context) (bool, error) { } } - serials := make([]string, 0, len(unrevokedCerts)) + serials := make([]string, 0) for _, cert := range unrevokedCerts { serials = append(serials, cert.Serial) } diff --git a/cmd/expiration-mailer/main.go b/cmd/expiration-mailer/main.go index 664a58067ae..eed76527388 100644 --- a/cmd/expiration-mailer/main.go +++ b/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 errors.New("daily mail limit exceeded") + return errors.New("daily mail limit exceeded for this email address") } return nil } diff --git a/policy/pa.go b/policy/pa.go index dab7e80c200..fac69d3b9b9 100644 --- a/policy/pa.go +++ b/policy/pa.go @@ -345,7 +345,9 @@ func ValidEmail(address string) error { return berrors.InvalidEmailError("contact email has invalid domain: %s", err) } if forbiddenMailDomains[domain] { - return berrors.InvalidEmailError("contact email has 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 } diff --git a/policy/pa_test.go b/policy/pa_test.go index 853c9870555..6031e33dd03 100644 --- a/policy/pa_test.go +++ b/policy/pa_test.go @@ -479,7 +479,7 @@ func TestValidEmailError(t *testing.T) { test.AssertEquals(t, err.Error(), "unable to parse email address") err = ValidEmail("example@example.com") - test.AssertEquals(t, err.Error(), "contact email has forbidden domain") + test.AssertEquals(t, err.Error(), "contact email has forbidden domain \"example.com\"") err = ValidEmail("example@-foobar.com") test.AssertEquals(t, err.Error(), "contact email has invalid domain: Domain name contains an invalid character") diff --git a/test/integration/errors_test.go b/test/integration/errors_test.go index 0822f21b566..c1d9b4d3b48 100644 --- a/test/integration/errors_test.go +++ b/test/integration/errors_test.go @@ -96,7 +96,7 @@ func TestAccountEmailError(t *testing.T) { name: "forbidden contact domain", contacts: []string{"mailto:valid@valid.com", "mailto:a@example.com"}, expectedProbType: "urn:ietf:params:acme:error:invalidContact", - expectedProbDetail: "contact email has forbidden domain", + expectedProbDetail: "contact email has forbidden domain \"example.com\"", }, { name: "contact domain invalid TLD", From 88405f3f3cab893e2e7f60ba3cd990836387db0e Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 25 Nov 2024 11:24:45 -0800 Subject: [PATCH 4/4] Update cmd/bad-key-revoker/main.go Co-authored-by: Jacob Hoffman-Andrews --- cmd/bad-key-revoker/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/bad-key-revoker/main.go b/cmd/bad-key-revoker/main.go index 43d5dcf3807..c333b88c368 100644 --- a/cmd/bad-key-revoker/main.go +++ b/cmd/bad-key-revoker/main.go @@ -370,7 +370,7 @@ func (bkr *badKeyRevoker) invoke(ctx context.Context) (bool, error) { } } - serials := make([]string, 0) + var serials []string for _, cert := range unrevokedCerts { serials = append(serials, cert.Serial) }