Merge pull request #17788 from openshift-cherrypick-robot/cherry-pick-17550-to-release-3.8

Automatic merge from submit-queue.

Automated cherry-pick of #17550 on release-3.8

This is an automated cherry-pick of #17550

/assign enj

Author: openshift-merge-robot

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/client-go/util/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
+}

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 {

test/integration/oauth_serviceaccount_client_test.go

@@ -69,6 +69,7 @@ func TestOAuthServiceAccountClient(t *testing.T) {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	clusterAdminOAuthClient := oauthclient.NewForConfigOrDie(clusterAdminClientConfig).Oauth()
+	clusterAdminUserClient := userclient.NewForConfigOrDie(clusterAdminClientConfig)
 
 	projectName := "hammer-project"
 	if _, _, err := testserver.CreateNewProject(clusterAdminClientConfig, projectName, "harold"); err != nil {
@@ -375,6 +376,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) {