fix dataraces, add -race install, add load-gen. (#232)

The `Authz()` method of the WFE was racey. First because it didn't lock the authorizations and orders it was working with. Second because the handling of displaying authorization challenges was working with `acme.Challenge` objects owned by `core.Challenge`'s that should have been locked for reading but were not. This mean the VA would datarace with the WFE when updating a validated challenge status.

To prevent future occurrences `travis.yml` is updated to install Pebble with the race detector enabled, and to run Pebble such that it will exit non-zero if a race is detected.

Since `Chisel2.py` is single threaded the Boulder `load-generator` is used for a short duration to drive concurrent request traffic. In practice before fixing the dataraces I found this would crash Pebble <30s.

Resolves https://github.com/letsencrypt/pebble/issues/230
Resolves https://github.com/letsencrypt/pebble/issues/228

.travis.yml

@@ -29,15 +29,16 @@ before_install:
 install:
   # Install `golangci-lint` using their installer script
   - curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.15.0
-  # Install `cover` and `goveralls` without `GO111MODULE` enabled so that we
-  # don't download ct-woodpecker dependencies and just put the tools in our
+  # Install tools without `GO111MODULE` enabled so that we
+  # don't download Pebble's deps and just put the tools in our
   # gobin.
   - GO111MODULE=off go get golang.org/x/tools/cmd/cover
   - GO111MODULE=off go get github.com/mattn/goveralls
-  - go install -v -mod=vendor ./...
+  - GO111MODULE=off go get github.com/letsencrypt/boulder/test/load-generator
+  - go install -v -race -mod=vendor ./...
 
 before_script:
-  - pebble &
+  - GORACE="halt_on_error=1" pebble &
 
 script:
   - go mod download
@@ -50,6 +51,10 @@ script:
   - goveralls -coverprofile=coverage.out -service=travis-ci
   # Perform a test issuance with chisel2.py
   - REQUESTS_CA_BUNDLE=./test/certs/pebble.minica.pem python ./test/chisel2.py example.letsencrypt.org elpmaxe.letsencrypt.org
+  # Run the load-generator briefly - note, because Pebble isn't using the
+  # load-generator's mock DNS server none of the issuances will succeed. This
+  # step is performed just to shake out data races with concurrent requests.
+  - load-generator -config ./test/config/load-generator-config.json > /dev/null
 
 deploy:
   - provider: script

acme/common.go

@@ -51,7 +51,7 @@ type Order struct {
 type Authorization struct {
 	Status     string      `json:"status"`
 	Identifier Identifier  `json:"identifier"`
-	Challenges []*Challenge `json:"challenges"`
+	Challenges []Challenge `json:"challenges"`
 	Expires    string      `json:"expires"`
 	// Wildcard is a Let's Encrypt specific Authorization field that indicates the
 	// authorization was created as a result of an order containing a name with

core/types.go

@@ -120,6 +120,7 @@ type Authorization struct {
 	URL         string
 	ExpiresDate time.Time
 	Order       *Order
+	Challenges  []*Challenge
 }
 
 type Challenge struct {

test/config/load-generator-config.json

@@ -0,0 +1,26 @@
+{
+    "plan": {
+        "actions": [
+            "newAccount",
+            "newOrder",
+            "fulfillOrder",
+            "finalizeOrder"
+        ],
+        "rate": 10,
+        "runtime": "10s",
+        "rateDelta": "1/10s"
+    },
+    "directoryURL": "https://localhost:14000/dir",
+    "domainBase": "com",
+    "challengeStrategy": "random",
+    "httpOneAddrs": [":5002"],
+    "tlsAlpnOneAddrs": [":5001"],
+    "dnsAddrs": [":8053"],
+    "fakeDNS": "127.0.0.1",
+    "regKeySize": 2048,
+    "certKeySize": 2048,
+    "regEmail": "loadtesting@letsencrypt.org",
+    "maxRegs": 20,
+    "maxNamesPerCert": 20,
+    "dontSaveState": true
+}

wfe/wfe.go

@@ -1179,10 +1179,7 @@ func (wfe *WebFrontEndImpl) makeChallenges(authz *core.Authorization, request *h
 
 	// Lock the authorization for writing to update the challenges
 	authz.Lock()
-	authz.Challenges = nil
-	for _, c := range chals {
-		authz.Challenges = append(authz.Challenges, &c.Challenge)
-	}
+	authz.Challenges = chals
 	authz.Unlock()
 	return nil
 }
@@ -1496,12 +1493,13 @@ func (wfe *WebFrontEndImpl) FinalizeOrder(
 }
 
 // prepAuthorizationForDisplay prepares the provided acme.Authorization for
-// display to an ACME client.
-func prepAuthorizationForDisplay(authz acme.Authorization) acme.Authorization {
+// display to an ACME client. It assumes the `authz` is already locked for
+// reading by the caller.
+func prepAuthorizationForDisplay(authz *core.Authorization) acme.Authorization {
 	// Copy the authz to mutate and return
-	result := authz
-
+	result := authz.Authorization
 	identVal := result.Identifier.Value
+
 	// If the authorization identifier has a wildcard in the value, remove it and
 	// set the Wildcard field to true
 	if strings.HasPrefix(identVal, "*.") {
@@ -1509,20 +1507,20 @@ func prepAuthorizationForDisplay(authz acme.Authorization) acme.Authorization {
 		result.Wildcard = true
 	}
 
+	// Build a list of plain acme.Challenges to display using the core.Challenge
+	// objects from the authorization.
+	var chals []acme.Challenge
+	for _, c := range authz.Challenges {
+		c.RLock()
 		// If the authz isn't pending then we need to filter the challenges displayed
 		// to only those that were used to make the authz valid || invalid.
-	if result.Status != acme.StatusPending {
-		var chals []*acme.Challenge
-		// Scan each of the authz's challenges
-		for _, c := range result.Challenges {
-			// Include any that have an associated error, or that are status valid
-			if c.Error != nil || c.Status == acme.StatusValid {
-				chals = append(chals, c)
+		if result.Status != acme.StatusPending && (c.Error == nil && c.Status != acme.StatusValid) {
+			continue
 		}
+		chals = append(chals, c.Challenge)
+		c.RUnlock()
 	}
-		// Replace the authz's challenges with the filtered set
 	result.Challenges = chals
-	}
 
 	// Randomize the order of the challenges in the returned authorization.
 	// Clients should not make any assumptions about the sort order.
@@ -1553,6 +1551,12 @@ func (wfe *WebFrontEndImpl) Authz(
 		return
 	}
 
+	authz.Lock()
+	defer authz.Unlock()
+	authz.Order.RLock()
+	orderAcctID := authz.Order.AccountID
+	authz.Order.RUnlock()
+
 	// If the postData is not a POST-as-GET, treat this as case A) and update
 	// the authorization based on the postData
 	if !postData.postAsGet {
@@ -1562,7 +1566,7 @@ func (wfe *WebFrontEndImpl) Authz(
 			return
 		}
 
-		if authz.Order.AccountID != existingAcct.ID {
+		if orderAcctID != existingAcct.ID {
 			wfe.sendError(acme.UnauthorizedProblem(
 				"Account does not own authorization"), response)
 			return
@@ -1596,7 +1600,7 @@ func (wfe *WebFrontEndImpl) Authz(
 			return
 		}
 
-		if authz.Order.AccountID != account.ID {
+		if orderAcctID != account.ID {
 			response.WriteHeader(http.StatusForbidden)
 			wfe.sendError(acme.UnauthorizedProblem(
 				"Account authorizing the request is not the owner of the authorization"),
@@ -1608,7 +1612,7 @@ func (wfe *WebFrontEndImpl) Authz(
 	err := wfe.writeJSONResponse(
 		response,
 		http.StatusOK,
-		prepAuthorizationForDisplay(authz.Authorization))
+		prepAuthorizationForDisplay(authz))
 	if err != nil {
 		wfe.sendError(acme.InternalErrorProblem("Error marshalling authz"), response)
 		return
@@ -1787,7 +1791,13 @@ func (wfe *WebFrontEndImpl) updateChallenge(
 		return
 	}
 
-	if authz.Order.AccountID != existingAcct.ID {
+	authz.RLock()
+	authz.Order.RLock()
+	orderAcctID := authz.Order.AccountID
+	authz.Order.RUnlock()
+	authz.RUnlock()
+
+	if orderAcctID != existingAcct.ID {
 		response.WriteHeader(http.StatusUnauthorized)
 		wfe.sendError(acme.UnauthorizedProblem(
 			"Account authenticating request is not the owner of the challenge"), response)