From 10d7f6a1241249ceb90b701f43b8df75318913af Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Mon, 4 Dec 2017 18:28:47 -0500 Subject: [PATCH] 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 --- pkg/auth/authenticator/interfaces.go | 27 ++++++++++ .../oauth/registry/expirationvalidator.go | 32 ++++++++++++ pkg/auth/oauth/registry/registry_test.go | 49 +++++++++++++++++-- pkg/auth/oauth/registry/tokenauthenticator.go | 45 +++++++---------- pkg/auth/oauth/registry/uidvalidator.go | 22 +++++++++ pkg/cmd/server/origin/authenticator.go | 4 +- 6 files changed, 147 insertions(+), 32 deletions(-) create mode 100644 pkg/auth/oauth/registry/expirationvalidator.go create mode 100644 pkg/auth/oauth/registry/uidvalidator.go diff --git a/pkg/auth/authenticator/interfaces.go b/pkg/auth/authenticator/interfaces.go index e63c804f7198..26bef8499ee5 100644 --- 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 new file mode 100644 index 000000000000..876a7b9c7013 --- /dev/null +++ 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 index 0f5ed8976bfc..dfe2481386df 100644 --- 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 index 6f85171d7a70..34dd64eeb2dc 100644 --- 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 new file mode 100644 index 000000000000..8b0fd2dc5b85 --- /dev/null +++ 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 index 1ff582d94e78..de48ef7841e6 100644 --- 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