Fix ineffectual assignments (letsencrypt#4052)

Roland Bracewell Shoemaker · cpu · commit 232a5f828fa9 · 2019-02-13T15:39:58.000-05:00
* in boulder-ra we connected to the publisher and created a publisher gRPC client twice for no apparent reason
* in the SA we ignored errors from `getChallenges` in `GetAuthorizations` which could result in a nil challenge being returned in an authorization
diff --git a/ca/ca_test.go b/ca/ca_test.go
@@ -356,7 +356,7 @@ func TestIssueCertificate(t *testing.T) {
 
 				issueReq := &caPB.IssueCertificateRequest{Csr: testCase.csr, RegistrationID: &arbitraryRegID}
 
-				certDER := []byte{}
+				var certDER []byte
 				if mode.issuePrecertificate {
 					response, err := ca.IssuePrecertificate(ctx, issueReq)
 
diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go
@@ -150,14 +150,10 @@ func main() {
 	// with a plain caPB.NewCertificateAuthorityClient.
 	cac := bgrpc.NewCertificateAuthorityClient(caPB.NewCertificateAuthorityClient(caConn), nil)
 
-	raConn, err := bgrpc.ClientSetup(c.RA.PublisherService, tlsConfig, clientMetrics, clk)
-	cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to Publisher")
-	pubc := bgrpc.NewPublisherClientWrapper(pubPB.NewPublisherClient(raConn))
-
 	var ctp *ctpolicy.CTPolicy
 	conn, err := bgrpc.ClientSetup(c.RA.PublisherService, tlsConfig, clientMetrics, clk)
 	cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to Publisher")
-	pubc = bgrpc.NewPublisherClientWrapper(pubPB.NewPublisherClient(conn))
+	pubc := bgrpc.NewPublisherClientWrapper(pubPB.NewPublisherClient(conn))
 
 	// Boulder's components assume that there will always be CT logs configured.
 	// Issuing a certificate without SCTs embedded is a miss-issuance event in the
diff --git a/cmd/boulder-wfe2/main_test.go b/cmd/boulder-wfe2/main_test.go
@@ -34,11 +34,13 @@ func TestLoadCertificateChains(t *testing.T) {
 	crlfPEM, _ := ioutil.TempFile("", "crlf.pem")
 	crlfPEMBytes := []byte(strings.Replace(string(certBytesB), "\n", "\r\n", -1))
 	err = ioutil.WriteFile(crlfPEM.Name(), crlfPEMBytes, 0640)
+	test.AssertNotError(t, err, "ioutil.WriteFile failed")
 
 	// Make a .pem file that is test-ca.pem but with no trailing newline
 	abruptPEM, _ := ioutil.TempFile("", "abrupt.pem")
 	abruptPEMBytes := certBytesA[:len(certBytesA)-1]
 	err = ioutil.WriteFile(abruptPEM.Name(), abruptPEMBytes, 0640)
+	test.AssertNotError(t, err, "ioutil.WriteFile failed")
 
 	testCases := []struct {
 		Name           string
diff --git a/cmd/config_test.go b/cmd/config_test.go
@@ -174,10 +174,10 @@ func TestLogInfo(t *testing.T) {
 
 	fc := clock.NewFake()
 	ld.TemporalSet = &TemporalSet{}
-	uri, key, err = ld.Info(fc.Now())
+	_, _, err = ld.Info(fc.Now())
 	test.AssertError(t, err, "Info should fail with a TemporalSet with no viable shards")
 	ld.TemporalSet.Shards = []LogShard{{WindowStart: fc.Now().Add(time.Hour), WindowEnd: fc.Now().Add(time.Hour * 2)}}
-	uri, key, err = ld.Info(fc.Now())
+	_, _, err = ld.Info(fc.Now())
 	test.AssertError(t, err, "Info should fail with a TemporalSet with no viable shards")
 
 	fc.Add(time.Hour * 4)
diff --git a/cmd/expiration-mailer/main_test.go b/cmd/expiration-mailer/main_test.go
@@ -377,6 +377,7 @@ func addExpiringCerts(t *testing.T, ctx *testCtx) []core.Certificate {
 	}
 
 	setupDBMap, err := sa.NewDbMap(vars.DBConnSAFullPerms, 0)
+	test.AssertNotError(t, err, "sa.NewDbMap failed")
 	err = setupDBMap.Insert(certA)
 	test.AssertNotError(t, err, "Couldn't add certA")
 	err = setupDBMap.Insert(certB)
@@ -604,6 +605,7 @@ func TestLifetimeOfACert(t *testing.T) {
 	}
 
 	setupDBMap, err := sa.NewDbMap(vars.DBConnSAFullPerms, 0)
+	test.AssertNotError(t, err, "sa.NewDbMap failed")
 	err = setupDBMap.Insert(certA)
 	test.AssertNotError(t, err, "unable to insert Certificate")
 	_, err = setupDBMap.Exec("INSERT INTO certificateStatus (serial, status, notAfter, lastExpirationNagSent, ocspLastUpdated, revokedDate, revokedReason, LockCol, subscriberApproved) VALUES (?,?,?,?,?,?,?,?,?)", serial1String, string(core.OCSPStatusGood), rawCertA.NotAfter, time.Time{}, time.Time{}, time.Time{}, 0, 0, false)
@@ -704,6 +706,7 @@ func TestDontFindRevokedCert(t *testing.T) {
 	}
 
 	setupDBMap, err := sa.NewDbMap(vars.DBConnSAFullPerms, 0)
+	test.AssertNotError(t, err, "sa.NewDbMap failed")
 	err = setupDBMap.Insert(certA)
 	test.AssertNotError(t, err, "unable to insert Certificate")
 	_, err = setupDBMap.Exec("INSERT INTO certificateStatus (serial,status, lastExpirationNagSent, ocspLastUpdated, revokedDate, revokedReason, LockCol, subscriberApproved) VALUES (?,?,?,?,?,?,?,?)", serial1String, string(core.OCSPStatusRevoked), time.Time{}, time.Time{}, time.Time{}, 0, 0, false)
@@ -765,6 +768,7 @@ func TestDedupOnRegistration(t *testing.T) {
 	}
 
 	setupDBMap, err := sa.NewDbMap(vars.DBConnSAFullPerms, 0)
+	test.AssertNotError(t, err, "sa.NewDbMap failed")
 	err = setupDBMap.Insert(certA)
 	test.AssertNotError(t, err, "Couldn't add certA")
 	err = setupDBMap.Insert(certB)
diff --git a/ra/ra_test.go b/ra/ra_test.go
@@ -932,7 +932,7 @@ func TestPerformValidationExpired(t *testing.T) {
 	test.AssertNotError(t, err, "AuthzToPB failed")
 
 	challIndex := int64(ResponseIndex)
-	authzPB, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{
+	_, err = ra.PerformValidation(ctx, &rapb.PerformValidationRequest{
 		Authz:          authzPB,
 		ChallengeIndex: &challIndex,
 	})
diff --git a/sa/model_test.go b/sa/model_test.go
@@ -86,12 +86,12 @@ func TestV2AuthzModel(t *testing.T) {
 		},
 	}
 
-	model, err := authzPBToModel(authzPB)
+	_, err := authzPBToModel(authzPB)
 	test.AssertError(t, err, "authzPBToModel didn't fail when V2 wasn't set")
 
 	v2 := true
 	authzPB.V2 = &v2
-	model, err = authzPBToModel(authzPB)
+	model, err := authzPBToModel(authzPB)
 	test.AssertNotError(t, err, "authzPBToModel failed")
 
 	authzPBOut, err := modelToAuthzPB(model)
diff --git a/sa/sa.go b/sa/sa.go
@@ -2063,6 +2063,9 @@ func (ssa *SQLStorageAuthority) GetAuthorizations(
 	// Fetch each of the authorizations' associated challenges
 	for _, authz := range authzMap {
 		authz.Challenges, err = ssa.getChallenges(ssa.dbMap.WithContext(ctx), authz.ID)
+		if err != nil {
+			return nil, err
+		}
 	}
 	return authzMapToPB(authzMap)
 }
diff --git a/sa/sa_test.go b/sa/sa_test.go
@@ -592,6 +592,7 @@ func TestCountCertificatesByNames(t *testing.T) {
 
 	// Time range including now should find the cert
 	counts, err = sa.CountCertificatesByNames(ctx, []string{"example.com"}, yesterday, now)
+	test.AssertNotError(t, err, "sa.CountCertificatesByName failed")
 	test.AssertEquals(t, len(counts), 1)
 	test.AssertEquals(t, *counts[0].Name, "example.com")
 	test.AssertEquals(t, *counts[0].Count, int64(1))
@@ -664,6 +665,7 @@ func TestMarkCertificateRevoked(t *testing.T) {
 	const ocspResponse = "this is a fake OCSP response"
 
 	certificateStatusObj, err := sa.GetCertificateStatus(ctx, serial)
+	test.AssertNotError(t, err, "sa.GetCertificateStatus failed")
 	test.AssertEquals(t, certificateStatusObj.Status, core.OCSPStatusGood)
 
 	fc.Add(1 * time.Hour)
@@ -2028,6 +2030,7 @@ func TestStatusForOrder(t *testing.T) {
 
 	// Create a valid authz
 	validAuthz, err := sa.NewPendingAuthorization(ctx, newAuthz)
+	test.AssertNotError(t, err, "sa.NewPendingAuthorization failed")
 	validAuthz.Status = core.StatusValid
 	validAuthz.Identifier.Value = "valid.your.order.is.up"
 	err = sa.FinalizeAuthorization(ctx, validAuthz)
diff --git a/test/load-generator/boulder-calls.go b/test/load-generator/boulder-calls.go
@@ -991,7 +991,7 @@ func revokeCertificate(s *State, ctx *context) error {
 	resp, err = s.post(fmt.Sprintf("%s%s", s.apiBase, revokeCertPath), requestPayload, ctx.ns)
 	finished := time.Now()
 	state := "good"
-	s.callLatency.Add("POST /acme/revoke-cert", started, finished, state)
+	defer func() { s.callLatency.Add("POST /acme/revoke-cert", started, finished, state) }()
 	if err != nil {
 		state = "error"
 		return err
diff --git a/test/ocsp/helper/helper.go b/test/ocsp/helper/helper.go
@@ -81,6 +81,9 @@ func parseCMS(body []byte) (*x509.Certificate, error) {
 	}
 	var msg cms
 	_, err := asn1.Unmarshal(body, &msg)
+	if err != nil {
+		return nil, fmt.Errorf("parsing CMS: %s", err)
+	}
 	cert, err := x509.ParseCertificate(msg.SignedData.Certificates.Bytes)
 	if err != nil {
 		return nil, fmt.Errorf("parsing CMS: %s", err)
diff --git a/va/va_test.go b/va/va_test.go
@@ -1617,10 +1617,6 @@ func TestFallbackTLS(t *testing.T) {
 	ident = dnsi("ipv6.localhost")
 	va.stats = metrics.NewNoopScope()
 	records, prob = va.validateChallenge(ctx, ident, chall)
-
-	// The validation is expected to fail since there is no IPv4 to fall back to
-	// and a broken IPv6
-	records, prob = va.validateChallenge(ctx, ident, chall)
 	test.Assert(t, prob != nil, "validation succeeded with broken IPv6 and no IPv4 fallback")
 	// We expect that the problem has the correct error message about nothing to fallback to
 	test.AssertEquals(t, prob.Detail,
diff --git a/wfe/wfe_test.go b/wfe/wfe_test.go
@@ -1402,6 +1402,7 @@ func TestNewRegistration(t *testing.T) {
 
 	responseWriter := httptest.NewRecorder()
 	result, err := signer.Sign([]byte(`{"resource":"new-reg","contact":["mailto:person@mail.com"],"agreement":"` + agreementURL + `"}`))
+	test.AssertNotError(t, err, "signer.Sign failed")
 	wfe.NewRegistration(ctx, newRequestEvent(), responseWriter,
 		makePostRequest(result.FullSerialize()))
 
@@ -1938,6 +1939,7 @@ func TestRegistration(t *testing.T) {
 
 	// Test POST valid JSON with registration up in the mock (with incorrect agreement URL)
 	result, err = signer.Sign([]byte(`{"resource":"reg","agreement":"https://letsencrypt.org/im-bad"}`))
+	test.AssertNotError(t, err, "signer.Sign failed")
 
 	// Test POST valid JSON with registration up in the mock
 	wfe.Registration(ctx, newRequestEvent(), responseWriter,
@@ -2300,6 +2302,7 @@ func TestHeaderBoulderRequester(t *testing.T) {
 
 	// requests that do not call sendError() have the requester header
 	result, err := signer.Sign([]byte(`{"resource":"reg","agreement":"` + agreementURL + `"}`))
+	test.AssertNotError(t, err, "signer.Sign failed")
 	request := makePostRequestWithPath(regPath+"1", result.FullSerialize())
 	mux.ServeHTTP(responseWriter, request)
 	test.AssertEquals(t, responseWriter.Header().Get("Boulder-Requester"), "1")
diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go
@@ -1248,7 +1248,7 @@ func TestNewECDSAAccount(t *testing.T) {
 	responseWriter = httptest.NewRecorder()
 	// POST, Valid JSON, Key already in use
 	wfe.NewAccount(ctx, newRequestEvent(), responseWriter, request)
-	responseBody = responseWriter.Body.String()
+	test.AssertEquals(t, responseWriter.Body.String(), "{\n  \"id\": 3,\n  \"key\": {\n    \"kty\": \"EC\",\n    \"crv\": \"P-256\",\n    \"x\": \"FwvSZpu06i3frSk_mz9HcD9nETn4wf3mQ-zDtG21Gao\",\n    \"y\": \"S8rR-0dWa8nAcw1fbunF_ajS3PQZ-QwLps-2adgLgPk\"\n  },\n  \"agreement\": \"http://example.invalid/terms\",\n  \"initialIp\": \"\",\n  \"createdAt\": \"0001-01-01T00:00:00Z\",\n  \"status\": \"\"\n}")
 	test.AssertEquals(t, responseWriter.Header().Get("Location"), "http://localhost/acme/acct/3")
 	test.AssertEquals(t, responseWriter.Code, 200)
 }

Original file line number	Diff line number	Diff line change
`@@ -2063,6 +2063,9 @@ func (ssa *SQLStorageAuthority) GetAuthorizations(`
`2063`	`2063`	`// Fetch each of the authorizations' associated challenges`
`2064`	`2064`	`for _, authz := range authzMap {`
`2065`	`2065`	`authz.Challenges, err = ssa.getChallenges(ssa.dbMap.WithContext(ctx), authz.ID)`
	`2066`	`+ if err != nil {`
	`2067`	`+ return nil, err`
	`2068`	`+ }`
`2066`	`2069`	`}`
`2067`	`2070`	`return authzMapToPB(authzMap)`
`2068`	`2071`	`}`