CI: Add golangci-lint, enforce Go best practices (#335)

* tidy: rename test functions so they are run. Unit tests functions must be named with `Test` as a prefix or they won't be run. This fixes an `unused` golangci-lint finding for this file. * tidy: remove unused functions. Neither of these functions are being used anywhere. Deleting them fixes a `golangci-lint` finding from the `unused` linter. * tidy: cleanup errcheck finding * tidy: cleanup errcheck finding * tidy: fixup all gosimple golangci-lint findings * tidy: fix ineffassign golangci-lint findings * tidy: cleanup gocritic golangci-lint finding * tidy: fix golangci-lint goimports findings * tidy: fix golangci-lint nakedret finding * tidy: ignore golangci-lint interfacer for one func * tidy: fix golangci-lint misspell finding * tidy: add some gocyclo lint ignores * CI: enforce golangci-lint.
zmap · Dec 9, 2019 · c74b45b · c74b45b
1 parent 872e431
commit c74b45b
Show file tree

Hide file tree

Showing 33 changed files with 109 additions and 88 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -0,0 +1,30 @@
+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
+    - godox
+    - funlen
+    - wsl
+    - whitespace
+    - gocognit
+    # TODO(@cpu): Uncomment goconst/golint once findings resolved.
+    - goconst
+    - golint
diff --git a/.travis.yml b/.travis.yml
@@ -5,13 +5,17 @@ dist: trusty
 go:
   - "1.13.x"
 
+install:
+  # Install `golangci-lint` using the installer script
+  - curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.21.0
+
 script:
   # Fast-fail on non-zero exit codes
   - set -e
   # Build commands
   - make
-  # Verify that all files have been gofmt'd with simplification
-  - make format-check
+  # Verify that all files pass the golangci-lint code lints
+  - make code-lint
   # Run unit tests
   - make test
   # Run integration tests

diff --git a/cmd/zlint/main.go b/cmd/zlint/main.go
@@ -180,7 +180,7 @@ func includeLints() {
 		includesMap[includeName] = struct{}{}
 	}
 
-	// clear all initialised lints except for includes
+	// clear all initialized lints except for includes
 	for lintName := range lints.Lints {
 		if _, ok := includesMap[lintName]; !ok {
 			delete(lints.Lints, lintName)

diff --git a/gofmt_test.go b/gofmt_test.go
@@ -32,7 +32,7 @@ func TestGofmt(t *testing.T) {
 		cmd := exec.Command("/bin/sh", "-c", gofmtCmd)
 		var out bytes.Buffer
 		cmd.Stdout = &out
-		cmd.Run()
+		_ = cmd.Run()
 		if out.String() != "" {
 			t.Errorf("glob %s not gofmt'ed", glob)
 		}

diff --git a/lints/lint_ca_is_ca.go b/lints/lint_ca_is_ca.go
@@ -43,7 +43,7 @@ func (l *caIsCA) Execute(c *x509.Certificate) *LintResult {
 	if err != nil {
 		return &LintResult{Status: Fatal}
 	}
-	if constraints.IsCA == true {
+	if constraints.IsCA {
 		return &LintResult{Status: Pass}
 	} else {
 		return &LintResult{Status: Error}

diff --git a/lints/lint_distribution_point_incomplete_test.go b/lints/lint_distribution_point_incomplete_test.go
@@ -18,7 +18,7 @@ import (
 	"testing"
 )
 
-func crlCompleteDp(t *testing.T) {
+func TestCRLCompleteDp(t *testing.T) {
 	inputPath := "../testlint/testCerts/crlComlepteDp.pem"
 	expected := Pass
 	out := Lints["e_distribution_point_incomplete"].Execute(ReadCertificate(inputPath))
@@ -27,7 +27,7 @@ func crlCompleteDp(t *testing.T) {
 	}
 }
 
-func crlIncompleteDp(t *testing.T) {
+func TestCRLIncompleteDp(t *testing.T) {
 	inputPath := "../testlint/testCerts/crlIncomlepteDp.pem"
 	expected := Error
 	out := Lints["e_distribution_point_incomplete"].Execute(ReadCertificate(inputPath))

diff --git a/lints/lint_ec_improper_curves.go b/lints/lint_ec_improper_curves.go
@@ -41,12 +41,11 @@ func (l *ecImproperCurves) Execute(c *x509.Certificate) *LintResult {
 	/* Declare theKey to be a ECDSA Public Key */
 	var theKey *ecdsa.PublicKey
 	/* Need to do different things based on what c.PublicKey is */
-	switch c.PublicKey.(type) {
+	switch keyType := c.PublicKey.(type) {
 	case *x509.AugmentedECDSA:
-		temp := c.PublicKey.(*x509.AugmentedECDSA)
-		theKey = temp.Pub
+		theKey = keyType.Pub
 	case *ecdsa.PublicKey:
-		theKey = c.PublicKey.(*ecdsa.PublicKey)
+		theKey = keyType
 	}
 	/* Now can actually check the params */
 	theParams := theKey.Curve.Params()

diff --git a/lints/lint_ext_crl_distribution_marked_critical.go b/lints/lint_ext_crl_distribution_marked_critical.go
@@ -35,7 +35,7 @@ func (l *ExtCrlDistributionMarkedCritical) CheckApplies(cert *x509.Certificate)
 
 func (l *ExtCrlDistributionMarkedCritical) Execute(cert *x509.Certificate) *LintResult {
 	if e := util.GetExtFromCert(cert, util.CrlDistOID); e != nil {
-		if e.Critical == false {
+		if !e.Critical {
 			return &LintResult{Status: Pass}
 		} else {
 			return &LintResult{Status: Warn}

diff --git a/lints/lint_ext_san_uri_host_not_fqdn_or_ip.go b/lints/lint_ext_san_uri_host_not_fqdn_or_ip.go
@@ -27,9 +27,10 @@ Section 7.4.
 *********************************************************************/
 
 import (
+	"net/url"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
-	"net/url"
 )
 
 type SANURIHost struct{}

diff --git a/lints/lint_name_constraint_maximum_not_absent.go b/lints/lint_name_constraint_maximum_not_absent.go
@@ -40,6 +40,7 @@ func (l *nameConstraintMax) CheckApplies(c *x509.Certificate) bool {
 	return util.IsExtInCert(c, util.NameConstOID)
 }
 
+//nolint:gocyclo
 func (l *nameConstraintMax) Execute(c *x509.Certificate) *LintResult {
 	for _, i := range c.PermittedDNSNames {
 		if i.Max != 0 {

diff --git a/lints/lint_name_constraint_minimum_non_zero.go b/lints/lint_name_constraint_minimum_non_zero.go
@@ -40,6 +40,7 @@ func (l *nameConstMin) CheckApplies(c *x509.Certificate) bool {
 	return util.IsExtInCert(c, util.NameConstOID)
 }
 
+//nolint:gocyclo
 func (l *nameConstMin) Execute(c *x509.Certificate) *LintResult {
 	for _, i := range c.PermittedDNSNames {
 		if i.Min != 0 {

diff --git a/lints/lint_qcstatem_etsi_present_qcs_critical.go b/lints/lint_qcstatem_etsi_present_qcs_critical.go
@@ -15,17 +15,12 @@
 package lints
 
 import (
-	"encoding/asn1"
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )
 
 type qcStatemQcEtsiPresentQcsCritical struct{}
 
-func (this *qcStatemQcEtsiPresentQcsCritical) getStatementOid() *asn1.ObjectIdentifier {
-	return &util.IdEtsiQcsQcCompliance
-}
-
 func (l *qcStatemQcEtsiPresentQcsCritical) Initialize() error {
 	return nil
 }

diff --git a/lints/lint_qcstatem_etsi_type_as_statem.go b/lints/lint_qcstatem_etsi_type_as_statem.go
@@ -17,6 +17,7 @@ package lints
 import (
 	"encoding/asn1"
 	"fmt"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )

diff --git a/lints/lint_qcstatem_mandatory_etsi_statems.go b/lints/lint_qcstatem_mandatory_etsi_statems.go
@@ -16,6 +16,7 @@ package lints
 
 import (
 	"encoding/asn1"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )

diff --git a/lints/lint_qcstatem_qccompliance_valid.go b/lints/lint_qcstatem_qccompliance_valid.go
@@ -16,6 +16,7 @@ package lints
 
 import (
 	"encoding/asn1"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )

diff --git a/lints/lint_qcstatem_qcpds_lang_case.go b/lints/lint_qcstatem_qcpds_lang_case.go
@@ -17,9 +17,10 @@ package lints
 import (
 	"encoding/asn1"
 	"fmt"
+	"unicode"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
-	"unicode"
 )
 
 type qcStatemQcPdsLangCase struct{}

diff --git a/lints/lint_qcstatem_qcpds_valid.go b/lints/lint_qcstatem_qcpds_valid.go
@@ -17,9 +17,10 @@ package lints
 import (
 	"encoding/asn1"
 	"fmt"
+	"strings"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
-	"strings"
 )
 
 type qcStatemQcPdsValid struct{}

diff --git a/lints/lint_qcstatem_qcretentionperiod_valid.go b/lints/lint_qcstatem_qcretentionperiod_valid.go
@@ -16,6 +16,7 @@ package lints
 
 import (
 	"encoding/asn1"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )

diff --git a/lints/lint_qcstatem_qcsscd_valid.go b/lints/lint_qcstatem_qcsscd_valid.go
@@ -16,6 +16,7 @@ package lints
 
 import (
 	"encoding/asn1"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )

diff --git a/lints/lint_qcstatem_qctype_valid.go b/lints/lint_qcstatem_qctype_valid.go
@@ -17,6 +17,7 @@ package lints
 import (
 	"encoding/asn1"
 	"fmt"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )

diff --git a/lints/lint_qcstatem_qctype_web.go b/lints/lint_qcstatem_qctype_web.go
@@ -17,6 +17,7 @@ package lints
 import (
 	"encoding/asn1"
 	"fmt"
+
 	"github.com/zmap/zcrypto/x509"
 	"github.com/zmap/zlint/util"
 )
@@ -60,9 +61,8 @@ func (l *qcStatemQctypeWeb) Execute(c *x509.Certificate) *LintResult {
 				found = true
 			}
 		}
-		if found != true {
+		if !found {
 			wrnString += fmt.Sprintf("etsi Type does not indicate certificate as a 'web' certificate")
-
 		}
 	}
 

diff --git a/lints/lint_sub_cert_certificate_policies_marked_critical.go b/lints/lint_sub_cert_certificate_policies_marked_critical.go
@@ -37,7 +37,7 @@ func (l *subCertPolicyCrit) CheckApplies(c *x509.Certificate) bool {
 
 func (l *subCertPolicyCrit) Execute(c *x509.Certificate) *LintResult {
 	e := util.GetExtFromCert(c, util.CertPolicyOID)
-	if e.Critical == false {
+	if !e.Critical {
 		return &LintResult{Status: Pass}
 	} else {
 		return &LintResult{Status: Warn}

diff --git a/lints/lint_sub_cert_crl_distribution_points_marked_critical.go b/lints/lint_sub_cert_crl_distribution_points_marked_critical.go
@@ -37,9 +37,8 @@ func (l *subCrlDistCrit) CheckApplies(c *x509.Certificate) bool {
 }
 
 func (l *subCrlDistCrit) Execute(c *x509.Certificate) *LintResult {
-	// Add actual lint here
 	e := util.GetExtFromCert(c, util.CrlDistOID)
-	if e.Critical == false {
+	if !e.Critical {
 		return &LintResult{Status: Pass}
 	} else {
 		return &LintResult{Status: Error}

diff --git a/lints/lint_sub_cert_is_ca.go b/lints/lint_sub_cert_is_ca.go
@@ -37,7 +37,7 @@ func (l *subCertNotCA) Execute(c *x509.Certificate) *LintResult {
 	if _, err := asn1.Unmarshal(e.Value, &constraints); err != nil {
 		return &LintResult{Status: Fatal}
 	}
-	if constraints.IsCA == true {
+	if constraints.IsCA {
 		return &LintResult{Status: Error}
 	} else {
 		return &LintResult{Status: Pass}

diff --git a/lints/lint_subject_empty_without_san.go b/lints/lint_subject_empty_without_san.go
@@ -51,10 +51,7 @@ func (l *emptyWithoutSAN) Execute(cert *x509.Certificate) *LintResult {
 }
 
 func subjectIsEmpty(cert *x509.Certificate) bool {
-	if cert.Subject.Names == nil {
-		return true
-	}
-	return false
+	return len(cert.Subject.Names) == 0
 }
 
 func init() {

diff --git a/makefile b/makefile
@@ -32,7 +32,7 @@ test:
 integration:
 	$(INT_TEST)
 
-format-check:
-	diff <(find . -name '*.go' -not -path './vendor/*' -print | xargs -n1 gofmt -l) <(printf "")
+code-lint:
+	golangci-lint run
 
-.PHONY: clean zlint zlint-gtld-update test integration format-check
+.PHONY: clean zlint zlint-gtld-update test integration code-lint
diff --git a/util/algorithm_identifier.go b/util/algorithm_identifier.go
@@ -43,7 +43,7 @@ func CheckAlgorithmIDParamNotNULL(algorithmIdentifier []byte, requiredAlgoID asn
 	// byte comparison of algorithm sequence and checking no trailing data is present
 	var algorithmBytes []byte
 	if algorithmSequence.ReadBytes(&algorithmBytes, len(expectedAlgoIDBytes)) {
-		if bytes.Compare(algorithmBytes, expectedAlgoIDBytes) == 0 && algorithmSequence.Empty() {
+		if bytes.Equal(algorithmBytes, expectedAlgoIDBytes) && algorithmSequence.Empty() {
 			return nil
 		}
 	}

diff --git a/util/fqdn.go b/util/fqdn.go
@@ -32,10 +32,7 @@ func RemovePrependedQuestionMarks(domain string) string {
 }
 
 func RemovePrependedWildcard(domain string) string {
-	if strings.HasPrefix(domain, "*.") {
-		domain = domain[2:]
-	}
-	return domain
+	return strings.TrimPrefix(domain, "*.")
 }
 
 func IsFQDN(domain string) bool {

diff --git a/util/gtld.go b/util/gtld.go
@@ -107,9 +107,7 @@ func IsInTLDMap(label string) bool {
 // return false.
 func CertificateSubjInTLD(c *x509.Certificate, label string) bool {
 	label = strings.ToLower(label)
-	if strings.HasPrefix(label, ".") {
-		label = label[1:]
-	}
+	label = strings.TrimPrefix(label, ".")
 	if !IsInTLDMap(label) {
 		return false
 	}