From 58dce9bed8e52c0eb4c64dfa3cef791bc537fc83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 12 Apr 2023 12:00:22 +0200 Subject: [PATCH] use our oidc client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- ocis-pkg/oidc/client.go | 96 ++++++++++++---------- ocis-pkg/oidc/options.go | 10 +++ services/proxy/pkg/command/server.go | 18 ++-- services/proxy/pkg/middleware/oidc_auth.go | 40 +-------- services/proxy/pkg/middleware/options.go | 9 +- 5 files changed, 75 insertions(+), 98 deletions(-) diff --git a/ocis-pkg/oidc/client.go b/ocis-pkg/oidc/client.go index deb1daeda..eb3b2c61e 100644 --- a/ocis-pkg/oidc/client.go +++ b/ocis-pkg/oidc/client.go @@ -9,6 +9,7 @@ import ( "mime" "net/http" "strings" + "sync" gOidc "github.com/coreos/go-oidc/v3/oidc" "github.com/owncloud/ocis/v2/ocis-pkg/log" @@ -37,7 +38,8 @@ type KeySet interface { type oidcClient struct { issuer string - provider ProviderMetadata + provider *ProviderMetadata + providerLock *sync.Mutex skipIssuerValidation bool remoteKeySet KeySet algorithms []string @@ -67,52 +69,56 @@ func NewOIDCClient(opts ...Option) OIDCProvider { options := newOptions(opts...) return &oidcClient{ - Logger: options.Logger, - issuer: options.OidcIssuer, + Logger: options.Logger, + issuer: options.OidcIssuer, + client: options.HTTPClient, + providerLock: &sync.Mutex{}, } } func (c *oidcClient) lookupWellKnownOpenidConfiguration(ctx context.Context) error { - - wellKnown := strings.TrimSuffix(c.issuer, "/") + "/.well-known/openid-configuration" - req, err := http.NewRequest("GET", wellKnown, nil) - if err != nil { - return err - } - resp, err := c.client.Do(req.WithContext(ctx)) - if err != nil { - return err - } - defer resp.Body.Close() - - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("unable to read response body: %v", err) - } - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("%s: %s", resp.Status, body) - } - - var p ProviderMetadata - err = unmarshalResp(resp, body, &p) - if err != nil { - return fmt.Errorf("oidc: failed to decode provider discovery object: %v", err) - } - - if !c.skipIssuerValidation && p.Issuer != c.issuer { - return fmt.Errorf("oidc: issuer did not match the issuer returned by provider, expected %q got %q", c.issuer, p.Issuer) - } - var algs []string - for _, a := range p.IDTokenSigningAlgValuesSupported { - if supportedAlgorithms[a] { - algs = append(algs, a) + c.providerLock.Lock() + defer c.providerLock.Unlock() + if c.provider == nil { + wellKnown := strings.TrimSuffix(c.issuer, "/") + "/.well-known/openid-configuration" + req, err := http.NewRequest("GET", wellKnown, nil) + if err != nil { + return err } - } - c.provider = p - c.algorithms = algs - c.remoteKeySet = gOidc.NewRemoteKeySet(ctx, p.JwksURI) + resp, err := c.client.Do(req.WithContext(ctx)) + if err != nil { + return err + } + defer resp.Body.Close() + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("unable to read response body: %v", err) + } + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("%s: %s", resp.Status, body) + } + + var p ProviderMetadata + err = unmarshalResp(resp, body, &p) + if err != nil { + return fmt.Errorf("oidc: failed to decode provider discovery object: %v", err) + } + + if !c.skipIssuerValidation && p.Issuer != c.issuer { + return fmt.Errorf("oidc: issuer did not match the issuer returned by provider, expected %q got %q", c.issuer, p.Issuer) + } + var algs []string + for _, a := range p.IDTokenSigningAlgValuesSupported { + if supportedAlgorithms[a] { + algs = append(algs, a) + } + } + c.provider = &p + c.algorithms = algs + c.remoteKeySet = gOidc.NewRemoteKeySet(ctx, p.JwksURI) + } return nil } @@ -159,11 +165,11 @@ func (u *UserInfo) Claims(v interface{}) error { } func (c *oidcClient) UserInfo(ctx context.Context, tokenSource oauth2.TokenSource) (*UserInfo, error) { + if err := c.lookupWellKnownOpenidConfiguration(ctx); err != nil { + return nil, err + } + if c.provider.UserinfoEndpoint == "" { - // try lazy initialization TODO use sync.once - if err := c.lookupWellKnownOpenidConfiguration(ctx); err != nil { - return nil, err - } if c.provider.UserinfoEndpoint == "" { return nil, errors.New("oidc: user info endpoint is not supported by this provider") } diff --git a/ocis-pkg/oidc/options.go b/ocis-pkg/oidc/options.go index d7876d096..1f7cd64cc 100644 --- a/ocis-pkg/oidc/options.go +++ b/ocis-pkg/oidc/options.go @@ -1,6 +1,8 @@ package oidc import ( + "net/http" + "github.com/owncloud/ocis/v2/ocis-pkg/log" ) @@ -9,6 +11,8 @@ type Option func(o *Options) // Options defines the available options for this package. type Options struct { + // HTTPClient to use for requests + HTTPClient *http.Client // Logger to use for logging, must be set Logger log.Logger // The OpenID Connect Issuer URL @@ -39,3 +43,9 @@ func WithLogger(val log.Logger) Option { o.Logger = val } } + +func WithHTTPClient(val *http.Client) Option { + return func(o *Options) { + o.HTTPClient = val + } +} diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index e45b824dd..cc3ea1082 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -7,8 +7,6 @@ import ( "net/http" "time" - "github.com/coreos/go-oidc/v3/oidc" - "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/v2/pkg/token/manager/jwt" "github.com/go-chi/chi/v5" @@ -19,6 +17,7 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/config/configlog" "github.com/owncloud/ocis/v2/ocis-pkg/log" pkgmiddleware "github.com/owncloud/ocis/v2/ocis-pkg/middleware" + "github.com/owncloud/ocis/v2/ocis-pkg/oidc" "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" "github.com/owncloud/ocis/v2/ocis-pkg/store" "github.com/owncloud/ocis/v2/ocis-pkg/version" @@ -39,7 +38,6 @@ import ( "github.com/owncloud/ocis/v2/services/proxy/pkg/userroles" "github.com/urfave/cli/v2" microstore "go-micro.dev/v4/store" - "golang.org/x/oauth2" ) type LogoutHandler struct { @@ -295,15 +293,11 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config, middleware.OIDCIss(cfg.OIDC.Issuer), middleware.JWKSOptions(cfg.OIDC.JWKS), middleware.AccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), - middleware.OIDCProviderFunc(func() (middleware.OIDCProvider, error) { - // Initialize a provider by specifying the issuer URL. - // it will fetch the keys from the issuer using the .well-known - // endpoint - return oidc.NewProvider( - context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient), - cfg.OIDC.Issuer, - ) - }), + middleware.OIDCClient(oidc.NewOIDCClient( + oidc.WithLogger(logger), + oidc.WithHTTPClient(oidcHTTPClient), + oidc.WithOidcIssuer(cfg.OIDC.Issuer), + )), )) authenticators = append(authenticators, middleware.PublicShareAuthenticator{ Logger: logger, diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 0b23590c1..a33d24cdb 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -14,7 +14,6 @@ import ( "github.com/owncloud/ocis/v2/services/proxy/pkg/config" "github.com/MicahParks/keyfunc" - gOidc "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt/v4" "github.com/pkg/errors" "github.com/shamaton/msgpack/v2" @@ -28,11 +27,6 @@ const ( _bearerPrefix = "Bearer " ) -// OIDCProvider used to mock the oidc provider during tests -type OIDCProvider interface { - UserInfo(ctx context.Context, ts oauth2.TokenSource) (*gOidc.UserInfo, error) -} - // NewOIDCAuthenticator returns a ready to use authenticator which can handle OIDC authentication. func NewOIDCAuthenticator(opts ...Option) *OIDCAuthenticator { options := newOptions(opts...) @@ -44,10 +38,9 @@ func NewOIDCAuthenticator(opts ...Option) *OIDCAuthenticator { DefaultTokenCacheTTL: options.DefaultAccessTokenTTL, HTTPClient: options.HTTPClient, OIDCIss: options.OIDCIss, - ProviderFunc: options.OIDCProviderFunc, + oidcClient: options.OIDCClient, JWKSOptions: options.JWKS, AccessTokenVerifyMethod: options.AccessTokenVerifyMethod, - providerLock: &sync.Mutex{}, jwksLock: &sync.Mutex{}, } } @@ -60,13 +53,10 @@ type OIDCAuthenticator struct { userInfoCache store.Store sessionLookupCache store.Store DefaultTokenCacheTTL time.Duration - ProviderFunc func() (OIDCProvider, error) + oidcClient oidc.OIDCProvider AccessTokenVerifyMethod string JWKSOptions config.JWKS - providerLock *sync.Mutex - provider OIDCProvider - jwksLock *sync.Mutex JWKS *keyfunc.JWKS } @@ -108,7 +98,7 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri AccessToken: token, } - userInfo, err := m.getProvider().UserInfo( + userInfo, err := m.oidcClient.UserInfo( context.WithValue(req.Context(), oauth2.HTTPClient, m.HTTPClient), oauth2.StaticTokenSource(oauth2Token), ) @@ -252,26 +242,6 @@ func (m *OIDCAuthenticator) getKeyfunc() *keyfunc.JWKS { return m.JWKS } -func (m *OIDCAuthenticator) getProvider() OIDCProvider { - m.providerLock.Lock() - defer m.providerLock.Unlock() - if m.provider == nil { - // Lazily initialize a provider - - // provider needs to be cached as when it is created - // it will fetch the keys from the issuer using the .well-known - // endpoint - provider, err := m.ProviderFunc() - if err != nil { - m.Logger.Error().Err(err).Msg("could not initialize oidcAuth provider") - return nil - } - - m.provider = provider - } - return m.provider -} - // Authenticate implements the authenticator interface to authenticate requests via oidc auth. func (m *OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { // there is no bearer token on the request, @@ -282,10 +252,6 @@ func (m *OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) return nil, false } - if m.getProvider() == nil { - return nil, false - } - // Force init of jwks keyfunc if needed (contacts the .well-known and jwks endpoints on first call) if m.AccessTokenVerifyMethod == config.AccessTokenVerificationJWT && m.getKeyfunc() == nil { return nil, false diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index 1e9d05739..bc030f04e 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -6,6 +6,7 @@ import ( gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" "github.com/owncloud/ocis/v2/ocis-pkg/log" + "github.com/owncloud/ocis/v2/ocis-pkg/oidc" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" storesvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/store/v0" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" @@ -34,7 +35,7 @@ type Options struct { // SettingsRoleService for the roles API in settings SettingsRoleService settingssvc.RoleService // OIDCProviderFunc to lazily initialize an oidc provider, must be set for the oidc_auth middleware - OIDCProviderFunc func() (OIDCProvider, error) + OIDCClient oidc.OIDCProvider // OIDCIss is the oidcAuth-issuer OIDCIss string // RevaGatewayClient to send requests to the reva gateway @@ -113,10 +114,10 @@ func SettingsRoleService(rc settingssvc.RoleService) Option { } } -// OIDCProviderFunc provides a function to set the the oidc provider function option. -func OIDCProviderFunc(f func() (OIDCProvider, error)) Option { +// OIDCClient provides a function to set the the oidc client option. +func OIDCClient(val oidc.OIDCProvider) Option { return func(o *Options) { - o.OIDCProviderFunc = f + o.OIDCClient = val } }