From eb945304338eafbe523bb0b836cb5c52149c8b35 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 20 Jul 2022 17:45:43 +0200 Subject: [PATCH] Add option to configure access token verification Allow to switch jwt access token verification and off. Many (most?) IDP provide JWT encoded access tokens. If ocis is configure to assume jwt access tokens (access_token_verify_method==jwt) we now properly verify the tokens signature and a set of standard claims ("exp", "iat" and nbf" by way of the jwt module's standard verification and "iss" explicitliy). This change also allows for introduction of other access token verification mechanism in the future (e.g. through introspection (RFC7662). --- services/proxy/pkg/command/server.go | 1 + services/proxy/pkg/config/config.go | 14 ++- .../pkg/config/defaults/defaultconfig.go | 1 + services/proxy/pkg/config/parser/parse.go | 10 ++ .../proxy/pkg/middleware/authentication.go | 1 + services/proxy/pkg/middleware/oidc_auth.go | 109 ++++++++++++------ .../proxy/pkg/middleware/oidc_auth_test.go | 2 + services/proxy/pkg/middleware/options.go | 10 ++ 8 files changed, 112 insertions(+), 36 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 88a6ed76ff..c25400801d 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -189,6 +189,7 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) middleware.HTTPClient(oidcHTTPClient), middleware.TokenCacheSize(cfg.OIDC.UserinfoCache.Size), middleware.TokenCacheTTL(time.Second*time.Duration(cfg.OIDC.UserinfoCache.TTL)), + middleware.AccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), // basic Options middleware.Logger(logger), diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 60a881657a..06ca1df8ef 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -80,12 +80,20 @@ type AuthMiddleware struct { CredentialsByUserAgent map[string]string `yaml:"credentials_by_user_agent"` } +const ( + AccessTokenVerificationNone = "none" + AccessTokenVerificationJWT = "jwt" + // tdb: + // AccessTokenVerificationIntrospect = "introspect" +) + // OIDC is the config for the OpenID-Connect middleware. If set the proxy will try to authenticate every request // with the configured oidc-provider type OIDC struct { - Issuer string `yaml:"issuer" env:"OCIS_URL;OCIS_OIDC_ISSUER;PROXY_OIDC_ISSUER" desc:"URL of the OIDC issuer. It defaults to URL of the builtin IDP."` - Insecure bool `yaml:"insecure" env:"OCIS_INSECURE;PROXY_OIDC_INSECURE" desc:"Disable TLS certificate validation for connections to the IDP. Note that this is not recommended for production environments."` - UserinfoCache UserinfoCache `yaml:"user_info_cache"` + Issuer string `yaml:"issuer" env:"OCIS_URL;OCIS_OIDC_ISSUER;PROXY_OIDC_ISSUER" desc:"URL of the OIDC issuer. It defaults to URL of the builtin IDP."` + Insecure bool `yaml:"insecure" env:"OCIS_INSECURE;PROXY_OIDC_INSECURE" desc:"Disable TLS certificate validation for connections to the IDP. Note that this is not recommended for production environments."` + AccessTokenVerifyMethod string `yaml:"access_token_verify_method" env:"PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD" desc:"Sets how OIDC access tokens should be verified. Possible values: 'none', which means that no special validation apart from using it for accessing the IPD's userinfo endpoint will be done. Or 'jwt', which tries to parse the access token as a jwt token and verifies the signature using the keys published on the IDP's 'jwks_uri'."` + UserinfoCache UserinfoCache `yaml:"user_info_cache"` } // UserinfoCache is a TTL cache configuration. diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index 708fef8eff..62ad229800 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -36,6 +36,7 @@ func DefaultConfig() *config.Config { Issuer: "https://localhost:9200", Insecure: true, //Insecure: true, + AccessTokenVerifyMethod: config.AccessTokenVerificationJWT, UserinfoCache: config.UserinfoCache{ Size: 1024, TTL: 10, diff --git a/services/proxy/pkg/config/parser/parse.go b/services/proxy/pkg/config/parser/parse.go index 14218d9e8e..877f95ee9f 100644 --- a/services/proxy/pkg/config/parser/parse.go +++ b/services/proxy/pkg/config/parser/parse.go @@ -2,6 +2,7 @@ package parser import ( "errors" + "fmt" ociscfg "github.com/owncloud/ocis/v2/ocis-pkg/config" "github.com/owncloud/ocis/v2/ocis-pkg/shared" @@ -41,5 +42,14 @@ func Validate(cfg *config.Config) error { return shared.MissingMachineAuthApiKeyError(cfg.Service.Name) } + if cfg.OIDC.AccessTokenVerifyMethod != config.AccessTokenVerificationNone && + cfg.OIDC.AccessTokenVerifyMethod != config.AccessTokenVerificationJWT { + return fmt.Errorf( + "Invalid value '%s' for 'access_token_verify_method' in service %s. Possible values are: '%s' or '%s'.", + cfg.OIDC.AccessTokenVerifyMethod, cfg.Service.Name, + config.AccessTokenVerificationJWT, config.AccessTokenVerificationNone, + ) + } + return nil } diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index f8476a67bc..1a06157cd6 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -115,6 +115,7 @@ func newOIDCAuth(options Options) func(http.Handler) http.Handler { TokenCacheSize(options.UserinfoCacheSize), TokenCacheTTL(options.UserinfoCacheTTL), CredentialsByUserAgent(options.CredentialsByUserAgent), + AccessTokenVerifyMethod(options.AccessTokenVerifyMethod), ) } diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index fbb13ec65b..bc4ba99ebd 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -3,6 +3,7 @@ package middleware import ( "context" "encoding/json" + "errors" "io/ioutil" "net/http" "strings" @@ -14,6 +15,7 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/log" "github.com/owncloud/ocis/v2/ocis-pkg/oidc" "github.com/owncloud/ocis/v2/ocis-pkg/sync" + "github.com/owncloud/ocis/v2/services/proxy/pkg/config" "golang.org/x/oauth2" ) @@ -28,12 +30,13 @@ func OIDCAuth(optionSetters ...Option) func(next http.Handler) http.Handler { tokenCache := sync.NewCache(options.UserinfoCacheSize) h := oidcAuth{ - logger: options.Logger, - providerFunc: options.OIDCProviderFunc, - httpClient: options.HTTPClient, - oidcIss: options.OIDCIss, - tokenCache: &tokenCache, - tokenCacheTTL: options.UserinfoCacheTTL, + logger: options.Logger, + providerFunc: options.OIDCProviderFunc, + httpClient: options.HTTPClient, + oidcIss: options.OIDCIss, + tokenCache: &tokenCache, + tokenCacheTTL: options.UserinfoCacheTTL, + accessTokenVerifyMethod: options.AccessTokenVerifyMethod, } return func(next http.Handler) http.Handler { @@ -51,6 +54,12 @@ func OIDCAuth(optionSetters ...Option) func(next http.Handler) http.Handler { return } + // Force init of jwks keyfunc if needed (contacts the .well-known and jwks endpoints on first call) + if h.accessTokenVerifyMethod == config.AccessTokenVerificationJWT && h.getKeyfunc() == nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + token := strings.TrimPrefix(req.Header.Get("Authorization"), "Bearer ") claims, status := h.getClaims(token, req) @@ -66,19 +75,27 @@ func OIDCAuth(optionSetters ...Option) func(next http.Handler) http.Handler { } type oidcAuth struct { - logger log.Logger - provider OIDCProvider - jwks *keyfunc.JWKS - providerFunc func() (OIDCProvider, error) - httpClient *http.Client - oidcIss string - tokenCache *sync.Cache - tokenCacheTTL time.Duration + logger log.Logger + provider OIDCProvider + jwks *keyfunc.JWKS + providerFunc func() (OIDCProvider, error) + httpClient *http.Client + oidcIss string + tokenCache *sync.Cache + tokenCacheTTL time.Duration + accessTokenVerifyMethod string } func (m oidcAuth) getClaims(token string, req *http.Request) (claims map[string]interface{}, status int) { hit := m.tokenCache.Load(token) if hit == nil { + aClaims, err := m.verifyAccessToken(token) + if err != nil { + m.logger.Error().Err(err).Msg("Failed to verify access token") + status = http.StatusUnauthorized + return + } + oauth2Token := &oauth2.Token{ AccessToken: token, } @@ -99,7 +116,7 @@ func (m oidcAuth) getClaims(token string, req *http.Request) (claims map[string] return } - expiration := m.extractExpiration(token) + expiration := m.extractExpiration(aClaims) m.tokenCache.Store(token, claims, expiration) m.logger.Debug().Interface("claims", claims).Interface("userInfo", userInfo).Time("expiration", expiration.UTC()).Msg("unmarshalled and cached userinfo") @@ -115,28 +132,52 @@ func (m oidcAuth) getClaims(token string, req *http.Request) (claims map[string] return } -// extractExpiration tries to extract the expriration time from the access token -// It tries so by parsing (and verifying the signature) the access_token as JWT. -// If it is a valid JWT the `exp` claim will be used that the token expiration time. -// If it is not a valid JWT we fallback to the configured cache TTL. -// This could still be enhanced by trying a to use the introspection endpoint (RFC7662), -// to validate the token. If it exists. -func (m oidcAuth) extractExpiration(token string) time.Time { - defaultExpiration := time.Now().Add(m.tokenCacheTTL) +func (m oidcAuth) verifyAccessToken(token string) (jwt.RegisteredClaims, error) { + switch m.accessTokenVerifyMethod { + case config.AccessTokenVerificationJWT: + return m.verifyAccessTokenJWT(token) + case config.AccessTokenVerificationNone: + m.logger.Debug().Msg("Access Token verification disabled") + return jwt.RegisteredClaims{}, nil + default: + m.logger.Error().Str("access_token_verify_method", m.accessTokenVerifyMethod).Msg("Unknown Access Token verification setting") + return jwt.RegisteredClaims{}, errors.New("Unknown Access Token Verification method") + } +} + +// verifyAccessTokenJWT tries to parse and verify the access token as a JWT. +func (m oidcAuth) verifyAccessTokenJWT(token string) (jwt.RegisteredClaims, error) { + var claims jwt.RegisteredClaims jwks := m.getKeyfunc() if jwks == nil { - return defaultExpiration + return claims, errors.New("Error initializing jwks keyfunc") } - claims := jwt.RegisteredClaims{} _, err := jwt.ParseWithClaims(token, &claims, jwks.Keyfunc) + m.logger.Debug().Interface("access token", &claims).Msg("parsed access token") if err != nil { - m.logger.Info().Err(err).Msg("Error parsing access_token as JWT") - return defaultExpiration + m.logger.Info().Err(err).Msg("Failed to parse/verify the access token.") + return claims, err } - if claims.ExpiresAt != nil { - m.logger.Debug().Str("exp", claims.ExpiresAt.String()).Msg("Expiration Time from access_token") - return claims.ExpiresAt.Time + + if !claims.VerifyIssuer(m.oidcIss, true) { + vErr := jwt.ValidationError{} + vErr.Inner = jwt.ErrTokenInvalidIssuer + vErr.Errors |= jwt.ValidationErrorIssuer + return claims, vErr + } + + return claims, nil +} + +// extractExpiration tries to extract the expriration time from the access token +// If the access token does not have an exp claim it will fallback to the configured +// default expiration +func (m oidcAuth) extractExpiration(aClaims jwt.RegisteredClaims) time.Time { + defaultExpiration := time.Now().Add(m.tokenCacheTTL) + if aClaims.ExpiresAt != nil { + m.logger.Debug().Str("exp", aClaims.ExpiresAt.String()).Msg("Expiration Time from access_token") + return aClaims.ExpiresAt.Time } return defaultExpiration } @@ -166,8 +207,10 @@ type jwksJSON struct { func (m *oidcAuth) getKeyfunc() *keyfunc.JWKS { if m.jwks == nil { wellKnown := strings.TrimSuffix(m.oidcIss, "/") + "/.well-known/openid-configuration" + resp, err := m.httpClient.Get(wellKnown) if err != nil { + m.logger.Error().Err(err).Msg("Failed to set request for .well-known/openid-configuration") return nil } defer resp.Body.Close() @@ -184,15 +227,15 @@ func (m *oidcAuth) getKeyfunc() *keyfunc.JWKS { } var j jwksJSON - err = json. - Unmarshal(body, &j) + err = json.Unmarshal(body, &j) if err != nil { - m.logger.Error().Err(err).Msg("failed to decode provider discovered openid-configuration") + m.logger.Error().Err(err).Msg("failed to decode provider openid-configuration") return nil } m.logger.Debug().Str("jwks", j.JWKSURL).Msg("discovered jwks endpoint") // FIXME: make configurable options := keyfunc.Options{ + Client: m.httpClient, RefreshErrorHandler: func(err error) { m.logger.Error().Err(err).Msg("There was an error with the jwt.Keyfunc") }, diff --git a/services/proxy/pkg/middleware/oidc_auth_test.go b/services/proxy/pkg/middleware/oidc_auth_test.go index 33045101be..6b7e97bceb 100644 --- a/services/proxy/pkg/middleware/oidc_auth_test.go +++ b/services/proxy/pkg/middleware/oidc_auth_test.go @@ -9,6 +9,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/owncloud/ocis/v2/ocis-pkg/log" + "github.com/owncloud/ocis/v2/services/proxy/pkg/config" "golang.org/x/oauth2" ) @@ -21,6 +22,7 @@ func TestOIDCAuthMiddleware(t *testing.T) { return mockOP(false), nil }), OIDCIss("https://localhost:9200"), + AccessTokenVerifyMethod(config.AccessTokenVerificationNone), )(next) r := httptest.NewRequest(http.MethodGet, "https://idp.example.com", nil) diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index 63994ee8e4..d963e50b24 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -55,6 +55,9 @@ type Options struct { UserinfoCacheTTL time.Duration // CredentialsByUserAgent sets the auth challenges on a per user-agent basis CredentialsByUserAgent map[string]string + // AccessTokenVerifyMethod configures how access_tokens should be verified but the oidc_auth middleware. + // Possible values currently: "jwt" and "none" + AccessTokenVerifyMethod string } // newOptions initializes the available default options. @@ -193,3 +196,10 @@ func UserProvider(up backend.UserBackend) Option { o.UserProvider = up } } + +// AccessTokenVerifyMethod set the mechanism for access token verification +func AccessTokenVerifyMethod(method string) Option { + return func(o *Options) { + o.AccessTokenVerifyMethod = method + } +}