Enable all linters for ct-go (#1064)

Fixed lint errors in best local way possible. Some of these cases seem like they should be propagated further, but that is outside the scope of this work and requires more context than I can load in now. The result of these changes is that errors should at least no longer silently be dropped.

There are still some lint rules that are not strictly enforced by this change, specifically https://golangci-lint.run/usage/false-positives/#exc0001. These are enforced by the trillian and trillian-examples repos. We should aim to get consistency here, but change represents a positive step in the right direction whether we choose to live with this exception or remove it and fix the remaining errors.

.golangci.yaml

@@ -7,7 +7,7 @@ run:
 
 linters-settings:
   gocyclo:
-    min-complexity: 40
+    min-complexity: 25
   depguard:
     list-type: blacklist
     packages:
@@ -16,22 +16,7 @@ linters-settings:
       - encoding/asn1
       - crypto/x509
 
-linters:
-  disable-all: true
-  enable:
-    - depguard
-    - gocyclo
-    - gofmt
-    - goimports
-    - govet
-    - ineffassign
-    - megacheck
-    - misspell
-    - revive
-    - unused
-    # TODO(gbelvin): write license linter and commit to upstream.
-    # ./scripts/check_license.sh is run by ./scripts/presubmit.sh
-
 issues:
-  # Don't turn off any checks by default. We can do this explicitly if needed.
-  exclude-use-default: false
+  exclude-use-default: false
+  exclude:
+    - "Error return value of .((os\\.)?std(out|err)\\..*|.*Close|.*Flush|os\\.Remove(All)?|.*printf?|os\\.(Un)?Setenv). is not checked"

client/multilog_test.go

@@ -340,10 +340,14 @@ func TestTemporalAddChain(t *testing.T) {
 		switch r.URL.Path {
 		case "/ct/v1/add-chain":
 			data, _ := sctToJSON(testdata.TestCertProof)
-			w.Write(data)
+			if _, err := w.Write(data); err != nil {
+				t.Error(err)
+			}
 		case "/ct/v1/add-pre-chain":
 			data, _ := sctToJSON(testdata.TestPreCertProof)
-			w.Write(data)
+			if _, err := w.Write(data); err != nil {
+				t.Error(err)
+			}
 		default:
 			t.Fatalf("Incorrect URL path: %s", r.URL.Path)
 		}

fixchain/chainfix/chainfix.go

@@ -76,14 +76,18 @@ func contentStore(baseDir string, subDir string, content []byte) {
 	r := sha256.Sum256(content)
 	h := base64.URLEncoding.EncodeToString(r[:])
 	d := baseDir + "/" + subDir
-	os.MkdirAll(d, 0777)
+	if err := os.MkdirAll(d, 0777); err != nil {
+		log.Fatalf("Can't create directories %q: %v", d, err)
+	}
 	fn := d + "/" + h
 	f, err := os.Create(fn)
 	if err != nil {
 		log.Fatalf("Can't create %q: %s", fn, err)
 	}
 	defer f.Close()
-	f.Write(content)
+	if _, err := f.Write(content); err != nil {
+		log.Fatalf("Can't write to %q: %v", fn, err)
+	}
 }
 
 func logStringErrors(wg *sync.WaitGroup, errors chan *fixchain.FixError, baseDir string) {

fixchain/logger.go

@@ -163,7 +163,9 @@ func (l *Logger) postChain(p *toPost) {
 		derChain = append(derChain, ct.ASN1Cert{Data: cert.Raw})
 	}
 
-	l.limiter.Wait(l.ctx)
+	if err := l.limiter.Wait(l.ctx); err != nil {
+		log.Println(err)
+	}
 	atomic.AddUint32(&l.posted, 1)
 	_, err := l.client.AddChain(l.ctx, derChain)
 	if err != nil {

internal/witness/cmd/witness/impl/witness.go

@@ -134,6 +134,8 @@ func Main(ctx context.Context, opts ServerOpts) error {
 	}()
 	<-ctx.Done()
 	klog.Info("Server shutting down")
-	hServer.Shutdown(ctx)
+	if err := hServer.Shutdown(ctx); err != nil {
+		klog.Errorf("Shutdown(): %v", err)
+	}
 	return <-e
 }

internal/witness/cmd/witness/internal/http/server.go

@@ -27,6 +27,7 @@ import (
 	"github.com/gorilla/mux"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"k8s.io/klog/v2"
 )
 
 // Server is the core handler implementation of the witness.
@@ -76,7 +77,9 @@ func (s *Server) update(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 	w.Header().Set("Content-Type", "text/plain")
-	w.Write(sth)
+	if _, err := w.Write(sth); err != nil {
+		klog.Errorf("Write(): %v", err)
+	}
 }
 
 // getSTH returns an STH stored for a given log.
@@ -93,7 +96,9 @@ func (s *Server) getSTH(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	w.Header().Set("Content-Type", "text/plain")
-	w.Write(sth)
+	if _, err := w.Write(sth); err != nil {
+		klog.Errorf("Write(): %v", err)
+	}
 }
 
 // getLogs returns a list of all logs the witness is aware of.
@@ -109,7 +114,9 @@ func (s *Server) getLogs(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	w.Header().Set("Content-Type", "text/json")
-	w.Write(logList)
+	if _, err := w.Write(logList); err != nil {
+		klog.Errorf("Write(): %v", err)
+	}
 }
 
 // RegisterHandlers registers HTTP handlers for witness endpoints.

internal/witness/cmd/witness/internal/http/server_test.go

@@ -122,13 +122,17 @@ func dh(h string, expLen int) []byte {
 	return r
 }
 
-func mustCreateDB(t *testing.T) (*sql.DB, func() error) {
+func mustCreateDB(t *testing.T) (*sql.DB, func()) {
 	t.Helper()
 	db, err := sql.Open("sqlite3", ":memory:")
 	if err != nil {
 		t.Fatalf("failed to open temporary in-memory DB: %v", err)
 	}
-	return db, db.Close
+	return db, func() {
+		if err := db.Close(); err != nil {
+			t.Error(err)
+		}
+	}
 }
 
 func mustCreatePK(pkPem string) crypto.PublicKey {

internal/witness/cmd/witness/internal/witness/witness.go

@@ -36,6 +36,7 @@ import (
 	"github.com/transparency-dev/merkle/rfc6962"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"k8s.io/klog/v2"
 )
 
 // Opts is the options passed to a witness.
@@ -174,7 +175,11 @@ func (w *Witness) Update(ctx context.Context, logID string, nextRaw []byte, pf [
 	if err != nil {
 		return nil, fmt.Errorf("couldn't create db tx: %v", err)
 	}
-	defer tx.Rollback()
+	defer func() {
+		if err := tx.Rollback(); err != nil {
+			klog.Errorf("Rollback(): %v", err)
+		}
+	}()
 
 	// Get the latest STH (if one exists).
 	prevRaw, err := w.getLatestSTH(tx.QueryRow, logID)

internal/witness/cmd/witness/internal/witness/witness_test.go

@@ -125,13 +125,17 @@ func dh(h string, expLen int) []byte {
 	return r
 }
 
-func mustCreateDB(t *testing.T) (*sql.DB, func() error) {
+func mustCreateDB(t *testing.T) (*sql.DB, func()) {
 	t.Helper()
 	db, err := sql.Open("sqlite3", ":memory:")
 	if err != nil {
 		t.Fatalf("failed to open temporary in-memory DB: %v", err)
 	}
-	return db, db.Close
+	return db, func() {
+		if err := db.Close(); err != nil {
+			t.Error(err)
+		}
+	}
 }
 
 func TestGetLogs(t *testing.T) {

preload/preloader/preloader.go

@@ -241,7 +241,9 @@ func main() {
 		}
 		precerts <- entry
 	}
-	s.Scan(ctx, addChainFunc, addPreChainFunc)
+	if err := s.Scan(ctx, addChainFunc, addPreChainFunc); err != nil {
+		klog.Errorf("Scan(): %v", err)
+	}
 
 	close(certs)
 	close(precerts)

scanner/scanlog/scanlog.go

@@ -220,8 +220,12 @@ func main() {
 
 	ctx := context.Background()
 	if *printChains {
-		s.Scan(ctx, logFullChain, logFullChain)
+		if err := s.Scan(ctx, logFullChain, logFullChain); err != nil {
+			log.Fatal(err)
+		}
 	} else {
-		s.Scan(ctx, logCertInfo, logPrecertInfo)
+		if err := s.Scan(ctx, logCertInfo, logPrecertInfo); err != nil {
+			log.Fatal(err)
+		}
 	}
 }

submission/distributor.go

@@ -307,7 +307,9 @@ func (d *Distributor) addSomeChain(ctx context.Context, rawChain [][]byte, loadP
 			if err != nil {
 				return
 			}
-			GetSCTs(ctx, d, chain, asPreChain, pendingGroup)
+			if _, err := GetSCTs(ctx, d, chain, asPreChain, pendingGroup); err != nil {
+				klog.Errorf("GetSCTs(): %v", err)
+			}
 		}()
 	}
 	return GetSCTs(ctx, d, chain, asPreChain, groups)
@@ -369,7 +371,9 @@ func NewDistributor(ll *loglist3.LogList, plc ctpolicy.CTPolicy, lcBuilder LogCl
 	if err := d.buildLogClients(lcBuilder, d.usableLl); err != nil {
 		return nil, err
 	}
-	d.buildLogClients(lcBuilder, d.pendingQualifiedLl)
+	if err := d.buildLogClients(lcBuilder, d.pendingQualifiedLl); err != nil {
+		return nil, err
+	}
 
 	if mf == nil {
 		mf = monitoring.InertMetricFactory{}

submission/proxy_server.go

@@ -145,5 +145,8 @@ func (s *ProxyServer) HandleInfo(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	t.Execute(w, data)
+	if err := t.Execute(w, data); err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
 }

submission/races_test.go

@@ -29,7 +29,9 @@ import (
 
 func testdataSCT() *ct.SignedCertificateTimestamp {
 	var sct ct.SignedCertificateTimestamp
-	tls.Unmarshal(testdata.TestPreCertProof, &sct)
+	if _, err := tls.Unmarshal(testdata.TestPreCertProof, &sct); err != nil {
+		panic(err)
+	}
 	return &sct
 }
 

trillian/ctfe/ct_server/main.go

@@ -148,17 +148,25 @@ func main() {
 
 		etcdHTTPKey := fmt.Sprintf("%s/%s", *etcdHTTPService, *httpEndpoint)
 		klog.Infof("Announcing our presence at %v with %+v", etcdHTTPKey, *httpEndpoint)
-		httpManager.AddEndpoint(ctx, etcdHTTPKey, endpoints.Endpoint{Addr: *httpEndpoint})
+		if err := httpManager.AddEndpoint(ctx, etcdHTTPKey, endpoints.Endpoint{Addr: *httpEndpoint}); err != nil {
+			klog.Exitf("AddEndpoint(): %v", err)
+		}
 
 		etcdMetricsKey := fmt.Sprintf("%s/%s", *etcdMetricsService, metricsAt)
 		klog.Infof("Announcing our presence in %v with %+v", *etcdMetricsService, metricsAt)
-		metricsManager.AddEndpoint(ctx, etcdMetricsKey, endpoints.Endpoint{Addr: metricsAt})
+		if err := metricsManager.AddEndpoint(ctx, etcdMetricsKey, endpoints.Endpoint{Addr: metricsAt}); err != nil {
+			klog.Exitf("AddEndpoint(): %v", err)
+		}
 
 		defer func() {
 			klog.Infof("Removing our presence in %v", etcdHTTPKey)
-			httpManager.DeleteEndpoint(ctx, etcdHTTPKey)
+			if err := httpManager.DeleteEndpoint(ctx, etcdHTTPKey); err != nil {
+				klog.Errorf("DeleteEndpoint(): %v", err)
+			}
 			klog.Infof("Removing our presence in %v", etcdMetricsKey)
-			metricsManager.DeleteEndpoint(ctx, etcdMetricsKey)
+			if err := metricsManager.DeleteEndpoint(ctx, etcdMetricsKey); err != nil {
+				klog.Errorf("DeleteEndpoint(): %v", err)
+			}
 		}()
 	} else if strings.Contains(*rpcBackend, ",") {
 		// This should probably not be used in production. Either use etcd or a gRPC
@@ -249,7 +257,9 @@ func main() {
 	// Export a healthz target.
 	corsMux.HandleFunc("/healthz", func(resp http.ResponseWriter, req *http.Request) {
 		// TODO(al): Wire this up to tell the truth.
-		resp.Write([]byte("ok"))
+		if _, err := resp.Write([]byte("ok")); err != nil {
+			klog.Errorf("resp.Write(): %v", err)
+		}
 	})
 
 	if metricsAt != *httpEndpoint {
@@ -285,7 +295,9 @@ func main() {
 		ctx, cancel := context.WithTimeout(context.Background(), time.Second*60)
 		defer cancel()
 		klog.Info("Shutting down HTTP server...")
-		srv.Shutdown(ctx)
+		if err := srv.Shutdown(ctx); err != nil {
+			klog.Errorf("srv.Shutdown(): %v", err)
+		}
 		klog.Info("HTTP server shutdown")
 	})
 

trillian/integration/copier.go

@@ -152,15 +152,22 @@ func NewCopyChainGeneratorFromOpts(ctx context.Context, client *client.LogClient
 		Continuous:    true,
 		StartIndex:    startIndex,
 	}
-	certFetcher := scanner.NewFetcher(client, &fetchOpts)
-	go certFetcher.Run(ctx, func(batch scanner.EntryBatch) {
-		generator.processBatch(batch, generator.certs, ct.X509LogEntryType)
-	})
-
-	precertFetcher := scanner.NewFetcher(client, &fetchOpts)
-	go precertFetcher.Run(ctx, func(batch scanner.EntryBatch) {
-		generator.processBatch(batch, generator.precerts, ct.PrecertLogEntryType)
-	})
+	go func() {
+		certFetcher := scanner.NewFetcher(client, &fetchOpts)
+		if err := certFetcher.Run(ctx, func(batch scanner.EntryBatch) {
+			generator.processBatch(batch, generator.certs, ct.X509LogEntryType)
+		}); err != nil {
+			klog.Errorf("processBatch(): %v", err)
+		}
+	}()
+	go func() {
+		precertFetcher := scanner.NewFetcher(client, &fetchOpts)
+		if err := precertFetcher.Run(ctx, func(batch scanner.EntryBatch) {
+			generator.processBatch(batch, generator.precerts, ct.PrecertLogEntryType)
+		}); err != nil {
+			klog.Errorf("processBatch(): %v", err)
+		}
+	}()
 
 	return generator, nil
 }

trillian/integration/ct_hammer/main.go

@@ -253,7 +253,9 @@ func main() {
 		mcData, _ := base64.StdEncoding.DecodeString(mc)
 		b := bytes.NewReader(mcData)
 		r, _ := gzip.NewReader(b)
-		io.Copy(os.Stdout, r)
+		if _, err := io.Copy(os.Stdout, r); err != nil {
+			klog.Exitf("Failed to print banner!")
+		}
 		r.Close()
 		fmt.Print("\n\nHammer Time\n\n")
 	}

trillian/integration/ct_integration.go

@@ -25,7 +25,6 @@ import (
 	"crypto/sha256"
 	"encoding/pem"
 	"fmt"
-	"io"
 	"math/rand"
 	"net"
 	"net/http"
@@ -913,7 +912,6 @@ func (ls *logStats) fromServer(ctx context.Context, servers string) (*logStats,
 			return nil, fmt.Errorf("getting stats failed: %v", err)
 		}
 		defer httpRsp.Body.Close()
-		defer io.ReadAll(httpRsp.Body)
 		if httpRsp.StatusCode != http.StatusOK {
 			return nil, fmt.Errorf("got HTTP Status %q", httpRsp.Status)
 		}

trillian/integration/hammer.go

@@ -976,7 +976,9 @@ func (s *hammerState) String() string {
 }
 
 func (s *hammerState) performOp(ctx context.Context, ep ctfe.EntrypointName) (int, error) {
-	s.cfg.Limiter.Wait(ctx)
+	if err := s.cfg.Limiter.Wait(ctx); err != nil {
+		return http.StatusRequestTimeout, fmt.Errorf("Limiter.Wait(): %v", err)
+	}
 
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -1014,7 +1016,9 @@ func (s *hammerState) performOp(ctx context.Context, ep ctfe.EntrypointName) (in
 }
 
 func (s *hammerState) performInvalidOp(ctx context.Context, ep ctfe.EntrypointName) error {
-	s.cfg.Limiter.Wait(ctx)
+	if err := s.cfg.Limiter.Wait(ctx); err != nil {
+		return fmt.Errorf("Limiter.Wait(): %v", err)
+	}
 	switch ep {
 	case ctfe.AddChainName:
 		return s.addChainInvalid(ctx)