From b85f8de05f55852a9f1874e3cc817aaa92a5b06e Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Sat, 13 Jul 2019 18:28:51 -0700 Subject: [PATCH] development: use golangci-lint --- .golangci.yml | 210 ++++++++++++++++++++++ Makefile | 23 +-- authenticate/authenticate.go | 2 +- authenticate/authenticate_test.go | 3 - authenticate/grpc_test.go | 4 +- authenticate/handlers.go | 2 +- authenticate/handlers_test.go | 2 +- authorize/authorize_test.go | 3 - cmd/pomerium/main.go | 3 +- cmd/pomerium/main_test.go | 2 +- internal/config/options.go | 15 +- internal/config/options_test.go | 2 +- internal/cryptutil/encrypt.go | 2 +- internal/cryptutil/encrypt_test.go | 4 +- internal/httputil/client.go | 1 - internal/httputil/https.go | 15 +- internal/log/log_test.go | 6 +- internal/log/middleware_test.go | 17 +- internal/metrics/exporter.go | 4 +- internal/metrics/interceptors.go | 28 +-- internal/metrics/middleware.go | 2 +- internal/metrics/tags.go | 2 +- internal/middleware/chain.go | 2 +- internal/middleware/chain_test.go | 2 +- internal/middleware/middleware_test.go | 12 +- internal/middleware/reverse_proxy_test.go | 2 +- internal/sessions/cookie_store.go | 2 +- internal/sessions/cookie_store_test.go | 2 +- internal/sessions/rest_store_test.go | 2 +- internal/sessions/session_state_test.go | 4 +- internal/tripper/chain.go | 2 +- proxy/clients/mock_clients_test.go | 16 +- proxy/handlers.go | 2 +- proxy/handlers_test.go | 4 +- proxy/proxy.go | 3 +- 35 files changed, 292 insertions(+), 115 deletions(-) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..40dc39eb5 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,210 @@ +# forked from istio +service: + # When updating this, also update bin/linters.sh accordingly + golangci-lint-version: 1.17.x # use the fixed version to not introduce new linters unexpectedly +run: + # timeout for analysis, e.g. 30s, 5m, default is 1m + deadline: 5m + + # which dirs to skip: they won't be analyzed; + # can use regexp here: generated.*, regexp is applied on full path; + # default value is empty list, but next dirs are always skipped independently + # from this option's value: + # vendor$, third_party$, testdata$, examples$, Godeps$, builtin$ + skip-dirs: + - genfiles$ + - vendor$ + + # which files to skip: they will be analyzed, but issues from them + # won't be reported. Default value is empty list, but there is + # no need to include all autogenerated files, we confidently recognize + # autogenerated files. If it's not please let us know. + skip-files: + - ".*\\.pb\\.go" + - ".*\\.gen\\.go" + +linters: + enable-all: true + disable: + - depguard + - dupl + - gochecknoglobals + - gochecknoinits + - goconst + - gocyclo + - nakedret + - prealloc + - scopelint + - maligned + - interfacer + fast: false + +linters-settings: + errcheck: + # report about not checking of errors in type assetions: `a := b.(MyStruct)`; + # default is false: such cases aren't reported by default. + check-type-assertions: false + + # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`; + # default is false: such cases aren't reported by default. + check-blank: false + + govet: + # report about shadowed variables + check-shadowing: false + golint: + # minimal confidence for issues, default is 0.8 + min-confidence: 0.0 + gofmt: + # simplify code: gofmt with `-s` option, true by default + simplify: true + misspell: + # Correct spellings using locale preferences for US or UK. + # Default is to use a neutral variety of English. + # Setting locale to US will correct the British spelling of 'colour' to 'color'. + locale: US + lll: + # max line length, lines longer will be reported. Default is 120. + # '\t' is counted as 1 character by default, and can be changed with the tab-width option + line-length: 160 + # tab width in spaces. Default to 1. + tab-width: 1 + unused: + # treat code as a program (not a library) and report unused exported identifiers; default is false. + # XXX: if you enable this setting, unused will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find funcs usages. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + unparam: + # call graph construction algorithm (cha, rta). In general, use cha for libraries, + # and rta for programs with main packages. Default is cha. + algo: cha + + # Inspect exported functions, default is false. Set to true if no external program/library imports your code. + # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find external interfaces. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + gocritic: + enabled-checks: + - appendCombine + - argOrder + - assignOp + - badCond + - boolExprSimplify + - builtinShadow + - captLocal + - caseOrder + - codegenComment + - commentedOutCode + - commentedOutImport + - defaultCaseOrder + - deprecatedComment + - docStub + - dupArg + - dupBranchBody + - dupCase + - dupSubExpr + - elseif + - emptyFallthrough + - equalFold + - flagDeref + - flagName + - hexLiteral + - indexAlloc + - initClause + - methodExprCall + - nilValReturn + - octalLiteral + - offBy1 + - rangeExprCopy + - regexpMust + - sloppyLen + - stringXbytes + - switchTrue + - typeAssertChain + - typeSwitchVar + - typeUnparen + - underef + - unlambda + - unnecessaryBlock + - unslice + - valSwap + - weakCond + - yodaStyleExpr + + # Unused + # - appendAssign + # - commentFormatting + # - emptyStringTest + # - exitAfterDefer + # - ifElseChain + # - hugeParam + # - importShadow + # - nestingReduce + # - paramTypeCombine + # - ptrToRefParam + # - rangeValCopy + # - singleCaseSwitch + # - sloppyReassign + # - unlabelStmt + # - unnamedResult + # - wrapperFunc + +issues: + # List of regexps of issue texts to exclude, empty list by default. + # But independently from this option we use default exclude patterns, + # it can be disabled by `exclude-use-default: false`. To list all + # excluded by default patterns execute `golangci-lint run --help` + exclude: + # Mostly harmless buffer writes where we skip error checking + # https://golang.org/pkg/bytes/#Buffer.Write + - "Error return value of `w.Write` is not checked" + - "Error return value of `io.WriteString` is not checked" + - "Error return value of `viper.BindEnv` is not checked" + - "Error return value of `h.Write` is not checked" + - "ExecuteTemplate` is not checked" + + # go sec : we want to allow skipping tls auth + - "TLS InsecureSkipVerify set true." + - "goroutine calls T.Fatalf, which must be called in the same goroutine as the test" + # good job Protobuffs! + - "method XXX" + + exclude-rules: + # Exclude some linters from running on test files. + - path: _test\.go$|^tests/|^samples/|templates\.go$ + linters: + - errcheck + - maligned + - lll + - gosec + - bodyclose + - unparam + # erroneously thinks google api url is a cred + - path: internal/identity/google.go + text: "Potential hardcoded credentials" + linters: + - gosec + # deprecated but every example still uses New + - path: internal/identity/google.go + text: "please use NewService instead" + linters: + - staticcheck + + # todo(bdd): replace in go 1.13 + - path: proxy/proxy.go + text: "copylocks: assignment copies lock value to transport" + linters: + - govet + # Independently from option `exclude` we use default exclude patterns, + # it can be disabled by this option. To list all + # excluded by default patterns execute `golangci-lint run --help`. + # Default value for this option is true. + exclude-use-default: true + + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. + max-per-linter: 0 + + # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. + max-same-issues: 0 diff --git a/Makefile b/Makefile index aadb56bb4..78136f8d4 100644 --- a/Makefile +++ b/Makefile @@ -17,7 +17,7 @@ VERSION := $(shell cat VERSION) GITCOMMIT := $(shell git rev-parse --short HEAD) GITUNTRACKEDCHANGES := $(shell git status --porcelain --untracked-files=no) BUILDMETA:= -ifneq ($(GITUNTRACKEDCHANGES),) +ifneq ($(GITUNTRACKEDCHANGES),"") BUILDMETA := dirty endif CTIMEVAR=-X $(PKG)/internal/version.GitCommit=$(GITCOMMIT) \ @@ -30,7 +30,7 @@ GOOSARCHES = linux/amd64 darwin/amd64 windows/amd64 .PHONY: all -all: clean build fmt lint vet test ## Runs a clean, build, fmt, lint, test, and vet. +all: clean build lint test ## Runs a clean, build, fmt, lint, test, and vet. .PHONY: tag tag: ## Create a new git tag to prepare to build a release @@ -42,26 +42,11 @@ build: ## Builds dynamic executables and/or packages. @echo "==> $@" @CGO_ENABLED=0 GO111MODULE=on go build -tags "$(BUILDTAGS)" ${GO_LDFLAGS} -o $(BINDIR)/$(NAME) ./cmd/"$(NAME)" -.PHONY: fmt -fmt: ## Verifies all files have been `gofmt`ed. - @echo "==> $@" - @gofmt -s -l . | grep -v '.pb.go:' | grep -v vendor | tee /dev/stderr - .PHONY: lint lint: ## Verifies `golint` passes. @echo "==> $@" - @go get golang.org/x/lint/golint - @golint ./... | grep -v '.pb.go:' | grep -v vendor | tee /dev/stderr - -.PHONY: staticcheck -staticcheck: ## Verifies `staticcheck` passes - @echo "+ $@" - @staticcheck $(shell go list ./... | grep -v vendor) | grep -v '.pb.go:' | tee /dev/stderr - -.PHONY: vet -vet: ## Verifies `go vet` passes. - @echo "==> $@" - @go vet $(shell go list ./... | grep -v vendor) | grep -v '.pb.go:' | tee /dev/stderr + @GO111MODULE=off go get -u github.com/golangci/golangci-lint/cmd/golangci-lint + @golangci-lint run ./... .PHONY: test test: ## Runs the go tests. diff --git a/authenticate/authenticate.go b/authenticate/authenticate.go index 11c5b872a..bb8be6c83 100644 --- a/authenticate/authenticate.go +++ b/authenticate/authenticate.go @@ -59,7 +59,7 @@ func New(opts config.Options) (*Authenticate, error) { return nil, err } decodedCookieSecret, _ := base64.StdEncoding.DecodeString(opts.CookieSecret) - cipher, err := cryptutil.NewCipher([]byte(decodedCookieSecret)) + cipher, err := cryptutil.NewCipher(decodedCookieSecret) if err != nil { return nil, err } diff --git a/authenticate/authenticate_test.go b/authenticate/authenticate_test.go index 45219b8fd..a7166ad6b 100644 --- a/authenticate/authenticate_test.go +++ b/authenticate/authenticate_test.go @@ -85,9 +85,6 @@ func TestNew(t *testing.T) { t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr) return } - // if !reflect.DeepEqual(got, tt.want) { - // t.Errorf("New() = %v, want %v", got, tt.want) - // } }) } } diff --git a/authenticate/grpc_test.go b/authenticate/grpc_test.go index f4596b4f1..bf90730fb 100644 --- a/authenticate/grpc_test.go +++ b/authenticate/grpc_test.go @@ -93,12 +93,12 @@ func TestAuthenticate_Refresh(t *testing.T) { func TestAuthenticate_Authenticate(t *testing.T) { secret := cryptutil.GenerateKey() - c, err := cryptutil.NewCipher([]byte(secret)) + c, err := cryptutil.NewCipher(secret) if err != nil { t.Fatalf("expected to be able to create cipher: %v", err) } newSecret := cryptutil.GenerateKey() - c2, err := cryptutil.NewCipher([]byte(newSecret)) + c2, err := cryptutil.NewCipher(newSecret) if err != nil { t.Fatalf("expected to be able to create cipher: %v", err) } diff --git a/authenticate/handlers.go b/authenticate/handlers.go index 350bb14c4..c357f5bf8 100644 --- a/authenticate/handlers.go +++ b/authenticate/handlers.go @@ -119,7 +119,7 @@ func (a *Authenticate) SignIn(w http.ResponseWriter, r *http.Request) { httputil.ErrorResponse(w, r, httpErr) return } - http.Redirect(w, r, getAuthCodeRedirectURL(redirectURL, state, string(encrypted)), http.StatusFound) + http.Redirect(w, r, getAuthCodeRedirectURL(redirectURL, state, encrypted), http.StatusFound) } func getAuthCodeRedirectURL(redirectURL *url.URL, state, authCode string) string { diff --git a/authenticate/handlers_test.go b/authenticate/handlers_test.go index 79514a84b..5ceca1500 100644 --- a/authenticate/handlers_test.go +++ b/authenticate/handlers_test.go @@ -224,7 +224,7 @@ func (a mockCipher) Decrypt(s []byte) ([]byte, error) { } func (a mockCipher) Marshal(s interface{}) (string, error) { return "ok", nil } func (a mockCipher) Unmarshal(s string, i interface{}) error { - if string(s) == "unmarshal error" || string(s) == "error" { + if s == "unmarshal error" || s == "error" { return errors.New("error") } return nil diff --git a/authorize/authorize_test.go b/authorize/authorize_test.go index 8e2617d98..37dfb4f1e 100644 --- a/authorize/authorize_test.go +++ b/authorize/authorize_test.go @@ -34,9 +34,6 @@ func TestNew(t *testing.T) { t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr) return } - // if !reflect.DeepEqual(got, tt.want) { - // t.Errorf("New() = %v, want %v", got, tt.want) - // } }) } } diff --git a/cmd/pomerium/main.go b/cmd/pomerium/main.go index 2d00cd168..2f21233da 100644 --- a/cmd/pomerium/main.go +++ b/cmd/pomerium/main.go @@ -167,7 +167,7 @@ func newPromListener(addr string) { log.Error().Err(metrics.NewPromHTTPListener(addr)).Str("MetricsAddr", addr).Msg("cmd/pomerium: could not start metrics exporter") } -func wrapMiddleware(o *config.Options, mux *http.ServeMux) http.Handler { +func wrapMiddleware(o *config.Options, mux http.Handler) http.Handler { c := middleware.NewChain() c = c.Append(metrics.HTTPMetricsHandler("proxy")) c = c.Append(log.NewHandler(log.Logger)) @@ -202,7 +202,6 @@ func parseOptions(configFile string) (*config.Options, error) { } if o.Debug { log.SetDebugMode() - // log.Debug().Interface("options", o).Msg("cmd/pomerium") } if o.LogLevel != "" { log.SetLevel(o.LogLevel) diff --git a/cmd/pomerium/main_test.go b/cmd/pomerium/main_test.go index 70a8700fe..2ec946c85 100644 --- a/cmd/pomerium/main_test.go +++ b/cmd/pomerium/main_test.go @@ -268,7 +268,7 @@ func (m *mockService) UpdateOptions(o config.Options) error { m.Updated = true if m.fail { - return fmt.Errorf("Failed") + return fmt.Errorf("failed") } return nil } diff --git a/internal/config/options.go b/internal/config/options.go index ddbe82d2a..2ac8239ce 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -137,10 +137,10 @@ var defaultOptions = Options{ Services: "all", CookieHTTPOnly: true, CookieSecure: true, - CookieExpire: time.Duration(14) * time.Hour, - CookieRefresh: time.Duration(30) * time.Minute, + CookieExpire: 14 * time.Hour, + CookieRefresh: 30 * time.Minute, CookieName: "_pomerium", - DefaultUpstreamTimeout: time.Duration(30) * time.Second, + DefaultUpstreamTimeout: 30 * time.Second, Headers: map[string]string{ "X-Content-Type-Options": "nosniff", "X-Frame-Options": "SAMEORIGIN", @@ -154,7 +154,7 @@ var defaultOptions = Options{ ReadTimeout: 30 * time.Second, WriteTimeout: 0, // support streaming by default IdleTimeout: 5 * time.Minute, - RefreshCooldown: time.Duration(5 * time.Minute), + RefreshCooldown: 5 * time.Minute, } // NewOptions returns a minimal options configuration built from default options. @@ -263,11 +263,8 @@ func (o *Options) parsePolicy() error { if err := yaml.Unmarshal(policyBytes, &policies); err != nil { return fmt.Errorf("could not unmarshal policy yaml: %s", err) } - } else { - // Parse from file - if err := viper.UnmarshalKey("policy", &policies); err != nil { - return err - } + } else if err := viper.UnmarshalKey("policy", &policies); err != nil { + return err } if len(policies) != 0 { o.Policies = policies diff --git a/internal/config/options_test.go b/internal/config/options_test.go index 8fd7279a1..fa982daed 100644 --- a/internal/config/options_test.go +++ b/internal/config/options_test.go @@ -304,7 +304,7 @@ func Test_Checksum(t *testing.T) { t.Error("Checksum() not returning data") } - if o.Checksum() != o.Checksum() { + if o.Checksum() != newChecksum { t.Error("Checksum() inconsistent") } } diff --git a/internal/cryptutil/encrypt.go b/internal/cryptutil/encrypt.go index 29826dd4a..ac44774ff 100644 --- a/internal/cryptutil/encrypt.go +++ b/internal/cryptutil/encrypt.go @@ -89,7 +89,7 @@ func (c *XChaCha20Cipher) Encrypt(plaintext []byte) (joined []byte, err error) { ciphertext := c.aead.Seal(nil, nonce, plaintext, nil) // we return the nonce as part of the returned value - joined = append(ciphertext[:], nonce[:]...) + joined = append(ciphertext, nonce...) return joined, nil } diff --git a/internal/cryptutil/encrypt_test.go b/internal/cryptutil/encrypt_test.go index 14e02a60e..a5f83658f 100644 --- a/internal/cryptutil/encrypt_test.go +++ b/internal/cryptutil/encrypt_test.go @@ -14,7 +14,7 @@ func TestEncodeAndDecodeAccessToken(t *testing.T) { plaintext := []byte("my plain text value") key := GenerateKey() - c, err := NewCipher([]byte(key)) + c, err := NewCipher(key) if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -43,7 +43,7 @@ func TestEncodeAndDecodeAccessToken(t *testing.T) { func TestMarshalAndUnmarshalStruct(t *testing.T) { key := GenerateKey() - c, err := NewCipher([]byte(key)) + c, err := NewCipher(key) if err != nil { t.Fatalf("unexpected err: %v", err) } diff --git a/internal/httputil/client.go b/internal/httputil/client.go index f9967d004..c966eeddb 100644 --- a/internal/httputil/client.go +++ b/internal/httputil/client.go @@ -62,7 +62,6 @@ func Client(method, endpoint, userAgent string, headers map[string]string, param if err != nil { return err } - // log.Info().Msgf("%s", respBody) if resp.StatusCode != http.StatusOK { switch resp.StatusCode { case http.StatusBadRequest: diff --git a/internal/httputil/https.go b/internal/httputil/https.go index 4a9180c57..4928b146e 100644 --- a/internal/httputil/https.go +++ b/internal/httputil/https.go @@ -14,8 +14,6 @@ import ( "github.com/pomerium/pomerium/internal/fileutil" "github.com/pomerium/pomerium/internal/log" - - "google.golang.org/grpc" ) // Options contains the configurations settings for a TLS http server. @@ -81,7 +79,7 @@ func (o *Options) applyDefaults() { // ListenAndServeTLS serves the provided handlers by HTTPS // using the provided options. -func ListenAndServeTLS(opt *Options, httpHandler http.Handler, grpcHandler *grpc.Server) error { +func ListenAndServeTLS(opt *Options, httpHandler http.Handler, grpcHandler http.Handler) error { if opt == nil { opt = defaultOptions } else { @@ -97,10 +95,7 @@ func ListenAndServeTLS(opt *Options, httpHandler http.Handler, grpcHandler *grpc if err != nil { return fmt.Errorf("https: failed loading x509 certificate: %v", err) } - config, err := newDefaultTLSConfig(cert) - if err != nil { - return fmt.Errorf("https: setting up TLS config: %v", err) - } + config := newDefaultTLSConfig(cert) ln, err := net.Listen("tcp", opt.Addr) if err != nil { return err @@ -168,7 +163,7 @@ func readCertificateFile(certFile, certKeyFile string) (*tls.Certificate, error) // https://blog.cloudflare.com/exposing-go-on-the-internet/ // https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices // https://github.com/golang/go/blob/df91b8044dbe790c69c16058330f545be069cc1f/src/crypto/tls/common.go#L919 -func newDefaultTLSConfig(cert *tls.Certificate) (*tls.Config, error) { +func newDefaultTLSConfig(cert *tls.Certificate) *tls.Config { tlsConfig := &tls.Config{ MinVersion: tls.VersionTLS12, // Prioritize cipher suites sped up by AES-NI (AES-GCM) @@ -191,12 +186,12 @@ func newDefaultTLSConfig(cert *tls.Certificate) (*tls.Config, error) { NextProtos: []string{"h2"}, } tlsConfig.BuildNameToCertificate() - return tlsConfig, nil + return tlsConfig } // grpcHandlerFunc splits request serving between gRPC and HTTPS depending on the request type. // Requires HTTP/2. -func grpcHandlerFunc(rpcServer *grpc.Server, other http.Handler) http.Handler { +func grpcHandlerFunc(rpcServer http.Handler, other http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ct := r.Header.Get("Content-Type") if r.ProtoMajor == 2 && strings.Contains(ct, "application/grpc") { diff --git a/internal/log/log_test.go b/internal/log/log_test.go index 1757dd4e5..fb1f0de38 100644 --- a/internal/log/log_test.go +++ b/internal/log/log_test.go @@ -22,7 +22,7 @@ func setup() { // examples to pass, we need to override zerolog.TimestampFunc // and log.Logger globals -- you would not normally need to do this zerolog.TimestampFunc = func() time.Time { - return time.Date(2008, 1, 8, 17, 5, 05, 0, time.UTC) + return time.Date(2008, 1, 8, 17, 5, 5, 0, time.UTC) } log.Logger = zerolog.New(os.Stdout).With().Timestamp().Logger() } @@ -95,7 +95,7 @@ func ExampleError() { // Example of a log at a particular "level" (in this case, "fatal") func ExampleFatal() { setup() - err := errors.New("A repo man spends his life getting into tense situations") + err := errors.New("a repo man spends his life getting into tense situations") service := "myservice" log.Fatal(). @@ -103,7 +103,7 @@ func ExampleFatal() { Str("service", service). Msg("Cannot start") - // Outputs: {"level":"fatal","time":1199811905,"error":"A repo man spends his life getting into tense situations","service":"myservice","message":"Cannot start myservice"} + // Outputs: {"level":"fatal","time":1199811905,"error":"a repo man spends his life getting into tense situations","service":"myservice","message":"Cannot start myservice"} } // This example uses command-line flags to demonstrate various outputs diff --git a/internal/log/middleware_test.go b/internal/log/middleware_test.go index 6622474cd..a3e391bb1 100644 --- a/internal/log/middleware_test.go +++ b/internal/log/middleware_test.go @@ -24,19 +24,15 @@ func TestGenerateUUID(t *testing.T) { if prev == id { t.Fatalf("Should get a new ID!") } - matched, err := regexp.MatchString("[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}", id) - if !matched || err != nil { - t.Fatalf("expected match %s %v %s", id, matched, err) + matched := regexp.MustCompile("[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}").MatchString(id) + if !matched { + t.Fatalf("expected match %s %v", id, matched) } } } -func decodeIfBinary(out *bytes.Buffer) string { - // p := out.Bytes() - // if len(p) == 0 || p[0] < 0x7F { - // return out.String() - // } - return out.String() //cbor.DecodeObjectToStr(p) + "\n" +func decodeIfBinary(out fmt.Stringer) string { + return out.String() } func TestNewHandler(t *testing.T) { @@ -182,9 +178,6 @@ func TestRequestIDHandler(t *testing.T) { if !ok { t.Fatal("Missing id in request") } - // if want, got := id.String(), w.Header().Get("Request-Id"); got != want { - // t.Errorf("Invalid Request-Id header, got: %s, want: %s", got, want) - // } l := FromRequest(r) l.Log().Msg("") if want, got := fmt.Sprintf(`{"id":"%s"}`+"\n", id), decodeIfBinary(out); want != got { diff --git a/internal/metrics/exporter.go b/internal/metrics/exporter.go index d32f799c4..832cd8363 100644 --- a/internal/metrics/exporter.go +++ b/internal/metrics/exporter.go @@ -9,8 +9,8 @@ import ( ) //NewPromHTTPListener creates a prometheus exporter on ListenAddr -func NewPromHTTPListener(ListenAddr string) error { - return http.ListenAndServe(ListenAddr, newPromHTTPHandler()) +func NewPromHTTPListener(addr string) error { + return http.ListenAndServe(addr, newPromHTTPHandler()) } // newPromHTTPHandler creates a new prometheus exporter handler for /metrics diff --git a/internal/metrics/interceptors.go b/internal/metrics/interceptors.go index 0cb62557c..b736a7559 100644 --- a/internal/metrics/interceptors.go +++ b/internal/metrics/interceptors.go @@ -22,7 +22,8 @@ var ( 100, 250, 500, 750, 1000, ) - // GRPCServerRequestCountView is an OpenCensus view which counts GRPC Server requests by pomerium service, grpc service, grpc method, and status + // GRPCServerRequestCountView is an OpenCensus view which counts GRPC Server + // requests by pomerium service, grpc service, grpc method, and status GRPCServerRequestCountView = &view.View{ Name: "grpc_server_requests_total", Measure: ocgrpc.ServerLatency, @@ -31,7 +32,8 @@ var ( Aggregation: view.Count(), } - // GRPCServerRequestDurationView is an OpenCensus view which tracks GRPC Server request duration by pomerium service, grpc service, grpc method, and status + // GRPCServerRequestDurationView is an OpenCensus view which tracks GRPC Server + // request duration by pomerium service, grpc service, grpc method, and status GRPCServerRequestDurationView = &view.View{ Name: "grpc_server_request_duration_ms", Measure: ocgrpc.ServerLatency, @@ -40,7 +42,8 @@ var ( Aggregation: grcpLatencyDistribution, } - // GRPCServerResponseSizeView is an OpenCensus view which tracks GRPC Server response size by pomerium service, grpc service, grpc method, and status + // GRPCServerResponseSizeView is an OpenCensus view which tracks GRPC Server + // response size by pomerium service, grpc service, grpc method, and status GRPCServerResponseSizeView = &view.View{ Name: "grpc_server_response_size_bytes", Measure: ocgrpc.ServerSentBytesPerRPC, @@ -49,7 +52,8 @@ var ( Aggregation: grpcSizeDistribution, } - // GRPCServerRequestSizeView is an OpenCensus view which tracks GRPC Server request size by pomerium service, grpc service, grpc method, and status + // GRPCServerRequestSizeView is an OpenCensus view which tracks GRPC Server + // request size by pomerium service, grpc service, grpc method, and status GRPCServerRequestSizeView = &view.View{ Name: "grpc_server_request_size_bytes", Measure: ocgrpc.ServerReceivedBytesPerRPC, @@ -58,7 +62,8 @@ var ( Aggregation: grpcSizeDistribution, } - // GRPCClientRequestCountView is an OpenCensus view which tracks GRPC Client requests by pomerium service, target host, grpc service, grpc method, and status + // GRPCClientRequestCountView is an OpenCensus view which tracks GRPC Client + // requests by pomerium service, target host, grpc service, grpc method, and status GRPCClientRequestCountView = &view.View{ Name: "grpc_client_requests_total", Measure: ocgrpc.ClientRoundtripLatency, @@ -67,7 +72,8 @@ var ( Aggregation: view.Count(), } - // GRPCClientRequestDurationView is an OpenCensus view which tracks GRPC Client request duration by pomerium service, target host, grpc service, grpc method, and status + // GRPCClientRequestDurationView is an OpenCensus view which tracks GRPC Client + // request duration by pomerium service, target host, grpc service, grpc method, and status GRPCClientRequestDurationView = &view.View{ Name: "grpc_client_request_duration_ms", Measure: ocgrpc.ClientRoundtripLatency, @@ -76,7 +82,8 @@ var ( Aggregation: grcpLatencyDistribution, } - // GRPCClientResponseSizeView is an OpenCensus view which tracks GRPC Client response size by pomerium service, target host, grpc service, grpc method, and status + // GRPCClientResponseSizeView is an OpenCensus view which tracks GRPC Client + // response size by pomerium service, target host, grpc service, grpc method, and status GRPCClientResponseSizeView = &view.View{ Name: "grpc_client_response_size_bytes", Measure: ocgrpc.ClientReceivedBytesPerRPC, @@ -85,7 +92,8 @@ var ( Aggregation: grpcSizeDistribution, } - // GRPCClientRequestSizeView is an OpenCensus view which tracks GRPC Client request size by pomerium service, target host, grpc service, grpc method, and status + // GRPCClientRequestSizeView is an OpenCensus view which tracks GRPC Client + // request size by pomerium service, target host, grpc service, grpc method, and status GRPCClientRequestSizeView = &view.View{ Name: "grpc_client_request_size_bytes", Measure: ocgrpc.ClientSentBytesPerRPC, @@ -95,8 +103,8 @@ var ( } ) -// GRPCClientInterceptor creates a UnaryClientInterceptor which updates the RPC context with metric tag -// metadata +// GRPCClientInterceptor creates a UnaryClientInterceptor which updates the RPC +// context with metric tag metadata func GRPCClientInterceptor(service string) grpc.UnaryClientInterceptor { return func( ctx context.Context, diff --git a/internal/metrics/middleware.go b/internal/metrics/middleware.go index 655e6e70a..283cd25a6 100644 --- a/internal/metrics/middleware.go +++ b/internal/metrics/middleware.go @@ -69,7 +69,7 @@ var ( // HTTPClientRequestCountView is an OpenCensus View that tracks HTTP client requests by pomerium service, destination, host, method and status HTTPClientRequestCountView = &view.View{ Name: "http_client_requests_total", - Measure: ochttp.ClientLatency, + Measure: ochttp.ClientRoundtripLatency, Description: "Total HTTP Client Requests", TagKeys: []tag.Key{keyService, keyHost, keyHTTPMethod, ochttp.StatusCode, keyDestination}, Aggregation: view.Count(), diff --git a/internal/metrics/tags.go b/internal/metrics/tags.go index bd4ffae55..e1c52fc9e 100644 --- a/internal/metrics/tags.go +++ b/internal/metrics/tags.go @@ -5,8 +5,8 @@ import ( ) var ( + // keyStatus tag.Key = tag.MustNewKey("status") keyHTTPMethod tag.Key = tag.MustNewKey("http_method") - keyStatus tag.Key = tag.MustNewKey("status") keyService tag.Key = tag.MustNewKey("service") keyGRPCService tag.Key = tag.MustNewKey("grpc_service") keyGRPCMethod tag.Key = tag.MustNewKey("grpc_method") diff --git a/internal/middleware/chain.go b/internal/middleware/chain.go index d89aae0d0..58fa2af0c 100644 --- a/internal/middleware/chain.go +++ b/internal/middleware/chain.go @@ -18,7 +18,7 @@ type Chain struct { // New serves no other function, // constructors are only called upon a call to Then(). func NewChain(constructors ...Constructor) Chain { - return Chain{append(([]Constructor)(nil), constructors...)} + return Chain{append([]Constructor(nil), constructors...)} } // Then chains the middleware and returns the final http.Handler. diff --git a/internal/middleware/chain_test.go b/internal/middleware/chain_test.go index 876030cbc..422272923 100644 --- a/internal/middleware/chain_test.go +++ b/internal/middleware/chain_test.go @@ -77,7 +77,7 @@ func TestThenFuncConstructsHandlerFunc(t *testing.T) { chained.ServeHTTP(rec, (*http.Request)(nil)) - if reflect.TypeOf(chained) != reflect.TypeOf((http.HandlerFunc)(nil)) { + if reflect.TypeOf(chained) != reflect.TypeOf(http.HandlerFunc(nil)) { t.Error("ThenFunc does not construct HandlerFunc") } } diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index e817b53c8..eb7a1ee91 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -56,12 +56,12 @@ func Test_ValidSignature(t *testing.T) { secret string want bool }{ - {"good signature", goodURL, string(sig), now, secretA, true}, - {"empty redirect url", "", string(sig), now, secretA, false}, - {"bad redirect url", "https://google.com^", string(sig), now, secretA, false}, - {"malformed signature", goodURL, string(sig + "^"), now, "&*&@**($&#(", false}, - {"malformed timestamp", goodURL, string(sig), now + "^", secretA, false}, - {"stale timestamp", goodURL, string(sig), staleTime, secretA, false}, + {"good signature", goodURL, sig, now, secretA, true}, + {"empty redirect url", "", sig, now, secretA, false}, + {"bad redirect url", "https://google.com^", sig, now, secretA, false}, + {"malformed signature", goodURL, sig + "^", now, "&*&@**($&#(", false}, + {"malformed timestamp", goodURL, sig, now + "^", secretA, false}, + {"stale timestamp", goodURL, sig, staleTime, secretA, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/middleware/reverse_proxy_test.go b/internal/middleware/reverse_proxy_test.go index f41b57916..b71942017 100644 --- a/internal/middleware/reverse_proxy_test.go +++ b/internal/middleware/reverse_proxy_test.go @@ -84,7 +84,7 @@ func TestStripPomeriumCookie(t *testing.T) { Name: tt.pomeriumCookie, Value: "pomerium cookie!", }) - req := &http.Request{Header: http.Header{"Cookie": rr.HeaderMap["Set-Cookie"]}} + req := &http.Request{Header: http.Header{"Cookie": rr.Header()["Set-Cookie"]}} handler := StripPomeriumCookie(tt.pomeriumCookie)(testHandler) handler.ServeHTTP(rr, req) diff --git a/internal/sessions/cookie_store.go b/internal/sessions/cookie_store.go index 6922bfa82..ce6e973bf 100644 --- a/internal/sessions/cookie_store.go +++ b/internal/sessions/cookie_store.go @@ -140,7 +140,7 @@ func (s *CookieStore) setCookie(w http.ResponseWriter, cookie *http.Cookie) { } else { // subsequent parts will be postfixed with their part number nc.Name = fmt.Sprintf("%s_%d", cookie.Name, i) - nc.Value = fmt.Sprintf("%s", c) + nc.Value = c } http.SetCookie(w, &nc) } diff --git a/internal/sessions/cookie_store_test.go b/internal/sessions/cookie_store_test.go index 44b484400..53dc98394 100644 --- a/internal/sessions/cookie_store_test.go +++ b/internal/sessions/cookie_store_test.go @@ -31,7 +31,7 @@ func (a mockCipher) Decrypt(s []byte) ([]byte, error) { } func (a mockCipher) Marshal(s interface{}) (string, error) { return "", errors.New("error") } func (a mockCipher) Unmarshal(s string, i interface{}) error { - if string(s) == "unmarshal error" || string(s) == "error" { + if s == "unmarshal error" || s == "error" { return errors.New("error") } return nil diff --git a/internal/sessions/rest_store_test.go b/internal/sessions/rest_store_test.go index cfddedd32..8f5943f6f 100644 --- a/internal/sessions/rest_store_test.go +++ b/internal/sessions/rest_store_test.go @@ -14,7 +14,7 @@ import ( ) func TestRestStore_SaveSession(t *testing.T) { - now := time.Date(2008, 1, 8, 17, 5, 05, 0, time.UTC) + now := time.Date(2008, 1, 8, 17, 5, 5, 0, time.UTC) tests := []struct { name string diff --git a/internal/sessions/session_state_test.go b/internal/sessions/session_state_test.go index 19277c7c0..6d4f55e72 100644 --- a/internal/sessions/session_state_test.go +++ b/internal/sessions/session_state_test.go @@ -13,7 +13,7 @@ import ( func TestSessionStateSerialization(t *testing.T) { secret := cryptutil.GenerateKey() - c, err := cryptutil.NewCipher([]byte(secret)) + c, err := cryptutil.NewCipher(secret) if err != nil { t.Fatalf("expected to be able to create cipher: %v", err) } @@ -144,7 +144,7 @@ func TestSessionState_Impersonating(t *testing.T) { func TestMarshalSession(t *testing.T) { secret := cryptutil.GenerateKey() - c, err := cryptutil.NewCipher([]byte(secret)) + c, err := cryptutil.NewCipher(secret) if err != nil { t.Fatalf("expected to be able to create cipher: %v", err) } diff --git a/internal/tripper/chain.go b/internal/tripper/chain.go index af68a0999..67eaf8741 100644 --- a/internal/tripper/chain.go +++ b/internal/tripper/chain.go @@ -18,7 +18,7 @@ type Chain struct { // New serves no other function, // constructors are only called upon a call to Then(). func NewChain(constructors ...Constructor) Chain { - return Chain{append(([]Constructor)(nil), constructors...)} + return Chain{append([]Constructor(nil), constructors...)} } // Then chains the trippers and returns the final http.RoundTripper. diff --git a/proxy/clients/mock_clients_test.go b/proxy/clients/mock_clients_test.go index 694890d0d..8cd5330de 100644 --- a/proxy/clients/mock_clients_test.go +++ b/proxy/clients/mock_clients_test.go @@ -16,26 +16,26 @@ func TestMockAuthenticate(t *testing.T) { RefreshToken: "RefreshToken", } ma := &MockAuthenticate{ - RedeemError: errors.New("RedeemError"), + RedeemError: errors.New("redeem error"), RedeemResponse: redeemResponse, RefreshResponse: &sessions.SessionState{ AccessToken: "AccessToken", RefreshToken: "RefreshToken", }, - RefreshError: errors.New("RefreshError"), + RefreshError: errors.New("refresh error"), ValidateResponse: true, - ValidateError: errors.New("ValidateError"), - CloseError: errors.New("CloseError"), + ValidateError: errors.New("validate error"), + CloseError: errors.New("close error"), } got, gotErr := ma.Redeem(context.Background(), "a") - if gotErr.Error() != "RedeemError" { + if gotErr.Error() != "redeem error" { t.Errorf("unexpected value for gotErr %s", gotErr) } if !reflect.DeepEqual(redeemResponse, got) { t.Errorf("unexpected value for redeemResponse %s", got) } newSession, gotErr := ma.Refresh(context.Background(), nil) - if gotErr.Error() != "RefreshError" { + if gotErr.Error() != "refresh error" { t.Errorf("unexpected value for gotErr %s", gotErr) } if !reflect.DeepEqual(newSession, redeemResponse) { @@ -46,11 +46,11 @@ func TestMockAuthenticate(t *testing.T) { if !ok { t.Errorf("unexpected value for ok : %t", ok) } - if gotErr.Error() != "ValidateError" { + if gotErr.Error() != "validate error" { t.Errorf("unexpected value for gotErr %s", gotErr) } gotErr = ma.Close() - if gotErr.Error() != "CloseError" { + if gotErr.Error() != "close error" { t.Errorf("unexpected value for ma.CloseError %s", gotErr) } diff --git a/proxy/handlers.go b/proxy/handlers.go index 33413c8af..a5e914eca 100644 --- a/proxy/handlers.go +++ b/proxy/handlers.go @@ -339,7 +339,7 @@ func (p *Proxy) UserDashboard(w http.ResponseWriter, r *http.Request) { SignoutURL: p.GetSignOutURL(p.AuthenticateURL, redirectURL).String(), IsAdmin: isAdmin, ImpersonateEmail: session.ImpersonateEmail, - ImpersonateGroup: strings.Join(session.ImpersonateGroups[:], ","), + ImpersonateGroup: strings.Join(session.ImpersonateGroups, ","), CSRF: csrf.SessionID, } templates.New().ExecuteTemplate(w, "dashboard.html", t) diff --git a/proxy/handlers_test.go b/proxy/handlers_test.go index 96f5ada6b..aa7ecbe74 100644 --- a/proxy/handlers_test.go +++ b/proxy/handlers_test.go @@ -40,7 +40,7 @@ func (a mockCipher) Marshal(s interface{}) (string, error) { return "ok", nil } func (a mockCipher) Unmarshal(s string, i interface{}) error { - if string(s) == "unmarshal error" || string(s) == "error" { + if s == "unmarshal error" || s == "error" { return errors.New("error") } return nil @@ -432,7 +432,6 @@ func TestProxy_Refresh(t *testing.T) { p.Refresh(w, r) if status := w.Code; status != tt.wantStatus { t.Errorf("status code: got %v want %v", status, tt.wantStatus) - // t.Errorf("\n%+v", w.Body.String()) t.Errorf("\n%+v", opts) } }) @@ -494,7 +493,6 @@ func TestProxy_Impersonate(t *testing.T) { p.Impersonate(w, r) if status := w.Code; status != tt.wantStatus { t.Errorf("status code: got %v want %v", status, tt.wantStatus) - // t.Errorf("\n%+v", w.Body.String()) t.Errorf("\n%+v", opts) } }) diff --git a/proxy/proxy.go b/proxy/proxy.go index cadd85c22..1abb7c702 100755 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -276,8 +276,7 @@ func (p *Proxy) customCAPool(cert string) (*x509.CertPool, error) { } // newReverseProxyHandler applies handler specific options to a given route. -func (p *Proxy) newReverseProxyHandler(rp *httputil.ReverseProxy, route *config.Policy) (http.Handler, error) { - var handler http.Handler +func (p *Proxy) newReverseProxyHandler(rp *httputil.ReverseProxy, route *config.Policy) (handler http.Handler, err error) { handler = &UpstreamProxy{ name: route.Destination.Host, handler: rp,