Skip to content

Commit

Permalink
Make client authorizations tolerant of UID changes
Browse files Browse the repository at this point in the history 
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 <mkhan@redhat.com>
  • Loading branch information
@enj
enj committed Dec 14, 2017
1 parent bfd1f00 commit 4800340
Show file tree
Hide file tree
Showing 3 changed files with 185 additions and 13 deletions.
70 changes: 58 additions & 12 deletions pkg/auth/oauth/registry/grantchecker.go
View file
Original file line number Diff line number Diff line change
@@ -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
}
Expand All @@ -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
}
46 changes: 46 additions & 0 deletions pkg/auth/oauth/registry/registry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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"},
},
Expand All @@ -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"},
},
Expand All @@ -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"},
},
Expand All @@ -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 {
Expand Down
82 changes: 81 additions & 1 deletion test/integration/oauth_serviceaccount_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 4800340

Please sign in to comment.