Merge remote-tracking branch 'origin/main' into trust-but-verify-2

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/boulder-ra/main.go

@@ -295,7 +295,7 @@ func main() {
 		apc,
 		issuerCerts,
 	)
-	defer rai.DrainFinalize()
+	defer rai.Drain()
 
 	policyErr := rai.LoadRateLimitPoliciesFile(c.RA.RateLimitPoliciesFilename)
 	cmd.FailOnError(policyErr, "Couldn't load rate limit policies file")

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) {

docs/CONTRIBUTING.md

@@ -33,6 +33,19 @@ guidelines for Boulder contributions.
 * Are there new RPCs or config fields? Make sure the patch meets the
   Deployability rules below.
 
+# Merge Requirements
+
+We have a bot that will comment on some PRs indicating there are:
+
+ 1. configuration changes
+ 2. SQL schema changes
+ 3. feature flag changes
+
+These may require either a CP/CPS review or filing of a ticket to make matching changes
+in production. It is the responsibility of the person merging the PR to make sure
+the required action has been performed before merging. Usually this will be confirmed
+in a comment or in the PR description.
+
 # Patch Guidelines
 
 * Please include helpful comments. No need to gratuitously comment clear code,
@@ -139,11 +152,12 @@ separate deploy-triggered problems from config-triggered problems.
 
 When adding significant new features or replacing existing RPCs the
 `boulder/features` package should be used to gate its usage. To add a flag, a
-new `const FeatureFlag` should be added and its default value specified in
-`features.features` in `features/features.go`. In order to test if the flag
-is enabled elsewhere in the codebase you can use
-`features.Enabled(features.ExampleFeatureName)` which returns a `bool`
-indicating if the flag is enabled or not.
+new field of the `features.Config` struct should be added. All flags default
+to false.
+
+In order to test if the flag is enabled elsewhere in the codebase you can use
+`features.Get().ExampleFeatureName` which gets the `bool` value from a global
+config.
 
 Each service should include a `map[string]bool` named `Features` in its
 configuration object at the top level and call `features.Set` with that map
@@ -160,13 +174,24 @@ immediately after parsing the configuration. For example to enable
 }
 ```
 
-Avoid negative flag names such as `"DontCancelRequest": false` because such
-names are difficult to reason about.
-
 Feature flags are meant to be used temporarily and should not be used for
-permanent boolean configuration options. Once a feature has been enabled in
-both staging and production the flag should be removed making the previously
-gated functionality the default in future deployments.
+permanent boolean configuration options.
+
+### Deprecating a feature flag
+
+Once a feature has been enabled in both staging and production, someone on the
+team should deprecate it:
+
+ - Remove any instances of `features.Get().ExampleFeatureName`, adjusting code
+   as needed.
+ - Move the field to the top of the `features.Config` struct, under a comment
+   saying it's deprecated.
+ - Remove all references to the feature flag from `test/config-next`.
+ - Add the feature flag to `test/config`. This serves to check that we still
+   tolerate parsing the flag at startup, even though it is ineffective.
+ - File a ticket to remove the feature flag in staging and production.
+ - Once the feature flag is removed in staging and production, delete it from
+   `test/config` and `features.Config`.
 
 ### Gating RPCs
 

features/features.go

@@ -15,6 +15,9 @@ import (
 // then call features.Set(parsedConfig) to load the parsed struct into this
 // package's global Config.
 type Config struct {
+	// Deprecated flags.
+	IncrementRateLimits bool
+
 	// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
 	// GET requests. WARNING: This feature is a draft and highly unstable.
 	ServeRenewalInfo bool
@@ -47,16 +50,9 @@ type Config struct {
 
 	// EnforceMultiCAA causes the VA to kick off remote CAA rechecks when true.
 	// When false, no remote CAA rechecks will be performed. The primary VA will
-	// make a valid/invalid decision with the results. The primary VA will
-	// return an early decision if MultiCAAFullResults is false.
+	// make a valid/invalid decision with the results.
 	EnforceMultiCAA bool
 
-	// MultiCAAFullResults will cause the main VA to block and wait for all of
-	// the remote VA CAA recheck results instead of returning early if the
-	// number of failures is greater than the number allowed by MPIC.
-	// Only used when EnforceMultiCAA is true.
-	MultiCAAFullResults bool
-
 	// MultipleCertificateProfiles, when enabled, triggers the following
 	// behavior:
 	//   - SA.NewOrderAndAuthzs: upon receiving a NewOrderRequest with a
@@ -106,11 +102,6 @@ type Config struct {
 	// and pause issuance for each (account, hostname) pair that repeatedly
 	// fails validation.
 	AutomaticallyPauseZombieClients bool
-
-	// IncrementRateLimits uses Redis' IncrBy, instead of Set, for rate limit
-	// accounting. This catches and denies spikes of requests much more
-	// reliably.
-	IncrementRateLimits bool
 }
 
 var fMu = new(sync.RWMutex)

grpc/pb-marshalling.go

@@ -188,13 +188,13 @@ func ValidationResultToPB(records []core.ValidationRecord, prob *probs.ProblemDe
 			return nil, err
 		}
 	}
-	marshalledProbs, err := ProblemDetailsToPB(prob)
+	marshalledProb, err := ProblemDetailsToPB(prob)
 	if err != nil {
 		return nil, err
 	}
 	return &vapb.ValidationResult{
 		Records:     recordAry,
-		Problems:    marshalledProbs,
+		Problem:     marshalledProb,
 		Perspective: perspective,
 		Rir:         rir,
 	}, nil
@@ -212,7 +212,7 @@ func pbToValidationResult(in *vapb.ValidationResult) ([]core.ValidationRecord, *
 			return nil, nil, err
 		}
 	}
-	prob, err := PBToProblemDetails(in.Problems)
+	prob, err := PBToProblemDetails(in.Problem)
 	if err != nil {
 		return nil, nil, err
 	}

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) {