From 4c963a3ec2954e752be8bcd1e08de850de190386 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Wed, 13 Apr 2022 16:58:58 -0600 Subject: [PATCH] Fix Okta OIDC (#11718) Using the OIDC connector with Okta would fail due to an issue in our fork of go-oidc. Update this dependency to get the fix. Additionally, clean up the logic for syncing the connector configuration, which was using a context.Context in order to implement a timeout. This can be expressed in a simpler way with time.After() --- .gitignore | 5 +++ Makefile | 2 +- constants.go | 2 + go.mod | 2 +- go.sum | 4 +- lib/auth/oidc.go | 41 +++++++------------ .../coreos/go-oidc/oidc/provider.go | 16 +++++--- vendor/modules.txt | 4 +- 8 files changed, 38 insertions(+), 38 deletions(-) diff --git a/.gitignore b/.gitignore index 22f8872e7906b..796e098659ba8 100644 --- a/.gitignore +++ b/.gitignore @@ -27,6 +27,8 @@ build _obj _test tmp +target/ +test-logs/ # Architecture specific extensions/prefixes [568vq].out @@ -63,3 +65,6 @@ ssh.config # Code signing certificate for Windows binaries /windows-signing-cert.pfx + +# build tooling +build.assets/tooling/bin/** diff --git a/Makefile b/Makefile index 3a50a1199a900..da63092613ac8 100644 --- a/Makefile +++ b/Makefile @@ -795,7 +795,7 @@ update-vendor: # delete the vendored api package. In its place # create a symlink to the the original api package rm -r vendor/github.com/gravitational/teleport/api - ln -s -r $(shell readlink -f api) vendor/github.com/gravitational/teleport + cd vendor/github.com/gravitational/teleport && ln -s ../../../../api api # update-webassets updates the minified code in the webassets repo using the latest webapps # repo and creates a PR in the teleport repo to update webassets submodule. diff --git a/constants.go b/constants.go index 5850574f81648..e8e63a7625bc1 100644 --- a/constants.go +++ b/constants.go @@ -438,6 +438,8 @@ const ( // Ping is the common backend for all Ping Identity-branded identity // providers (including PingOne, PingFederate, etc). Ping = "ping" + // Okta should be used for Okta OIDC providers. + Okta = "okta" ) const ( diff --git a/go.mod b/go.mod index 838b61a66d509..550371c9e3a4a 100644 --- a/go.mod +++ b/go.mod @@ -131,7 +131,7 @@ require ( ) replace ( - github.com/coreos/go-oidc => github.com/gravitational/go-oidc v0.0.5 + github.com/coreos/go-oidc => github.com/gravitational/go-oidc v0.0.6 github.com/gogo/protobuf => github.com/gravitational/protobuf v1.3.2-0.20201123192827-2b9fcfaffcbf github.com/gravitational/teleport/api => ./api github.com/siddontang/go-mysql v1.1.0 => github.com/gravitational/go-mysql v1.1.1-0.20210212011549-886316308a77 diff --git a/go.sum b/go.sum index 99fd37c2c53ab..012ab849cf170 100644 --- a/go.sum +++ b/go.sum @@ -357,8 +357,8 @@ github.com/gravitational/form v0.0.0-20151109031454-c4048f792f70 h1:To76nCJtM3DI github.com/gravitational/form v0.0.0-20151109031454-c4048f792f70/go.mod h1:88hFR45MpUd23d2vNWE/dYtesU50jKsbz0I9kH7UaBY= github.com/gravitational/go-mysql v1.1.1-0.20210212011549-886316308a77 h1:ivambM2XeST8qfxeSm+0Y8CP/DlNbS3o/9tSF2KtGFk= github.com/gravitational/go-mysql v1.1.1-0.20210212011549-886316308a77/go.mod h1:re0JQZ1Cy5dVlIDGq0YksfDIla/GRZlxqOoC0XPSSGE= -github.com/gravitational/go-oidc v0.0.5 h1:kxsCknoOZ+KqIAoYLLdHuQcvcc+SrQlnT7xxIM8oo6o= -github.com/gravitational/go-oidc v0.0.5/go.mod h1:SevmOUNdOB0aD9BAIgjptZ6oHkKxMZZgA70nwPfgU/w= +github.com/gravitational/go-oidc v0.0.6 h1:DCllahGYxDAvxWsq8UILgO+/i1EheQRxcNzS+D+wP5I= +github.com/gravitational/go-oidc v0.0.6/go.mod h1:SevmOUNdOB0aD9BAIgjptZ6oHkKxMZZgA70nwPfgU/w= github.com/gravitational/kingpin v2.1.11-0.20190130013101-742f2714c145+incompatible h1:CfyZl3nyo9K5lLqOmqvl9/IElY1UCnOWKZiQxJ8HKdA= github.com/gravitational/kingpin v2.1.11-0.20190130013101-742f2714c145+incompatible/go.mod h1:LWxG30M3FcrjhOn3T4zz7JmBoQJ45MWZmOXgy9Ganoc= github.com/gravitational/license v0.0.0-20210218173955-6d8fb49b117a h1:PN5vAN1ZA0zqdpM6wNdx6+bkdlQ5fImd75oaIHSbOhY= diff --git a/lib/auth/oidc.go b/lib/auth/oidc.go index 3a4feee8a33b4..4bbb4a90597d3 100644 --- a/lib/auth/oidc.go +++ b/lib/auth/oidc.go @@ -23,6 +23,7 @@ import ( "io/ioutil" "net/http" "net/url" + "time" "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/constants" @@ -80,36 +81,22 @@ func (a *Server) createOIDCClient(conn types.OIDCConnector) (*oidc.Client, error return nil, trace.Wrap(err) } - ctx, cancel := context.WithTimeout(context.Background(), defaults.WebHeadersTimeout) - defer cancel() - + doneSyncing := make(chan struct{}) go func() { - defer cancel() + defer close(doneSyncing) client.SyncProviderConfig(conn.GetIssuerURL()) }() select { - case <-ctx.Done(): + case <-doneSyncing: + case <-time.After(defaults.WebHeadersTimeout): + return nil, trace.ConnectionProblem(nil, + "timed out syncing oidc connector %v, ensure URL %q is valid and accessible and check configuration", + conn.GetName(), conn.GetIssuerURL()) case <-a.closeCtx.Done(): return nil, trace.ConnectionProblem(nil, "auth server is shutting down") } - // Canceled is expected in case if sync provider config finishes faster - // than the deadline - if ctx.Err() != nil && ctx.Err() != context.Canceled { - var err error - if ctx.Err() == context.DeadlineExceeded { - err = trace.ConnectionProblem(err, - "failed to reach out to oidc connector %v, most likely URL %q is not valid or not accessible, check configuration and try to re-create the connector", - conn.GetName(), conn.GetIssuerURL()) - } else { - err = trace.ConnectionProblem(err, - "unknown problem with connector %v, most likely URL %q is not valid or not accessible, check configuration and try to re-create the connector", - conn.GetName(), conn.GetIssuerURL()) - } - return nil, err - } - a.lock.Lock() defer a.lock.Unlock() @@ -914,20 +901,20 @@ func (a *Server) getClaims(oidcClient *oidc.Client, connector types.OIDCConnecto // getOAuthClient returns a Oauth2 client from the oidc.Client. If the connector is set as a Ping provider sets the Client Secret Post auth method func (a *Server) getOAuthClient(oidcClient *oidc.Client, connector types.OIDCConnector) (*oauth2.Client, error) { - oac, err := oidcClient.OAuthClient() if err != nil { return nil, trace.Wrap(err) } - //If the default client secret basic is used the Ping OIDC - // will throw an error of multiple client credentials. Even if you set in Ping - // to use Client Secret Post it will return to use client secret basic. - // Issue https://github.com/gravitational/teleport/issues/8374 - if connector.GetProvider() == teleport.Ping { + // For OIDC, Ping and Okta will throw an error when the + // default client secret basic method is used. + // See: https://github.com/gravitational/teleport/issues/8374 + switch connector.GetProvider() { + case teleport.Ping, teleport.Okta: oac.SetAuthMethod(oauth2.AuthMethodClientSecretPost) } + return oac, err } diff --git a/vendor/github.com/coreos/go-oidc/oidc/provider.go b/vendor/github.com/coreos/go-oidc/oidc/provider.go index 1906ba0ca1561..ce751a6a9437d 100644 --- a/vendor/github.com/coreos/go-oidc/oidc/provider.go +++ b/vendor/github.com/coreos/go-oidc/oidc/provider.go @@ -4,7 +4,7 @@ import ( "encoding/json" "errors" "fmt" - "github.com/gravitational/trace" + "io" "io/ioutil" "log" "net/http" @@ -13,6 +13,8 @@ import ( "sync" "time" + "github.com/gravitational/trace" + "github.com/coreos/pkg/timeutil" "github.com/jonboulle/clockwork" @@ -355,9 +357,6 @@ func (p ProviderConfig) Valid() error { if !contains(p.IDTokenSigningAlgValues, "RS256") { return errors.New("id_token_signing_alg_values_supported must include 'RS256'") } - if contains(p.TokenEndpointAuthMethodsSupported, "none") { - return errors.New("token_endpoint_auth_signing_alg_values_supported cannot include 'none'") - } uris := []struct { val *url.URL @@ -649,11 +648,18 @@ func (r *httpProviderConfigGetter) Get() (cfg ProviderConfig, err error) { } defer resp.Body.Close() - data, err := ioutil.ReadAll(resp.Body) + const MaxDataSize = 1024 * 1024 + data, err := io.ReadAll(io.LimitReader(resp.Body, MaxDataSize+1)) if err != nil { return cfg, trace.Wrap(err) } + if len(data) > MaxDataSize { + //discard rest of the body to free up connection + io.Copy(ioutil.Discard, resp.Body) + return cfg, trace.Errorf("response exceeds maximum size of %d bytes", MaxDataSize) + } + if err = json.Unmarshal(data, &cfg); err != nil { return cfg, trace.Wrap(err, "failed to decode provider response %q", string(data)) } diff --git a/vendor/modules.txt b/vendor/modules.txt index 6a357ee4d0957..37eec9475f56a 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -208,7 +208,7 @@ github.com/boombuler/barcode/qr github.com/boombuler/barcode/utils # github.com/cespare/xxhash/v2 v2.1.1 github.com/cespare/xxhash/v2 -# github.com/coreos/go-oidc v0.0.4 => github.com/gravitational/go-oidc v0.0.5 +# github.com/coreos/go-oidc v0.0.4 => github.com/gravitational/go-oidc v0.0.6 ## explicit github.com/coreos/go-oidc/http github.com/coreos/go-oidc/jose @@ -1175,6 +1175,6 @@ k8s.io/utils/integer sigs.k8s.io/structured-merge-diff/v4/value # sigs.k8s.io/yaml v1.2.0 sigs.k8s.io/yaml -# github.com/coreos/go-oidc => github.com/gravitational/go-oidc v0.0.5 +# github.com/coreos/go-oidc => github.com/gravitational/go-oidc v0.0.6 # github.com/gogo/protobuf => github.com/gravitational/protobuf v1.3.2-0.20201123192827-2b9fcfaffcbf # github.com/gravitational/teleport/api => ./api