From 48003405635be0587e806be81bad7bd61c55e9e0 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 29 Nov 2017 20:33:58 -0500 Subject: [PATCH] Make client authorizations tolerant of UID changes This change makes it so that if an OAuthClientAuthorization references a user with the wrong UID, that client authorization is deleted. This UID mismatch is caused by the deletion and recreation of users. Thus this makes it so that the "new" user is unaffected by the "old" user's client authorization. The new user simply goes through the grant flow just as if no client authorization existed. Signed-off-by: Monis Khan --- pkg/auth/oauth/registry/grantchecker.go | 70 +++++++++++++--- pkg/auth/oauth/registry/registry_test.go | 46 +++++++++++ .../oauth_serviceaccount_client_test.go | 82 ++++++++++++++++++- 3 files changed, 185 insertions(+), 13 deletions(-) diff --git a/pkg/auth/oauth/registry/grantchecker.go b/pkg/auth/oauth/registry/grantchecker.go index 5afbe50878ae..6c8da52df419 100644 --- a/pkg/auth/oauth/registry/grantchecker.go +++ b/pkg/auth/oauth/registry/grantchecker.go @@ -1,18 +1,24 @@ package registry import ( - "fmt" + stderrors "errors" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apiserver/pkg/authentication/user" + kuser "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/kubernetes/pkg/client/retry" "github.com/openshift/origin/pkg/auth/api" + "github.com/openshift/origin/pkg/oauth/apis/oauth" oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset/typed/oauth/internalversion" "github.com/openshift/origin/pkg/oauth/registry/oauthclientauthorization" "github.com/openshift/origin/pkg/oauth/scope" + + "github.com/golang/glog" ) +var errEmptyUID = stderrors.New("user from request has empty UID and thus cannot perform a grant flow") + type ClientAuthorizationGrantChecker struct { client oauthclient.OAuthClientAuthorizationInterface } @@ -21,21 +27,61 @@ func NewClientAuthorizationGrantChecker(client oauthclient.OAuthClientAuthorizat return &ClientAuthorizationGrantChecker{client} } -func (c *ClientAuthorizationGrantChecker) HasAuthorizedClient(user user.Info, grant *api.Grant) (approved bool, err error) { - id := oauthclientauthorization.ClientAuthorizationName(user.GetName(), grant.Client.GetId()) - authorization, err := c.client.Get(id, metav1.GetOptions{}) - if errors.IsNotFound(err) { - return false, nil +func (c *ClientAuthorizationGrantChecker) HasAuthorizedClient(user kuser.Info, grant *api.Grant) (approved bool, err error) { + // Validation prevents OAuthClientAuthorization.UserUID from being empty (and always has). + // However, user.GetUID() is empty during impersonation, meaning this flow does not work for impersonation. + // This is fine because no OAuth / grant flow works with impersonation in general. + if len(user.GetUID()) == 0 { + return false, errEmptyUID } - if err != nil { + + id := oauthclientauthorization.ClientAuthorizationName(user.GetName(), grant.Client.GetId()) + var authorization *oauth.OAuthClientAuthorization + + // getClientAuthorization ignores not found errors, thus it is possible for authorization to be nil + // getClientAuthorization does not ignore conflict errors, so we retry those in case we having multiple clients racing on this grant flow + // all other errors are considered fatal + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + authorization, err = c.getClientAuthorization(id, user) + return err + }); err != nil { return false, err } - if len(authorization.UserUID) != 0 && authorization.UserUID != user.GetUID() { - return false, fmt.Errorf("user %s UID %s does not match stored client authorization value for UID %s", user.GetName(), user.GetUID(), authorization.UserUID) - } + // TODO: improve this to allow the scope implementation to determine overlap - if !scope.Covers(authorization.Scopes, scope.Split(grant.Scope)) { + if authorization == nil || !scope.Covers(authorization.Scopes, scope.Split(grant.Scope)) { return false, nil } + return true, nil } + +// getClientAuthorization gets the OAuthClientAuthorization with the given name and validates that it matches the given user +// it attempts to delete stale client authorizations, and thus must be retried in case of conflicts +func (c *ClientAuthorizationGrantChecker) getClientAuthorization(name string, user kuser.Info) (*oauth.OAuthClientAuthorization, error) { + authorization, err := c.client.Get(name, metav1.GetOptions{}) + if errors.IsNotFound(err) { + // if no such authorization exists, it simply means the user needs to go through the grant flow + return nil, nil + } + if err != nil { + // any other error is fatal (this will never be retried since it cannot return a conflict error) + return nil, err + } + + // check to see if we have a stale authorization + // user.GetUID() and authorization.UserUID are both guaranteed to be non-empty + if user.GetUID() != authorization.UserUID { + glog.Infof("%#v does not match stored client authorization %#v, attempting to delete stale authorization", user, authorization) + if err := c.client.Delete(name, metav1.NewPreconditionDeleteOptions(string(authorization.UID))); err != nil && !errors.IsNotFound(err) { + // ignore not found since that could be caused by multiple grant flows occurring at once (the other flow deleted the authorization before we did) + // this could be a conflict error, which will cause this whole function to be retried + return nil, err + } + // we successfully deleted the authorization so the user needs to go through the grant flow + return nil, nil + } + + // everything looks good so we can return the authorization + return authorization, nil +} diff --git a/pkg/auth/oauth/registry/registry_test.go b/pkg/auth/oauth/registry/registry_test.go index 0f5ed8976bfc..86840c91be21 100644 --- a/pkg/auth/oauth/registry/registry_test.go +++ b/pkg/auth/oauth/registry/registry_test.go @@ -130,6 +130,7 @@ func TestRegistryAndServer(t *testing.T) { AuthSuccess: true, AuthUser: &user.DefaultInfo{ Name: "user", + UID: "1", }, Check: func(h *testHandlers, _ *http.Request) { if h.AuthNeed || !h.GrantNeed || h.AuthErr != nil || h.GrantErr != nil || h.HandleAuthorizeReq.Authorized { @@ -168,10 +169,12 @@ func TestRegistryAndServer(t *testing.T) { AuthSuccess: true, AuthUser: &user.DefaultInfo{ Name: "user", + UID: "1", }, ClientAuth: &oapi.OAuthClientAuthorization{ ObjectMeta: metav1.ObjectMeta{Name: "user:test"}, UserName: "user", + UserUID: "1", ClientName: "test", Scopes: []string{"user:info"}, }, @@ -187,10 +190,12 @@ func TestRegistryAndServer(t *testing.T) { AuthSuccess: true, AuthUser: &user.DefaultInfo{ Name: "user", + UID: "1", }, ClientAuth: &oapi.OAuthClientAuthorization{ ObjectMeta: metav1.ObjectMeta{Name: "user:test"}, UserName: "user", + UserUID: "1", ClientName: "test", Scopes: []string{"user:info", "user:check-access"}, }, @@ -206,10 +211,12 @@ func TestRegistryAndServer(t *testing.T) { AuthSuccess: true, AuthUser: &user.DefaultInfo{ Name: "user", + UID: "1", }, ClientAuth: &oapi.OAuthClientAuthorization{ ObjectMeta: metav1.ObjectMeta{Name: "user:test"}, UserName: "user", + UserUID: "1", ClientName: "test", Scopes: []string{"user:full"}, }, @@ -227,6 +234,45 @@ func TestRegistryAndServer(t *testing.T) { } }, }, + "has auth with no UID, mimics impersonation": { + Client: validClient, + AuthSuccess: true, + AuthUser: &user.DefaultInfo{ + Name: "user", + }, + ClientAuth: &oapi.OAuthClientAuthorization{ + ObjectMeta: metav1.ObjectMeta{Name: "user:test"}, + UserName: "user", + UserUID: "2", + ClientName: "test", + Scopes: []string{"user:full"}, + }, + Check: func(h *testHandlers, r *http.Request) { + if h.AuthNeed || h.GrantNeed || h.AuthErr != nil || h.GrantErr != nil || h.HandleAuthorizeReq.Authorized || h.HandleAuthorizeResp.ErrorId != "server_error" { + t.Errorf("expected server_error error: %#v, %#v, %#v", h, h.HandleAuthorizeReq, h.HandleAuthorizeResp) + } + }, + }, + "has auth and grant with different UIDs": { + Client: validClient, + AuthSuccess: true, + AuthUser: &user.DefaultInfo{ + Name: "user", + UID: "1", + }, + ClientAuth: &oapi.OAuthClientAuthorization{ + ObjectMeta: metav1.ObjectMeta{Name: "user:test"}, + UserName: "user", + UserUID: "2", + ClientName: "test", + Scopes: []string{"user:full"}, + }, + Check: func(h *testHandlers, _ *http.Request) { + if h.AuthNeed || !h.GrantNeed || h.AuthErr != nil || h.GrantErr != nil || h.HandleAuthorizeReq.Authorized { + t.Errorf("expected request to need to grant access due to UID mismatch: %#v", h) + } + }, + }, } for _, testCase := range testCases { diff --git a/test/integration/oauth_serviceaccount_client_test.go b/test/integration/oauth_serviceaccount_client_test.go index 69969675094f..80f9c22a34b6 100644 --- a/test/integration/oauth_serviceaccount_client_test.go +++ b/test/integration/oauth_serviceaccount_client_test.go @@ -67,7 +67,8 @@ func TestOAuthServiceAccountClient(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - clusterAdminOAuthClient := oauthclient.NewForConfigOrDie(clusterAdminClientConfig) + clusterAdminOAuthClient := oauthclient.NewForConfigOrDie(clusterAdminClientConfig).Oauth() + clusterAdminUserClient := userclient.NewForConfigOrDie(clusterAdminClientConfig) projectName := "hammer-project" if _, _, err := testserver.CreateNewProject(clusterAdminClientConfig, projectName, "harold"); err != nil { @@ -374,6 +375,85 @@ func TestOAuthServiceAccountClient(t *testing.T) { }) clusterAdminOAuthClient.OAuthClientAuthorizations().Delete("harold:"+oauthClientConfig.ClientId, nil) } + + { + oauthClientConfig := &osincli.ClientConfig{ + ClientId: apiserverserviceaccount.MakeUsername(defaultSA.Namespace, defaultSA.Name), + ClientSecret: string(oauthSecret.Data[kapi.ServiceAccountTokenKey]), + AuthorizeUrl: clusterAdminClientConfig.Host + "/oauth/authorize", + TokenUrl: clusterAdminClientConfig.Host + "/oauth/token", + RedirectUrl: redirectURL, + Scope: scope.Join([]string{"user:info", "role:edit:" + projectName}), + SendClientSecretInParams: true, + } + t.Log("Testing grant flow is reentrant") + // First time, the approval steps are needed + // Second time, the approval steps are skipped + // Then we delete and recreate the user to make the client authorization UID no longer match + // Third time, the approval steps are needed + // Fourth time, the approval steps are skipped + runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{ + "GET /oauth/authorize", + "received challenge", + "GET /oauth/authorize", + "redirect to /oauth/authorize/approve", + "form", + "POST /oauth/authorize/approve", + "redirect to /oauth/authorize", + "redirect to /oauthcallback", + "code", + "scope:" + oauthClientConfig.Scope, + }) + runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{ + "GET /oauth/authorize", + "received challenge", + "GET /oauth/authorize", + "redirect to /oauthcallback", + "code", + "scope:" + oauthClientConfig.Scope, + }) + + // Delete the user to make the client authorization UID no longer match + // runOAuthFlow will cause the creation of the same user with a different UID during its challenge phase + if err := deleteUser(clusterAdminUserClient, "harold"); err != nil { + t.Fatalf("Failed to delete and recreate harold user: %v", err) + } + + runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{ + "GET /oauth/authorize", + "received challenge", + "GET /oauth/authorize", + "redirect to /oauth/authorize/approve", + "form", + "POST /oauth/authorize/approve", + "redirect to /oauth/authorize", + "redirect to /oauthcallback", + "code", + "scope:" + oauthClientConfig.Scope, + }) + runOAuthFlow(t, clusterAdminClientConfig, projectName, oauthClientConfig, nil, authorizationCodes, authorizationErrors, true, true, []string{ + "GET /oauth/authorize", + "received challenge", + "GET /oauth/authorize", + "redirect to /oauthcallback", + "code", + "scope:" + oauthClientConfig.Scope, + }) + clusterAdminOAuthClient.OAuthClientAuthorizations().Delete("harold:"+oauthClientConfig.ClientId, nil) + } +} + +func deleteUser(clusterAdminUserClient userclient.UserInterface, name string) error { + oldUser, err := clusterAdminUserClient.Users().Get(name, metav1.GetOptions{}) + if err != nil { + return err + } + for _, identity := range oldUser.Identities { + if err := clusterAdminUserClient.Identities().Delete(identity, nil); err != nil { + return err + } + } + return clusterAdminUserClient.Users().Delete(name, nil) } func drain(ch chan string) {