From 5b7dc876358c22ed486bc30e107076023366c432 Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Tue, 27 Feb 2024 02:58:53 +0100 Subject: [PATCH] Use GitHub Actions (#442) Add lint, build (multi-platform), and test (linux-only) GitHub Actions. Fix a variety of golangci-lint findings. For now, does not modify the windows Appveyor setup, and does not run the loadtester. Fixes #356 --- .github/workflows/checks.yml | 53 ++++++ .github/workflows/go-cross.yml | 38 +++++ .github/workflows/tests.yml | 70 ++++++++ .golangci.yaml | 94 +++++++--- ca/ca.go | 13 +- cmd/pebble-challtestsrv/history.go | 3 +- cmd/pebble-challtestsrv/httpone.go | 10 +- cmd/pebble-challtestsrv/mockdns.go | 8 +- cmd/pebble/main.go | 2 +- core/types.go | 7 +- db/memorystore.go | 16 +- va/va.go | 27 ++- va/va_test.go | 2 +- wfe/jose.go | 3 +- wfe/wfe.go | 264 +++++++++++++++-------------- 15 files changed, 416 insertions(+), 194 deletions(-) create mode 100644 .github/workflows/checks.yml create mode 100644 .github/workflows/go-cross.yml create mode 100644 .github/workflows/tests.yml diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml new file mode 100644 index 00000000..6d623182 --- /dev/null +++ b/.github/workflows/checks.yml @@ -0,0 +1,53 @@ +name: Checks + +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read + pull-requests: read + +jobs: + + checks: + name: Check Process + runs-on: ubuntu-latest + env: + GO_VERSION: oldstable + GOLANGCI_LINT_VERSION: v1.56.2 + CGO_ENABLED: 0 + + steps: + + - name: Check out code + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: ${{ env.GO_VERSION }} + + - name: Check and get dependencies + run: | + go mod tidy + git diff --exit-code go.mod + git diff --exit-code go.sum + + - name: vendoring + run: go mod vendor + + - name: vendoring diff + run: git diff --exit-code vendor/ + + # https://golangci-lint.run/usage/install#other-ci + - name: Install golangci-lint ${{ env.GOLANGCI_LINT_VERSION }} + run: | + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin ${GOLANGCI_LINT_VERSION} + golangci-lint run + + diff --git a/.github/workflows/go-cross.yml b/.github/workflows/go-cross.yml new file mode 100644 index 00000000..e28cc18a --- /dev/null +++ b/.github/workflows/go-cross.yml @@ -0,0 +1,38 @@ +name: Go Matrix +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read + pull-requests: read + +jobs: + + cross: + name: Build + runs-on: ${{ matrix.os }} + env: + CGO_ENABLED: 0 + + strategy: + matrix: + go-version: [ oldstable, stable ] + os: [ubuntu-latest, macos-latest, windows-latest] + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: ${{ matrix.go-version }} + + - name: Build pebble + run: go build -v -ldflags "-s -w" -trimpath -o pebble ./cmd/pebble + + - name: Build pebble-challtestsrv + run: go build -v -ldflags "-s -w" -trimpath -o pebble-challtestsrv ./cmd/pebble-challtestsrv diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 00000000..5c1ac6f5 --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,70 @@ +name: Tests + +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read + pull-requests: read + +jobs: + + test-linux: + name: Test on Linux + runs-on: ubuntu-latest + env: + GO_VERSION: oldstable + steps: + + - name: Setup /etc/hosts + run: | + echo "127.0.0.1 example.letsencrypt.org" | sudo tee -a /etc/hosts + echo "127.0.0.1 elpmaxe.letsencrypt.org" | sudo tee -a /etc/hosts + + # https://github.com/marketplace/actions/checkout + - name: Check out code + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + # https://github.com/marketplace/actions/setup-go-environment + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: ${{ env.GO_VERSION }} + + - name: apt install + run: sudo apt-get install snapd python3-acme python3-josepy + + - name: snap install + run: sudo snap install core && sudo snap refresh core + + - name: snap install certbot + run: sudo snap install --classic certbot && sudo ln -s /snap/bin/certbot /usr/bin/certbot + + - name: go install tools + run: go install golang.org/x/tools/cmd/cover@latest + + - name: go install goveralls + run: go install github.com/mattn/goveralls@latest + + - name: go install pebble + run: go install -v -race ./... + + - name: launch pebble + run: GORACE="halt_on_error=1" PEBBLE_WFE_NONCEREJECT=0 pebble & + + # Run project unit tests (with the race detector enabled and atomic coverage profile collection) + - name: unittests + run: go test -v -race -covermode=atomic -coverprofile=coverage.out ./... + +# # Upload collected coverage profile to goveralls +# - name: goveralls +# run: goveralls -coverprofile=coverage.out -service=github + + # Perform a test issuance with chisel2.py + - name: chisel + run: REQUESTS_CA_BUNDLE=./test/certs/pebble.minica.pem python ./test/chisel2.py example.letsencrypt.org elpmaxe.letsencrypt.org diff --git a/.golangci.yaml b/.golangci.yaml index c1aee64f..8f8d6d67 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -7,39 +7,79 @@ linters-settings: locale: "US" linters: - enable-all: true - disable: - - stylecheck - - gosec - - dupl - - maligned - - depguard - - lll - - prealloc - - scopelint - - gocritic - - gochecknoinits - - gochecknoglobals - - godox - - funlen - - wsl - - whitespace - - gomnd - - typecheck - - godot - - errname - - nlreturn - - wrapcheck + disable-all: false + enable: + - asasalint + - asciicheck + - bidichk + - bodyclose + - containedctx + - decorder + - dogsled + - dupword + - durationcheck + - errcheck + - errchkjson + - errorlint + - exportloopref + - forcetypeassert + - ginkgolinter + - gocheckcompilerdirectives + - gocognit + - goconst + - gocyclo + - gofmt + - gofumpt + - goheader + - goimports + - gomoddirectives + - gomodguard + - goprintffuncname + - gosimple + - govet + - importas + - inamedparam + - ineffassign + - ireturn + - loggercheck + - makezero + - mirror + - misspell + - nakedret + - nolintlint + - nonamedreturns + - nosprintfhostport + - perfsprint + - predeclared + - reassign + - revive + - staticcheck + - tagalign + - tenv + - testableexamples + - testifylint + - thelper + - unconvert + - unparam + - unused + - usestdlibvars + - wastedassign issues: exclude-use-default: true - max-per-linter: 0 + max-issues-per-linter: 0 max-same-issues: 0 # The following excludes are considered false-positives/known-OK. + exclude: + - fmt.Sprintf can be replaced with string exclude-rules: - - path: ca([/|\\])ca.go + - path: ca/ca.go text: 'type name will be used as ca.CAImpl by other packages, and that stutters; consider calling this Impl' - - path: va([/|\\])va.go + - path: va/va.go text: 'type name will be used as va.VAImpl by other packages, and that stutters; consider calling this Impl' - - path: wfe([/|\\])wfe.go + - path: wfe/wfe.go text: 'if` block ends with a `return` statement, so drop this `else` and outdent its block' + - path: va/va.go + linters: + - goconst + text: 'string `Incorrect validation certificate for %s challenge. ` has \d occurrences, make it a constant' diff --git a/ca/ca.go b/ca/ca.go index ccea6ead..d35b5f6d 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -9,6 +9,7 @@ import ( "crypto/x509/pkix" "encoding/asn1" "encoding/hex" + "errors" "fmt" "log" "math" @@ -110,8 +111,8 @@ func (ca *CAImpl) makeRootCert( subjectKey crypto.Signer, subject pkix.Name, subjectKeyID []byte, - signer *issuer) (*core.Certificate, error) { - + signer *issuer, +) (*core.Certificate, error) { serial := makeSerial() template := &x509.Certificate{ Subject: subject, @@ -187,7 +188,7 @@ func (ca *CAImpl) newRootIssuer(name string) (*issuer, error) { func (ca *CAImpl) newIntermediateIssuer(root *issuer, intermediateKey crypto.Signer, subject pkix.Name, subjectKeyID []byte) (*issuer, error) { if root == nil { - return nil, fmt.Errorf("Internal error: root must not be nil") + return nil, errors.New("internal error: root must not be nil") } // Make an intermediate certificate with the root issuer ic, err := ca.makeRootCert(intermediateKey, subject, subjectKeyID, root) @@ -253,12 +254,12 @@ func (ca *CAImpl) newChain(intermediateKey crypto.Signer, intermediateSubject pk func (ca *CAImpl) newCertificate(domains []string, ips []net.IP, key crypto.PublicKey, accountID, notBefore, notAfter string) (*core.Certificate, error) { if len(domains) == 0 && len(ips) == 0 { - return nil, fmt.Errorf("must specify at least one domain name or IP address") + return nil, errors.New("must specify at least one domain name or IP address") } defaultChain := ca.chains[0].intermediates if len(defaultChain) == 0 || defaultChain[0].cert == nil { - return nil, fmt.Errorf("cannot sign certificate - nil issuer") + return nil, errors.New("cannot sign certificate - nil issuer") } issuer := defaultChain[0] @@ -443,7 +444,7 @@ func (ca *CAImpl) GetRootKey(no int) *rsa.PrivateKey { return nil } -// GetIntermediateCert returns the first (closest the the leaf) issuer certificate +// GetIntermediateCert returns the first (closest the leaf) issuer certificate // in the chain identified by `no`. func (ca *CAImpl) GetIntermediateCert(no int) *core.Certificate { chain := ca.getChain(no) diff --git a/cmd/pebble-challtestsrv/history.go b/cmd/pebble-challtestsrv/history.go index e23987bc..e2b6734f 100644 --- a/cmd/pebble-challtestsrv/history.go +++ b/cmd/pebble-challtestsrv/history.go @@ -103,7 +103,8 @@ func requestHost(r *http.Request) (string, error) { // writeHistory writes the provided list of challtestsrv.RequestEvents to the // provided http.ResponseWriter in JSON form. func (srv *managementServer) writeHistory( - history []challtestsrv.RequestEvent, w http.ResponseWriter) { + history []challtestsrv.RequestEvent, w http.ResponseWriter, +) { // Always write an empty JSON list instead of `null` if history == nil { history = []challtestsrv.RequestEvent{} diff --git a/cmd/pebble-challtestsrv/httpone.go b/cmd/pebble-challtestsrv/httpone.go index 56a1947b..924e2b08 100644 --- a/cmd/pebble-challtestsrv/httpone.go +++ b/cmd/pebble-challtestsrv/httpone.go @@ -7,7 +7,8 @@ import "net/http" // // The POST body is expected to have two non-empty parameters: // "token" - the HTTP-01 challenge token to add the mock HTTP-01 response under -// in the `/.well-known/acme-challenge/` path. +// in the `/.well-known/acme-challenge/` path. +// // "content" - the key authorization value to return in the HTTP response. // // A successful POST will write http.StatusOK to the client. @@ -40,7 +41,7 @@ func (srv *managementServer) addHTTP01(w http.ResponseWriter, r *http.Request) { // // The POST body is expected to have one non-empty parameter: // "token" - the HTTP-01 challenge token to remove the mock HTTP-01 response -// from. +// from. // // A successful POST will write http.StatusOK to the client. func (srv *managementServer) delHTTP01(w http.ResponseWriter, r *http.Request) { @@ -70,9 +71,10 @@ func (srv *managementServer) delHTTP01(w http.ResponseWriter, r *http.Request) { // // The POST body is expected to have two non-empty parameters: // "path" - the path that when matched in an HTTP request will return the -// redirect. +// redirect. +// // "targetURL" - the URL that the client will be redirected to when making HTTP -// requests for the redirected path. +// requests for the redirected path. // // A successful POST will write http.StatusOK to the client. func (srv *managementServer) addHTTPRedirect(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/pebble-challtestsrv/mockdns.go b/cmd/pebble-challtestsrv/mockdns.go index b19de4a1..5b3151f1 100644 --- a/cmd/pebble-challtestsrv/mockdns.go +++ b/cmd/pebble-challtestsrv/mockdns.go @@ -14,7 +14,7 @@ import ( // // The POST body is expected to have one parameter: // "ip" - the string representation of an IPv4 address to use for all A queries -// that do not match more specific mocks. +// that do not match more specific mocks. // // Providing an empty string as the IP value will disable the default // A responses. @@ -30,7 +30,7 @@ func (srv *managementServer) setDefaultDNSIPv4(w http.ResponseWriter, r *http.Re } // Set the challenge server's default IPv4 address - we allow request.IP to be - // the empty string so that the default can be be cleared using the same + // the empty string so that the default can be cleared using the same // method. srv.challSrv.SetDefaultDNSIPv4(request.IP) srv.log.Printf("Set default IPv4 address for DNS A queries to %q\n", request.IP) @@ -43,7 +43,7 @@ func (srv *managementServer) setDefaultDNSIPv4(w http.ResponseWriter, r *http.Re // // The POST body is expected to have one parameter: // "ip" - the string representation of an IPv6 address to use for all AAAA -// queries that do not match more specific mocks. +// queries that do not match more specific mocks. // // Providing an empty string as the IP value will disable the default // A responses. @@ -59,7 +59,7 @@ func (srv *managementServer) setDefaultDNSIPv6(w http.ResponseWriter, r *http.Re } // Set the challenge server's default IPv6 address - we allow request.IP to be - // the empty string so that the default can be be cleared using the same + // the empty string so that the default can be cleared using the same // method. srv.challSrv.SetDefaultDNSIPv6(request.IP) srv.log.Printf("Set default IPv6 address for DNS AAAA queries to %q\n", request.IP) diff --git a/cmd/pebble/main.go b/cmd/pebble/main.go index 259fda81..253a3022 100644 --- a/cmd/pebble/main.go +++ b/cmd/pebble/main.go @@ -30,7 +30,7 @@ type config struct { DomainBlocklist []string CertificateValidityPeriod uint64 - RetryAfter struct { + RetryAfter struct { Authz int Order int } diff --git a/core/types.go b/core/types.go index dc6cd1da..4452e8ee 100644 --- a/core/types.go +++ b/core/types.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "encoding/base64" "encoding/pem" + "errors" "fmt" "sync" "time" @@ -80,8 +81,8 @@ func (o *Order) GetStatus() (string, error) { // early. Somehow we made it this far but also don't have the correct number // of valid authzs. if !fullyAuthorized { - return "", fmt.Errorf( - "Order has the incorrect number of valid authorizations & no pending, " + + return "", errors.New( + "order has the incorrect number of valid authorizations & no pending, " + "deactivated or invalid authorizations") } @@ -104,7 +105,7 @@ func (o *Order) GetStatus() (string, error) { } // If none of the above cases match something weird & unexpected has happened. - return "", fmt.Errorf("Order is in an unknown state") + return "", errors.New("order is in an unknown state") } type Account struct { diff --git a/db/memorystore.go b/db/memorystore.go index c93c64eb..7b836509 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -117,7 +117,7 @@ func (m *MemoryStore) AddAccount(acct *core.Account) (int, error) { defer m.Unlock() if acct.Key == nil { - return 0, fmt.Errorf("account must not have a nil Key") + return 0, errors.New("account must not have a nil Key") } keyID, err := keyToID(acct.Key) @@ -134,7 +134,7 @@ func (m *MemoryStore) AddAccount(acct *core.Account) (int, error) { } if _, present := m.accountsByKeyID[keyID]; present { - return 0, fmt.Errorf("account with key already exists") + return 0, errors.New("account with key already exists") } acct.ID = acctID @@ -177,7 +177,7 @@ func (m *MemoryStore) AddOrder(order *core.Order) (int, error) { accountID := order.AccountID order.RUnlock() if len(orderID) == 0 { - return 0, fmt.Errorf("order must have a non-empty ID to add to MemoryStore") + return 0, errors.New("order must have a non-empty ID to add to MemoryStore") } if _, present := m.ordersByID[orderID]; present { @@ -238,7 +238,7 @@ func (m *MemoryStore) AddAuthorization(authz *core.Authorization) (int, error) { authz.RLock() authzID := authz.ID if len(authzID) == 0 { - return 0, fmt.Errorf("authz must have a non-empty ID to add to MemoryStore") + return 0, errors.New("authz must have a non-empty ID to add to MemoryStore") } authz.RUnlock() @@ -285,7 +285,7 @@ func (m *MemoryStore) AddChallenge(chal *core.Challenge) (int, error) { chalID := chal.ID chal.RUnlock() if len(chalID) == 0 { - return 0, fmt.Errorf("challenge must have a non-empty ID to add to MemoryStore") + return 0, errors.New("challenge must have a non-empty ID to add to MemoryStore") } if _, present := m.challengesByID[chalID]; present { @@ -308,7 +308,7 @@ func (m *MemoryStore) AddCertificate(cert *core.Certificate) (int, error) { certID := cert.ID if len(certID) == 0 { - return 0, fmt.Errorf("cert must have a non-empty ID to add to MemoryStore") + return 0, errors.New("cert must have a non-empty ID to add to MemoryStore") } if _, present := m.certificatesByID[certID]; present { @@ -372,7 +372,7 @@ func keyToID(key crypto.PublicKey) (string, error) { switch t := key.(type) { case *jose.JSONWebKey: if t == nil { - return "", fmt.Errorf("Cannot compute ID of nil key") + return "", errors.New("cannot compute ID of nil key") } return keyToID(t.Key) case jose.JSONWebKey: @@ -426,7 +426,7 @@ func (m *MemoryStore) AddExternalAccountKeyByID(keyID, key string) error { keyDecoded, err := base64.RawURLEncoding.DecodeString(key) if err != nil { - return fmt.Errorf("failed to decode base64 URL encoded key %q: %s", key, err) + return fmt.Errorf("failed to decode base64 URL encoded key %q: %w", key, err) } m.Lock() diff --git a/va/va.go b/va/va.go index 13e5d172..08a94377 100644 --- a/va/va.go +++ b/va/va.go @@ -110,7 +110,8 @@ type VAImpl struct { func New( log *log.Logger, httpPort, tlsPort int, - strict bool, customResolverAddr string) *VAImpl { + strict bool, customResolverAddr string, +) *VAImpl { va := &VAImpl{ log: log, httpPort: httpPort, @@ -215,7 +216,8 @@ func (va VAImpl) setOrderError(order *core.Order, err *acme.ProblemDetails) { func (va VAImpl) setAuthzInvalid( authz *core.Authorization, chal *core.Challenge, - err *acme.ProblemDetails) { + err *acme.ProblemDetails, +) { authz.Lock() defer authz.Unlock() // Update the authz status @@ -268,9 +270,9 @@ func (va VAImpl) process(task *vaTask) { func (va VAImpl) performValidation(task *vaTask, results chan<- *core.ValidationRecord) { if va.sleep { // Sleep for a random amount of time between 0 and va.sleepTime seconds - len := time.Duration(rand.Intn(va.sleepTime)) - va.log.Printf("Sleeping for %s seconds before validating", time.Second*len) - time.Sleep(time.Second * len) + length := time.Duration(rand.Intn(va.sleepTime)) * time.Second + va.log.Printf("Sleeping for %s seconds before validating", length) + time.Sleep(length) } // If `alwaysValid` is true then return a validation record immediately @@ -318,7 +320,7 @@ func (va VAImpl) validateDNS01(task *vaTask) *core.ValidationRecord { } if len(txts) == 0 { - msg := fmt.Sprintf("No TXT records found for DNS challenge") + msg := "No TXT records found for DNS challenge" result.Error = acme.UnauthorizedProblem(msg) return result } @@ -335,7 +337,7 @@ func (va VAImpl) validateDNS01(task *vaTask) *core.ValidationRecord { } } - msg := fmt.Sprintf("Correct value not found for DNS challenge") + msg := "Correct value not found for DNS challenge" result.Error = acme.UnauthorizedProblem(msg) return result } @@ -356,7 +358,6 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { } addrs, err := va.resolveIP(task.Identifier.Value) - if err != nil { result.Error = acme.MalformedProblem( fmt.Sprintf("Error occurred while resolving URL %q: %q", task.Identifier.Value, err)) @@ -451,7 +452,6 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { func (va VAImpl) fetchConnectionState(hostPort string, config *tls.Config) (*tls.ConnectionState, *acme.ProblemDetails) { conn, err := tls.DialWithDialer(&net.Dialer{Timeout: validationTimeout}, "tcp", hostPort, config) - if err != nil { // TODO(@cpu): Return better err - see parseHTTPConnError from boulder return nil, acme.UnauthorizedProblem( @@ -505,7 +505,7 @@ func (va VAImpl) fetchHTTP(identifier string, token string) ([]byte, string, *ac } va.log.Printf("Attempting to validate w/ HTTP: %s\n", url) - httpRequest, err := http.NewRequest("GET", url.String(), nil) + httpRequest, err := http.NewRequest(http.MethodGet, url.String(), nil) if err != nil { return nil, url.String(), acme.MalformedProblem( fmt.Sprintf("Invalid URL %q\n", url.String())) @@ -535,7 +535,7 @@ func (va VAImpl) fetchHTTP(identifier string, token string) ([]byte, string, *ac // Control specifically which IP will be used for this request addrs, err := va.resolveIP(host) if err != nil { - return nil, fmt.Errorf("error occurred while resolving URL %q: %q", url.String(), err) + return nil, fmt.Errorf("error occurred while resolving URL %q: %w", url.String(), err) } if len(addrs) == 0 { return nil, fmt.Errorf("could not resolve URL %q", url.String()) @@ -567,7 +567,7 @@ func (va VAImpl) fetchHTTP(identifier string, token string) ([]byte, string, *ac return nil, url.String(), acme.InternalErrorProblem(err.Error()) } - if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { return nil, url.String(), acme.UnauthorizedProblem( fmt.Sprintf("Non-200 status code from HTTP: %s returned %d", url.String(), resp.StatusCode)) @@ -590,7 +590,6 @@ func (va VAImpl) getTXTEntry(name string) ([]string, error) { message := new(dns.Msg) message.SetQuestion(dns.Fqdn(name), dns.TypeTXT) in, _, err := va.dnsClient.ExchangeContext(ctx, message, va.customResolverAddr) - if err != nil { return nil, err } @@ -629,7 +628,6 @@ func (va VAImpl) resolveIP(name string) ([]string, error) { messageAAAA := new(dns.Msg) messageAAAA.SetQuestion(dns.Fqdn(name), dns.TypeAAAA) inAAAA, _, err := va.dnsClient.ExchangeContext(ctx, messageAAAA, va.customResolverAddr) - if err != nil { return nil, err } @@ -643,7 +641,6 @@ func (va VAImpl) resolveIP(name string) ([]string, error) { messageA := new(dns.Msg) messageA.SetQuestion(dns.Fqdn(name), dns.TypeA) inA, _, err := va.dnsClient.ExchangeContext(ctx, messageA, va.customResolverAddr) - if err != nil { return nil, err } diff --git a/va/va_test.go b/va/va_test.go index 1c9c1bfa..d8d40fe9 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -11,7 +11,7 @@ import ( "github.com/letsencrypt/pebble/v2/db" ) -func TestAuthzRace(t *testing.T) { +func TestAuthzRace(_ *testing.T) { // Exercises a specific (fixed) race condition: // WARNING: DATA RACE // Read at 0x00c00040cde8 by goroutine 55: diff --git a/wfe/jose.go b/wfe/jose.go index 47cd7603..3d7acb6e 100644 --- a/wfe/jose.go +++ b/wfe/jose.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "crypto/x509" "encoding/base64" + "errors" "fmt" "github.com/letsencrypt/pebble/v2/acme" @@ -61,7 +62,7 @@ func keyDigest(key crypto.PublicKey) (string, error) { switch t := key.(type) { case *jose.JSONWebKey: if t == nil { - return "", fmt.Errorf("Cannot compute digest of nil key") + return "", errors.New("cannot compute digest of nil key") } return keyDigest(t.Key) case jose.JSONWebKey: diff --git a/wfe/wfe.go b/wfe/wfe.go index 314f07b2..99925bbb 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -175,10 +175,11 @@ func New( db *db.MemoryStore, va *va.VAImpl, ca *ca.CAImpl, - strict, requireEAB bool, retryAfterAuthz int, retryAfterOrder int) WebFrontEndImpl { + strict, requireEAB bool, retryAfterAuthz int, retryAfterOrder int, +) WebFrontEndImpl { // Seed rand from the current time so test environments don't always have // the same nonce rejection and sleep time patterns. - rand.Seed(time.Now().UnixNano()) + rand.New(rand.NewSource(time.Now().UnixNano())) // Read the % of good nonces that should be rejected as bad nonces from the // environment @@ -245,8 +246,8 @@ func (wfe *WebFrontEndImpl) HandleFunc( mux *http.ServeMux, pattern string, handler wfeHandlerFunc, - methods ...string) { - + methods ...string, +) { methodsMap := make(map[string]bool) for _, m := range methods { methodsMap[m] = true @@ -298,7 +299,8 @@ func (wfe *WebFrontEndImpl) HandleFunc( handler(ctx, response, request) cancel() }, - )}) + ), + }) mux.Handle(pattern, defaultHandler) } @@ -306,7 +308,8 @@ func (wfe *WebFrontEndImpl) HandleFunc( // enable use by CORS-aware user agents. If the request is a CORS preflight request, the // function returns true, in which case no further data may be written to the response. func (wfe *WebFrontEndImpl) processCORS(request *http.Request, response http.ResponseWriter, - allowedMethodsMap map[string]bool) bool { + allowedMethodsMap map[string]bool, +) bool { // 6.1.1, 6.2.1. No Origin header means CORS is not relevant. // 6.1.2, 6.2.2. Origin's value is not processed because it always matches. if request.Header.Get("Origin") == "" { @@ -345,7 +348,8 @@ func (wfe *WebFrontEndImpl) processCORS(request *http.Request, response http.Res func (wfe *WebFrontEndImpl) HandleManagementFunc( mux *http.ServeMux, pattern string, - handler wfeHandlerFunc) { // nolint:interfacer + handler wfeHandlerFunc, +) { mux.Handle(pattern, http.StripPrefix(pattern, handler)) } @@ -360,8 +364,10 @@ func (wfe *WebFrontEndImpl) sendError(prob *acme.ProblemDetails, response http.R _, _ = response.Write(problemDoc) } -type certGetter func(no int) *core.Certificate -type keyGetter func(no int) *rsa.PrivateKey +type ( + certGetter func(no int) *core.Certificate + keyGetter func(no int) *rsa.PrivateKey +) func (wfe *WebFrontEndImpl) handleCert( certGet certGetter, @@ -369,7 +375,7 @@ func (wfe *WebFrontEndImpl) handleCert( ctx context.Context, response http.ResponseWriter, request *http.Request) { - return func(ctx context.Context, response http.ResponseWriter, request *http.Request) { + return func(_ context.Context, response http.ResponseWriter, request *http.Request) { // Check for parameter no, err := strconv.Atoi(request.URL.Path) if err != nil { @@ -407,7 +413,7 @@ func (wfe *WebFrontEndImpl) handleKey( ctx context.Context, response http.ResponseWriter, request *http.Request) { - return func(ctx context.Context, response http.ResponseWriter, request *http.Request) { + return func(_ context.Context, response http.ResponseWriter, request *http.Request) { // Check for parameter no, err := strconv.Atoi(request.URL.Path) if err != nil { @@ -451,10 +457,10 @@ func (wfe *WebFrontEndImpl) handleKey( } func (wfe *WebFrontEndImpl) handleCertStatusBySerial( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { serialStr := strings.TrimPrefix(request.URL.Path, certStatusBySerial) serial := big.NewInt(0) if _, ok := serial.SetString(serialStr, 16); !ok { @@ -538,10 +544,10 @@ func (wfe *WebFrontEndImpl) ManagementHandler() http.Handler { } func (wfe *WebFrontEndImpl) Directory( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { directoryEndpoints := map[string]string{ "newNonce": noncePath, "newAccount": newAccountPath, @@ -621,9 +627,10 @@ func (wfe *WebFrontEndImpl) relativeEndpoint(request *http.Request, endpoint str } func (wfe *WebFrontEndImpl) Nonce( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { statusCode := http.StatusNoContent // The ACME specification says GET requets should receive http.StatusNoContent // and HEAD requests should receive http.StatusOK. @@ -829,8 +836,8 @@ type authenticatedPOST struct { // accounts. func (wfe *WebFrontEndImpl) verifyPOST( request *http.Request, - kx keyExtractor) (*authenticatedPOST, *acme.ProblemDetails) { - + kx keyExtractor, +) (*authenticatedPOST, *acme.ProblemDetails) { if prob := wfe.validPOST(request); prob != nil { return nil, prob } @@ -863,7 +870,8 @@ func (wfe *WebFrontEndImpl) verifyPOST( // acceptable and that the JWS verifies with the provided pubkey. func (wfe *WebFrontEndImpl) verifyJWSSignatureAndAlgorithm( pubKey *jose.JSONWebKey, - parsedJWS *jose.JSONWebSignature) ([]byte, *acme.ProblemDetails) { + parsedJWS *jose.JSONWebSignature, +) ([]byte, *acme.ProblemDetails) { if prob := checkAlgorithm(pubKey, parsedJWS); prob != nil { return nil, prob } @@ -878,7 +886,8 @@ func (wfe *WebFrontEndImpl) verifyJWSSignatureAndAlgorithm( // Extracts URL header parameter from parsed JWS. // Second return value indicates whether header was found. func (wfe *WebFrontEndImpl) extractJWSURL( - parsedJWS *jose.JSONWebSignature) (string, bool) { + parsedJWS *jose.JSONWebSignature, +) (string, bool) { headerURL, ok := parsedJWS.Signatures[0].Header.ExtraHeaders[jose.HeaderKey("url")].(string) if !ok || len(headerURL) == 0 { return "", false @@ -889,7 +898,8 @@ func (wfe *WebFrontEndImpl) extractJWSURL( func (wfe *WebFrontEndImpl) verifyJWS( pubKey *jose.JSONWebKey, parsedJWS *jose.JSONWebSignature, - request *http.Request) (*authenticatedPOST, *acme.ProblemDetails) { + request *http.Request, +) (*authenticatedPOST, *acme.ProblemDetails) { payload, prob := wfe.verifyJWSSignatureAndAlgorithm(pubKey, parsedJWS) if prob != nil { return nil, prob @@ -947,7 +957,8 @@ func (wfe *WebFrontEndImpl) verifyJWS( postAsGet: string(payload) == "", body: payload, url: headerURL, - jwk: pubKey}, nil + jwk: pubKey, + }, nil } // isASCII determines if every character in a string is encoded in @@ -1007,9 +1018,10 @@ func (wfe *WebFrontEndImpl) verifyContacts(acct acme.Account) *acme.ProblemDetai } func (wfe *WebFrontEndImpl) UpdateAccount( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { postData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) @@ -1101,9 +1113,10 @@ func (wfe *WebFrontEndImpl) UpdateAccount( } func (wfe *WebFrontEndImpl) ListOrders( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { postData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) @@ -1174,7 +1187,8 @@ func (wfe *WebFrontEndImpl) verifyKeyRollover( innerPayload []byte, existingAcct *core.Account, newKey *jose.JSONWebKey, - request *http.Request) *acme.ProblemDetails { + request *http.Request, +) *acme.ProblemDetails { var innerContent struct { Account string OldKey jose.JSONWebKey @@ -1211,9 +1225,10 @@ func (wfe *WebFrontEndImpl) verifyKeyRollover( } func (wfe *WebFrontEndImpl) KeyRollover( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { // Extract and parse outer JWS, and retrieve account outerPostData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { @@ -1267,12 +1282,11 @@ func (wfe *WebFrontEndImpl) KeyRollover( // Ok, now change account key err = wfe.db.ChangeAccountKey(existingAcct, newPubKey) if err != nil { - if existingAccountError, ok := err.(*db.ExistingAccountError); ok { + var existingAccountError *db.ExistingAccountError + if errors.As(err, &existingAccountError) { acctURL := wfe.relativeEndpoint(request, fmt.Sprintf("%s%s", acctPath, existingAccountError.MatchingAccount.ID)) response.Header().Set("Location", acctURL) response.WriteHeader(http.StatusConflict) - } else { - wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error rolling over account key (%s)", err.Error())), response) } return } @@ -1281,10 +1295,10 @@ func (wfe *WebFrontEndImpl) KeyRollover( } func (wfe *WebFrontEndImpl) NewAccount( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { // We use extractJWK rather than lookupJWK here because the account is not yet // created, so the user provides the full key in a JWS header rather than // referring to an existing key. @@ -1401,14 +1415,15 @@ func isDNSCharacter(ch byte) bool { // verifyOrder checks that a new order is considered well formed. Light // validation is done on the order identifiers. func (wfe *WebFrontEndImpl) verifyOrder(order *core.Order) *acme.ProblemDetails { - // Lock the order for reading - order.RLock() - defer order.RUnlock() - // Shouldn't happen - defensive check if order == nil { return acme.InternalErrorProblem("Order is nil") } + + // Lock the order for reading + order.RLock() + defer order.RUnlock() + idents := order.Identifiers if len(idents) == 0 { return acme.MalformedProblem("Order did not specify any identifiers") @@ -1440,8 +1455,7 @@ func (wfe *WebFrontEndImpl) verifyOrder(order *core.Order) *acme.ProblemDetails func (wfe *WebFrontEndImpl) validateDNSName(ident acme.Identifier) *acme.ProblemDetails { rawDomain := ident.Value if rawDomain == "" { - return acme.MalformedProblem(fmt.Sprintf( - "Order included DNS identifier with empty value")) + return acme.MalformedProblem("Order included DNS identifier with empty value") } for _, ch := range []byte(rawDomain) { @@ -1559,7 +1573,8 @@ func (wfe *WebFrontEndImpl) makeAuthorizations(order *core.Order, request *http. func (wfe *WebFrontEndImpl) makeChallenge( chalType string, authz *core.Authorization, - request *http.Request) (*core.Challenge, error) { + request *http.Request, +) (*core.Challenge, error) { // Create a new challenge of the requested type id := newToken() chal := &core.Challenge{ @@ -1621,10 +1636,10 @@ func (wfe *WebFrontEndImpl) makeChallenges(authz *core.Authorization, request *h // NewOrder creates a new Order request and populates its authorizations func (wfe *WebFrontEndImpl) NewOrder( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { postData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) @@ -1729,7 +1744,8 @@ func (wfe *WebFrontEndImpl) NewOrder( // rendered to JSON for display to an API client. func (wfe *WebFrontEndImpl) orderForDisplay( order *core.Order, - request *http.Request) acme.Order { + request *http.Request, +) acme.Order { // Lock the order for reading order.RLock() defer order.RUnlock() @@ -1768,9 +1784,10 @@ func (wfe *WebFrontEndImpl) orderForDisplay( // Order retrieves the details of an existing order func (wfe *WebFrontEndImpl) Order( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { postData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) @@ -1816,10 +1833,10 @@ func (wfe *WebFrontEndImpl) Order( } func (wfe *WebFrontEndImpl) FinalizeOrder( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { // Verify the POST request postData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { @@ -2039,9 +2056,10 @@ func prepAuthorizationForDisplay(authz *core.Authorization) acme.Authorization { } func (wfe *WebFrontEndImpl) Authz( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { // There are two types of requests we might get: // A) a POST to update the authorization // B) a POST-as-GET to get the authorization @@ -2140,9 +2158,10 @@ func (wfe *WebFrontEndImpl) Authz( } func (wfe *WebFrontEndImpl) Challenge( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { // There are two possibilities: // A) request is a POST to begin a challenge // B) request is a POST-as-GET to poll a challenge @@ -2164,13 +2183,13 @@ func (wfe *WebFrontEndImpl) Challenge( if !postData.postAsGet { wfe.updateChallenge(postData, response, request) return - } else { - // Otherwise it is case B) - account, prob = wfe.validPOSTAsGET(postData) - if prob != nil { - wfe.sendError(prob, response) - return - } + } + + // Otherwise it is case B) + account, prob = wfe.validPOSTAsGET(postData) + if prob != nil { + wfe.sendError(prob, response) + return } // Lock the challenge for reading in order to write the response @@ -2210,7 +2229,8 @@ func (wfe *WebFrontEndImpl) getAcctByKey(key crypto.PublicKey) (*core.Account, * } func (wfe *WebFrontEndImpl) validateChallengeUpdate( - chal *core.Challenge) (*core.Authorization, *acme.ProblemDetails) { + chal *core.Challenge, +) (*core.Authorization, *acme.ProblemDetails) { // Lock the challenge for reading to do validation chal.RLock() defer chal.RUnlock() @@ -2260,8 +2280,8 @@ func (wfe *WebFrontEndImpl) validateAuthzForChallenge(authz *core.Authorization) func (wfe *WebFrontEndImpl) updateChallenge( postData *authenticatedPOST, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { existingAcct, prob := wfe.getAcctByKey(postData.jwk) if prob != nil { wfe.sendError(prob, response) @@ -2278,31 +2298,31 @@ func (wfe *WebFrontEndImpl) updateChallenge( wfe.sendError( acme.MalformedProblem(`challenge initiation POST JWS body was not "{}"`), response) return - } else { - // When not in strict mode we still want to be strict about the legacy key - // authorization field not being present in the POST JSON. - var chalResp struct { - KeyAuthorization *string - } - err := json.Unmarshal(postData.body, &chalResp) - if err != nil { - wfe.sendError( - acme.MalformedProblem("Error unmarshaling body JSON"), response) - return - } + } - // Historically challenges were updated by POSTing a KeyAuthorization. This is - // unnecessary, the server can calculate this itself. We could ignore this if - // sent (and that's what Boulder will do) but for Pebble we'd like to offer - // a way to be more aggressive about pushing clients implementations in the - // right direction, so we treat this as a malformed request. - if chalResp.KeyAuthorization != nil { - wfe.sendError( - acme.MalformedProblem( - "Challenge response body contained legacy KeyAuthorization field, "+ - "POST body should be `{}`"), response) - return - } + // When not in strict mode we still want to be strict about the legacy key + // authorization field not being present in the POST JSON. + var chalResp struct { + KeyAuthorization *string + } + err := json.Unmarshal(postData.body, &chalResp) + if err != nil { + wfe.sendError( + acme.MalformedProblem("Error unmarshaling body JSON"), response) + return + } + + // Historically challenges were updated by POSTing a KeyAuthorization. This is + // unnecessary, the server can calculate this itself. We could ignore this if + // sent (and that's what Boulder will do) but for Pebble we'd like to offer + // a way to be more aggressive about pushing clients implementations in the + // right direction, so we treat this as a malformed request. + if chalResp.KeyAuthorization != nil { + wfe.sendError( + acme.MalformedProblem( + "Challenge response body contained legacy KeyAuthorization field, "+ + "POST body should be `{}`"), response) + return } chalID := strings.TrimPrefix(request.URL.Path, challengePath) @@ -2362,9 +2382,7 @@ func (wfe *WebFrontEndImpl) updateChallenge( // If the identifier value is for a wildcard domain then strip the wildcard // prefix before dispatching the validation to ensure the base domain is // validated. - if strings.HasPrefix(ident.Value, "*.") { - ident.Value = strings.TrimPrefix(ident.Value, "*.") - } + ident.Value = strings.TrimPrefix(ident.Value, "*.") // Confirm challenge status again and update it immediately before sending it to the VA prob = nil @@ -2388,7 +2406,7 @@ func (wfe *WebFrontEndImpl) updateChallenge( existingChal.RLock() defer existingChal.RUnlock() response.Header().Add("Link", link(existingChal.Authz.URL, "up")) - err := wfe.writeJSONResponse(response, http.StatusOK, existingChal.Challenge) + err = wfe.writeJSONResponse(response, http.StatusOK, existingChal.Challenge) if err != nil { wfe.sendError(acme.InternalErrorProblem("Error marshaling challenge"), response) return @@ -2416,7 +2434,7 @@ func getAlternateNo(url string) (string, int, error) { return url, 0, err } if no < 0 { - return url, 0, fmt.Errorf("number is negative") + return url, 0, errors.New("number is negative") } return urlSplit[0], no, nil } @@ -2438,9 +2456,10 @@ func addAlternateLinks(response http.ResponseWriter, url string, no int, number } func (wfe *WebFrontEndImpl) Certificate( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { + request *http.Request, +) { postData, prob := wfe.verifyPOST(request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) @@ -2568,10 +2587,10 @@ func uniqueIPs(IPs []net.IP) []net.IP { // Pebble's idea of certificate revocation is to forget the certificate exists. // This method does not percolate to a CRL or an OCSP response. func (wfe *WebFrontEndImpl) RevokeCert( - ctx context.Context, + _ context.Context, response http.ResponseWriter, - request *http.Request) { - + request *http.Request, +) { // The ACME specification handles the verification of revocation requests // differently from other endpoints that always use one JWS authentication // method. For this endpoint we need to accept a JWS with an embedded JWK, or @@ -2624,8 +2643,8 @@ func (wfe *WebFrontEndImpl) RevokeCert( func (wfe *WebFrontEndImpl) revokeCertByKeyID( jws *jose.JSONWebSignature, - request *http.Request) *acme.ProblemDetails { - + request *http.Request, +) *acme.ProblemDetails { pubKey, prob := wfe.lookupJWK(request, jws) if prob != nil { return prob @@ -2662,8 +2681,8 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID( func (wfe *WebFrontEndImpl) revokeCertByJWK( jws *jose.JSONWebSignature, - request *http.Request) *acme.ProblemDetails { - + request *http.Request, +) *acme.ProblemDetails { var requestKey *jose.JSONWebKey pubKey, prob := wfe.extractJWK(request, jws) if prob != nil { @@ -2698,8 +2717,8 @@ type authorizedToRevokeCert func(*core.Certificate) *acme.ProblemDetails func (wfe *WebFrontEndImpl) processRevocation( jwsBody []byte, - authorizedToRevoke authorizedToRevokeCert) *acme.ProblemDetails { - + authorizedToRevoke authorizedToRevokeCert, +) *acme.ProblemDetails { // revokeCertReq is the ACME certificate information submitted by the client var revokeCertReq struct { Certificate string `json:"certificate"` @@ -2726,12 +2745,10 @@ func (wfe *WebFrontEndImpl) processRevocation( if cert == nil { cert := wfe.db.GetRevokedCertificateByDER(derBytes) if cert != nil { - return acme.AlreadyRevokedProblem( - "Certificate has already been revoked.") - } else { - return acme.MalformedProblem( - "Unable to find specified certificate.") + return acme.AlreadyRevokedProblem("Certificate has already been revoked.") } + + return acme.MalformedProblem("Unable to find specified certificate.") } if prob := authorizedToRevoke(cert); prob != nil { @@ -2752,7 +2769,8 @@ func (wfe *WebFrontEndImpl) processRevocation( // then error. func (wfe *WebFrontEndImpl) verifyEAB( newAcctReq newAccountRequest, - outerPostData *authenticatedPOST) (*acme.JSONSigned, *acme.ProblemDetails) { + outerPostData *authenticatedPOST, +) (*acme.JSONSigned, *acme.ProblemDetails) { if newAcctReq.ExternalAccountBinding == nil { if wfe.requireEAB { return nil, acme.ExternalAccountRequiredProblem( @@ -2762,7 +2780,7 @@ func (wfe *WebFrontEndImpl) verifyEAB( return nil, nil } - //1. Verify that the value of the field is a well-formed JWS + // 1. Verify that the value of the field is a well-formed JWS eabBytes, err := json.Marshal(newAcctReq.ExternalAccountBinding) if err != nil { return nil, acme.InternalErrorProblem( @@ -2775,17 +2793,17 @@ func (wfe *WebFrontEndImpl) verifyEAB( fmt.Sprintf("failed to decode external account binding: %s", err)) } - //2. Verify that the JWS protected field meets the following criteria - //- The "alg" field MUST indicate a MAC-based algorithm - //- The "kid" field MUST contain the key identifier provided by the CA - //- The "nonce" field MUST NOT be present - //- The "url" field MUST be set to the same value as the outer JWS + // 2. Verify that the JWS protected field meets the following criteria + // - The "alg" field MUST indicate a MAC-based algorithm + // - The "kid" field MUST contain the key identifier provided by the CA + // - The "nonce" field MUST NOT be present + // - The "url" field MUST be set to the same value as the outer JWS keyID, prob := wfe.verifyEABPayloadHeader(eab, outerPostData) if prob != nil { return nil, prob } - //3. Retrieve the MAC key corresponding to the key identifier in the + // 3. Retrieve the MAC key corresponding to the key identifier in the // "kid" field key, ok := wfe.db.GetExtenalAccountKeyByID(keyID) if !ok { @@ -2793,14 +2811,14 @@ func (wfe *WebFrontEndImpl) verifyEAB( "the field 'kid' references a key that is not known to the ACME server") } - //4. Verify that the MAC on the JWS verifies using that MAC key + // 4. Verify that the MAC on the JWS verifies using that MAC key payload, err := eab.Verify(key) if err != nil { return nil, acme.UnauthorizedProblem( fmt.Sprintf("external account binding JWS verification error: %s", err)) } - //5. Verify that the payload of the JWS represents the same key as was + // 5. Verify that the payload of the JWS represents the same key as was // used to verify the outer JWS (i.e., the "jwk" field of the outer // JWS) if prob := wfe.verifyEABMatchesKey(payload, outerPostData.jwk); prob != nil {