Add go1.23.0 to CI (#7665)

Begin testing on go1.23. To facilitate this, also update /x/net, golangci-lint, staticcheck, and pebble-challtestsrv to versions which support go1.23. As a result of these updates, also fix a handful of new lint findings, mostly regarding passing non-static (i.e. potentially user-controlled) format strings into Sprintf-style functions. Additionally, delete one VA unittest that was duplicating the checks performed by a different VA unittest, but with a context timeout bug that caused it to break when go1.23 subtly changed DialContext behavior.
letsencrypt · Aug 23, 2024 · da7865c · da7865c
1 parent cac431c
commit da7865c
Show file tree

Hide file tree

Showing 16 changed files with 64 additions and 119 deletions.
diff --git a/.github/workflows/boulder-ci.yml b/.github/workflows/boulder-ci.yml
@@ -36,7 +36,8 @@ jobs:
       matrix:
         # Add additional docker image tags here and all tests will be run with the additional image.
         BOULDER_TOOLS_TAG:
-          - go1.22.5_2024-07-03
+          - go1.22.5_2024-08-13
+          - go1.23.0_2024-08-13
         # Tests command definitions. Use the entire "docker compose" command you want to run.
         tests:
           # Run ./test.sh --help for a description of each of the flags.

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -16,6 +16,7 @@ jobs:
       matrix:
         GO_VERSION:
           - "1.22.5"
+          - "1.23.0"
     runs-on: ubuntu-20.04
     permissions:
       contents: write

diff --git a/.github/workflows/try-release.yml b/.github/workflows/try-release.yml
@@ -16,6 +16,7 @@ jobs:
       matrix:
         GO_VERSION:
           - "1.22.5"
+          - "1.23.0"
     runs-on: ubuntu-20.04
     steps:
       - uses: actions/checkout@v4

diff --git a/cmd/cert-checker/main_test.go b/cmd/cert-checker/main_test.go
@@ -134,7 +134,7 @@ func TestCheckWildcardCert(t *testing.T) {
 	}
 	_, problems := checker.checkCert(context.Background(), cert, nil)
 	for _, p := range problems {
-		t.Errorf(p)
+		t.Error(p)
 	}
 }
 

diff --git a/core/objects.go b/core/objects.go
@@ -200,7 +200,7 @@ func (ch Challenge) ExpectedKeyAuthorization(key *jose.JSONWebKey) (string, erro
 // RecordsSane checks the sanity of a ValidationRecord object before sending it
 // back to the RA to be stored.
 func (ch Challenge) RecordsSane() bool {
-	if ch.ValidationRecord == nil || len(ch.ValidationRecord) == 0 {
+	if len(ch.ValidationRecord) == 0 {
 		return false
 	}
 

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -8,7 +8,7 @@ services:
       context: test/boulder-tools/
       # Should match one of the GO_CI_VERSIONS in test/boulder-tools/tag_and_upload.sh.
       args:
-        GO_VERSION: 1.22.5
+        GO_VERSION: 1.23.0
     environment:
       # To solve HTTP-01 and TLS-ALPN-01 challenges, change the IP in FAKE_DNS
       # to the IP address where your ACME client's solver is listening.

diff --git a/grpc/errors.go b/grpc/errors.go
@@ -91,7 +91,7 @@ func unwrapError(err error, md metadata.MD) error {
 			inErrMsg,
 		)
 	}
-	inErr := berrors.New(berrors.ErrorType(inErrType), inErrMsg)
+	inErr := berrors.New(berrors.ErrorType(inErrType), inErrMsg) //nolint:govet
 	var outErr *berrors.BoulderError
 	if !errors.As(inErr, &outErr) {
 		return fmt.Errorf(

diff --git a/grpc/internal/resolver/dns/dns_resolver_test.go b/grpc/internal/resolver/dns/dns_resolver_test.go
@@ -600,7 +600,7 @@ func TestCustomAuthority(t *testing.T) {
 
 			err = <-errChan
 			if err != nil {
-				t.Errorf(err.Error())
+				t.Error(err.Error())
 			}
 
 			if a.expectError {

diff --git a/probs/probs.go b/probs/probs.go
@@ -92,19 +92,19 @@ func AccountDoesNotExist(detail string) *ProblemDetails {
 
 // AlreadyRevoked returns a ProblemDetails with a AlreadyRevokedProblem and a 400 Bad
 // Request status code.
-func AlreadyRevoked(detail string, a ...any) *ProblemDetails {
+func AlreadyRevoked(detail string) *ProblemDetails {
 	return &ProblemDetails{
 		Type:       AlreadyRevokedProblem,
-		Detail:     fmt.Sprintf(detail, a...),
+		Detail:     detail,
 		HTTPStatus: http.StatusBadRequest,
 	}
 }
 
 // BadCSR returns a ProblemDetails representing a BadCSRProblem.
-func BadCSR(detail string, a ...any) *ProblemDetails {
+func BadCSR(detail string) *ProblemDetails {
 	return &ProblemDetails{
 		Type:       BadCSRProblem,
-		Detail:     fmt.Sprintf(detail, a...),
+		Detail:     detail,
 		HTTPStatus: http.StatusBadRequest,
 	}
 }
@@ -121,30 +121,30 @@ func BadNonce(detail string) *ProblemDetails {
 
 // BadPublicKey returns a ProblemDetails with a BadPublicKeyProblem and a 400 Bad
 // Request status code.
-func BadPublicKey(detail string, a ...any) *ProblemDetails {
+func BadPublicKey(detail string) *ProblemDetails {
 	return &ProblemDetails{
 		Type:       BadPublicKeyProblem,
-		Detail:     fmt.Sprintf(detail, a...),
+		Detail:     detail,
 		HTTPStatus: http.StatusBadRequest,
 	}
 }
 
 // BadRevocationReason returns a ProblemDetails representing
 // a BadRevocationReasonProblem
-func BadRevocationReason(detail string, a ...any) *ProblemDetails {
+func BadRevocationReason(detail string) *ProblemDetails {
 	return &ProblemDetails{
 		Type:       BadRevocationReasonProblem,
-		Detail:     fmt.Sprintf(detail, a...),
+		Detail:     detail,
 		HTTPStatus: http.StatusBadRequest,
 	}
 }
 
 // BadSignatureAlgorithm returns a ProblemDetails with a BadSignatureAlgorithmProblem
 // and a 400 Bad Request status code.
-func BadSignatureAlgorithm(detail string, a ...any) *ProblemDetails {
+func BadSignatureAlgorithm(detail string) *ProblemDetails {
 	return &ProblemDetails{
 		Type:       BadSignatureAlgorithmProblem,
-		Detail:     fmt.Sprintf(detail, a...),
+		Detail:     detail,
 		HTTPStatus: http.StatusBadRequest,
 	}
 }
@@ -200,10 +200,10 @@ func Malformed(detail string, a ...any) *ProblemDetails {
 }
 
 // OrderNotReady returns a ProblemDetails representing a OrderNotReadyProblem
-func OrderNotReady(detail string, a ...any) *ProblemDetails {
+func OrderNotReady(detail string) *ProblemDetails {
 	return &ProblemDetails{
 		Type:       OrderNotReadyProblem,
-		Detail:     fmt.Sprintf(detail, a...),
+		Detail:     detail,
 		HTTPStatus: http.StatusForbidden,
 	}
 }

diff --git a/ra/ra.go b/ra/ra.go
@@ -874,10 +874,10 @@ func (ra *RegistrationAuthorityImpl) checkOrderAuthorizations(
 func validatedBefore(authz *core.Authorization, caaRecheckTime time.Time) (bool, error) {
 	numChallenges := len(authz.Challenges)
 	if numChallenges != 1 {
-		return false, fmt.Errorf("authorization has incorrect number of challenges. 1 expected, %d found for: id %s", numChallenges, authz.ID)
+		return false, berrors.InternalServerError("authorization has incorrect number of challenges. 1 expected, %d found for: id %s", numChallenges, authz.ID)
 	}
 	if authz.Challenges[0].Validated == nil {
-		return false, fmt.Errorf("authorization's challenge has no validated timestamp for: id %s", authz.ID)
+		return false, berrors.InternalServerError("authorization's challenge has no validated timestamp for: id %s", authz.ID)
 	}
 	return authz.Challenges[0].Validated.Before(caaRecheckTime), nil
 }
@@ -905,7 +905,7 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA(
 
 	for _, authz := range authzs {
 		if staleCAA, err := validatedBefore(authz, caaRecheckAfter); err != nil {
-			return berrors.InternalServerError(err.Error())
+			return err
 		} else if staleCAA {
 			// Ensure that CAA is rechecked for this name
 			recheckAuthzs = append(recheckAuthzs, authz)
@@ -977,7 +977,7 @@ func (ra *RegistrationAuthorityImpl) recheckCAA(ctx context.Context, authzs []*c
 					authz.ID, name,
 				)
 			} else if resp.Problem != nil {
-				err = berrors.CAAError(resp.Problem.Detail)
+				err = berrors.CAAError("rechecking caa: %s", resp.Problem.Detail)
 			}
 			ch <- authzCAAResult{
 				authz: authz,
@@ -1460,16 +1460,13 @@ func (ra *RegistrationAuthorityImpl) getSCTs(ctx context.Context, cert []byte, e
 	started := ra.clk.Now()
 	scts, err := ra.ctpolicy.GetSCTs(ctx, cert, expiration)
 	took := ra.clk.Since(started)
-	// The final cert has already been issued so actually return it to the
-	// user even if this fails since we aren't actually doing anything with
-	// the SCTs yet.
 	if err != nil {
 		state := "failure"
 		if err == context.DeadlineExceeded {
 			state = "deadlineExceeded"
 			// Convert the error to a missingSCTsError to communicate the timeout,
 			// otherwise it will be a generic serverInternalError
-			err = berrors.MissingSCTsError(err.Error())
+			err = berrors.MissingSCTsError("failed to get SCTs: %s", err.Error())
 		}
 		ra.log.Warningf("ctpolicy.GetSCTs failed: %s", err)
 		ra.ctpolicyResults.With(prometheus.Labels{"result": state}).Observe(took.Seconds())
@@ -1942,11 +1939,11 @@ func (ra *RegistrationAuthorityImpl) PerformValidation(
 	// Look up the account key for this authorization
 	regPB, err := ra.SA.GetRegistration(ctx, &sapb.RegistrationID{Id: authz.RegistrationID})
 	if err != nil {
-		return nil, berrors.InternalServerError(err.Error())
+		return nil, berrors.InternalServerError("getting acct for authorization: %s", err.Error())
 	}
 	reg, err := bgrpc.PbToRegistration(regPB)
 	if err != nil {
-		return nil, berrors.InternalServerError(err.Error())
+		return nil, berrors.InternalServerError("getting acct for authorization: %s", err.Error())
 	}
 
 	// Compute the key authorization field based on the registration key
@@ -1957,7 +1954,7 @@ func (ra *RegistrationAuthorityImpl) PerformValidation(
 
 	// Double check before sending to VA
 	if cErr := ch.CheckPending(); cErr != nil {
-		return nil, berrors.MalformedError(cErr.Error())
+		return nil, berrors.MalformedError("cannot validate challenge: %s", cErr.Error())
 	}
 
 	// Dispatch to the VA for service
@@ -2463,7 +2460,7 @@ func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context,
 	}
 	_, err := ra.SA.DeactivateRegistration(ctx, &sapb.RegistrationID{Id: reg.Id})
 	if err != nil {
-		return nil, berrors.InternalServerError(err.Error())
+		return nil, err
 	}
 	return &emptypb.Empty{}, nil
 }

diff --git a/sfe/sfe_test.go b/sfe/sfe_test.go
@@ -123,7 +123,7 @@ func TestUnpausePaths(t *testing.T) {
 	fc.Add(time.Hour * 337)
 	sfe.UnpauseForm(responseWriter, &http.Request{
 		Method: "GET",
-		URL:    mustParseURL(fmt.Sprintf(unpause.GetForm + "?jwt=" + expiredJWT)),
+		URL:    mustParseURL(unpause.GetForm + "?jwt=" + expiredJWT),
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
 	test.AssertContains(t, responseWriter.Body.String(), "Expired unpause URL")
@@ -134,7 +134,7 @@ func TestUnpausePaths(t *testing.T) {
 	responseWriter = httptest.NewRecorder()
 	sfe.UnpauseForm(responseWriter, &http.Request{
 		Method: "GET",
-		URL:    mustParseURL(fmt.Sprintf(unpause.GetForm + "?jwt=" + validJWT)),
+		URL:    mustParseURL(unpause.GetForm + "?jwt=" + validJWT),
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
 	test.AssertContains(t, responseWriter.Body.String(), "Action required to unpause your account")
@@ -143,7 +143,7 @@ func TestUnpausePaths(t *testing.T) {
 	responseWriter = httptest.NewRecorder()
 	sfe.UnpauseSubmit(responseWriter, &http.Request{
 		Method: "POST",
-		URL:    mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=" + expiredJWT)),
+		URL:    mustParseURL(unpausePostForm + "?jwt=" + expiredJWT),
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
 	test.AssertContains(t, responseWriter.Body.String(), "Expired unpause URL")
@@ -161,7 +161,7 @@ func TestUnpausePaths(t *testing.T) {
 	responseWriter = httptest.NewRecorder()
 	sfe.UnpauseSubmit(responseWriter, &http.Request{
 		Method: "POST",
-		URL:    mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=x.x")),
+		URL:    mustParseURL(unpausePostForm + "?jwt=x.x"),
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
 	test.AssertContains(t, responseWriter.Body.String(), "Invalid unpause URL")
@@ -170,7 +170,7 @@ func TestUnpausePaths(t *testing.T) {
 	responseWriter = httptest.NewRecorder()
 	sfe.UnpauseSubmit(responseWriter, &http.Request{
 		Method: "POST",
-		URL:    mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=x.x.x")),
+		URL:    mustParseURL(unpausePostForm + "?jwt=x.x.x"),
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
 	test.AssertContains(t, responseWriter.Body.String(), "Invalid unpause URL")
@@ -179,7 +179,7 @@ func TestUnpausePaths(t *testing.T) {
 	responseWriter = httptest.NewRecorder()
 	sfe.UnpauseSubmit(responseWriter, &http.Request{
 		Method: "POST",
-		URL:    mustParseURL(fmt.Sprintf(unpausePostForm + "?jwt=" + validJWT)),
+		URL:    mustParseURL(unpausePostForm + "?jwt=" + validJWT),
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusFound)
 	test.AssertEquals(t, unpauseStatus+"?count=0", responseWriter.Result().Header.Get("Location"))

diff --git a/test/boulder-tools/Dockerfile b/test/boulder-tools/Dockerfile
@@ -12,9 +12,9 @@ RUN curl "https://dl.google.com/go/go${GO_VERSION}.$(echo $TARGETPLATFORM | sed
 RUN go install github.com/rubenv/sql-migrate/sql-migrate@v1.1.2
 RUN go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.34.1
 RUN go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@bb9882e6ae58f0a80a6390b50a5ec3bd63e46a3c
-RUN go install github.com/letsencrypt/pebble/v2/cmd/pebble-challtestsrv@66511d8
-RUN go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.57.2
-RUN go install honnef.co/go/tools/cmd/staticcheck@2023.1.7
+RUN go install github.com/letsencrypt/pebble/v2/cmd/pebble-challtestsrv@17d64a3
+RUN go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.60.0
+RUN go install honnef.co/go/tools/cmd/staticcheck@2024.1
 RUN go install github.com/jsha/minica@v1.1.0
 
 FROM rust:bullseye as rustdeps

diff --git a/test/boulder-tools/tag_and_upload.sh b/test/boulder-tools/tag_and_upload.sh
@@ -12,7 +12,7 @@ DOCKER_REPO="letsencrypt/boulder-tools"
 # .github/workflows/release.yml,
 # .github/workflows/try-release.yml if appropriate,
 # and .github/workflows/boulder-ci.yml with the new container tag.
-GO_CI_VERSIONS=( "1.22.5" )
+GO_CI_VERSIONS=( "1.22.5" "1.23.0" )
 
 echo "Please login to allow push to DockerHub"
 docker login