From 59a803b6e127ec215fe768a5b1c488f28237f44c Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Wed, 1 Nov 2017 10:13:14 -0400 Subject: [PATCH] Introduce inactivity timeout tracking for access tokens And a thread to handle delayed updates to reduce writes Signed-off-by: Simo Sorce --- pkg/auth/oauth/handlers/authenticator.go | 7 + pkg/auth/oauth/registry/registry_test.go | 206 +++++++++++++++- pkg/auth/oauth/registry/timeoutvalidator.go | 222 ++++++++++++++++++ pkg/cmd/server/api/types.go | 13 + pkg/cmd/server/api/v1/types.go | 13 + pkg/cmd/server/api/validation/oauth.go | 10 + pkg/cmd/server/origin/authenticator.go | 13 +- pkg/cmd/server/start/master_args.go | 5 +- pkg/oauth/apis/oauth/validation/validation.go | 50 +++- .../apis/oauth/validation/validation_test.go | 74 +++++- pkg/oauth/apiserver/auth.go | 6 +- .../osinserver/registrystorage/storage.go | 18 +- test/integration/oauthstorage_test.go | 2 +- 13 files changed, 627 insertions(+), 12 deletions(-) create mode 100644 pkg/auth/oauth/registry/timeoutvalidator.go diff --git a/pkg/auth/oauth/handlers/authenticator.go b/pkg/auth/oauth/handlers/authenticator.go index b93ad4a3fa01..b496d1489ea2 100644 --- a/pkg/auth/oauth/handlers/authenticator.go +++ b/pkg/auth/oauth/handlers/authenticator.go @@ -32,6 +32,13 @@ type TokenMaxAgeSeconds interface { GetTokenMaxAgeSeconds() *int32 } +type TokenTimeoutSeconds interface { + // GetAccessTokenInactivityTimeoutSeconds returns the inactivity timeout + // for the token in seconds. 0 means no timeout. + // nil means to use the default expiration. + GetAccessTokenInactivityTimeoutSeconds() *int32 +} + // HandleAuthorize implements osinserver.AuthorizeHandler to ensure the AuthorizeRequest is authenticated. // If the request is authenticated, UserData and Authorized are set and false is returned. // If the request is not authenticated, the auth handler is called and the request is not authorized diff --git a/pkg/auth/oauth/registry/registry_test.go b/pkg/auth/oauth/registry/registry_test.go index 4d22451407a8..c95fdee546bc 100644 --- a/pkg/auth/oauth/registry/registry_test.go +++ b/pkg/auth/oauth/registry/registry_test.go @@ -12,7 +12,9 @@ import ( apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" clienttesting "k8s.io/client-go/testing" @@ -21,6 +23,7 @@ import ( "github.com/openshift/origin/pkg/auth/userregistry/identitymapper" oapi "github.com/openshift/origin/pkg/oauth/apis/oauth" oauthfake "github.com/openshift/origin/pkg/oauth/generated/internalclientset/fake" + oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset/typed/oauth/internalversion" "github.com/openshift/origin/pkg/oauth/server/osinserver" "github.com/openshift/origin/pkg/oauth/server/osinserver/registrystorage" userapi "github.com/openshift/origin/pkg/user/apis/user" @@ -287,7 +290,7 @@ func TestRegistryAndServer(t *testing.T) { objs = append(objs, testCase.ClientAuth) } fakeOAuthClient := oauthfake.NewSimpleClientset(objs...) - storage := registrystorage.New(fakeOAuthClient.Oauth().OAuthAccessTokens(), fakeOAuthClient.Oauth().OAuthAuthorizeTokens(), fakeOAuthClient.Oauth().OAuthClients(), NewUserConversion()) + storage := registrystorage.New(fakeOAuthClient.Oauth().OAuthAccessTokens(), fakeOAuthClient.Oauth().OAuthAuthorizeTokens(), fakeOAuthClient.Oauth().OAuthClients(), NewUserConversion(), 0) config := osinserver.NewDefaultServerConfig() h.AuthorizeHandler = osinserver.AuthorizeHandlers{ @@ -473,3 +476,204 @@ func TestAuthenticateTokenValidated(t *testing.T) { t.Error("Did not get a user!") } } + +type fakeOAuthClientLister struct { + clients oauthclient.OAuthClientInterface +} + +func (f fakeOAuthClientLister) Get(name string) (*oapi.OAuthClient, error) { + return f.clients.Get(name, metav1.GetOptions{}) +} + +func (f fakeOAuthClientLister) List(selector labels.Selector) ([]*oapi.OAuthClient, error) { + var list []*oapi.OAuthClient + ret, _ := f.clients.List(metav1.ListOptions{}) + for i := range ret.Items { + list = append(list, &ret.Items[i]) + } + return list, nil +} + +func checkToken(t *testing.T, name string, authf authenticator.Token, tokens oauthclient.OAuthAccessTokenInterface, present bool) { + userInfo, found, err := authf.AuthenticateToken(name) + if present { + if !found { + t.Errorf("Did not find token %s!", name) + } + if err != nil { + t.Errorf("Unexpected error checking for token %s: %v", name, err) + } + if userInfo == nil { + t.Errorf("Did not get a user for token %s!", name) + } + } else { + if found { + token, _ := tokens.Get(name, metav1.GetOptions{}) + t.Errorf("Found token (created=%s, timeout=%di, now=%s), but it should be gone!", + token.CreationTimestamp, token.InactivityTimeoutSeconds, time.Now()) + } + if err != errTimedout { + t.Errorf("Unexpected error checking absence of token %s: %v", name, err) + } + if userInfo != nil { + t.Errorf("Unexpected user checking absence of token %s: %v", name, userInfo) + } + } +} + +func TestAuthenticateTokenTimeout(t *testing.T) { + stopCh := make(chan struct{}) + defer close(stopCh) + + defaultTimeout := int32(30) // 30 seconds + clientTimeout := int32(15) // 15 seconds so flush -> 5 seconds + minTimeout := int32(10) // 10 seconds + + testClient := oapi.OAuthClient{ + ObjectMeta: metav1.ObjectMeta{Name: "testClient"}, + AccessTokenInactivityTimeoutSeconds: &clientTimeout, + } + quickClient := oapi.OAuthClient{ + ObjectMeta: metav1.ObjectMeta{Name: "quickClient"}, + AccessTokenInactivityTimeoutSeconds: &minTimeout, + } + slowClient := oapi.OAuthClient{ + ObjectMeta: metav1.ObjectMeta{Name: "slowClient"}, + } + testToken := oapi.OAuthAccessToken{ + ObjectMeta: metav1.ObjectMeta{Name: "testToken", CreationTimestamp: metav1.Time{Time: time.Now()}}, + ClientName: "testClient", + ExpiresIn: 600, // 10 minutes + UserName: "foo", + UserUID: string("bar"), + InactivityTimeoutSeconds: clientTimeout, + } + quickToken := oapi.OAuthAccessToken{ + ObjectMeta: metav1.ObjectMeta{Name: "quickToken", CreationTimestamp: metav1.Time{Time: time.Now()}}, + ClientName: "quickClient", + ExpiresIn: 600, // 10 minutes + UserName: "foo", + UserUID: string("bar"), + InactivityTimeoutSeconds: minTimeout, + } + slowToken := oapi.OAuthAccessToken{ + ObjectMeta: metav1.ObjectMeta{Name: "slowToken", CreationTimestamp: metav1.Time{Time: time.Now()}}, + ClientName: "slowClient", + ExpiresIn: 600, // 10 minutes + UserName: "foo", + UserUID: string("bar"), + InactivityTimeoutSeconds: defaultTimeout, + } + emergToken := oapi.OAuthAccessToken{ + ObjectMeta: metav1.ObjectMeta{Name: "emergToken", CreationTimestamp: metav1.Time{Time: time.Now()}}, + ClientName: "quickClient", + ExpiresIn: 600, // 10 minutes + UserName: "foo", + UserUID: string("bar"), + InactivityTimeoutSeconds: 5, // super short timeout + } + fakeOAuthClient := oauthfake.NewSimpleClientset(&testToken, &quickToken, &slowToken, &emergToken, &testClient, &quickClient, &slowClient) + userRegistry := usertest.NewUserRegistry() + userRegistry.GetUsers["foo"] = &userapi.User{ObjectMeta: metav1.ObjectMeta{UID: "bar"}} + accessTokenGetter := fakeOAuthClient.Oauth().OAuthAccessTokens() + oauthClients := fakeOAuthClient.Oauth().OAuthClients() + lister := &fakeOAuthClientLister{ + clients: oauthClients, + } + timeouts := NewTimeoutValidator(accessTokenGetter, lister, defaultTimeout, minTimeout) + tokenAuthenticator := NewTokenAuthenticator(accessTokenGetter, userRegistry, identitymapper.NoopGroupMapper{}, timeouts) + + go timeouts.Run(stopCh) + + // wait to see that the other thread has updated the timeouts + time.Sleep(1 * time.Second) + + // TIME: 1 seconds have passed here + + // first time should succeed for all + checkToken(t, "testToken", tokenAuthenticator, accessTokenGetter, true) + checkToken(t, "quickToken", tokenAuthenticator, accessTokenGetter, true) + checkToken(t, "slowToken", tokenAuthenticator, accessTokenGetter, true) + // this should cause an emergency flush, if not the next auth will fail, + // as the token will be timed out + checkToken(t, "emergToken", tokenAuthenticator, accessTokenGetter, true) + + // wait 5 seconds + time.Sleep(5 * time.Second) + + // TIME: 6th second + + // See if emergency flush happened + checkToken(t, "emergToken", tokenAuthenticator, accessTokenGetter, true) + + // wait for timeout (minTimeout + 1 - the previously waited 6 seconds) + time.Sleep(time.Duration(minTimeout-5) * time.Second) + + // TIME: 11th second + + // now we change the testClient and see if the testToken will still be + // valid instead of timing out + changeClient, ret := oauthClients.Get("testClient", metav1.GetOptions{}) + if ret != nil { + t.Error("Failed to get testClient") + } else { + longTimeout := int32(20) + changeClient.AccessTokenInactivityTimeoutSeconds = &longTimeout + _, ret = oauthClients.Update(changeClient) + if ret != nil { + t.Error("Failed to update testClient") + } + } + + // this should fail + checkToken(t, "quickToken", tokenAuthenticator, accessTokenGetter, false) + // while this should get updated + checkToken(t, "testToken", tokenAuthenticator, accessTokenGetter, true) + + // wait for timeout + time.Sleep(time.Duration(clientTimeout+1) * time.Second) + + // TIME: 27th second + + // this should get updated + checkToken(t, "slowToken", tokenAuthenticator, accessTokenGetter, true) + + // while this should not fail + checkToken(t, "testToken", tokenAuthenticator, accessTokenGetter, true) + // and should be updated to last at least till the 31st second + token, err := accessTokenGetter.Get("testToken", metav1.GetOptions{}) + if err != nil { + t.Error("Failed to get testToken") + } else { + if token.InactivityTimeoutSeconds < 31 { + t.Errorf("Expected timeout in more than 31 seconds, found: %d", token.InactivityTimeoutSeconds) + } + } + + //now change testClient again, so that tokens do not expire anymore + changeclient, ret := oauthClients.Get("testClient", metav1.GetOptions{}) + if ret != nil { + t.Error("Failed to get testClient") + } else { + changeclient.AccessTokenInactivityTimeoutSeconds = new(int32) + _, ret = oauthClients.Update(changeclient) + if ret != nil { + t.Error("Failed to update testClient") + } + } + + // and wait until test token should time out, and has been flushed for sure + time.Sleep(time.Duration(minTimeout) * time.Second) + + // while this should not fail + checkToken(t, "testToken", tokenAuthenticator, accessTokenGetter, true) + // and should be updated to have a ZERO timeout + token, err = accessTokenGetter.Get("testToken", metav1.GetOptions{}) + if err != nil { + t.Error("Failed to get testToken") + } else { + if token.InactivityTimeoutSeconds != 0 { + t.Errorf("Expected timeout of 0 seconds, found: %d", token.InactivityTimeoutSeconds) + } + } +} diff --git a/pkg/auth/oauth/registry/timeoutvalidator.go b/pkg/auth/oauth/registry/timeoutvalidator.go new file mode 100644 index 000000000000..57b4b2a78b96 --- /dev/null +++ b/pkg/auth/oauth/registry/timeoutvalidator.go @@ -0,0 +1,222 @@ +package registry + +import ( + "errors" + "time" + + "github.com/golang/glog" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/runtime" + + "github.com/openshift/origin/pkg/oauth/apis/oauth" + oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset/typed/oauth/internalversion" + oauthclientlister "github.com/openshift/origin/pkg/oauth/generated/listers/oauth/internalversion" + "github.com/openshift/origin/pkg/user/apis/user" + "github.com/openshift/origin/pkg/util/rankedset" +) + +var errTimedout = errors.New("token timed out") + +// Implements rankedset.Item +var _ = rankedset.Item(&tokenData{}) + +type tokenData struct { + token *oauth.OAuthAccessToken + seen time.Time +} + +func (a *tokenData) timeout() time.Time { + return a.token.CreationTimestamp.Time.Add(time.Duration(a.token.InactivityTimeoutSeconds) * time.Second) +} + +func (a *tokenData) Key() string { + return a.token.Name +} + +func (a *tokenData) Rank() int64 { + return a.timeout().Unix() +} + +func timeoutAsDuration(timeout int32) time.Duration { + return time.Duration(timeout) * time.Second +} + +type TimeoutValidator struct { + oauthClients oauthclientlister.OAuthClientLister + tokens oauthclient.OAuthAccessTokenInterface + tokenChannel chan *tokenData + data *rankedset.RankedSet + defaultTimeout time.Duration + tickerInterval time.Duration +} + +func NewTimeoutValidator(tokens oauthclient.OAuthAccessTokenInterface, oauthClients oauthclientlister.OAuthClientLister, defaultTimeout int32, minValidTimeout int32) *TimeoutValidator { + a := &TimeoutValidator{ + oauthClients: oauthClients, + tokens: tokens, + tokenChannel: make(chan *tokenData), + data: rankedset.New(), + defaultTimeout: timeoutAsDuration(defaultTimeout), + tickerInterval: timeoutAsDuration(minValidTimeout / 3), // we tick at least 3 times within each timeout period + } + glog.V(5).Infof("Token Timeout Validator primed with defaultTimeout=%s tickerInterval=%s", a.defaultTimeout, a.tickerInterval) + return a +} + +// Validate is called with a token when it is seen by an authenticator +// it touches only the tokenChannel so it is safe to call from other threads +func (a *TimeoutValidator) Validate(token *oauth.OAuthAccessToken, _ *user.User) error { + if token.InactivityTimeoutSeconds == 0 { + // We care only if the token was created with a timeout to start with + return nil + } + + td := &tokenData{ + token: token, + seen: time.Now(), + } + if td.timeout().Before(td.seen) { + return errTimedout + } + + if token.ExpiresIn != 0 && token.ExpiresIn <= int64(token.InactivityTimeoutSeconds) { + // skip if the timeout is already larger than expiration deadline + return nil + } + // After a positive timeout check we need to update the timeout and + // schedule an update so that we can either set or update the Timeout + // we do that launching a micro goroutine to avoid blocking + go a.putToken(td) + + return nil +} + +func (a *TimeoutValidator) putToken(td *tokenData) { + a.tokenChannel <- td +} + +func (a *TimeoutValidator) clientTimeout(name string) time.Duration { + oauthClient, err := a.oauthClients.Get(name) + if err != nil { + glog.V(5).Infof("Failed to fetch OAuthClient %q for timeout value: %v", name, err) + return a.defaultTimeout + } + if oauthClient.AccessTokenInactivityTimeoutSeconds == nil { + return a.defaultTimeout + } + return timeoutAsDuration(*oauthClient.AccessTokenInactivityTimeoutSeconds) +} + +func (a *TimeoutValidator) update(td *tokenData) error { + // Obtain the timeout interval for this client + delta := a.clientTimeout(td.token.ClientName) + // if delta is 0 it means the OAuthClient has been changed to the + // no-timeout value. In this case we set newTimeout also to 0 so + // that the token will no longer timeout once updated. + newTimeout := int32(0) + if delta > 0 { + // InactivityTimeoutSeconds is the number of seconds since creation: + // InactivityTimeoutSeconds = Seen(Time) - CreationTimestamp(Time) + delta(Duration) + newTimeout = int32((td.seen.Sub(td.token.CreationTimestamp.Time) + delta) / time.Second) + } + // We need to get the token again here because it may have changed in the + // DB and we need to verify it is still worth updating + token, err := a.tokens.Get(td.token.Name, v1.GetOptions{}) + if err != nil { + return err + } + if newTimeout != 0 && token.InactivityTimeoutSeconds >= newTimeout { + // if the token was already updated with a higher or equal timeout we + // do not have anything to do + return nil + } + token.InactivityTimeoutSeconds = newTimeout + _, err = a.tokens.Update(token) + return err +} + +func (a *TimeoutValidator) flush(flushHorizon time.Time) { + // flush all tokens that are about to expire before the flushHorizon. + // Typically the flushHorizon is set to a time slightly past the next + // ticker interval, so that not token ends up timing out between flushes + glog.V(5).Infof("Flushing tokens timing out before %s", flushHorizon) + + // grab all tokens that need to be update in this flush interval + // and remove them from the stored data, they either flush now or never + tokenList := a.data.LessThan(flushHorizon.Unix(), true) + + var retryList []*tokenData + flushedTokens := 0 + + for _, item := range tokenList { + td := item.(*tokenData) + err := a.update(td) + // not logging the full errors here as it would leak the token. + switch { + case err == nil: + flushedTokens++ + case apierrors.IsConflict(err) || apierrors.IsServerTimeout(err): + glog.V(5).Infof("Token update deferred for token belonging to %s", + td.token.UserName) + retryList = append(retryList, td) + default: + glog.V(5).Infof("Token timeout for user=%q client=%q scopes=%v was not updated", + td.token.UserName, td.token.ClientName, td.token.Scopes) + } + } + + // we try once more and if it still fails we stop trying here and defer + // to a future regular update if the token is used again + for _, td := range retryList { + err := a.update(td) + if err != nil { + glog.V(5).Infof("Token timeout for user=%q client=%q scopes=%v was not updated", + td.token.UserName, td.token.ClientName, td.token.Scopes) + } else { + flushedTokens++ + } + } + + glog.V(5).Infof("Successfully flushed %d tokens out of %d", + flushedTokens, len(tokenList)) +} + +func (a *TimeoutValidator) nextTick() time.Time { + // Add a small safety Margin so flushes tend to + // overlap a little rather than have gaps + return time.Now().Add(a.tickerInterval + 10*time.Second) +} + +func (a *TimeoutValidator) Run(stopCh <-chan struct{}) { + defer runtime.HandleCrash() + glog.V(5).Infof("Started Token Timeout Flush Handling thread!") + + ticker := time.NewTicker(a.tickerInterval) + // make sure to kill the ticker when we exit + defer ticker.Stop() + + nextTick := a.nextTick() + + for { + select { + case <-stopCh: + // if channel closes terminate + return + case td := <-a.tokenChannel: + a.data.Insert(td) + // if this token is going to time out before the timer, flush now + tokenTimeout := td.timeout() + if tokenTimeout.Before(nextTick) { + glog.V(5).Infof("Timeout for user=%q client=%q scopes=%v falls before next ticker (%s < %s), forcing flush!", + td.token.UserName, td.token.ClientName, td.token.Scopes, tokenTimeout, nextTick) + a.flush(nextTick) + } + + case <-ticker.C: + nextTick = a.nextTick() + a.flush(nextTick) + } + } +} diff --git a/pkg/cmd/server/api/types.go b/pkg/cmd/server/api/types.go index 950ec6396884..0b4f1977b9e8 100644 --- a/pkg/cmd/server/api/types.go +++ b/pkg/cmd/server/api/types.go @@ -948,6 +948,19 @@ type TokenConfig struct { AuthorizeTokenMaxAgeSeconds int32 // AccessTokenMaxAgeSeconds defines the maximum age of access tokens AccessTokenMaxAgeSeconds int32 + // AccessTokenInactivityTimeoutSeconds defined the default token + // inactivity timeout for tokens granted by any client. + // Setting it to nil means the feature is completely disabled (default) + // The default setting can be overriden on OAuthClient basis. + // The value represents the maximum amount of time that can occur between + // consecutive uses of the token. Tokens become invalid if they are not + // used within this temporal window. The user will need to acquire a new + // token to regain access once a token times out. + // Valid values are: + // - 0: Tokens never time out + // - X: Tokens time out if there is no activity for X seconds + // The current minimum allowed value for X is 300 (5 minutes) + AccessTokenInactivityTimeoutSeconds *int32 } // SessionConfig specifies options for cookie-based sessions. Used by AuthRequestHandlerSession diff --git a/pkg/cmd/server/api/v1/types.go b/pkg/cmd/server/api/v1/types.go index a0ed8c23d0b4..8909f394db32 100644 --- a/pkg/cmd/server/api/v1/types.go +++ b/pkg/cmd/server/api/v1/types.go @@ -839,6 +839,19 @@ type TokenConfig struct { AuthorizeTokenMaxAgeSeconds int32 `json:"authorizeTokenMaxAgeSeconds"` // AccessTokenMaxAgeSeconds defines the maximum age of access tokens AccessTokenMaxAgeSeconds int32 `json:"accessTokenMaxAgeSeconds"` + // AccessTokenInactivityTimeoutSeconds defined the default token + // inactivity timeout for tokens granted by any client. + // Setting it to nil means the feature is completely disabled (default) + // The default setting can be overriden on OAuthClient basis. + // The value represents the maximum amount of time that can occur between + // consecutive uses of the token. Tokens become invalid if they are not + // used within this temporal window. The user will need to acquire a new + // token to regain access once a token times out. + // Valid values are: + // - 0: Tokens never time out + // - X: Tokens time out if there is no activity for X seconds + // The current minimum allowed value for X is 300 (5 minutes) + AccessTokenInactivityTimeoutSeconds *int32 `json:"accessTokenInactivityTimeoutSeconds,omitempty"` } // SessionConfig specifies options for cookie-based sessions. Used by AuthRequestHandlerSession diff --git a/pkg/cmd/server/api/validation/oauth.go b/pkg/cmd/server/api/validation/oauth.go index 445cd68ea950..24f0756cfa3f 100644 --- a/pkg/cmd/server/api/validation/oauth.go +++ b/pkg/cmd/server/api/validation/oauth.go @@ -17,6 +17,7 @@ import ( "github.com/openshift/origin/pkg/auth/userregistry/identitymapper" "github.com/openshift/origin/pkg/cmd/server/api" "github.com/openshift/origin/pkg/cmd/server/api/latest" + oauthvalidation "github.com/openshift/origin/pkg/oauth/apis/oauth/validation" "github.com/openshift/origin/pkg/user/apis/user/validation" ) @@ -102,6 +103,15 @@ func ValidateOAuthConfig(config *api.OAuthConfig, fldPath *field.Path) Validatio ))) } + if timeout := config.TokenConfig.AccessTokenInactivityTimeoutSeconds; timeout != nil { + if *timeout != 0 && *timeout < oauthvalidation.MinimumInactivityTimeoutSeconds { + validationResults.AddErrors(field.Invalid( + fldPath.Child("tokenConfig", "accessTokenInactivityTimeoutSeconds"), *timeout, + fmt.Sprintf("The minimum acceptable token timeout value is %d seconds", + oauthvalidation.MinimumInactivityTimeoutSeconds))) + } + } + if config.Templates != nil { if len(config.Templates.Login) > 0 { content, err := ioutil.ReadFile(config.Templates.Login) diff --git a/pkg/cmd/server/origin/authenticator.go b/pkg/cmd/server/origin/authenticator.go index 5e3e230db0be..80dcbbb21751 100644 --- a/pkg/cmd/server/origin/authenticator.go +++ b/pkg/cmd/server/origin/authenticator.go @@ -27,7 +27,9 @@ import ( "github.com/openshift/origin/pkg/auth/userregistry/identitymapper" configapi "github.com/openshift/origin/pkg/cmd/server/api" "github.com/openshift/origin/pkg/cmd/server/bootstrappolicy" + oauthvalidation "github.com/openshift/origin/pkg/oauth/apis/oauth/validation" oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset/typed/oauth/internalversion" + oauthclientlister "github.com/openshift/origin/pkg/oauth/generated/listers/oauth/internalversion" usercache "github.com/openshift/origin/pkg/user/cache" userclient "github.com/openshift/origin/pkg/user/generated/internalclientset" usertypedclient "github.com/openshift/origin/pkg/user/generated/internalclientset/typed/user/internalversion" @@ -63,6 +65,7 @@ func NewAuthenticator( return newAuthenticator( options, oauthClient.OAuthAccessTokens(), + informers.GetOauthInformers().Oauth().InternalVersion().OAuthClients().Lister(), serviceAccountTokenGetter, userClient.User().Users(), apiClientCAs, @@ -70,7 +73,7 @@ func NewAuthenticator( ) } -func newAuthenticator(config configapi.MasterConfig, accessTokenGetter oauthclient.OAuthAccessTokenInterface, tokenGetter serviceaccount.ServiceAccountTokenGetter, userGetter usertypedclient.UserResourceInterface, apiClientCAs *x509.CertPool, groupMapper identitymapper.UserToGroupMapper) (authenticator.Request, map[string]genericapiserver.PostStartHookFunc, error) { +func newAuthenticator(config configapi.MasterConfig, accessTokenGetter oauthclient.OAuthAccessTokenInterface, oauthClientLister oauthclientlister.OAuthClientLister, tokenGetter serviceaccount.ServiceAccountTokenGetter, userGetter usertypedclient.UserResourceInterface, apiClientCAs *x509.CertPool, groupMapper identitymapper.UserToGroupMapper) (authenticator.Request, map[string]genericapiserver.PostStartHookFunc, error) { postStartHooks := map[string]genericapiserver.PostStartHookFunc{} authenticators := []authenticator.Request{} tokenAuthenticators := []authenticator.Token{} @@ -92,6 +95,14 @@ func newAuthenticator(config configapi.MasterConfig, accessTokenGetter oauthclie // OAuth token if config.OAuthConfig != nil { validators := []openshiftauthenticator.OAuthTokenValidator{authnregistry.NewExpirationValidator(), authnregistry.NewUIDValidator()} + if inactivityTimeout := config.OAuthConfig.TokenConfig.AccessTokenInactivityTimeoutSeconds; inactivityTimeout != nil { + timeoutValidator := authnregistry.NewTimeoutValidator(accessTokenGetter, oauthClientLister, *inactivityTimeout, oauthvalidation.MinimumInactivityTimeoutSeconds) + validators = append(validators, timeoutValidator) + postStartHooks["openshift.io-TokenTimeoutUpdater"] = func(context genericapiserver.PostStartHookContext) error { + go timeoutValidator.Run(context.StopCh) + return nil + } + } oauthTokenAuthenticator := authnregistry.NewTokenAuthenticator(accessTokenGetter, userGetter, groupMapper, validators...) tokenAuthenticators = append(tokenAuthenticators, // if you have a bearer token, you're a human (usually) diff --git a/pkg/cmd/server/start/master_args.go b/pkg/cmd/server/start/master_args.go index 199bbfd176f6..9a292d39c5d7 100644 --- a/pkg/cmd/server/start/master_args.go +++ b/pkg/cmd/server/start/master_args.go @@ -413,8 +413,9 @@ func (args MasterArgs) BuildSerializeableOAuthConfig() (*configapi.OAuthConfig, }, TokenConfig: configapi.TokenConfig{ - AuthorizeTokenMaxAgeSeconds: 5 * 60, // 5 minutes - AccessTokenMaxAgeSeconds: 24 * 60 * 60, // 1 day + AuthorizeTokenMaxAgeSeconds: 5 * 60, // 5 minutes + AccessTokenMaxAgeSeconds: 24 * 60 * 60, // 1 day + AccessTokenInactivityTimeoutSeconds: nil, // no timeouts by default }, } diff --git a/pkg/oauth/apis/oauth/validation/validation.go b/pkg/oauth/apis/oauth/validation/validation.go index 80e6afca18ef..f241d9a595c2 100644 --- a/pkg/oauth/apis/oauth/validation/validation.go +++ b/pkg/oauth/apis/oauth/validation/validation.go @@ -16,7 +16,14 @@ import ( uservalidation "github.com/openshift/origin/pkg/user/apis/user/validation" ) -const MinTokenLength = 32 +const ( + MinTokenLength = 32 + // MinimumInactivityTimeoutSeconds defines the the smallest value allowed + // for AccessTokenInactivityTimeoutSeconds. + // It also defines the ticker interval for the token update routine as + // MinimumInactivityTimeoutSeconds / 3 is used there. + MinimumInactivityTimeoutSeconds = 5 * 60 +) // PKCE [RFC7636] code challenge methods supported // https://tools.ietf.org/html/rfc7636#section-4.3 @@ -70,6 +77,11 @@ func ValidateAccessToken(accessToken *oauthapi.OAuthAccessToken) field.ErrorList if len(accessToken.UserUID) == 0 { allErrs = append(allErrs, field.Required(field.NewPath("userUID"), "")) } + // negative values are not allowed + if accessToken.InactivityTimeoutSeconds < 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("inactivityTimeoutSeconds"), + accessToken.InactivityTimeoutSeconds, "cannot be a negative value")) + } if ok, msg := ValidateRedirectURI(accessToken.RedirectURI); !ok { allErrs = append(allErrs, field.Invalid(field.NewPath("redirectURI"), accessToken.RedirectURI, msg)) } @@ -79,8 +91,29 @@ func ValidateAccessToken(accessToken *oauthapi.OAuthAccessToken) field.ErrorList func ValidateAccessTokenUpdate(newToken, oldToken *oauthapi.OAuthAccessToken) field.ErrorList { allErrs := validation.ValidateObjectMetaUpdate(&newToken.ObjectMeta, &oldToken.ObjectMeta, field.NewPath("metadata")) + // since InactivityTimeoutSeconds can be concurrently updated by multipe masters we do + // not allow it to decrease in value, as that would cause "updated" tokens + // to timeout earlier than they should. 0 is the exception, as this value + // indicates that the token does not expire, and it is an allowed new + // value. + if newToken.InactivityTimeoutSeconds > 0 && newToken.InactivityTimeoutSeconds < oldToken.InactivityTimeoutSeconds { + allErrs = append(allErrs, field.Invalid(field.NewPath("inactivityTimeoutSeconds"), newToken.InactivityTimeoutSeconds, + fmt.Sprintf("cannot be less than the current value=%d", oldToken.InactivityTimeoutSeconds))) + } + // negative values are not allowed either + if newToken.InactivityTimeoutSeconds < 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("inactivityTimeoutSeconds"), newToken.InactivityTimeoutSeconds, + "cannot be a negative value")) + } + // we do not allow tokens to turn into timing out tokens after issuance + if oldToken.InactivityTimeoutSeconds == 0 && newToken.InactivityTimeoutSeconds != 0 { + allErrs = append(allErrs, field.Invalid(field.NewPath("inactivityTimeoutSeconds"), newToken.InactivityTimeoutSeconds, + "cannot update non-timing-out token")) + } copied := *oldToken copied.ObjectMeta = newToken.ObjectMeta + // allow only InactivityTimeoutSeconds to be changed + copied.InactivityTimeoutSeconds = newToken.InactivityTimeoutSeconds return append(allErrs, validation.ValidateImmutableField(newToken, &copied, field.NewPath(""))...) } @@ -139,6 +172,21 @@ func ValidateClient(client *oauthapi.OAuthClient) field.ErrorList { allErrs = append(allErrs, ValidateScopeRestriction(restriction, field.NewPath("scopeRestrictions").Index(i))...) } + if client.AccessTokenInactivityTimeoutSeconds != nil { + timeout := *client.AccessTokenInactivityTimeoutSeconds + if timeout > 0 && timeout < MinimumInactivityTimeoutSeconds { + allErrs = append(allErrs, field.Invalid( + field.NewPath("accessTokenInactivityTimeoutSeconds"), + client.AccessTokenInactivityTimeoutSeconds, + fmt.Sprintf("The minimum valid timeout value is %d seconds", MinimumInactivityTimeoutSeconds))) + } + if timeout < 0 { + allErrs = append(allErrs, field.Invalid( + field.NewPath("accessTokenInactivityTimeoutSeconds"), + client.AccessTokenInactivityTimeoutSeconds, "value cannot be negative")) + } + } + return allErrs } diff --git a/pkg/oauth/apis/oauth/validation/validation_test.go b/pkg/oauth/apis/oauth/validation/validation_test.go index 0ce4b52548f4..00fcdc4d958d 100644 --- a/pkg/oauth/apis/oauth/validation/validation_test.go +++ b/pkg/oauth/apis/oauth/validation/validation_test.go @@ -179,6 +179,9 @@ func TestValidateClient(t *testing.T) { t.Errorf("expected success: %v", errs) } + var badTimeout int32 = MinimumInactivityTimeoutSeconds - 1 + var negTimeout int32 = -1 + errorCases := map[string]struct { Client oapi.OAuthClient T field.ErrorType @@ -226,6 +229,22 @@ func TestValidateClient(t *testing.T) { T: field.ErrorTypeRequired, F: "scopeRestrictions[0].clusterRole.namespaces", }, + "minimum timeout value": { + Client: oapi.OAuthClient{ + ObjectMeta: metav1.ObjectMeta{Name: "client-name"}, + AccessTokenInactivityTimeoutSeconds: &badTimeout, + }, + T: field.ErrorTypeInvalid, + F: "accessTokenInactivityTimeoutSeconds", + }, + "negative timeout value": { + Client: oapi.OAuthClient{ + ObjectMeta: metav1.ObjectMeta{Name: "client-name"}, + AccessTokenInactivityTimeoutSeconds: &negTimeout, + }, + T: field.ErrorTypeInvalid, + F: "accessTokenInactivityTimeoutSeconds", + }, } for k, v := range errorCases { errs := ValidateClient(&v.Client) @@ -301,6 +320,17 @@ func TestValidateAccessTokens(t *testing.T) { T: field.ErrorTypeInvalid, F: "scopes[0]", }, + "negative timeout": { + Token: oapi.OAuthAccessToken{ + ObjectMeta: metav1.ObjectMeta{Name: "accessTokenNameWithMinimumLength"}, + ClientName: "myclient", + UserName: "myusername", + UserUID: "myuseruid", + InactivityTimeoutSeconds: -1, + }, + T: field.ErrorTypeInvalid, + F: "inactivityTimeoutSeconds", + }, } for k, v := range errorCases { errs := ValidateAccessToken(&v.Token) @@ -435,15 +465,27 @@ func TestValidateAuthorizeTokens(t *testing.T) { func TestValidateAccessTokensUpdate(t *testing.T) { valid := &oapi.OAuthAccessToken{ - ObjectMeta: metav1.ObjectMeta{Name: "accessTokenNameWithMinimumLength", ResourceVersion: "1"}, - ClientName: "myclient", - UserName: "myusername", - UserUID: "myuseruid", + ObjectMeta: metav1.ObjectMeta{Name: "accessTokenNameWithMinimumLength", ResourceVersion: "1"}, + ClientName: "myclient", + UserName: "myusername", + UserUID: "myuseruid", + InactivityTimeoutSeconds: 300, + } + validNoTimeout := &oapi.OAuthAccessToken{ + ObjectMeta: metav1.ObjectMeta{Name: "accessTokenNameWithMinimumLength", ResourceVersion: "1"}, + ClientName: "myclient", + UserName: "myusername", + UserUID: "myuseruid", + InactivityTimeoutSeconds: 0, } errs := ValidateAccessTokenUpdate(valid, valid) if len(errs) != 0 { t.Errorf("expected success: %v", errs) } + errs = ValidateAccessTokenUpdate(validNoTimeout, validNoTimeout) + if len(errs) != 0 { + t.Errorf("expected success: %v", errs) + } errorCases := map[string]struct { Token oapi.OAuthAccessToken @@ -467,6 +509,30 @@ func TestValidateAccessTokensUpdate(t *testing.T) { T: field.ErrorTypeInvalid, F: "[]", }, + "change InactivityTimeoutSeconds to smaller value": { + Token: *valid, + Change: func(obj *oapi.OAuthAccessToken) { + obj.InactivityTimeoutSeconds = 299 + }, + T: field.ErrorTypeInvalid, + F: "inactivityTimeoutSeconds", + }, + "change InactivityTimeoutSeconds to negative value": { + Token: *valid, + Change: func(obj *oapi.OAuthAccessToken) { + obj.InactivityTimeoutSeconds = -1 + }, + T: field.ErrorTypeInvalid, + F: "inactivityTimeoutSeconds", + }, + "change InactivityTimeoutSeconds from 0 value": { + Token: *validNoTimeout, + Change: func(obj *oapi.OAuthAccessToken) { + obj.InactivityTimeoutSeconds = MinimumInactivityTimeoutSeconds + }, + T: field.ErrorTypeInvalid, + F: "inactivityTimeoutSeconds", + }, } for k, v := range errorCases { newToken := v.Token.DeepCopy() diff --git a/pkg/oauth/apiserver/auth.go b/pkg/oauth/apiserver/auth.go index c22f243853a5..23d0da82ce30 100644 --- a/pkg/oauth/apiserver/auth.go +++ b/pkg/oauth/apiserver/auth.go @@ -99,7 +99,11 @@ func (c *OAuthServerConfig) WithOAuth(handler http.Handler, requestContextMapper glog.Fatal(err) } - storage := registrystorage.New(c.ExtraOAuthConfig.OAuthAccessTokenClient, c.ExtraOAuthConfig.OAuthAuthorizeTokenClient, combinedOAuthClientGetter, registry.NewUserConversion()) + tokentimeout := int32(0) + if timeout := c.ExtraOAuthConfig.Options.TokenConfig.AccessTokenInactivityTimeoutSeconds; timeout != nil { + tokentimeout = *timeout + } + storage := registrystorage.New(c.ExtraOAuthConfig.OAuthAccessTokenClient, c.ExtraOAuthConfig.OAuthAuthorizeTokenClient, combinedOAuthClientGetter, registry.NewUserConversion(), tokentimeout) config := osinserver.NewDefaultServerConfig() if c.ExtraOAuthConfig.Options.TokenConfig.AuthorizeTokenMaxAgeSeconds > 0 { config.AuthorizationExpiration = c.ExtraOAuthConfig.Options.TokenConfig.AuthorizeTokenMaxAgeSeconds diff --git a/pkg/oauth/server/osinserver/registrystorage/storage.go b/pkg/oauth/server/osinserver/registrystorage/storage.go index 3b899ebabb96..e89b7d700d30 100644 --- a/pkg/oauth/server/osinserver/registrystorage/storage.go +++ b/pkg/oauth/server/osinserver/registrystorage/storage.go @@ -29,14 +29,16 @@ type storage struct { authorizetoken oauthclient.OAuthAuthorizeTokenInterface client oauthclientregistry.Getter user UserConversion + tokentimeout int32 } -func New(access oauthclient.OAuthAccessTokenInterface, authorize oauthclient.OAuthAuthorizeTokenInterface, client oauthclientregistry.Getter, user UserConversion) osin.Storage { +func New(access oauthclient.OAuthAccessTokenInterface, authorize oauthclient.OAuthAuthorizeTokenInterface, client oauthclientregistry.Getter, user UserConversion, tokentimeout int32) osin.Storage { return &storage{ accesstoken: access, authorizetoken: authorize, client: client, user: user, + tokentimeout: tokentimeout, } } @@ -49,6 +51,7 @@ type clientWrapper struct { var _ = osin.Client(&clientWrapper{}) var _ = osin.ClientSecretMatcher(&clientWrapper{}) var _ = handlers.TokenMaxAgeSeconds(&clientWrapper{}) +var _ = handlers.TokenTimeoutSeconds(&clientWrapper{}) func (w *clientWrapper) GetId() string { return w.id @@ -88,6 +91,10 @@ func (w *clientWrapper) GetTokenMaxAgeSeconds() *int32 { return w.client.AccessTokenMaxAgeSeconds } +func (w *clientWrapper) GetAccessTokenInactivityTimeoutSeconds() *int32 { + return w.client.AccessTokenInactivityTimeoutSeconds +} + // Clone the storage if needed. For example, using mgo, you can clone the session with session.Clone // to avoid concurrent access problems. // This is to avoid cloning the connection at each method access. @@ -248,6 +255,15 @@ func (s *storage) convertToAccessToken(data *osin.AccessData) (*oauthapi.OAuthAc if err := s.user.ConvertToAccessToken(data.UserData, token); err != nil { return nil, err } + + token.InactivityTimeoutSeconds = s.tokentimeout + // Check if we have a client specific inactivity Timeout to set + if w, ok := data.Client.(handlers.TokenTimeoutSeconds); ok { + if tt := w.GetAccessTokenInactivityTimeoutSeconds(); tt != nil { + token.InactivityTimeoutSeconds = *tt + } + } + return token, nil } diff --git a/test/integration/oauthstorage_test.go b/test/integration/oauthstorage_test.go index df6e6f529a66..a37849a84a02 100644 --- a/test/integration/oauthstorage_test.go +++ b/test/integration/oauthstorage_test.go @@ -67,7 +67,7 @@ func TestOAuthStorage(t *testing.T) { } user := &testUser{UserName: "test", UserUID: "1"} - storage := registrystorage.New(oauthClient.OAuthAccessTokens(), oauthClient.OAuthAuthorizeTokens(), oauthClient.OAuthClients(), user) + storage := registrystorage.New(oauthClient.OAuthAccessTokens(), oauthClient.OAuthAuthorizeTokens(), oauthClient.OAuthClients(), user, 0) oauthServer := osinserver.New( osinserver.NewDefaultServerConfig(),