From 42d61191c4816977b39a7f2527b9281209df641e Mon Sep 17 00:00:00 2001 From: Tomasz Kleczek Date: Tue, 24 Sep 2019 11:20:35 +0200 Subject: [PATCH 01/29] storage: conformance tests improvements --- storage/conformance/conformance.go | 8 +++++--- storage/conformance/transactions.go | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/storage/conformance/conformance.go b/storage/conformance/conformance.go index a1399807..0be3661c 100644 --- a/storage/conformance/conformance.go +++ b/storage/conformance/conformance.go @@ -161,6 +161,8 @@ func testAuthRequestCRUD(t *testing.T, s storage.Storage) { t.Fatalf("failed to delete auth request: %v", err) } + _, err = s.GetAuthRequest(a1.ID) + mustBeErrNotFound(t, "auth request", err) } func testAuthCodeCRUD(t *testing.T, s storage.Storage) { @@ -214,7 +216,7 @@ func testAuthCodeCRUD(t *testing.T, s storage.Storage) { got, err := s.GetAuthCode(a1.ID) if err != nil { - t.Fatalf("failed to get auth req: %v", err) + t.Fatalf("failed to get auth code: %v", err) } if a1.Expiry.Unix() != got.Expiry.Unix() { t.Errorf("auth code expiry did not match want=%s vs got=%s", a1.Expiry, got.Expiry) @@ -812,7 +814,7 @@ func testGC(t *testing.T, s storage.Storage) { } } if _, err := s.GetAuthRequest(a.ID); err != nil { - t.Errorf("expected to be able to get auth code after GC: %v", err) + t.Errorf("expected to be able to get auth request after GC: %v", err) } } @@ -823,7 +825,7 @@ func testGC(t *testing.T, s storage.Storage) { } if _, err := s.GetAuthRequest(a.ID); err == nil { - t.Errorf("expected auth code to be GC'd") + t.Errorf("expected auth request to be GC'd") } else if err != storage.ErrNotFound { t.Errorf("expected storage.ErrNotFound, got %v", err) } diff --git a/storage/conformance/transactions.go b/storage/conformance/transactions.go index dc1be1b6..2fc6755b 100644 --- a/storage/conformance/transactions.go +++ b/storage/conformance/transactions.go @@ -135,8 +135,8 @@ func testKeysConcurrentUpdate(t *testing.T, s storage.Storage) { for i := 0; i < 2; i++ { n := time.Now().UTC().Round(time.Second) keys1 := storage.Keys{ - SigningKey: jsonWebKeys[0].Private, - SigningKeyPub: jsonWebKeys[0].Public, + SigningKey: jsonWebKeys[i].Private, + SigningKeyPub: jsonWebKeys[i].Public, NextRotation: n, } From e11b2ceeee9063198ac2a02df7a6222c301d9b55 Mon Sep 17 00:00:00 2001 From: YanJin Date: Mon, 25 Nov 2019 12:15:07 +0100 Subject: [PATCH 02/29] add issuer in the templates.md --- Documentation/templates.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/templates.md b/Documentation/templates.md index 7906363b..043938d9 100644 --- a/Documentation/templates.md +++ b/Documentation/templates.md @@ -14,11 +14,13 @@ Steps: ```yaml frontend: dir: /path/to/custom/web + issuer: my-dex extra: tos_footer_link: "https://example.com/terms" client_logo_url: "../theme/client-logo.png" foo: "bar" ``` 5. Set the `frontend.dir` value to your own `web` directory. +6. Write the issuer in the `issuer` directory in order to modify the Dex title and the `Log in to <>` tag. To test your templates simply run Dex with a valid configuration and go through a login flow. \ No newline at end of file From 9346e328ef8616a85d10031419ecf8488dc8c265 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 14:55:52 +0100 Subject: [PATCH 03/29] Add golangci linter --- .github/workflows/ci.yml | 3 +++ .golangci.yml | 45 ++++++++++++++++++++++++++++++++++++++++ Makefile | 23 +++++++++++++++++--- 3 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3027be5f..46c9c928 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,6 +71,9 @@ jobs: - name: Run Kubernetes tests run: ./scripts/test-k8s.sh + - name: Run linter + run: make lint + # Ensure proto generation doesn't depend on external packages. - name: Verify proto run: make verify-proto diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..824484d6 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,45 @@ +run: + skip-dirs: + - vendor + +linters-settings: + golint: + min-confidence: 0.1 + goimports: + local-prefixes: github.com/dexidp/dex + +linters: + enable-all: true + disable: + - funlen + - maligned + - wsl + + # TODO: fix me + - unused + - structcheck + - stylecheck + - deadcode + - misspell + - unparam + - goimports + - golint + - whitespace + - goconst + - unconvert + - bodyclose + - staticcheck + - nakedret + - ineffassign + - errcheck + - gosec + - gochecknoinits + - gochecknoglobals + - prealloc + - scopelint + - lll + - dupl + - gocritic + - gocyclo + - gocognit + - godox diff --git a/Makefile b/Makefile index 820e7ea3..c1c6d788 100644 --- a/Makefile +++ b/Makefile @@ -18,6 +18,9 @@ export GOBIN=$(PWD)/bin LD_FLAGS="-w -X $(REPO_PATH)/version.Version=$(VERSION)" +# Dependency versions +GOLANGCI_VERSION = 1.21.0 + build: bin/dex bin/example-app bin/grpc-client bin/dex: @@ -45,13 +48,27 @@ test: testrace: @go test -v --race ./... +bin/golangci-lint: bin/golangci-lint-${GOLANGCI_VERSION} + @ln -sf golangci-lint-${GOLANGCI_VERSION} bin/golangci-lint +bin/golangci-lint-${GOLANGCI_VERSION}: + curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | BINARY=golangci-lint bash -s -- v${GOLANGCI_VERSION} + @mv bin/golangci-lint $@ + +.PHONY: lint +lint: bin/golangci-lint ## Run linter + bin/golangci-lint run + +.PHONY: fix +fix: bin/golangci-lint ## Fix lint violations + bin/golangci-lint run --fix + vet: @go vet ./... fmt: @./scripts/gofmt ./... -lint: bin/golint +oldlint: bin/golint @./bin/golint -set_exit_status $(shell go list ./...) .PHONY: docker-image @@ -79,8 +96,8 @@ bin/golint: clean: @rm -rf bin/ -testall: testrace vet fmt lint +testall: testrace vet fmt oldlint FORCE: -.PHONY: test testrace vet fmt lint testall +.PHONY: test testrace vet fmt oldlint testall From bcd47fc6f37c7d51222faf248ed7a08e58448b81 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:07:53 +0100 Subject: [PATCH 04/29] Remove old lint targets --- Makefile | 17 ++--------------- scripts/gofmt | 7 ------- 2 files changed, 2 insertions(+), 22 deletions(-) delete mode 100755 scripts/gofmt diff --git a/Makefile b/Makefile index c1c6d788..2c041408 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,6 @@ PROJ=dex ORG_PATH=github.com/dexidp REPO_PATH=$(ORG_PATH)/$(PROJ) export PATH := $(PWD)/bin:$(PATH) -THIS_DIRECTORY:=$(shell dirname $(realpath $(lastword $(MAKEFILE_LIST)))) VERSION ?= $(shell ./scripts/git-version) @@ -62,15 +61,6 @@ lint: bin/golangci-lint ## Run linter fix: bin/golangci-lint ## Fix lint violations bin/golangci-lint run --fix -vet: - @go vet ./... - -fmt: - @./scripts/gofmt ./... - -oldlint: bin/golint - @./bin/golint -set_exit_status $(shell go list ./...) - .PHONY: docker-image docker-image: @sudo docker build -t $(DOCKER_IMAGE) . @@ -90,14 +80,11 @@ bin/protoc: scripts/get-protoc bin/protoc-gen-go: @go install -v $(REPO_PATH)/vendor/github.com/golang/protobuf/protoc-gen-go -bin/golint: - @go install -v $(THIS_DIRECTORY)/vendor/golang.org/x/lint/golint - clean: @rm -rf bin/ -testall: testrace vet fmt oldlint +testall: testrace FORCE: -.PHONY: test testrace vet fmt oldlint testall +.PHONY: test testrace testall diff --git a/scripts/gofmt b/scripts/gofmt deleted file mode 100755 index 8851bdab..00000000 --- a/scripts/gofmt +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/bash -e - -result=$( go fmt $@ ) -if [[ $result != "" ]]; then - >&2 echo "The following files are not formatted correctly: $result" - exit 1 -fi From 8c3dc0ca66e86d0af52bc0f056aaf0ddb3fe8c2a Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:46:49 +0100 Subject: [PATCH 05/29] Remove unused code (fixed: unused, structcheck, deadcode linters) --- .golangci.yml | 3 --- connector/google/google.go | 1 - server/templates.go | 13 ------------- 3 files changed, 17 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 824484d6..45cfc35f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -16,10 +16,7 @@ linters: - wsl # TODO: fix me - - unused - - structcheck - stylecheck - - deadcode - misspell - unparam - goimports diff --git a/connector/google/google.go b/connector/google/google.go index 84e5580d..7561ca4d 100644 --- a/connector/google/google.go +++ b/connector/google/google.go @@ -105,7 +105,6 @@ type googleConnector struct { redirectURI string oauth2Config *oauth2.Config verifier *oidc.IDTokenVerifier - ctx context.Context cancel context.CancelFunc logger log.Logger hostedDomains []string diff --git a/server/templates.go b/server/templates.go index 88aeace0..13dda8f1 100644 --- a/server/templates.go +++ b/server/templates.go @@ -47,19 +47,6 @@ type webConfig struct { extra map[string]string } -func join(base, path string) string { - b := strings.HasSuffix(base, "/") - p := strings.HasPrefix(path, "/") - switch { - case b && p: - return base + path[1:] - case b || p: - return base + path - default: - return base + "/" + path - } -} - func dirExists(dir string) error { stat, err := os.Stat(dir) if err != nil { From 142c96c210a8a5b856b91c79b148ff8e59eaf448 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:50:36 +0100 Subject: [PATCH 06/29] Fix stylecheck --- .golangci.yml | 1 - cmd/example-app/main.go | 4 +-- connector/saml/saml.go | 8 ++--- storage/kubernetes/client.go | 58 ++++++++++++++++++------------------ 4 files changed, 35 insertions(+), 36 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 45cfc35f..5bfe3e99 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -16,7 +16,6 @@ linters: - wsl # TODO: fix me - - stylecheck - misspell - unparam - goimports diff --git a/cmd/example-app/main.go b/cmd/example-app/main.go index a21b2e86..c16ddfdf 100644 --- a/cmd/example-app/main.go +++ b/cmd/example-app/main.go @@ -143,7 +143,7 @@ func cmd() *cobra.Command { ctx := oidc.ClientContext(context.Background(), a.client) provider, err := oidc.NewProvider(ctx, issuerURL) if err != nil { - return fmt.Errorf("Failed to query provider %q: %v", issuerURL, err) + return fmt.Errorf("failed to query provider %q: %v", issuerURL, err) } var s struct { @@ -153,7 +153,7 @@ func cmd() *cobra.Command { ScopesSupported []string `json:"scopes_supported"` } if err := provider.Claims(&s); err != nil { - return fmt.Errorf("Failed to parse provider scopes_supported: %v", err) + return fmt.Errorf("failed to parse provider scopes_supported: %v", err) } if len(s.ScopesSupported) == 0 { diff --git a/connector/saml/saml.go b/connector/saml/saml.go index 09414ea3..f4e52a69 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -325,7 +325,7 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str // Status is a required element. if resp.Status == nil { - return ident, fmt.Errorf("Response did not contain a Status element") + return ident, fmt.Errorf("response did not contain a Status element") } if err = p.validateStatus(resp.Status); err != nil { @@ -398,7 +398,7 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str if len(p.allowedGroups) > 0 && (!s.Groups || p.groupsAttr == "") { // allowedGroups set but no groups or groupsAttr. Disallowing. - return ident, fmt.Errorf("User not a member of allowed groups") + return ident, fmt.Errorf("user not a member of allowed groups") } // Grab the groups. @@ -427,7 +427,7 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str if len(groupMatches) == 0 { // No group membership matches found, disallowing - return ident, fmt.Errorf("User not a member of allowed groups") + return ident, fmt.Errorf("user not a member of allowed groups") } // Otherwise, we're good @@ -468,7 +468,7 @@ func (p *provider) validateStatus(status *status) error { func (p *provider) validateSubject(subject *subject, inResponseTo string) error { // Optional according to the spec, but again, we're going to be strict here. if len(subject.SubjectConfirmations) == 0 { - return fmt.Errorf("Subject contained no SubjectConfirmations") + return fmt.Errorf("subject contained no SubjectConfirmations") } var errs []error diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go index f2af0e57..795dcc22 100644 --- a/storage/kubernetes/client.go +++ b/storage/kubernetes/client.go @@ -55,14 +55,14 @@ type client struct { } // idToName maps an arbitrary ID, such as an email or client ID to a Kubernetes object name. -func (c *client) idToName(s string) string { - return idToName(s, c.hash) +func (cli *client) idToName(s string) string { + return idToName(s, cli.hash) } // offlineTokenName maps two arbitrary IDs, to a single Kubernetes object name. // This is used when more than one field is used to uniquely identify the object. -func (c *client) offlineTokenName(userID string, connID string) string { - return offlineTokenName(userID, connID, c.hash) +func (cli *client) offlineTokenName(userID string, connID string) string { + return offlineTokenName(userID, connID, cli.hash) } // Kubernetes names must match the regexp '[a-z0-9]([-a-z0-9]*[a-z0-9])?'. @@ -79,7 +79,7 @@ func offlineTokenName(userID string, connID string, h func() hash.Hash) string { return strings.TrimRight(encoding.EncodeToString(hash.Sum(nil)), "=") } -func (c *client) urlFor(apiVersion, namespace, resource, name string) string { +func (cli *client) urlFor(apiVersion, namespace, resource, name string) string { basePath := "apis/" if apiVersion == "v1" { basePath = "api/" @@ -91,10 +91,10 @@ func (c *client) urlFor(apiVersion, namespace, resource, name string) string { } else { p = path.Join(basePath, apiVersion, resource, name) } - if strings.HasSuffix(c.baseURL, "/") { - return c.baseURL + p + if strings.HasSuffix(cli.baseURL, "/") { + return cli.baseURL + p } - return c.baseURL + "/" + p + return cli.baseURL + "/" + p } // Define an error interface so we can get at the underlying status code if it's @@ -156,13 +156,13 @@ func closeResp(r *http.Response) { r.Body.Close() } -func (c *client) get(resource, name string, v interface{}) error { - return c.getResource(c.apiVersion, c.namespace, resource, name, v) +func (cli *client) get(resource, name string, v interface{}) error { + return cli.getResource(cli.apiVersion, cli.namespace, resource, name, v) } -func (c *client) getResource(apiVersion, namespace, resource, name string, v interface{}) error { - url := c.urlFor(apiVersion, namespace, resource, name) - resp, err := c.client.Get(url) +func (cli *client) getResource(apiVersion, namespace, resource, name string, v interface{}) error { + url := cli.urlFor(apiVersion, namespace, resource, name) + resp, err := cli.client.Get(url) if err != nil { return err } @@ -173,22 +173,22 @@ func (c *client) getResource(apiVersion, namespace, resource, name string, v int return json.NewDecoder(resp.Body).Decode(v) } -func (c *client) list(resource string, v interface{}) error { - return c.get(resource, "", v) +func (cli *client) list(resource string, v interface{}) error { + return cli.get(resource, "", v) } -func (c *client) post(resource string, v interface{}) error { - return c.postResource(c.apiVersion, c.namespace, resource, v) +func (cli *client) post(resource string, v interface{}) error { + return cli.postResource(cli.apiVersion, cli.namespace, resource, v) } -func (c *client) postResource(apiVersion, namespace, resource string, v interface{}) error { +func (cli *client) postResource(apiVersion, namespace, resource string, v interface{}) error { body, err := json.Marshal(v) if err != nil { return fmt.Errorf("marshal object: %v", err) } - url := c.urlFor(apiVersion, namespace, resource, "") - resp, err := c.client.Post(url, "application/json", bytes.NewReader(body)) + url := cli.urlFor(apiVersion, namespace, resource, "") + resp, err := cli.client.Post(url, "application/json", bytes.NewReader(body)) if err != nil { return err } @@ -196,13 +196,13 @@ func (c *client) postResource(apiVersion, namespace, resource string, v interfac return checkHTTPErr(resp, http.StatusCreated) } -func (c *client) delete(resource, name string) error { - url := c.urlFor(c.apiVersion, c.namespace, resource, name) +func (cli *client) delete(resource, name string) error { + url := cli.urlFor(cli.apiVersion, cli.namespace, resource, name) req, err := http.NewRequest("DELETE", url, nil) if err != nil { return fmt.Errorf("create delete request: %v", err) } - resp, err := c.client.Do(req) + resp, err := cli.client.Do(req) if err != nil { return fmt.Errorf("delete request: %v", err) } @@ -210,7 +210,7 @@ func (c *client) delete(resource, name string) error { return checkHTTPErr(resp, http.StatusOK) } -func (c *client) deleteAll(resource string) error { +func (cli *client) deleteAll(resource string) error { var list struct { k8sapi.TypeMeta `json:",inline"` k8sapi.ListMeta `json:"metadata,omitempty"` @@ -219,24 +219,24 @@ func (c *client) deleteAll(resource string) error { k8sapi.ObjectMeta `json:"metadata,omitempty"` } `json:"items"` } - if err := c.list(resource, &list); err != nil { + if err := cli.list(resource, &list); err != nil { return err } for _, item := range list.Items { - if err := c.delete(resource, item.Name); err != nil { + if err := cli.delete(resource, item.Name); err != nil { return err } } return nil } -func (c *client) put(resource, name string, v interface{}) error { +func (cli *client) put(resource, name string, v interface{}) error { body, err := json.Marshal(v) if err != nil { return fmt.Errorf("marshal object: %v", err) } - url := c.urlFor(c.apiVersion, c.namespace, resource, name) + url := cli.urlFor(cli.apiVersion, cli.namespace, resource, name) req, err := http.NewRequest("PUT", url, bytes.NewReader(body)) if err != nil { return fmt.Errorf("create patch request: %v", err) @@ -244,7 +244,7 @@ func (c *client) put(resource, name string, v interface{}) error { req.Header.Set("Content-Length", strconv.Itoa(len(body))) - resp, err := c.client.Do(req) + resp, err := cli.client.Do(req) if err != nil { return fmt.Errorf("patch request: %v", err) } From 367b187cf4d529ef57f457c99ae7ad53700e9c19 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:51:44 +0100 Subject: [PATCH 07/29] Fix missspell --- .golangci.yml | 1 - connector/github/github.go | 4 ++-- server/handlers.go | 4 ++-- storage/kubernetes/k8sapi/crd_extensions.go | 2 +- storage/sql/config.go | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 5bfe3e99..7ca89f0c 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -16,7 +16,6 @@ linters: - wsl # TODO: fix me - - misspell - unparam - goimports - golint diff --git a/connector/github/github.go b/connector/github/github.go index 6d915edc..d9356e73 100644 --- a/connector/github/github.go +++ b/connector/github/github.go @@ -144,7 +144,7 @@ type githubConnector struct { hostName string // Used to support untrusted/self-signed CA certs. rootCA string - // HTTP Client that trusts the custom delcared rootCA cert. + // HTTP Client that trusts the custom declared rootCA cert. httpClient *http.Client // optional choice between 'name' (default) or 'slug' teamNameField string @@ -206,7 +206,7 @@ func (e *oauth2Error) Error() string { return e.error + ": " + e.errorDescription } -// newHTTPClient returns a new HTTP client that trusts the custom delcared rootCA cert. +// newHTTPClient returns a new HTTP client that trusts the custom declared rootCA cert. func newHTTPClient(rootCA string) (*http.Client, error) { tlsConfig := tls.Config{RootCAs: x509.NewCertPool()} rootCABytes, err := ioutil.ReadFile(rootCA) diff --git a/server/handlers.go b/server/handlers.go index 0f5b0d23..ede474a4 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -45,8 +45,8 @@ func (s *Server) newHealthChecker(ctx context.Context) http.Handler { return h } -// healthChecker periodically performs health checks on server dependenices. -// Currently, it only checks that the storage layer is avialable. +// healthChecker periodically performs health checks on server dependencies. +// Currently, it only checks that the storage layer is available. type healthChecker struct { s *Server diff --git a/storage/kubernetes/k8sapi/crd_extensions.go b/storage/kubernetes/k8sapi/crd_extensions.go index 4b55e393..7a65410b 100644 --- a/storage/kubernetes/k8sapi/crd_extensions.go +++ b/storage/kubernetes/k8sapi/crd_extensions.go @@ -43,7 +43,7 @@ type CustomResourceDefinitionNames struct { ListKind string `json:"listKind,omitempty" protobuf:"bytes,5,opt,name=listKind"` } -// ResourceScope is an enum defining the different scopes availabe to a custom resource +// ResourceScope is an enum defining the different scopes available to a custom resource type ResourceScope string const ( diff --git a/storage/sql/config.go b/storage/sql/config.go index 24467169..827b0df1 100644 --- a/storage/sql/config.go +++ b/storage/sql/config.go @@ -311,7 +311,7 @@ func (s *MySQL) open(logger log.Logger) (*conn, error) { err = db.Ping() if err != nil { if mysqlErr, ok := err.(*mysql.MySQLError); ok && mysqlErr.Number == mysqlErrUnknownSysVar { - logger.Info("reconnecting with MySQL pre-5.7.20 compatibilty mode") + logger.Info("reconnecting with MySQL pre-5.7.20 compatibility mode") // MySQL 5.7.20 introduced transaction_isolation and deprecated tx_isolation. // MySQL 8.0 doesn't have tx_isolation at all. From 9bd5ae519739254ee75baa5f8507b6a18b39bfdf Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:53:34 +0100 Subject: [PATCH 08/29] Fix goimports --- .golangci.yml | 1 - cmd/dex/config_test.go | 2 +- connector/google/google.go | 4 ++-- connector/oidc/oidc_test.go | 3 ++- connector/saml/saml.go | 5 +++-- server/server.go | 8 ++++---- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 7ca89f0c..c658d914 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -17,7 +17,6 @@ linters: # TODO: fix me - unparam - - goimports - golint - whitespace - goconst diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go index d7875ad1..87f1a578 100644 --- a/cmd/dex/config_test.go +++ b/cmd/dex/config_test.go @@ -1,7 +1,6 @@ package main import ( - "github.com/dexidp/dex/server" "testing" "github.com/ghodss/yaml" @@ -9,6 +8,7 @@ import ( "github.com/dexidp/dex/connector/mock" "github.com/dexidp/dex/connector/oidc" + "github.com/dexidp/dex/server" "github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage/sql" ) diff --git a/connector/google/google.go b/connector/google/google.go index 7561ca4d..37b6dc15 100644 --- a/connector/google/google.go +++ b/connector/google/google.go @@ -11,12 +11,12 @@ import ( "github.com/coreos/go-oidc" "golang.org/x/oauth2" + "golang.org/x/oauth2/google" + admin "google.golang.org/api/admin/directory/v1" "github.com/dexidp/dex/connector" pkg_groups "github.com/dexidp/dex/pkg/groups" "github.com/dexidp/dex/pkg/log" - "golang.org/x/oauth2/google" - admin "google.golang.org/api/admin/directory/v1" ) const ( diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index f766a875..22b035c1 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -16,9 +16,10 @@ import ( "testing" "time" - "github.com/dexidp/dex/connector" "github.com/sirupsen/logrus" "gopkg.in/square/go-jose.v2" + + "github.com/dexidp/dex/connector" ) func TestKnownBrokenAuthHeaderProvider(t *testing.T) { diff --git a/connector/saml/saml.go b/connector/saml/saml.go index f4e52a69..70ab413c 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -14,11 +14,12 @@ import ( "time" "github.com/beevik/etree" + dsig "github.com/russellhaering/goxmldsig" + "github.com/russellhaering/goxmldsig/etreeutils" + "github.com/dexidp/dex/connector" "github.com/dexidp/dex/pkg/groups" "github.com/dexidp/dex/pkg/log" - dsig "github.com/russellhaering/goxmldsig" - "github.com/russellhaering/goxmldsig/etreeutils" ) // nolint diff --git a/server/server.go b/server/server.go index fa860330..4dc7337d 100644 --- a/server/server.go +++ b/server/server.go @@ -14,6 +14,10 @@ import ( "sync/atomic" "time" + "github.com/felixge/httpsnoop" + "github.com/gorilla/handlers" + "github.com/gorilla/mux" + "github.com/prometheus/client_golang/prometheus" "golang.org/x/crypto/bcrypt" "github.com/dexidp/dex/connector" @@ -31,10 +35,6 @@ import ( "github.com/dexidp/dex/connector/saml" "github.com/dexidp/dex/pkg/log" "github.com/dexidp/dex/storage" - "github.com/felixge/httpsnoop" - "github.com/gorilla/handlers" - "github.com/gorilla/mux" - "github.com/prometheus/client_golang/prometheus" ) // LocalConnector is the local passwordDB connector which is an internal From f141f2133b0e9e2ab602d3e41cddf57c0d659b54 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:56:12 +0100 Subject: [PATCH 09/29] Fix whitespace --- .golangci.yml | 1 - cmd/dex/config_test.go | 1 - cmd/dex/serve.go | 1 - connector/bitbucketcloud/bitbucketcloud.go | 2 -- connector/bitbucketcloud/bitbucketcloud_test.go | 3 --- connector/github/github.go | 2 -- connector/github/github_test.go | 2 -- connector/gitlab/gitlab_test.go | 4 ---- connector/keystone/keystone.go | 1 - connector/ldap/ldap.go | 2 -- connector/saml/saml.go | 1 - server/api.go | 2 -- server/api_test.go | 2 -- server/handlers.go | 1 - server/handlers_test.go | 1 - server/server_test.go | 3 --- server/templates.go | 1 - storage/conformance/conformance.go | 2 -- storage/sql/crud.go | 1 - 19 files changed, 33 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index c658d914..d0dcfd9b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -18,7 +18,6 @@ linters: # TODO: fix me - unparam - golint - - whitespace - goconst - unconvert - bodyclose diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go index 87f1a578..1df1f809 100644 --- a/cmd/dex/config_test.go +++ b/cmd/dex/config_test.go @@ -211,5 +211,4 @@ logger: if diff := pretty.Compare(c, want); diff != "" { t.Errorf("got!=want: %s", diff) } - } diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 7d0e0deb..293d3e66 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -182,7 +182,6 @@ func serve(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to initialize storage connectors: %v", err) } storageConnectors[i] = conn - } if c.EnablePasswordDB { diff --git a/connector/bitbucketcloud/bitbucketcloud.go b/connector/bitbucketcloud/bitbucketcloud.go index 7d58221a..d8ccf2e5 100644 --- a/connector/bitbucketcloud/bitbucketcloud.go +++ b/connector/bitbucketcloud/bitbucketcloud.go @@ -41,7 +41,6 @@ type Config struct { // Open returns a strategy for logging in through Bitbucket. func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) { - b := bitbucketConnector{ redirectURI: c.RedirectURI, teams: c.Teams, @@ -373,7 +372,6 @@ type userTeamsResponse struct { } func (b *bitbucketConnector) userTeams(ctx context.Context, client *http.Client) ([]string, error) { - var teams []string apiURL := b.apiURL + "/teams?role=member" diff --git a/connector/bitbucketcloud/bitbucketcloud_test.go b/connector/bitbucketcloud/bitbucketcloud_test.go index b9f4ba08..488c7886 100644 --- a/connector/bitbucketcloud/bitbucketcloud_test.go +++ b/connector/bitbucketcloud/bitbucketcloud_test.go @@ -14,7 +14,6 @@ import ( ) func TestUserGroups(t *testing.T) { - teamsResponse := userTeamsResponse{ pagedResponse: pagedResponse{ Size: 3, @@ -46,7 +45,6 @@ func TestUserGroups(t *testing.T) { } func TestUserWithoutTeams(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/teams?role=member": userTeamsResponse{}, }) @@ -61,7 +59,6 @@ func TestUserWithoutTeams(t *testing.T) { } func TestUsernameIncludedInFederatedIdentity(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/user": user{Username: "some-login"}, "/user/emails": userEmailResponse{ diff --git a/connector/github/github.go b/connector/github/github.go index d9356e73..41aaf589 100644 --- a/connector/github/github.go +++ b/connector/github/github.go @@ -67,7 +67,6 @@ type Org struct { // Open returns a strategy for logging in through GitHub. func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) { - if c.Org != "" { // Return error if both 'org' and 'orgs' fields are used. if len(c.Orgs) > 0 { @@ -107,7 +106,6 @@ func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) if g.httpClient, err = newHTTPClient(g.rootCA); err != nil { return nil, fmt.Errorf("failed to create HTTP client: %v", err) } - } g.loadAllGroups = c.LoadAllGroups diff --git a/connector/github/github_test.go b/connector/github/github_test.go index 539a2e69..76d7463c 100644 --- a/connector/github/github_test.go +++ b/connector/github/github_test.go @@ -126,7 +126,6 @@ func TestUserGroupsWithTeamNameAndSlugFieldConfig(t *testing.T) { // tests that the users login is used as their username when they have no username set func TestUsernameIncludedInFederatedIdentity(t *testing.T) { - s := newTestServer(map[string]testResponse{ "/user": {data: user{Login: "some-login", ID: 12345678}}, "/user/emails": {data: []userEmail{{ @@ -168,7 +167,6 @@ func TestUsernameIncludedInFederatedIdentity(t *testing.T) { } func TestLoginUsedAsIDWhenConfigured(t *testing.T) { - s := newTestServer(map[string]testResponse{ "/user": {data: user{Login: "some-login", ID: 12345678, Name: "Joe Bloggs"}}, "/user/emails": {data: []userEmail{{ diff --git a/connector/gitlab/gitlab_test.go b/connector/gitlab/gitlab_test.go index 331b486e..23cf9aac 100644 --- a/connector/gitlab/gitlab_test.go +++ b/connector/gitlab/gitlab_test.go @@ -65,7 +65,6 @@ func TestUserGroupsWithoutOrgs(t *testing.T) { // tests that the email is used as their username when they have no username set func TestUsernameIncludedInFederatedIdentity(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/api/v4/user": gitlabUser{Email: "some@email.com", ID: 12345678}, "/oauth/token": map[string]interface{}{ @@ -102,7 +101,6 @@ func TestUsernameIncludedInFederatedIdentity(t *testing.T) { } func TestLoginUsedAsIDWhenConfigured(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/api/v4/user": gitlabUser{Email: "some@email.com", ID: 12345678, Name: "Joe Bloggs", Username: "joebloggs"}, "/oauth/token": map[string]interface{}{ @@ -130,7 +128,6 @@ func TestLoginUsedAsIDWhenConfigured(t *testing.T) { } func TestLoginWithTeamWhitelisted(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/api/v4/user": gitlabUser{Email: "some@email.com", ID: 12345678, Name: "Joe Bloggs"}, "/oauth/token": map[string]interface{}{ @@ -158,7 +155,6 @@ func TestLoginWithTeamWhitelisted(t *testing.T) { } func TestLoginWithTeamNonWhitelisted(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/api/v4/user": gitlabUser{Email: "some@email.com", ID: 12345678, Name: "Joe Bloggs", Username: "joebloggs"}, "/oauth/token": map[string]interface{}{ diff --git a/connector/keystone/keystone.go b/connector/keystone/keystone.go index dc74a01f..71a59de8 100644 --- a/connector/keystone/keystone.go +++ b/connector/keystone/keystone.go @@ -150,7 +150,6 @@ func (p *conn) Prompt() string { return "username" } func (p *conn) Refresh( ctx context.Context, scopes connector.Scopes, identity connector.Identity) (connector.Identity, error) { - token, err := p.getAdminToken(ctx) if err != nil { return identity, fmt.Errorf("keystone: failed to obtain admin token: %v", err) diff --git a/connector/ldap/ldap.go b/connector/ldap/ldap.go index aed73194..cac3539f 100644 --- a/connector/ldap/ldap.go +++ b/connector/ldap/ldap.go @@ -189,7 +189,6 @@ func (c *Config) OpenConnector(logger log.Logger) (interface { } func (c *Config) openConnector(logger log.Logger) (*ldapConnector, error) { - requiredFields := []struct { name string val string @@ -365,7 +364,6 @@ func (c *ldapConnector) identityFromEntry(user ldap.Entry) (ident connector.Iden } func (c *ldapConnector) userEntry(conn *ldap.Conn, username string) (user ldap.Entry, found bool, err error) { - filter := fmt.Sprintf("(%s=%s)", c.UserSearch.Username, ldap.EscapeFilter(username)) if c.UserSearch.Filter != "" { filter = fmt.Sprintf("(&%s%s)", c.UserSearch.Filter, filter) diff --git a/connector/saml/saml.go b/connector/saml/saml.go index 70ab413c..1c1cb460 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -249,7 +249,6 @@ type provider struct { } func (p *provider) POSTData(s connector.Scopes, id string) (action, value string, err error) { - r := &authnRequest{ ProtocolBinding: bindingPOST, ID: id, diff --git a/server/api.go b/server/api.go index 9feb0e9a..55784b90 100644 --- a/server/api.go +++ b/server/api.go @@ -218,7 +218,6 @@ func (d dexAPI) DeletePassword(ctx context.Context, req *api.DeletePasswordReq) return nil, fmt.Errorf("delete password: %v", err) } return &api.DeletePasswordResp{}, nil - } func (d dexAPI) GetVersion(ctx context.Context, req *api.VersionReq) (*api.VersionResp, error) { @@ -248,7 +247,6 @@ func (d dexAPI) ListPasswords(ctx context.Context, req *api.ListPasswordReq) (*a return &api.ListPasswordResp{ Passwords: passwords, }, nil - } func (d dexAPI) VerifyPassword(ctx context.Context, req *api.VerifyPasswordReq) (*api.VerifyPasswordResp, error) { diff --git a/server/api_test.go b/server/api_test.go index 80a22486..511d5d39 100644 --- a/server/api_test.go +++ b/server/api_test.go @@ -167,7 +167,6 @@ func TestPassword(t *testing.T) { if _, err := client.DeletePassword(ctx, &deleteReq); err != nil { t.Fatalf("Unable to delete password: %v", err) } - } // Ensures checkCost returns expected values @@ -495,7 +494,6 @@ func TestUpdateClient(t *testing.T) { if tc.cleanup != nil { tc.cleanup(t, tc.req.Id) } - }) } } diff --git a/server/handlers.go b/server/handlers.go index ede474a4..97a46d57 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -922,7 +922,6 @@ func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client s deleteToken = true return } - } } s.writeAccessToken(w, idToken, accessToken, refreshToken, expiry) diff --git a/server/handlers_test.go b/server/handlers_test.go index 395b7e72..b30076dd 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -24,7 +24,6 @@ func TestHandleHealth(t *testing.T) { if rr.Code != http.StatusOK { t.Errorf("expected 200 got %d", rr.Code) } - } type badStorage struct { diff --git a/server/server_test.go b/server/server_test.go index 6759f240..ef49ed6f 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -799,7 +799,6 @@ func TestCrossClientScopes(t *testing.T) { if !reflect.DeepEqual(idToken.Audience, expAudience) { t.Errorf("expected audience %q, got %q", expAudience, idToken.Audience) } - } if gotState := q.Get("state"); gotState != state { t.Errorf("state did not match, want=%q got=%q", state, gotState) @@ -921,7 +920,6 @@ func TestCrossClientScopesWithAzpInAudienceByDefault(t *testing.T) { if !reflect.DeepEqual(idToken.Audience, expAudience) { t.Errorf("expected audience %q, got %q", expAudience, idToken.Audience) } - } if gotState := q.Get("state"); gotState != state { t.Errorf("state did not match, want=%q got=%q", state, gotState) @@ -1058,7 +1056,6 @@ func TestPasswordDB(t *testing.T) { t.Errorf("%s: %s", tc.name, diff) } } - } func TestPasswordDBUsernamePrompt(t *testing.T) { diff --git a/server/templates.go b/server/templates.go index 13dda8f1..ac0957af 100644 --- a/server/templates.go +++ b/server/templates.go @@ -176,7 +176,6 @@ func loadTemplates(c webConfig, templatesDir string) (*templates, error) { //assetPath is static/main.css //relativeURL("/dex", "/dex/auth", "static/main.css") = "../static/main.css" func relativeURL(serverPath, reqPath, assetPath string) string { - splitPath := func(p string) []string { res := []string{} parts := strings.Split(path.Clean(p), "/") diff --git a/storage/conformance/conformance.go b/storage/conformance/conformance.go index 9832a7d8..92a48d6a 100644 --- a/storage/conformance/conformance.go +++ b/storage/conformance/conformance.go @@ -160,7 +160,6 @@ func testAuthRequestCRUD(t *testing.T, s storage.Storage) { if err := s.DeleteAuthRequest(a2.ID); err != nil { t.Fatalf("failed to delete auth request: %v", err) } - } func testAuthCodeCRUD(t *testing.T, s storage.Storage) { @@ -509,7 +508,6 @@ func testPasswordCRUD(t *testing.T, s storage.Storage) { _, err = s.GetPassword(password1.Email) mustBeErrNotFound(t, "password", err) - } func testOfflineSessionCRUD(t *testing.T, s storage.Storage) { diff --git a/storage/sql/crud.go b/storage/sql/crud.go index e96a7b12..e87dc56a 100644 --- a/storage/sql/crud.go +++ b/storage/sql/crud.go @@ -169,7 +169,6 @@ func (c *conn) UpdateAuthRequest(id string, updater func(a storage.AuthRequest) } return nil }) - } func (c *conn) GetAuthRequest(id string) (storage.AuthRequest, error) { From 2f8d1f8e42eebe6b4a4b421cc97053dcdb6b7b4c Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 15:56:46 +0100 Subject: [PATCH 10/29] Fix unconvert --- .golangci.yml | 1 - connector/keystone/keystone_test.go | 2 +- storage/etcd/etcd.go | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index d0dcfd9b..89e09982 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -19,7 +19,6 @@ linters: - unparam - golint - goconst - - unconvert - bodyclose - staticcheck - nakedret diff --git a/connector/keystone/keystone_test.go b/connector/keystone/keystone_test.go index d5d65ef1..d8bb0268 100644 --- a/connector/keystone/keystone_test.go +++ b/connector/keystone/keystone_test.go @@ -274,7 +274,7 @@ func TestUseRefreshToken(t *testing.T) { delete(t, token, groupID, groupsURL) expectEquals(t, 1, len(identityRefresh.Groups)) - expectEquals(t, testGroup, string(identityRefresh.Groups[0])) + expectEquals(t, testGroup, identityRefresh.Groups[0]) } func TestUseRefreshTokenUserDeleted(t *testing.T) { diff --git a/storage/etcd/etcd.go b/storage/etcd/etcd.go index d106f443..ee588129 100644 --- a/storage/etcd/etcd.go +++ b/storage/etcd/etcd.go @@ -156,7 +156,7 @@ func (c *conn) UpdateRefreshToken(id string, updater func(old storage.RefreshTok return c.txnUpdate(ctx, keyID(refreshTokenPrefix, id), func(currentValue []byte) ([]byte, error) { var current RefreshToken if len(currentValue) > 0 { - if err := json.Unmarshal([]byte(currentValue), ¤t); err != nil { + if err := json.Unmarshal(currentValue, ¤t); err != nil { return nil, err } } From 65c77e9db2e1e32fe4837970ea2da06758be42da Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 16:04:03 +0100 Subject: [PATCH 11/29] Fix bodyclose --- .golangci.yml | 1 - connector/keystone/keystone.go | 3 +++ connector/keystone/keystone_test.go | 15 +++++++++++++-- server/server_test.go | 16 ++++++++++++++-- 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 89e09982..1ff5dd64 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -19,7 +19,6 @@ linters: - unparam - golint - goconst - - bodyclose - staticcheck - nakedret - ineffassign diff --git a/connector/keystone/keystone.go b/connector/keystone/keystone.go index 71a59de8..6ce22d1a 100644 --- a/connector/keystone/keystone.go +++ b/connector/keystone/keystone.go @@ -209,6 +209,8 @@ func (p *conn) getAdminToken(ctx context.Context) (string, error) { if err != nil { return "", err } + defer resp.Body.Close() + token := resp.Header.Get("X-Subject-Token") return token, nil } @@ -228,6 +230,7 @@ func (p *conn) checkIfUserExists(ctx context.Context, userID string, token strin if err != nil { return false, err } + defer resp.Body.Close() if resp.StatusCode == 200 { return true, nil diff --git a/connector/keystone/keystone_test.go b/connector/keystone/keystone_test.go index d8bb0268..784cdc4f 100644 --- a/connector/keystone/keystone_test.go +++ b/connector/keystone/keystone_test.go @@ -154,7 +154,12 @@ func delete(t *testing.T, token, id, uri string) { t.Fatalf("error: %v", err) } req.Header.Set("X-Auth-Token", token) - client.Do(req) + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("error: %v", err) + } + defer resp.Body.Close() } func createGroup(t *testing.T, token, description, name string) string { @@ -208,7 +213,13 @@ func addUserToGroup(t *testing.T, token, groupID, userID string) error { return err } req.Header.Set("X-Auth-Token", token) - client.Do(req) + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("error: %v", err) + } + defer resp.Body.Close() + return nil } diff --git a/server/server_test.go b/server/server_test.go index ef49ed6f..8fe84c9a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -590,6 +590,8 @@ func TestOAuth2CodeFlow(t *testing.T) { if err != nil { t.Fatalf("get failed: %v", err) } + defer resp.Body.Close() + if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil { t.Fatal(err) } @@ -726,6 +728,8 @@ func TestOAuth2ImplicitFlow(t *testing.T) { if err != nil { t.Fatalf("get failed: %v", err) } + defer resp.Body.Close() + if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil { t.Fatal(err) } @@ -847,6 +851,8 @@ func TestCrossClientScopes(t *testing.T) { if err != nil { t.Fatalf("get failed: %v", err) } + defer resp.Body.Close() + if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil { t.Fatal(err) } @@ -967,6 +973,8 @@ func TestCrossClientScopesWithAzpInAudienceByDefault(t *testing.T) { if err != nil { t.Fatalf("get failed: %v", err) } + defer resp.Body.Close() + if reqDump, err = httputil.DumpRequest(resp.Request, false); err != nil { t.Fatal(err) } @@ -1222,9 +1230,11 @@ func TestRefreshTokenFlow(t *testing.T) { RedirectURL: redirectURL, } - if _, err = http.Get(oauth2Client.server.URL + "/login"); err != nil { + resp, err := http.Get(oauth2Client.server.URL + "/login") + if err != nil { t.Fatalf("get failed: %v", err) } + defer resp.Body.Close() tok := &oauth2.Token{ RefreshToken: oauth2Client.token.RefreshToken, @@ -1232,9 +1242,11 @@ func TestRefreshTokenFlow(t *testing.T) { } // Login in again to receive a new token. - if _, err = http.Get(oauth2Client.server.URL + "/login"); err != nil { + resp, err = http.Get(oauth2Client.server.URL + "/login") + if err != nil { t.Fatalf("get failed: %v", err) } + defer resp.Body.Close() // try to refresh expired token with old refresh token. if _, err := oauth2Client.config.TokenSource(ctx, tok).Token(); err == nil { From 050d5af937626f5e1403aabff60ff07941dbc8a3 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 16:07:06 +0100 Subject: [PATCH 12/29] Fix ineffassign --- .golangci.yml | 1 - server/templates.go | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index 1ff5dd64..16d82a5e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -21,7 +21,6 @@ linters: - goconst - staticcheck - nakedret - - ineffassign - errcheck - gosec - gochecknoinits diff --git a/server/templates.go b/server/templates.go index ac0957af..b9beab33 100644 --- a/server/templates.go +++ b/server/templates.go @@ -206,6 +206,7 @@ func relativeURL(serverPath, reqPath, assetPath string) string { server, req, asset := splitPath(serverPath), splitPath(reqPath), splitPath(assetPath) // Remove common prefix of request path with server path + // nolint: ineffassign server, req = stripCommonParts(server, req) // Remove common prefix of request path with asset path From 309b33d05a2573f949f2fdd65fb991e24f73ed9d Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 17:00:57 +0100 Subject: [PATCH 13/29] Rewrite kubernetes storage test --- Makefile | 17 ++++- go.mod | 1 + go.sum | 13 ++++ storage/kubernetes/storage_test.go | 115 +++++++++++++++++++---------- 4 files changed, 107 insertions(+), 39 deletions(-) diff --git a/Makefile b/Makefile index 2c041408..81a6dbcd 100644 --- a/Makefile +++ b/Makefile @@ -41,12 +41,25 @@ revendor: @go mod vendor -v @go mod verify -test: +test: bin/test/kube-apiserver bin/test/etcd @go test -v ./... -testrace: +testrace: bin/test/kube-apiserver bin/test/etcd @go test -v --race ./... +export TEST_ASSET_KUBE_APISERVER=$(abspath bin/test/kube-apiserver) +export TEST_ASSET_ETCD=$(abspath bin/test/etcd) + +bin/test/kube-apiserver: + @mkdir -p bin/test + curl -L https://storage.googleapis.com/k8s-c10s-test-binaries/kube-apiserver-$(shell uname)-x86_64 > bin/test/kube-apiserver + chmod +x bin/test/kube-apiserver + +bin/test/etcd: + @mkdir -p bin/test + curl -L https://storage.googleapis.com/k8s-c10s-test-binaries/etcd-$(shell uname)-x86_64 > bin/test/etcd + chmod +x bin/test/etcd + bin/golangci-lint: bin/golangci-lint-${GOLANGCI_VERSION} @ln -sf golangci-lint-${GOLANGCI_VERSION} bin/golangci-lint bin/golangci-lint-${GOLANGCI_VERSION}: diff --git a/go.mod b/go.mod index 99a51004..7f489d7c 100644 --- a/go.mod +++ b/go.mod @@ -57,6 +57,7 @@ require ( gopkg.in/asn1-ber.v1 v1.0.0-20181015200546-f715ec2f112d // indirect gopkg.in/ldap.v2 v2.5.1 gopkg.in/square/go-jose.v2 v2.3.1 + sigs.k8s.io/testing_frameworks v0.1.2 ) go 1.13 diff --git a/go.sum b/go.sum index 73f3b2e1..a82bad50 100644 --- a/go.sum +++ b/go.sum @@ -92,6 +92,7 @@ github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6 h1:ZgQEtGgCBiWRM github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= github.com/golang/mock v1.2.0/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= +github.com/golang/protobuf v1.0.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs= @@ -172,9 +173,11 @@ github.com/modern-go/reflect2 v1.0.1/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3Rllmb github.com/morikuni/aec v0.0.0-20170113033406-39771216ff4c h1:nXxl5PrvVm2L/wCy8dQu6DMTwH4oIuGN8GJDAlqDdVE= github.com/morikuni/aec v0.0.0-20170113033406-39771216ff4c/go.mod h1:BbKIizmSmc5MMPqRYbxO4ZU0S0+P200+tUnFx7PXmsc= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= +github.com/onsi/ginkgo v1.4.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.8.0 h1:VkHVNpR4iVnU8XQR6DBm8BqYjN7CRzw+xKUbVVbbW9w= github.com/onsi/ginkgo v1.8.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= +github.com/onsi/gomega v1.3.0/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA= github.com/onsi/gomega v1.5.0 h1:izbySO9zDPmjJ8rDjLvkA2zJHIo+HkYXHnf7eN7SSyo= github.com/onsi/gomega v1.5.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/opencontainers/go-digest v0.0.0-20180430190053-c9281466c8b2/go.mod h1:cMLVZDEM3+U2I4VmLI6N8jQYUd2OVphdqWwCJHrFt2s= @@ -273,6 +276,7 @@ golang.org/x/lint v0.0.0-20190301231843-5614ed5bae6f/go.mod h1:UVdnD1Gm6xHRNCYTk golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/lint v0.0.0-20190409202823-959b441ac422 h1:QzoH/1pFpZguR8NrRHLcO6jKqfv2zpuSqZLgdm7ZmjI= golang.org/x/lint v0.0.0-20190409202823-959b441ac422/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= +golang.org/x/net v0.0.0-20180112015858-5ccada7d0a7b/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -300,6 +304,9 @@ golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e h1:vcxGaoTs7kV8m5Np9uUNQin4BrLOthgV7252N8V+FwY= +golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sys v0.0.0-20180117170059-2c42eef0765b/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -317,6 +324,7 @@ golang.org/x/sys v0.0.0-20190606165138-5da285871e9c/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3 h1:7TYNF4UdlohbFwpNH04CoPMp1cHUZgO1Ebq5r2hIjfo= golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +golang.org/x/text v0.3.1-0.20171227012246-e19ae1496984/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2 h1:tW2bmiBqwgJj/UpqtC8EpXEZVYOwU0yG4iWbprSVAcs= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= @@ -363,6 +371,8 @@ gopkg.in/asn1-ber.v1 v1.0.0-20181015200546-f715ec2f112d/go.mod h1:cuepJuh7vyXfUy gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogRM/Nc3DYOhEAlW+xobZo= +gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/fsnotify.v1 v1.4.7 h1:xOHLXZwVvI9hhs+cLKq5+I5onOuwQLhQwiu63xxlHs4= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= gopkg.in/ldap.v2 v2.5.1 h1:wiu0okdNfjlBzg6UWvd1Hn8Y+Ux17/u/4nlk4CQr6tU= @@ -373,6 +383,7 @@ gopkg.in/square/go-jose.v2 v2.3.1/go.mod h1:M9dMgbHiYLoDGQrXy7OpJDJWiKiU//h+vD76 gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= gopkg.in/yaml.v2 v2.0.0-20170812160011-eb3733d160e7/go.mod h1:JAlM8MvJe8wmxCU4Bli9HhUf9+ttbYbLASfIpnQbh74= +gopkg.in/yaml.v2 v2.0.0/go.mod h1:JAlM8MvJe8wmxCU4Bli9HhUf9+ttbYbLASfIpnQbh74= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= @@ -385,3 +396,5 @@ honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWh honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= k8s.io/kubernetes v1.13.0/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk= +sigs.k8s.io/testing_frameworks v0.1.2 h1:vK0+tvjF0BZ/RYFeZ1E6BYBwHJJXhjuZ3TdsEKH+UQM= +sigs.k8s.io/testing_frameworks v0.1.2/go.mod h1:ToQrwSC3s8Xf/lADdZp3Mktcql9CG0UAmdJG9th5i0w= diff --git a/storage/kubernetes/storage_test.go b/storage/kubernetes/storage_test.go index 27d65416..ea471427 100644 --- a/storage/kubernetes/storage_test.go +++ b/storage/kubernetes/storage_test.go @@ -1,39 +1,104 @@ package kubernetes import ( - "fmt" + "io/ioutil" "os" + "strings" "testing" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/suite" + "sigs.k8s.io/testing_frameworks/integration" "github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage/conformance" ) -const testKubeConfigEnv = "DEX_KUBECONFIG" +const kubeconfigTemplate = `apiVersion: v1 +kind: Config +clusters: +- name: local + cluster: + server: SERVERURL +users: +- name: local + user: +contexts: +- context: + cluster: local + user: local +` -func TestLoadClient(t *testing.T) { - loadClient(t) +func TestStorage(t *testing.T) { + if os.Getenv("TEST_ASSET_KUBE_APISERVER") == "" || os.Getenv("TEST_ASSET_ETCD") == "" { + t.Skip("control plane binaries are missing") + } + + suite.Run(t, new(StorageTestSuite)) } -func loadClient(t *testing.T) *client { +type StorageTestSuite struct { + suite.Suite + + controlPlane *integration.ControlPlane + + client *client +} + +func (s *StorageTestSuite) SetupSuite() { + s.controlPlane = &integration.ControlPlane{} + + err := s.controlPlane.Start() + s.Require().NoError(err) +} + +func (s *StorageTestSuite) TearDownSuite() { + s.controlPlane.Stop() +} + +func (s *StorageTestSuite) SetupTest() { + f, err := ioutil.TempFile("", "dex-kubeconfig-*") + s.Require().NoError(err) + defer f.Close() + + _, err = f.WriteString(strings.ReplaceAll(kubeconfigTemplate, "SERVERURL", s.controlPlane.APIURL().String())) + s.Require().NoError(err) + config := Config{ - KubeConfigFile: os.Getenv(testKubeConfigEnv), - } - if config.KubeConfigFile == "" { - t.Skipf("test environment variable %q not set, skipping", testKubeConfigEnv) + KubeConfigFile: f.Name(), } + logger := &logrus.Logger{ Out: os.Stderr, Formatter: &logrus.TextFormatter{DisableColors: true}, Level: logrus.DebugLevel, } - s, err := config.open(logger, true) - if err != nil { - t.Fatal(err) + + client, err := config.open(logger, true) + s.Require().NoError(err) + + s.client = client +} + +func (s *StorageTestSuite) TestStorage() { + newStorage := func() storage.Storage { + for _, resource := range []string{ + resourceAuthCode, + resourceAuthRequest, + resourceClient, + resourceRefreshToken, + resourceKeys, + resourcePassword, + } { + if err := s.client.deleteAll(resource); err != nil { + s.T().Fatalf("delete all %q failed: %v", resource, err) + } + } + return s.client } - return s + + conformance.RunTests(s.T(), newStorage) + conformance.RunTransactionTests(s.T(), newStorage) } func TestURLFor(t *testing.T) { @@ -83,27 +148,3 @@ func TestURLFor(t *testing.T) { } } } - -func TestStorage(t *testing.T) { - client := loadClient(t) - newStorage := func() storage.Storage { - for _, resource := range []string{ - resourceAuthCode, - resourceAuthRequest, - resourceClient, - resourceRefreshToken, - resourceKeys, - resourcePassword, - } { - if err := client.deleteAll(resource); err != nil { - // Fatalf sometimes doesn't print the error message. - fmt.Fprintf(os.Stderr, "delete all %q failed: %v\n", resource, err) - t.Fatalf("delete all %q failed: %v", resource, err) - } - } - return client - } - - conformance.RunTests(t, newStorage) - conformance.RunTransactionTests(t, newStorage) -} From e0c58d54496e8ce33918d04013d9e8378e87a76c Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 17:02:05 +0100 Subject: [PATCH 14/29] Remove old kubernetes storage test flow --- .github/workflows/ci.yml | 3 --- .travis.yml | 1 - scripts/test-k8s.sh | 56 ---------------------------------------- 3 files changed, 60 deletions(-) delete mode 100755 scripts/test-k8s.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46c9c928..8d4e906d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,9 +68,6 @@ jobs: DEX_KEYSTONE_ADMIN_USER: demo DEX_KEYSTONE_ADMIN_PASS: DEMO_PASS - - name: Run Kubernetes tests - run: ./scripts/test-k8s.sh - - name: Run linter run: make lint diff --git a/.travis.yml b/.travis.yml index d8ef066e..6c820183 100644 --- a/.travis.yml +++ b/.travis.yml @@ -48,7 +48,6 @@ install: script: - make testall - - ./scripts/test-k8s.sh - make verify-proto # Ensure proto generation doesn't depend on external packages. notifications: diff --git a/scripts/test-k8s.sh b/scripts/test-k8s.sh deleted file mode 100755 index 86127be1..00000000 --- a/scripts/test-k8s.sh +++ /dev/null @@ -1,56 +0,0 @@ -#!/bin/bash -e - -TEMPDIR=$( mktemp -d ) - -cat << EOF > $TEMPDIR/kubeconfig -apiVersion: v1 -kind: Config -clusters: -- name: local - cluster: - server: http://localhost:8080 -users: -- name: local - user: -contexts: -- context: - cluster: local - user: local -EOF - -cleanup () { - docker rm -f $( cat $TEMPDIR/etcd ) - docker rm -f $( cat $TEMPDIR/kube-apiserver ) - rm -rf $TEMPDIR -} - -trap "{ CODE=$?; cleanup ; exit $CODE; }" EXIT - -docker run \ - --cidfile=$TEMPDIR/etcd \ - -d \ - --net=host \ - gcr.io/google_containers/etcd:3.1.10 \ - etcd - -docker run \ - --cidfile=$TEMPDIR/kube-apiserver \ - -d \ - -v $TEMPDIR:/var/run/kube-test:ro \ - --net=host \ - gcr.io/google_containers/kube-apiserver-amd64:v1.7.4 \ - kube-apiserver \ - --etcd-servers=http://localhost:2379 \ - --service-cluster-ip-range=10.0.0.1/16 \ - --insecure-bind-address=0.0.0.0 \ - --insecure-port=8080 - -until $(curl --output /dev/null --silent --head --fail http://localhost:8080/healthz); do - printf '.' - sleep 1 -done -echo "API server ready" - -export DEX_KUBECONFIG=$TEMPDIR/kubeconfig -go test -v -i ./storage/kubernetes -go test -v ./storage/kubernetes From 3fb85ab0094b94f1348a9e0697279f93fa3cba96 Mon Sep 17 00:00:00 2001 From: Mark Sagi-Kazar Date: Wed, 18 Dec 2019 17:03:46 +0100 Subject: [PATCH 15/29] Remove instructions for kubernetes tests from docs --- Documentation/dev-integration-tests.md | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/Documentation/dev-integration-tests.md b/Documentation/dev-integration-tests.md index c9c84d2d..1b694580 100644 --- a/Documentation/dev-integration-tests.md +++ b/Documentation/dev-integration-tests.md @@ -1,21 +1,5 @@ # Running integration tests -## Kubernetes - -Kubernetes tests run against a Kubernetes API server, and are enabled by the `DEX_KUBECONFIG` environment variable: - -``` -$ export DEX_KUBECONFIG=~/.kube/config -$ go test -v -i ./storage/kubernetes -$ go test -v ./storage/kubernetes -``` - -These tests can be executed locally using docker by running the following script: - -``` -$ ./scripts/test-k8s.sh -``` - ## Postgres Running database tests locally requires: From 8e0ae820340f40593c497d155c27e2211f948df4 Mon Sep 17 00:00:00 2001 From: Lars Lehtonen Date: Fri, 15 Nov 2019 16:31:22 -0800 Subject: [PATCH 16/29] connector/oidc: replace deprecated oauth2.RegisterBrokenAuthHeaderProvider with oauth2.Endpoint.AuthStyle --- connector/oidc/oidc.go | 21 +++++---------------- connector/oidc/oidc_test.go | 2 ++ 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index 341e4e0a..2c199112 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -9,7 +9,6 @@ import ( "net/http" "net/url" "strings" - "sync" "time" "github.com/coreos/go-oidc" @@ -85,18 +84,6 @@ func knownBrokenAuthHeaderProvider(issuerURL string) bool { return false } -// golang.org/x/oauth2 doesn't do internal locking. Need to do it in this -// package ourselves and hope that other packages aren't calling it at the -// same time. -var registerMu = new(sync.Mutex) - -func registerBrokenAuthHeaderProvider(url string) { - registerMu.Lock() - defer registerMu.Unlock() - - oauth2.RegisterBrokenAuthHeaderProvider(url) -} - // Open returns a connector which can be used to login users through an upstream // OpenID Connect provider. func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, err error) { @@ -108,13 +95,15 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e return nil, fmt.Errorf("failed to get provider: %v", err) } + endpoint := provider.Endpoint() + if c.BasicAuthUnsupported != nil { // Setting "basicAuthUnsupported" always overrides our detection. if *c.BasicAuthUnsupported { - registerBrokenAuthHeaderProvider(provider.Endpoint().TokenURL) + endpoint.AuthStyle = oauth2.AuthStyleInParams } } else if knownBrokenAuthHeaderProvider(c.Issuer) { - registerBrokenAuthHeaderProvider(provider.Endpoint().TokenURL) + endpoint.AuthStyle = oauth2.AuthStyleInParams } scopes := []string{oidc.ScopeOpenID} @@ -131,7 +120,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e oauth2Config: &oauth2.Config{ ClientID: clientID, ClientSecret: c.ClientSecret, - Endpoint: provider.Endpoint(), + Endpoint: endpoint, Scopes: scopes, RedirectURL: c.RedirectURI, }, diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index f766a875..f5f53e6b 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -111,6 +111,7 @@ func TestHandleCallback(t *testing.T) { } defer testServer.Close() serverURL := testServer.URL + basicAuth := true config := Config{ Issuer: serverURL, ClientID: "clientID", @@ -120,6 +121,7 @@ func TestHandleCallback(t *testing.T) { UserIDKey: tc.userIDKey, UserNameKey: tc.userNameKey, InsecureSkipEmailVerified: tc.insecureSkipEmailVerified, + BasicAuthUnsupported: &basicAuth, } conn, err := newConnector(config) From 92e63771ac69563603ffbd68a0c8f82fcbb4f1ed Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Tue, 10 Dec 2019 10:51:09 -0300 Subject: [PATCH 17/29] Added OpenShift connector --- Documentation/connectors/openshift.md | 50 +++++ connector/openshift/openshift.go | 252 ++++++++++++++++++++++++++ connector/openshift/openshift_test.go | 223 +++++++++++++++++++++++ server/server.go | 2 + 4 files changed, 527 insertions(+) create mode 100644 Documentation/connectors/openshift.md create mode 100644 connector/openshift/openshift.go create mode 100644 connector/openshift/openshift_test.go diff --git a/Documentation/connectors/openshift.md b/Documentation/connectors/openshift.md new file mode 100644 index 00000000..a33ca772 --- /dev/null +++ b/Documentation/connectors/openshift.md @@ -0,0 +1,50 @@ +# Authentication using OpenShift + +## Overview + +Dex can make use of users and groups defined within OpenShift by querying the platform provided OAuth server. + +## Configuration + +Create a new OAuth Client by following the steps described in the documentation for [Registering Additional OAuth Clients[(https://docs.openshift.com/container-platform/latest/authentication/configuring-internal-oauth.html#oauth-register-additional-client_configuring-internal-oauth) + +This involves creating a resource similar the following + +```yaml +kind: OAuthClient +apiVersion: oauth.openshift.io/v1 +metadata: + name: dex +# The value that should be utilized as the `client_secret` +secret: "" +# List of valid addresses for the callback. Ensure one of the values that are provided is `(dex issuer)/callback` +redirectURIs: + - "https:////callback" +grantMethod: prompt +``` + +The following is an example of a configuration for `examples/config-dev.yaml`: + +```yaml +connectors: + - type: openshift + # Required field for connector id. + id: openshift + # Required field for connector name. + name: OppenShift + config: + # OpenShift API + baseURL: https://api.mycluster.example.com:6443 + # Credentials can be string literals or pulled from the environment. + clientID: $OPENSHIFT_OAUTH_CLIENT_ID + clientSecret: $OPENSHIFT_OAUTH_CLIENT_SECRET + redirectURI: http://127.0.0.1:5556/dex/ + # Optional: Specify whether to communicate to OpenShift without validating SSL ceertificates + insecureCA: false + # Optional: The location of file containing SSL certificates to commmunicate to OpenShift + rootCA: /etc/ssl/openshift.pem + # Optional list of required groups a user mmust be a member of + groups: + - users + +``` \ No newline at end of file diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go new file mode 100644 index 00000000..69c88741 --- /dev/null +++ b/connector/openshift/openshift.go @@ -0,0 +1,252 @@ +package openshift + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/json" + "fmt" + "io/ioutil" + "net" + "net/http" + "strings" + "time" + + "github.com/dexidp/dex/connector" + "github.com/dexidp/dex/pkg/groups" + "github.com/dexidp/dex/pkg/log" + "github.com/dexidp/dex/storage/kubernetes/k8sapi" + "golang.org/x/oauth2" +) + +// Config holds configuration options for OpenShift login +type Config struct { + Issuer string `json:"issuer"` + ClientID string `json:"clientID"` + ClientSecret string `json:"clientSecret"` + RedirectURI string `json:"redirectURI"` + Groups []string `json:"groups"` + InsecureCA bool `json:"insecureCA"` + RootCA string `json:"rootCA"` +} + +var ( + _ connector.CallbackConnector = (*openshiftConnector)(nil) +) + +type openshiftConnector struct { + apiURL string + redirectURI string + clientID string + clientSecret string + cancel context.CancelFunc + logger log.Logger + httpClient *http.Client + oauth2Config *oauth2.Config + insecureCA bool + rootCA string + groups []string +} + +type user struct { + k8sapi.TypeMeta `json:",inline"` + k8sapi.ObjectMeta `json:"metadata,omitempty"` + Identities []string `json:"identities" protobuf:"bytes,3,rep,name=identities"` + FullName string `json:"fullName,omitempty" protobuf:"bytes,2,opt,name=fullName"` + Groups []string `json:"groups" protobuf:"bytes,4,rep,name=groups"` +} + +// Open returns a connector which can be used to login users through an upstream +// OpenShift OAuth2 provider. +func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, err error) { + + ctx, cancel := context.WithCancel(context.Background()) + + wellKnownURL := strings.TrimSuffix(c.Issuer, "/") + "/.well-known/oauth-authorization-server" + req, err := http.NewRequest(http.MethodGet, wellKnownURL, nil) + + openshiftConnector := openshiftConnector{ + apiURL: c.Issuer, + cancel: cancel, + clientID: c.ClientID, + clientSecret: c.ClientSecret, + insecureCA: c.InsecureCA, + logger: logger, + redirectURI: c.RedirectURI, + rootCA: c.RootCA, + groups: c.Groups, + } + + if openshiftConnector.httpClient, err = newHTTPClient(c.InsecureCA, c.RootCA); err != nil { + cancel() + return nil, fmt.Errorf("failed to create HTTP client: %v", err) + } + + var metadata struct { + Auth string `json:"authorization_endpoint"` + Token string `json:"token_endpoint"` + } + + resp, err := openshiftConnector.httpClient.Do(req.WithContext(ctx)) + + if err != nil { + cancel() + return nil, fmt.Errorf("Failed to query OpenShift Endpoint %v", err) + } + + defer resp.Body.Close() + + if err := json.NewDecoder(resp.Body).Decode(&metadata); err != nil { + cancel() + return nil, fmt.Errorf("discovery through endpoint %s failed to decode body: %v", + wellKnownURL, err) + } + + openshiftConnector.oauth2Config = &oauth2.Config{ + ClientID: c.ClientID, + ClientSecret: c.ClientSecret, + Endpoint: oauth2.Endpoint{ + AuthURL: metadata.Auth, TokenURL: metadata.Token, + }, + Scopes: []string{"user:info", "user:check-access", "user:full"}, + RedirectURL: c.RedirectURI, + } + return &openshiftConnector, nil +} + +func (c *openshiftConnector) Close() error { + c.cancel() + return nil +} + +// LoginURL returns the URL to redirect the user to login with. +func (c *openshiftConnector) LoginURL(scopes connector.Scopes, callbackURL, state string) (string, error) { + if c.redirectURI != callbackURL { + return "", fmt.Errorf("expected callback URL %q did not match the URL in the config %q", callbackURL, c.redirectURI) + } + return c.oauth2Config.AuthCodeURL(state), nil +} + +type oauth2Error struct { + error string + errorDescription string +} + +func (e *oauth2Error) Error() string { + if e.errorDescription == "" { + return e.error + } + return e.error + ": " + e.errorDescription +} + +// HandleCallback parses the request and returns the user's identity +func (c *openshiftConnector) HandleCallback(s connector.Scopes, r *http.Request) (identity connector.Identity, err error) { + q := r.URL.Query() + if errType := q.Get("error"); errType != "" { + return identity, &oauth2Error{errType, q.Get("error_description")} + } + + ctx := r.Context() + if c.httpClient != nil { + ctx = context.WithValue(r.Context(), oauth2.HTTPClient, c.httpClient) + } + + token, err := c.oauth2Config.Exchange(ctx, q.Get("code")) + if err != nil { + return identity, fmt.Errorf("oidc: failed to get token: %v", err) + } + + client := c.oauth2Config.Client(ctx, token) + + user, err := c.user(ctx, client) + + if err != nil { + return identity, fmt.Errorf("openshift: get user: %v", err) + } + + validGroups := validateRequiredGroups(user.Groups, c.groups) + + if !validGroups { + return identity, fmt.Errorf("openshift: user %q is not in any of the required teams", user.Name) + } + + identity = connector.Identity{ + UserID: user.UID, + Username: user.Name, + PreferredUsername: user.Name, + Groups: user.Groups, + } + + return identity, nil +} + +// user function returns the OpenShift user associated with the authenticated user +func (c *openshiftConnector) user(ctx context.Context, client *http.Client) (u user, err error) { + url := c.apiURL + "/apis/user.openshift.io/v1/users/~" + + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return u, fmt.Errorf("new req: %v", err) + } + + resp, err := client.Do(req.WithContext(ctx)) + if err != nil { + return u, fmt.Errorf("get URL %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return u, fmt.Errorf("read body: %v", err) + } + return u, fmt.Errorf("%s: %s", resp.Status, body) + } + + if err := json.NewDecoder(resp.Body).Decode(&u); err != nil { + return u, fmt.Errorf("JSON decode: %v", err) + } + + return u, err +} + +func validateRequiredGroups(userGroups, requiredGroups []string) bool { + + matchingGroups := groups.Filter(userGroups, requiredGroups) + + return len(requiredGroups) == len(matchingGroups) +} + +// newHTTPClient returns a new HTTP client +func newHTTPClient(insecureCA bool, rootCA string) (*http.Client, error) { + tlsConfig := tls.Config{} + + if insecureCA { + tlsConfig = tls.Config{InsecureSkipVerify: true} + } else if rootCA != "" { + tlsConfig := tls.Config{RootCAs: x509.NewCertPool()} + rootCABytes, err := ioutil.ReadFile(rootCA) + if err != nil { + return nil, fmt.Errorf("failed to read root-ca: %v", err) + } + if !tlsConfig.RootCAs.AppendCertsFromPEM(rootCABytes) { + return nil, fmt.Errorf("no certs found in root CA file %q", rootCA) + } + } + + return &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tlsConfig, + Proxy: http.ProxyFromEnvironment, + DialContext: (&net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + DualStack: true, + }).DialContext, + MaxIdleConns: 100, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + }, + }, nil +} diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go new file mode 100644 index 00000000..474686e8 --- /dev/null +++ b/connector/openshift/openshift_test.go @@ -0,0 +1,223 @@ +package openshift + +import ( + "context" + "crypto/tls" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "testing" + + "github.com/dexidp/dex/connector" + "github.com/dexidp/dex/storage/kubernetes/k8sapi" + "golang.org/x/oauth2" + + "github.com/sirupsen/logrus" +) + +func TestOpen(t *testing.T) { + + s := newTestServer(map[string]interface{}{}) + defer s.Close() + + hostURL, err := url.Parse(s.URL) + expectNil(t, err) + + _, err = http.NewRequest("GET", hostURL.String(), nil) + expectNil(t, err) + + c := Config{ + Issuer: s.URL, + ClientID: "testClientId", + ClientSecret: "testClientSecret", + RedirectURI: "https://localhost/callback", + InsecureCA: true, + } + + logger := logrus.New() + + oconfig, err := c.Open("id", logger) + + oc, ok := oconfig.(*openshiftConnector) + + expectNil(t, err) + expectEquals(t, ok, true) + expectEquals(t, oc.apiURL, s.URL) + expectEquals(t, oc.clientID, "testClientId") + expectEquals(t, oc.clientSecret, "testClientSecret") + expectEquals(t, oc.redirectURI, "https://localhost/callback") + expectEquals(t, oc.oauth2Config.Endpoint.AuthURL, fmt.Sprintf("%s/oauth/authorize", s.URL)) + expectEquals(t, oc.oauth2Config.Endpoint.TokenURL, fmt.Sprintf("%s/oauth/token", s.URL)) +} + +func TestGetUser(t *testing.T) { + + s := newTestServer(map[string]interface{}{ + "/apis/user.openshift.io/v1/users/~": user{ + ObjectMeta: k8sapi.ObjectMeta{ + Name: "jdoe", + }, + FullName: "John Doe", + Groups: []string{"users"}, + }, + }) + defer s.Close() + + hostURL, err := url.Parse(s.URL) + expectNil(t, err) + + _, err = http.NewRequest("GET", hostURL.String(), nil) + expectNil(t, err) + + h, err := newHTTPClient(true, "") + + expectNil(t, err) + + oc := openshiftConnector{apiURL: s.URL, httpClient: h} + u, err := oc.user(context.Background(), h) + + expectNil(t, err) + expectEquals(t, u.Name, "jdoe") + expectEquals(t, u.FullName, "John Doe") + expectEquals(t, len(u.Groups), 1) + +} + +func TestVerifyGroupFn(t *testing.T) { + + requiredGroups := []string{"users"} + groupMembership := []string{"users","org1"} + + validGroupMembership := validateRequiredGroups(groupMembership, requiredGroups) + + expectEquals(t, validGroupMembership, true) + +} + +func TestVerifyGroup(t *testing.T) { + + s := newTestServer(map[string]interface{}{ + "/apis/user.openshift.io/v1/users/~": user{ + ObjectMeta: k8sapi.ObjectMeta{ + Name: "jdoe", + }, + FullName: "John Doe", + Groups: []string{"users"}, + }, + }) + defer s.Close() + + hostURL, err := url.Parse(s.URL) + expectNil(t, err) + + _, err = http.NewRequest("GET", hostURL.String(), nil) + expectNil(t, err) + + h, err := newHTTPClient(true, "") + + expectNil(t, err) + + oc := openshiftConnector{apiURL: s.URL, httpClient: h} + u, err := oc.user(context.Background(), h) + + expectNil(t, err) + expectEquals(t, u.Name, "jdoe") + expectEquals(t, u.FullName, "John Doe") + expectEquals(t, len(u.Groups), 1) + +} + +func TestCallbackIdentity(t *testing.T) { + + s := newTestServer(map[string]interface{}{ + "/apis/user.openshift.io/v1/users/~": user{ + ObjectMeta: k8sapi.ObjectMeta{ + Name: "jdoe", + UID: "12345", + }, + FullName: "John Doe", + Groups: []string{"users"}, + }, + "/oauth/token": map[string]interface{}{ + "access_token": "oRzxVjCnohYRHEYEhZshkmakKmoyVoTjfUGC", + "expires_in": "30", + }, + }) + defer s.Close() + + hostURL, err := url.Parse(s.URL) + expectNil(t, err) + + req, err := http.NewRequest("GET", hostURL.String(), nil) + expectNil(t, err) + + h, err := newHTTPClient(true, "") + + expectNil(t, err) + + oc := openshiftConnector{apiURL: s.URL, httpClient: h, oauth2Config: &oauth2.Config{ + Endpoint: oauth2.Endpoint{ + AuthURL: fmt.Sprintf("%s/oauth/authorize", s.URL), + TokenURL: fmt.Sprintf("%s/oauth/token", s.URL), + }, + }} + identity, err := oc.HandleCallback(connector.Scopes{Groups: true}, req) + + expectNil(t, err) + expectEquals(t, identity.UserID, "12345") + expectEquals(t, identity.Username, "jdoe") + expectEquals(t, identity.PreferredUsername, "jdoe") + expectEquals(t, len(identity.Groups), 1) + expectEquals(t, identity.Groups[0], "users") +} + +func newTestServer(responses map[string]interface{}) *httptest.Server { + + var s *httptest.Server + s = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + + responses["/.well-known/oauth-authorization-server"] = map[string]interface{}{ + "issuer": fmt.Sprintf("%s", s.URL), + "authorization_endpoint": fmt.Sprintf("%s/oauth/authorize", s.URL), + "token_endpoint": fmt.Sprintf("%s/oauth/token", s.URL), + "scopes_supported": []string{"user:full", "user:info", "user:check-access", "user:list-scoped-projects", "user:list-projects"}, + "response_types_supported": []string{"token", "code"}, + "grant_types_supported": []string{"authorization_code", "implicit"}, + "code_challenge_methods_supported": []string{"plain", "S256"}, + } + + response := responses[r.RequestURI] + w.Header().Add("Content-Type", "application/json") + json.NewEncoder(w).Encode(response) + })) + + return s +} + +func newClient() *http.Client { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + } + return &http.Client{Transport: tr} +} + +func expectNil(t *testing.T, a interface{}) { + if a != nil { + t.Errorf("Expected %+v to equal nil", a) + } +} + +func expectNotNil(t *testing.T, a interface{}, msg string) { + if a == nil { + t.Errorf("Expected %+v to not to be nil", msg) + } +} + +func expectEquals(t *testing.T, a interface{}, b interface{}) { + if !reflect.DeepEqual(a, b) { + t.Errorf("Expected %+v to equal %+v", a, b) + } +} diff --git a/server/server.go b/server/server.go index 4dc7337d..27d93064 100644 --- a/server/server.go +++ b/server/server.go @@ -32,6 +32,7 @@ import ( "github.com/dexidp/dex/connector/microsoft" "github.com/dexidp/dex/connector/mock" "github.com/dexidp/dex/connector/oidc" + "github.com/dexidp/dex/connector/openshift" "github.com/dexidp/dex/connector/saml" "github.com/dexidp/dex/pkg/log" "github.com/dexidp/dex/storage" @@ -461,6 +462,7 @@ var ConnectorsConfig = map[string]func() ConnectorConfig{ "linkedin": func() ConnectorConfig { return new(linkedin.Config) }, "microsoft": func() ConnectorConfig { return new(microsoft.Config) }, "bitbucket-cloud": func() ConnectorConfig { return new(bitbucketcloud.Config) }, + "openshift": func() ConnectorConfig { return new(openshift.Config) }, // Keep around for backwards compatibility. "samlExperimental": func() ConnectorConfig { return new(saml.Config) }, } From 48954ca716565751a89940aaa3193ee51f10b716 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sat, 21 Dec 2019 11:48:08 -0500 Subject: [PATCH 18/29] Corrected test formatting --- connector/openshift/openshift_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index 474686e8..a4a5c27a 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -89,7 +89,7 @@ func TestGetUser(t *testing.T) { func TestVerifyGroupFn(t *testing.T) { requiredGroups := []string{"users"} - groupMembership := []string{"users","org1"} + groupMembership := []string{"users", "org1"} validGroupMembership := validateRequiredGroups(groupMembership, requiredGroups) From 5881a2cfcad79a7d741f72cacad830b90679cb22 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sun, 22 Dec 2019 00:02:08 -0500 Subject: [PATCH 19/29] Test cleanup --- connector/openshift/openshift_test.go | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index a4a5c27a..a44e5f21 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -2,7 +2,6 @@ package openshift import ( "context" - "crypto/tls" "encoding/json" "fmt" "net/http" @@ -19,7 +18,6 @@ import ( ) func TestOpen(t *testing.T) { - s := newTestServer(map[string]interface{}{}) defer s.Close() @@ -180,7 +178,7 @@ func newTestServer(responses map[string]interface{}) *httptest.Server { s = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { responses["/.well-known/oauth-authorization-server"] = map[string]interface{}{ - "issuer": fmt.Sprintf("%s", s.URL), + "issuer": s.URL, "authorization_endpoint": fmt.Sprintf("%s/oauth/authorize", s.URL), "token_endpoint": fmt.Sprintf("%s/oauth/token", s.URL), "scopes_supported": []string{"user:full", "user:info", "user:check-access", "user:list-scoped-projects", "user:list-projects"}, @@ -197,25 +195,12 @@ func newTestServer(responses map[string]interface{}) *httptest.Server { return s } -func newClient() *http.Client { - tr := &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - } - return &http.Client{Transport: tr} -} - func expectNil(t *testing.T, a interface{}) { if a != nil { t.Errorf("Expected %+v to equal nil", a) } } -func expectNotNil(t *testing.T, a interface{}, msg string) { - if a == nil { - t.Errorf("Expected %+v to not to be nil", msg) - } -} - func expectEquals(t *testing.T, a interface{}, b interface{}) { if !reflect.DeepEqual(a, b) { t.Errorf("Expected %+v to equal %+v", a, b) From db7711d72ac90b77d509d230930656e794f5ff57 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sun, 22 Dec 2019 00:32:05 -0500 Subject: [PATCH 20/29] Test cleanup --- connector/openshift/openshift_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index a44e5f21..5d489259 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -52,7 +52,6 @@ func TestOpen(t *testing.T) { } func TestGetUser(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/apis/user.openshift.io/v1/users/~": user{ ObjectMeta: k8sapi.ObjectMeta{ @@ -85,7 +84,6 @@ func TestGetUser(t *testing.T) { } func TestVerifyGroupFn(t *testing.T) { - requiredGroups := []string{"users"} groupMembership := []string{"users", "org1"} @@ -96,7 +94,6 @@ func TestVerifyGroupFn(t *testing.T) { } func TestVerifyGroup(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/apis/user.openshift.io/v1/users/~": user{ ObjectMeta: k8sapi.ObjectMeta{ @@ -129,7 +126,6 @@ func TestVerifyGroup(t *testing.T) { } func TestCallbackIdentity(t *testing.T) { - s := newTestServer(map[string]interface{}{ "/apis/user.openshift.io/v1/users/~": user{ ObjectMeta: k8sapi.ObjectMeta{ From 02c8f85e4d941b76257c299cc809482082d1f951 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sun, 22 Dec 2019 01:20:46 -0500 Subject: [PATCH 21/29] Resolved newline issues --- connector/openshift/openshift.go | 4 +--- connector/openshift/openshift_test.go | 4 ---- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go index 69c88741..a19ae531 100644 --- a/connector/openshift/openshift.go +++ b/connector/openshift/openshift.go @@ -59,7 +59,6 @@ type user struct { // Open returns a connector which can be used to login users through an upstream // OpenShift OAuth2 provider. func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, err error) { - ctx, cancel := context.WithCancel(context.Background()) wellKnownURL := strings.TrimSuffix(c.Issuer, "/") + "/.well-known/oauth-authorization-server" @@ -91,7 +90,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e if err != nil { cancel() - return nil, fmt.Errorf("Failed to query OpenShift Endpoint %v", err) + return nil, fmt.Errorf("failed to query OpenShift endpoint %v", err) } defer resp.Body.Close() @@ -211,7 +210,6 @@ func (c *openshiftConnector) user(ctx context.Context, client *http.Client) (u u } func validateRequiredGroups(userGroups, requiredGroups []string) bool { - matchingGroups := groups.Filter(userGroups, requiredGroups) return len(requiredGroups) == len(matchingGroups) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index 5d489259..407531e7 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -80,7 +80,6 @@ func TestGetUser(t *testing.T) { expectEquals(t, u.Name, "jdoe") expectEquals(t, u.FullName, "John Doe") expectEquals(t, len(u.Groups), 1) - } func TestVerifyGroupFn(t *testing.T) { @@ -90,7 +89,6 @@ func TestVerifyGroupFn(t *testing.T) { validGroupMembership := validateRequiredGroups(groupMembership, requiredGroups) expectEquals(t, validGroupMembership, true) - } func TestVerifyGroup(t *testing.T) { @@ -122,7 +120,6 @@ func TestVerifyGroup(t *testing.T) { expectEquals(t, u.Name, "jdoe") expectEquals(t, u.FullName, "John Doe") expectEquals(t, len(u.Groups), 1) - } func TestCallbackIdentity(t *testing.T) { @@ -169,7 +166,6 @@ func TestCallbackIdentity(t *testing.T) { } func newTestServer(responses map[string]interface{}) *httptest.Server { - var s *httptest.Server s = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 7e89d8ca2498f1ee7f180ec47023f2b438dfec8e Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sun, 22 Dec 2019 01:46:35 -0500 Subject: [PATCH 22/29] Resolved newline issues --- connector/openshift/openshift_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index 407531e7..89910942 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -12,9 +12,8 @@ import ( "github.com/dexidp/dex/connector" "github.com/dexidp/dex/storage/kubernetes/k8sapi" - "golang.org/x/oauth2" - "github.com/sirupsen/logrus" + "golang.org/x/oauth2" ) func TestOpen(t *testing.T) { @@ -168,7 +167,6 @@ func TestCallbackIdentity(t *testing.T) { func newTestServer(responses map[string]interface{}) *httptest.Server { var s *httptest.Server s = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - responses["/.well-known/oauth-authorization-server"] = map[string]interface{}{ "issuer": s.URL, "authorization_endpoint": fmt.Sprintf("%s/oauth/authorize", s.URL), From 075ab0938ebe89f511e32838a4a2679f2e92e0bc Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sun, 22 Dec 2019 02:53:10 -0500 Subject: [PATCH 23/29] Fixed formatting --- connector/openshift/openshift.go | 2 ++ connector/openshift/openshift_test.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go index a19ae531..f3eb53dc 100644 --- a/connector/openshift/openshift.go +++ b/connector/openshift/openshift.go @@ -15,7 +15,9 @@ import ( "github.com/dexidp/dex/connector" "github.com/dexidp/dex/pkg/groups" "github.com/dexidp/dex/pkg/log" + "github.com/dexidp/dex/storage/kubernetes/k8sapi" + "golang.org/x/oauth2" ) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index 89910942..2ed50150 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -11,7 +11,9 @@ import ( "testing" "github.com/dexidp/dex/connector" + "github.com/dexidp/dex/storage/kubernetes/k8sapi" + "github.com/sirupsen/logrus" "golang.org/x/oauth2" ) From 058e72ef509991505b3324d46b5ac4087a7271d1 Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Wed, 30 Oct 2019 03:33:52 +0400 Subject: [PATCH 24/29] Pick icons on login screen by connector type instead of ID Signed-off-by: m.nabokikh --- server/handlers.go | 7 +++---- server/templates.go | 1 + web/static/main.css | 4 ++-- web/templates/login.html | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index 0f5b0d23..0449c891 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -259,16 +259,15 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { } connectorInfos := make([]connectorInfo, len(connectors)) - i := 0 - for _, conn := range connectors { - connectorInfos[i] = connectorInfo{ + for index, conn := range connectors { + connectorInfos[index] = connectorInfo{ ID: conn.ID, Name: conn.Name, + Type: conn.Type, // TODO(ericchiang): Make this pass on r.URL.RawQuery and let something latter // on create the auth request. URL: s.absPath("/auth", conn.ID) + "?req=" + authReq.ID, } - i++ } if err := s.templates.login(r, w, connectorInfos, r.URL.Path); err != nil { diff --git a/server/templates.go b/server/templates.go index 88aeace0..37ce60a4 100644 --- a/server/templates.go +++ b/server/templates.go @@ -246,6 +246,7 @@ type connectorInfo struct { ID string Name string URL string + Type string } type byName []connectorInfo diff --git a/web/static/main.css b/web/static/main.css index 552479ad..2e6ce338 100644 --- a/web/static/main.css +++ b/web/static/main.css @@ -68,12 +68,12 @@ body { background-size: contain; } -.dex-btn-icon--bitbucket { +.dex-btn-icon--bitbucket-cloud { background-color: #205081; background-image: url(../static/img/bitbucket-icon.svg); } -.dex-btn-icon--ldap, .dex-btn-icon--tectonic-ldap { +.dex-btn-icon--ldap { background-color: #84B6EF; background-image: url(../static/img/ldap-icon.svg); } diff --git a/web/templates/login.html b/web/templates/login.html index 56151a78..f432dd00 100644 --- a/web/templates/login.html +++ b/web/templates/login.html @@ -7,7 +7,7 @@
From 5afa02644a616bf9bc0d62ec972087c5547a62f2 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Sun, 22 Dec 2019 13:39:40 -0500 Subject: [PATCH 25/29] Added OpenShift documentation to README --- Documentation/connectors/openshift.md | 8 ++++---- README.md | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Documentation/connectors/openshift.md b/Documentation/connectors/openshift.md index a33ca772..c9650846 100644 --- a/Documentation/connectors/openshift.md +++ b/Documentation/connectors/openshift.md @@ -6,7 +6,7 @@ Dex can make use of users and groups defined within OpenShift by querying the pl ## Configuration -Create a new OAuth Client by following the steps described in the documentation for [Registering Additional OAuth Clients[(https://docs.openshift.com/container-platform/latest/authentication/configuring-internal-oauth.html#oauth-register-additional-client_configuring-internal-oauth) +Create a new OAuth Client by following the steps described in the documentation for [Registering Additional OAuth Clients](https://docs.openshift.com/container-platform/latest/authentication/configuring-internal-oauth.html#oauth-register-additional-client_configuring-internal-oauth) This involves creating a resource similar the following @@ -31,10 +31,10 @@ connectors: # Required field for connector id. id: openshift # Required field for connector name. - name: OppenShift + name: OpenShift config: # OpenShift API - baseURL: https://api.mycluster.example.com:6443 + issuer: https://api.mycluster.example.com:6443 # Credentials can be string literals or pulled from the environment. clientID: $OPENSHIFT_OAUTH_CLIENT_ID clientSecret: $OPENSHIFT_OAUTH_CLIENT_SECRET @@ -47,4 +47,4 @@ connectors: groups: - users -``` \ No newline at end of file +``` diff --git a/README.md b/README.md index 7247270a..e1d34604 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,7 @@ Dex implements the following connectors: | [Microsoft](Documentation/connectors/microsoft.md) | yes | yes | no | beta | | | [AuthProxy](Documentation/connectors/authproxy.md) | no | no | no | alpha | Authentication proxies such as Apache2 mod_auth, etc. | | [Bitbucket Cloud](Documentation/connectors/bitbucketcloud.md) | yes | yes | no | alpha | | +| [OpenShift](Documentation/connectors/openshift.md) | no | yes | no | stable | | Stable, beta, and alpha are defined as: From 296659cb504d365fa47c4104c0d229b5008ea570 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Thu, 26 Dec 2019 03:14:20 -0600 Subject: [PATCH 26/29] Reduced OpenShift scopes and enhanced documentation --- Documentation/connectors/openshift.md | 35 ++++++++++++++++++++++++--- connector/openshift/openshift.go | 4 +-- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/Documentation/connectors/openshift.md b/Documentation/connectors/openshift.md index c9650846..bd3df40d 100644 --- a/Documentation/connectors/openshift.md +++ b/Documentation/connectors/openshift.md @@ -6,9 +6,37 @@ Dex can make use of users and groups defined within OpenShift by querying the pl ## Configuration -Create a new OAuth Client by following the steps described in the documentation for [Registering Additional OAuth Clients](https://docs.openshift.com/container-platform/latest/authentication/configuring-internal-oauth.html#oauth-register-additional-client_configuring-internal-oauth) -This involves creating a resource similar the following +### Creating an OAuth Client + +Two forms of OAuth Clients can be utilized: + +* [Using a Service Account as an OAuth Client](https://docs.openshift.com/container-platform/latest/authentication/using-service-accounts-as-oauth-client.html) (Recommended) +* [Registering An Additional OAuth Client](https://docs.openshift.com/container-platform/latest/authentication/configuring-internal-oauth.html#oauth-register-additional-client_configuring-internal-oauth) + +#### Using a Service Account as an OAuth Client + +OpenShift Service Accounts can be used as a constrained form of OAuth client. Making use of a Service Account to represent an OAuth Client is the recommended option as it does not require elevated privileged within the OpenShift cluster. Create a new Service Account or make use of an existing Service Account. + +Patch the Service Account to add an annotation for location of the Redirect URI + +``` +oc patch serviceaccount --type='json' -p='[{"op": "add", "path": "/metadata/annotations/serviceaccounts.openshift.io~1oauth-redirecturi.dex", "value":"https:////callback"}]' +``` + +The Client ID for a Service Account representing an OAuth Client takes the form ` + +The Client Secret for a Service Account representing an OAuth Client is the long lived OAuth Token that is configued for the Service Account. Execute the following command to retrieve the OAuth Token. + +``` +oc serviceaccounts get-token +``` + +#### Registering An Additional OAuth Client + +Instead of using a constrained form of Service Account to represent an OAuth Client, an additional OAuthClient resource can be created. + +Create a new OAuthClient resource similar to the following: ```yaml kind: OAuthClient @@ -23,6 +51,8 @@ redirectURIs: grantMethod: prompt ``` +### Dex Configuration + The following is an example of a configuration for `examples/config-dev.yaml`: ```yaml @@ -46,5 +76,4 @@ connectors: # Optional list of required groups a user mmust be a member of groups: - users - ``` diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go index f3eb53dc..e1974694 100644 --- a/connector/openshift/openshift.go +++ b/connector/openshift/openshift.go @@ -109,7 +109,7 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e Endpoint: oauth2.Endpoint{ AuthURL: metadata.Auth, TokenURL: metadata.Token, }, - Scopes: []string{"user:info", "user:check-access", "user:full"}, + Scopes: []string{"user:info"}, RedirectURL: c.RedirectURI, } return &openshiftConnector, nil @@ -168,7 +168,7 @@ func (c *openshiftConnector) HandleCallback(s connector.Scopes, r *http.Request) validGroups := validateRequiredGroups(user.Groups, c.groups) if !validGroups { - return identity, fmt.Errorf("openshift: user %q is not in any of the required teams", user.Name) + return identity, fmt.Errorf("openshift: user %q is not in any of the required groups", user.Name) } identity = connector.Identity{ From d31f6eabd467dd2b9ddb1aae184b7c67f44f5dd2 Mon Sep 17 00:00:00 2001 From: Andrew Block Date: Thu, 26 Dec 2019 20:32:12 -0600 Subject: [PATCH 27/29] Corrected logic in group verification --- connector/openshift/openshift.go | 14 ++++++++------ connector/openshift/openshift_test.go | 24 +++++++++++++++++++++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go index e1974694..6ac5d044 100644 --- a/connector/openshift/openshift.go +++ b/connector/openshift/openshift.go @@ -165,10 +165,12 @@ func (c *openshiftConnector) HandleCallback(s connector.Scopes, r *http.Request) return identity, fmt.Errorf("openshift: get user: %v", err) } - validGroups := validateRequiredGroups(user.Groups, c.groups) + if len(c.groups) > 0 { + validGroups := validateAllowedGroups(user.Groups, c.groups) - if !validGroups { - return identity, fmt.Errorf("openshift: user %q is not in any of the required groups", user.Name) + if !validGroups { + return identity, fmt.Errorf("openshift: user %q is not in any of the required groups", user.Name) + } } identity = connector.Identity{ @@ -211,10 +213,10 @@ func (c *openshiftConnector) user(ctx context.Context, client *http.Client) (u u return u, err } -func validateRequiredGroups(userGroups, requiredGroups []string) bool { - matchingGroups := groups.Filter(userGroups, requiredGroups) +func validateAllowedGroups(userGroups, allowedGroups []string) bool { + matchingGroups := groups.Filter(userGroups, allowedGroups) - return len(requiredGroups) == len(matchingGroups) + return len(matchingGroups) != 0 } // newHTTPClient returns a new HTTP client diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index 2ed50150..316af60a 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -83,11 +83,29 @@ func TestGetUser(t *testing.T) { expectEquals(t, len(u.Groups), 1) } -func TestVerifyGroupFn(t *testing.T) { - requiredGroups := []string{"users"} +func TestVerifySingleGroupFn(t *testing.T) { + allowedGroups := []string{"users"} groupMembership := []string{"users", "org1"} - validGroupMembership := validateRequiredGroups(groupMembership, requiredGroups) + validGroupMembership := validateAllowedGroups(groupMembership, allowedGroups) + + expectEquals(t, validGroupMembership, true) +} + +func TestVerifySingleGroupFailureFn(t *testing.T) { + allowedGroups := []string{"admins"} + groupMembership := []string{"users"} + + validGroupMembership := validateAllowedGroups(groupMembership, allowedGroups) + + expectEquals(t, validGroupMembership, false) +} + +func TestVerifyMultipleGroupFn(t *testing.T) { + allowedGroups := []string{"users", "admins"} + groupMembership := []string{"users", "org1"} + + validGroupMembership := validateAllowedGroups(groupMembership, allowedGroups) expectEquals(t, validGroupMembership, true) } From 98f78db915a1021c187e14a2652b27d0598ac0e7 Mon Sep 17 00:00:00 2001 From: Aiden Andrews-McDermott Date: Fri, 27 Dec 2019 18:08:17 +0000 Subject: [PATCH 28/29] Updated config.go to remove the defaulting idle connection limit of 5 which is an issue for upstream https://github.com/go-sql-driver/mysql/issues/674 --- storage/sql/config.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/storage/sql/config.go b/storage/sql/config.go index 827b0df1..f3dede4a 100644 --- a/storage/sql/config.go +++ b/storage/sql/config.go @@ -308,6 +308,13 @@ func (s *MySQL) open(logger log.Logger) (*conn, error) { return nil, err } + if s.MaxIdleConns == 0 { + /*Override default behaviour to fix https://github.com/dexidp/dex/issues/1608*/ + db.SetMaxIdleConns(0) + } else { + db.SetMaxIdleConns(s.MaxIdleConns) + } + err = db.Ping() if err != nil { if mysqlErr, ok := err.(*mysql.MySQLError); ok && mysqlErr.Number == mysqlErrUnknownSysVar { From 383c2fe8b6ea78c1b6cfde4334e1dda81b59c106 Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Sat, 28 Dec 2019 12:18:51 +0400 Subject: [PATCH 29/29] Adding oidc email scope check This helps to avoid "no email claim" error if email scope was not specified. Signed-off-by: m.nabokikh --- connector/oidc/oidc.go | 14 ++++++-- connector/oidc/oidc_test.go | 70 +++++++++++++++++++++++++++++-------- 2 files changed, 67 insertions(+), 17 deletions(-) diff --git a/connector/oidc/oidc.go b/connector/oidc/oidc.go index 2c199112..ee6b24a8 100644 --- a/connector/oidc/oidc.go +++ b/connector/oidc/oidc.go @@ -262,15 +262,25 @@ func (c *oidcConnector) createIdentity(ctx context.Context, identity connector.I if !found { return identity, fmt.Errorf("missing \"%s\" claim", userNameKey) } + + hasEmailScope := false + for _, s := range c.oauth2Config.Scopes { + if s == "email" { + hasEmailScope = true + break + } + } + email, found := claims["email"].(string) - if !found { + if !found && hasEmailScope { return identity, errors.New("missing \"email\" claim") } + emailVerified, found := claims["email_verified"].(bool) if !found { if c.insecureSkipEmailVerified { emailVerified = true - } else { + } else if hasEmailScope { return identity, errors.New("missing \"email_verified\" claim") } } diff --git a/connector/oidc/oidc_test.go b/connector/oidc/oidc_test.go index a6118b2f..52afa158 100644 --- a/connector/oidc/oidc_test.go +++ b/connector/oidc/oidc_test.go @@ -50,16 +50,19 @@ func TestHandleCallback(t *testing.T) { userIDKey string userNameKey string insecureSkipEmailVerified bool + scopes []string expectUserID string expectUserName string + expectedEmailField string token map[string]interface{} }{ { - name: "simpleCase", - userIDKey: "", // not configured - userNameKey: "", // not configured - expectUserID: "subvalue", - expectUserName: "namevalue", + name: "simpleCase", + userIDKey: "", // not configured + userNameKey: "", // not configured + expectUserID: "subvalue", + expectUserName: "namevalue", + expectedEmailField: "emailvalue", token: map[string]interface{}{ "sub": "subvalue", "name": "namevalue", @@ -72,6 +75,7 @@ func TestHandleCallback(t *testing.T) { insecureSkipEmailVerified: true, expectUserID: "subvalue", expectUserName: "namevalue", + expectedEmailField: "emailvalue", token: map[string]interface{}{ "sub": "subvalue", "name": "namevalue", @@ -79,10 +83,11 @@ func TestHandleCallback(t *testing.T) { }, }, { - name: "withUserIDKey", - userIDKey: "name", - expectUserID: "namevalue", - expectUserName: "namevalue", + name: "withUserIDKey", + userIDKey: "name", + expectUserID: "namevalue", + expectUserName: "namevalue", + expectedEmailField: "emailvalue", token: map[string]interface{}{ "sub": "subvalue", "name": "namevalue", @@ -91,10 +96,11 @@ func TestHandleCallback(t *testing.T) { }, }, { - name: "withUserNameKey", - userNameKey: "user_name", - expectUserID: "subvalue", - expectUserName: "username", + name: "withUserNameKey", + userNameKey: "user_name", + expectUserID: "subvalue", + expectUserName: "username", + expectedEmailField: "emailvalue", token: map[string]interface{}{ "sub": "subvalue", "user_name": "username", @@ -102,6 +108,33 @@ func TestHandleCallback(t *testing.T) { "email_verified": true, }, }, + { + name: "emptyEmailScope", + expectUserID: "subvalue", + expectUserName: "namevalue", + expectedEmailField: "", + scopes: []string{"groups"}, + insecureSkipEmailVerified: true, + token: map[string]interface{}{ + "sub": "subvalue", + "name": "namevalue", + "user_name": "username", + }, + }, + { + name: "emptyEmailScopeButEmailProvided", + expectUserID: "subvalue", + expectUserName: "namevalue", + expectedEmailField: "emailvalue", + scopes: []string{"groups"}, + insecureSkipEmailVerified: true, + token: map[string]interface{}{ + "sub": "subvalue", + "name": "namevalue", + "user_name": "username", + "email": "emailvalue", + }, + }, } for _, tc := range tests { @@ -111,13 +144,20 @@ func TestHandleCallback(t *testing.T) { t.Fatal("failed to setup test server", err) } defer testServer.Close() + + var scopes []string + if len(tc.scopes) > 0 { + scopes = tc.scopes + } else { + scopes = []string{"email", "groups"} + } serverURL := testServer.URL basicAuth := true config := Config{ Issuer: serverURL, ClientID: "clientID", ClientSecret: "clientSecret", - Scopes: []string{"groups"}, + Scopes: scopes, RedirectURI: fmt.Sprintf("%s/callback", serverURL), UserIDKey: tc.userIDKey, UserNameKey: tc.userNameKey, @@ -142,7 +182,7 @@ func TestHandleCallback(t *testing.T) { expectEquals(t, identity.UserID, tc.expectUserID) expectEquals(t, identity.Username, tc.expectUserName) - expectEquals(t, identity.Email, "emailvalue") + expectEquals(t, identity.Email, tc.expectedEmailField) expectEquals(t, identity.EmailVerified, true) }) }