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) {