Add linting, test coverage, code of conduct, lint fixes. (#215)

* WFE: remove unused vars
* db: format file
* ca: format file
* wfe: fix misspells
* wfe: remove unnecessary []byte conversion.
* db: prefer ++ for incrementing by 1
* va: use challtestsrv's exported IdPeAcmeIdentifier.
* wfe: remove throwaway resultUrl var
* wfe: rename writeJsonResponse -> writeJSONResponse
* ci: add golangci-lint and test coverage collection
* repo: add CODE_OF_CONDUCT
* README: Add standard badges
* ci: use go mod download hack
* ci: install goveralls tooling
* ci: fix linter exclude typo

.golangci.yaml

@@ -0,0 +1,32 @@
+linters-settings:
+  gocyclo:
+    min-complexity: 25
+  govet:
+    check-shadowing: false
+  misspell:
+    locale: "US"
+
+linters:
+  enable-all: true
+  disable:
+    - stylecheck
+    - gosec
+    - dupl
+    - maligned
+    - depguard
+    - lll
+    - prealloc
+    - scopelint
+    - gocritic
+    - gochecknoinits
+    - gochecknoglobals
+
+issues:
+  exclude:
+    # The following excludes are considered false-positives/known-OK.
+    # ca/ca.go:
+    - "type name will be used as ca.CAImpl by other packages, and that stutters; consider calling this Impl"
+    # va/va.go:
+    - "type name will be used as va.VAImpl by other packages, and that stutters; consider calling this Impl"
+    # wfe/wfe.go: L1647 and L1878
+    - "if` block ends with a `return` statement, so drop this `else` and outdent its block"

.travis.yml

@@ -27,6 +27,13 @@ before_install:
 # Override the base install phase so that the project can be installed using
 # `-mod=vendor` to use the vendored dependencies
 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
+  # gobin.
+  - GO111MODULE=off go get golang.org/x/tools/cmd/cover
+  - GO111MODULE=off go get github.com/mattn/goveralls
   - go install -v -mod=vendor ./...
 
 before_script:
@@ -35,7 +42,15 @@ before_script:
   - until </dev/tcp/localhost/14000 ; do sleep 0.1 ; done
 
 script:
-  - go vet -mod=vendor ./...
+  - go mod download
+  # Vet Go source code using the linter config (see .golang-ci.yml)
+  - golangci-lint run
+  # Run project unit tests (with the race detector enabled and atomic
+  # coverage profile collection)
+  - go test -mod=vendor -v -race -covermode=atomic -coverprofile=coverage.out ./...
+  # Upload collected coverage profile to goveralls
+  - 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
 
 deploy:

CODE_OF_CONDUCT.md

@@ -0,0 +1,3 @@
+# Contributor Code of Conduct
+
+The contributor code of conduct is available for reference [on the community forum](https://community.letsencrypt.org/guidelines).

README.md

@@ -1,6 +1,9 @@
 # Pebble
 
 [![Build Status](https://travis-ci.org/letsencrypt/pebble.svg?branch=master)](https://travis-ci.org/letsencrypt/pebble)
+[![Coverage Status](https://coveralls.io/repos/github/letsencrypt/pebble/badge.svg?branch=cpu-goveralls)](https://coveralls.io/github/letsencrypt/pebble?branch=cpu-goveralls)
+[![Go Report Card](https://goreportcard.com/badge/github.com/letsencrypt/pebble)](https://goreportcard.com/report/github.com/letsencrypt/pebble)
+[![GolangCI](https://golangci.com/badges/github.com/letsencrypt/pebble.svg)](https://golangci.com/r/github.com/letsencrypt/pebble)
 
 A miniature version of [Boulder](https://github.com/letsencrypt/boulder), Pebble
 is a small [ACME](https://github.com/ietf-wg-acme/acme) test server not suited

ca/ca.go

@@ -73,7 +73,7 @@ func (ca *CAImpl) makeRootCert(
 		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
 		BasicConstraintsValid: true,
-		IsCA: true,
+		IsCA:                  true,
 	}
 
 	var signerKey crypto.Signer
@@ -182,7 +182,7 @@ func (ca *CAImpl) newCertificate(domains []string, key crypto.PublicKey, account
 		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
 		BasicConstraintsValid: true,
-		IsCA: false,
+		IsCA:                  false,
 	}
 	der, err := x509.CreateCertificate(rand.Reader, template, issuer.cert.Cert, key, issuer.key)
 	if err != nil {

db/memorystore.go

@@ -26,7 +26,6 @@ func (e ExistingAccountError) Error() string {
 	return fmt.Sprintf("New public key is already in use by account %s", e.MatchingAccount.ID)
 }
 
-
 // Pebble keeps all of its various objects (accounts, orders, etc)
 // in-memory, not persisted anywhere. MemoryStore implements this in-memory
 // "database"
@@ -49,19 +48,19 @@ type MemoryStore struct {
 
 	challengesByID map[string]*core.Challenge
 
-	certificatesByID map[string]*core.Certificate
+	certificatesByID        map[string]*core.Certificate
 	revokedCertificatesByID map[string]*core.Certificate
 }
 
 func NewMemoryStore(clk clock.Clock) *MemoryStore {
 	return &MemoryStore{
-		clk:                    clk,
-		accountIDCounter:       1,
-		accountsByID:           make(map[string]*core.Account),
-		accountsByKeyID:        make(map[string]*core.Account),
-		ordersByID:             make(map[string]*core.Order),
-		authorizationsByID:     make(map[string]*core.Authorization),
-		challengesByID:         make(map[string]*core.Challenge),
+		clk:                     clk,
+		accountIDCounter:        1,
+		accountsByID:            make(map[string]*core.Account),
+		accountsByKeyID:         make(map[string]*core.Account),
+		ordersByID:              make(map[string]*core.Order),
+		authorizationsByID:      make(map[string]*core.Authorization),
+		challengesByID:          make(map[string]*core.Challenge),
 		certificatesByID:        make(map[string]*core.Certificate),
 		revokedCertificatesByID: make(map[string]*core.Certificate),
 	}
@@ -107,7 +106,7 @@ func (m *MemoryStore) AddAccount(acct *core.Account) (int, error) {
 	defer m.Unlock()
 
 	acctID := strconv.Itoa(m.accountIDCounter)
-	m.accountIDCounter += 1
+	m.accountIDCounter++
 
 	if acct.Key == nil {
 		return 0, fmt.Errorf("account must not have a nil Key")

va/va.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/jmhodges/clock"
+	"github.com/letsencrypt/challtestsrv"
 	"github.com/letsencrypt/pebble/acme"
 	"github.com/letsencrypt/pebble/core"
 )
@@ -69,10 +70,6 @@ const (
 	noValidateEnvVar = "PEBBLE_VA_ALWAYS_VALID"
 )
 
-// This is the identifier defined in draft-04 and newer, as registered with IANA
-// (https://tools.ietf.org/html/draft-ietf-acme-tls-alpn-04#page-4)
-var IdPeAcmeIdentifier = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 31}
-
 func userAgent() string {
 	return fmt.Sprintf(
 		"%s (%s; %s)",
@@ -390,7 +387,7 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord {
 	h := sha256.Sum256([]byte(expectedKeyAuthorization))
 	for _, ext := range leafCert.Extensions {
 		if ext.Critical {
-			hasAcmeIdentifier := IdPeAcmeIdentifier.Equal(ext.Id)
+			hasAcmeIdentifier := challtestsrv.IdPeAcmeIdentifier.Equal(ext.Id)
 			if hasAcmeIdentifier {
 				var extValue []byte
 				if _, err := asn1.Unmarshal(ext.Value, &extValue); err != nil {

wfe/jose.go

@@ -80,7 +80,7 @@ func keyDigest(key crypto.PublicKey) (string, error) {
 func keyDigestEquals(j, k crypto.PublicKey) bool {
 	digestJ, errJ := keyDigest(j)
 	digestK, errK := keyDigest(k)
-	// Keys that don't have a valid digest (due to marshalling problems)
+	// Keys that don't have a valid digest (due to marshaling problems)
 	// are never equal. So, e.g. nil keys are not equal.
 	if errJ != nil || errK != nil {
 		return false