Add OAuth token and user validator interface

This change adds the OAuthTokenValidator interface for generically validating an OAuthAccessToken and User. The expiration and UID validation was pulled out from the tokenAuthenticator. tokenAuthenticator simply takes OAuthTokenValidators as input, and delegates validation to them. This allows all future validation to simply append itself to the list of validators without requiring any changes to tokenAuthenticator. Signed-off-by: Monis Khan <[email protected]>
openshift · Dec 5, 2017 · 10d7f6a · 10d7f6a
1 parent b638188
commit 10d7f6a
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 32 deletions.
diff --git a/pkg/auth/authenticator/interfaces.go b/pkg/auth/authenticator/interfaces.go
@@ -4,6 +4,8 @@ import (
 	"k8s.io/apiserver/pkg/authentication/user"
 
 	"github.com/openshift/origin/pkg/auth/api"
+	"github.com/openshift/origin/pkg/oauth/apis/oauth"
+	userapi "github.com/openshift/origin/pkg/user/apis/user"
 )
 
 type Assertion interface {
@@ -13,3 +15,28 @@ type Assertion interface {
 type Client interface {
 	AuthenticateClient(client api.Client) (user.Info, bool, error)
 }
+
+type OAuthTokenValidator interface {
+	Validate(token *oauth.OAuthAccessToken, user *userapi.User) error
+}
+
+var _ OAuthTokenValidator = OAuthTokenValidatorFunc(nil)
+
+type OAuthTokenValidatorFunc func(token *oauth.OAuthAccessToken, user *userapi.User) error
+
+func (f OAuthTokenValidatorFunc) Validate(token *oauth.OAuthAccessToken, user *userapi.User) error {
+	return f(token, user)
+}
+
+var _ OAuthTokenValidator = OAuthTokenValidators(nil)
+
+type OAuthTokenValidators []OAuthTokenValidator
+
+func (v OAuthTokenValidators) Validate(token *oauth.OAuthAccessToken, user *userapi.User) error {
+	for _, validator := range v {
+		if err := validator.Validate(token, user); err != nil {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/pkg/auth/oauth/registry/expirationvalidator.go b/pkg/auth/oauth/registry/expirationvalidator.go
@@ -0,0 +1,32 @@
+package registry
+
+import (
+	"errors"
+	"time"
+
+	"github.com/openshift/origin/pkg/auth/authenticator"
+	"github.com/openshift/origin/pkg/oauth/apis/oauth"
+	"github.com/openshift/origin/pkg/user/apis/user"
+)
+
+var errExpired = errors.New("token is expired")
+
+func NewExpirationValidator() authenticator.OAuthTokenValidator {
+	return authenticator.OAuthTokenValidatorFunc(
+		func(token *oauth.OAuthAccessToken, _ *user.User) error {
+			if token.ExpiresIn > 0 {
+				if expire(token).Before(time.Now()) {
+					return errExpired
+				}
+			}
+			if token.DeletionTimestamp != nil {
+				return errExpired
+			}
+			return nil
+		},
+	)
+}
+
+func expire(token *oauth.OAuthAccessToken) time.Time {
+	return token.CreationTimestamp.Add(time.Duration(token.ExpiresIn) * time.Second)
+}
diff --git a/pkg/auth/oauth/registry/registry_test.go b/pkg/auth/oauth/registry/registry_test.go
@@ -318,6 +318,7 @@ func TestAuthenticateTokenNotFound(t *testing.T) {
 		t.Errorf("Unexpected user: %v", userInfo)
 	}
 }
+
 func TestAuthenticateTokenOtherGetError(t *testing.T) {
 	fakeOAuthClient := oauthfake.NewSimpleClientset()
 	fakeOAuthClient.PrependReactor("get", "oauthaccesstokens", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
@@ -340,27 +341,67 @@ func TestAuthenticateTokenOtherGetError(t *testing.T) {
 		t.Errorf("Unexpected user: %v", userInfo)
 	}
 }
+
 func TestAuthenticateTokenExpired(t *testing.T) {
+	fakeOAuthClient := oauthfake.NewSimpleClientset(
+		// expired token that had a lifetime of 10 minutes
+		&oapi.OAuthAccessToken{
+			ObjectMeta: metav1.ObjectMeta{Name: "token1", CreationTimestamp: metav1.Time{Time: time.Now().Add(-1 * time.Hour)}},
+			ExpiresIn:  600,
+			UserName:   "foo",
+		},
+		// non-expired token that has a lifetime of 10 minutes, but has a non-nil deletion timestamp
+		&oapi.OAuthAccessToken{
+			ObjectMeta: metav1.ObjectMeta{Name: "token2", CreationTimestamp: metav1.Time{Time: time.Now()}, DeletionTimestamp: &metav1.Time{}},
+			ExpiresIn:  600,
+			UserName:   "foo",
+		},
+	)
+	userRegistry := usertest.NewUserRegistry()
+	userRegistry.GetUsers["foo"] = &userapi.User{ObjectMeta: metav1.ObjectMeta{UID: "bar"}}
+
+	tokenAuthenticator := NewTokenAuthenticator(fakeOAuthClient.Oauth().OAuthAccessTokens(), userRegistry, identitymapper.NoopGroupMapper{}, NewExpirationValidator())
+
+	for _, tokenName := range []string{"token1", "token2"} {
+		userInfo, found, err := tokenAuthenticator.AuthenticateToken(tokenName)
+		if found {
+			t.Error("Found token, but it should be missing!")
+		}
+		if err != errExpired {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		if userInfo != nil {
+			t.Errorf("Unexpected user: %v", userInfo)
+		}
+	}
+}
+
+func TestAuthenticateTokenInvalidUID(t *testing.T) {
 	fakeOAuthClient := oauthfake.NewSimpleClientset(
 		&oapi.OAuthAccessToken{
-			ObjectMeta: metav1.ObjectMeta{Name: "token", CreationTimestamp: metav1.Time{Time: time.Now().Add(-1 * time.Hour)}},
+			ObjectMeta: metav1.ObjectMeta{Name: "token", CreationTimestamp: metav1.Time{Time: time.Now()}},
 			ExpiresIn:  600, // 10 minutes
+			UserName:   "foo",
+			UserUID:    string("bar1"),
 		},
 	)
 	userRegistry := usertest.NewUserRegistry()
-	tokenAuthenticator := NewTokenAuthenticator(fakeOAuthClient.Oauth().OAuthAccessTokens(), userRegistry, identitymapper.NoopGroupMapper{})
+	userRegistry.GetUsers["foo"] = &userapi.User{ObjectMeta: metav1.ObjectMeta{UID: "bar2"}}
+
+	tokenAuthenticator := NewTokenAuthenticator(fakeOAuthClient.Oauth().OAuthAccessTokens(), userRegistry, identitymapper.NoopGroupMapper{}, NewUIDValidator())
 
 	userInfo, found, err := tokenAuthenticator.AuthenticateToken("token")
 	if found {
 		t.Error("Found token, but it should be missing!")
 	}
-	if err != ErrExpired {
+	if err.Error() != "user.UID (bar2) does not match token.userUID (bar1)" {
 		t.Errorf("Unexpected error: %v", err)
 	}
 	if userInfo != nil {
 		t.Errorf("Unexpected user: %v", userInfo)
 	}
 }
+
 func TestAuthenticateTokenValidated(t *testing.T) {
 	fakeOAuthClient := oauthfake.NewSimpleClientset(
 		&oapi.OAuthAccessToken{
@@ -373,7 +414,7 @@ func TestAuthenticateTokenValidated(t *testing.T) {
 	userRegistry := usertest.NewUserRegistry()
 	userRegistry.GetUsers["foo"] = &userapi.User{ObjectMeta: metav1.ObjectMeta{UID: "bar"}}
 
-	tokenAuthenticator := NewTokenAuthenticator(fakeOAuthClient.Oauth().OAuthAccessTokens(), userRegistry, identitymapper.NoopGroupMapper{})
+	tokenAuthenticator := NewTokenAuthenticator(fakeOAuthClient.Oauth().OAuthAccessTokens(), userRegistry, identitymapper.NoopGroupMapper{}, NewExpirationValidator(), NewUIDValidator())
 
 	userInfo, found, err := tokenAuthenticator.AuthenticateToken("token")
 	if !found {

diff --git a/pkg/auth/oauth/registry/tokenauthenticator.go b/pkg/auth/oauth/registry/tokenauthenticator.go
@@ -1,70 +1,61 @@
 package registry
 
 import (
-	"errors"
-	"fmt"
-	"time"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kauthenticator "k8s.io/apiserver/pkg/authentication/authenticator"
 	kuser "k8s.io/apiserver/pkg/authentication/user"
 
+	"github.com/openshift/origin/pkg/auth/authenticator"
 	"github.com/openshift/origin/pkg/auth/userregistry/identitymapper"
 	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
 	oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset/typed/oauth/internalversion"
 	userclient "github.com/openshift/origin/pkg/user/generated/internalclientset/typed/user/internalversion"
 )
 
-type TokenAuthenticator struct {
+type tokenAuthenticator struct {
 	tokens      oauthclient.OAuthAccessTokenInterface
 	users       userclient.UserResourceInterface
 	groupMapper identitymapper.UserToGroupMapper
+	validators  authenticator.OAuthTokenValidator
 }
 
-var ErrExpired = errors.New("Token is expired")
-
-func NewTokenAuthenticator(tokens oauthclient.OAuthAccessTokenInterface, users userclient.UserResourceInterface, groupMapper identitymapper.UserToGroupMapper) *TokenAuthenticator {
-	return &TokenAuthenticator{
+func NewTokenAuthenticator(tokens oauthclient.OAuthAccessTokenInterface, users userclient.UserResourceInterface, groupMapper identitymapper.UserToGroupMapper, validators ...authenticator.OAuthTokenValidator) kauthenticator.Token {
+	return &tokenAuthenticator{
 		tokens:      tokens,
 		users:       users,
 		groupMapper: groupMapper,
+		validators:  authenticator.OAuthTokenValidators(validators),
 	}
 }
 
-func (a *TokenAuthenticator) AuthenticateToken(value string) (kuser.Info, bool, error) {
-	token, err := a.tokens.Get(value, metav1.GetOptions{})
+func (a *tokenAuthenticator) AuthenticateToken(name string) (kuser.Info, bool, error) {
+	token, err := a.tokens.Get(name, metav1.GetOptions{})
 	if err != nil {
 		return nil, false, err
 	}
-	if token.ExpiresIn > 0 {
-		if token.CreationTimestamp.Time.Add(time.Duration(token.ExpiresIn) * time.Second).Before(time.Now()) {
-			return nil, false, ErrExpired
-		}
-	}
-	if token.DeletionTimestamp != nil {
-		return nil, false, ErrExpired
-	}
 
-	u, err := a.users.Get(token.UserName, metav1.GetOptions{})
+	user, err := a.users.Get(token.UserName, metav1.GetOptions{})
 	if err != nil {
 		return nil, false, err
 	}
-	if string(u.UID) != token.UserUID {
-		return nil, false, fmt.Errorf("user.UID (%s) does not match token.userUID (%s)", u.UID, token.UserUID)
+
+	if err := a.validators.Validate(token, user); err != nil {
+		return nil, false, err
 	}
 
-	groups, err := a.groupMapper.GroupsFor(u.Name)
+	groups, err := a.groupMapper.GroupsFor(user.Name)
 	if err != nil {
 		return nil, false, err
 	}
-	groupNames := []string{}
+	groupNames := make([]string, 0, len(groups)+len(user.Groups))
 	for _, group := range groups {
 		groupNames = append(groupNames, group.Name)
 	}
-	groupNames = append(groupNames, u.Groups...)
+	groupNames = append(groupNames, user.Groups...)
 
 	return &kuser.DefaultInfo{
-		Name:   u.Name,
-		UID:    string(u.UID),
+		Name:   user.Name,
+		UID:    string(user.UID),
 		Groups: groupNames,
 		Extra: map[string][]string{
 			authorizationapi.ScopesKey: token.Scopes,

diff --git a/pkg/auth/oauth/registry/uidvalidator.go b/pkg/auth/oauth/registry/uidvalidator.go
@@ -0,0 +1,22 @@
+package registry
+
+import (
+	"fmt"
+
+	"github.com/openshift/origin/pkg/auth/authenticator"
+	"github.com/openshift/origin/pkg/oauth/apis/oauth"
+	userapi "github.com/openshift/origin/pkg/user/apis/user"
+)
+
+const errInvalidUIDStr = "user.UID (%s) does not match token.userUID (%s)"
+
+func NewUIDValidator() authenticator.OAuthTokenValidator {
+	return authenticator.OAuthTokenValidatorFunc(
+		func(token *oauth.OAuthAccessToken, user *userapi.User) error {
+			if string(user.UID) != token.UserUID {
+				return fmt.Errorf(errInvalidUIDStr, user.UID, token.UserUID)
+			}
+			return nil
+		},
+	)
+}
diff --git a/pkg/cmd/server/origin/authenticator.go b/pkg/cmd/server/origin/authenticator.go
@@ -20,6 +20,7 @@ import (
 	sacontroller "k8s.io/kubernetes/pkg/controller/serviceaccount"
 	"k8s.io/kubernetes/pkg/serviceaccount"
 
+	openshiftauthenticator "github.com/openshift/origin/pkg/auth/authenticator"
 	"github.com/openshift/origin/pkg/auth/authenticator/request/paramtoken"
 	authnregistry "github.com/openshift/origin/pkg/auth/oauth/registry"
 	"github.com/openshift/origin/pkg/auth/userregistry/identitymapper"
@@ -88,7 +89,8 @@ func newAuthenticator(config configapi.MasterConfig, accessTokenGetter oauthclie
 
 	// OAuth token
 	if config.OAuthConfig != nil {
-		oauthTokenAuthenticator := authnregistry.NewTokenAuthenticator(accessTokenGetter, userGetter, groupMapper)
+		validators := []openshiftauthenticator.OAuthTokenValidator{authnregistry.NewExpirationValidator(), authnregistry.NewUIDValidator()}
+		oauthTokenAuthenticator := authnregistry.NewTokenAuthenticator(accessTokenGetter, userGetter, groupMapper, validators...)
 		tokenAuthenticators = append(tokenAuthenticators,
 			// if you have a bearer token, you're a human (usually)
 			// if you change this, have a look at the impersonationFilter where we attach groups to the impersonated user