From bd80d7590d1ca49ddb169dd54d655114b8de45a7 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 24 Oct 2023 16:41:54 -0400 Subject: [PATCH] reg/auth: remove contexts from Authorized method The details of how request-scoped information is propagated through the registry server app should be left as private implementation details so they can be changed without fear of breaking compatibility with third-party code which imports the distribution module. The AccessController interface unnecessarily bakes into the public API details of how authorization grants are propagated through request contexts. In practice the only values the in-tree authorizers attach to the request contexts are the UserInfo and Resources for the request. Change the AccessController interface to return the UserInfo and Resources directly to allow us to change how request contexts are used within the app without altering the AccessController interface contract. Signed-off-by: Cory Snider --- registry/auth/auth.go | 24 ++++++++++++-------- registry/auth/htpasswd/access.go | 4 ++-- registry/auth/htpasswd/access_test.go | 11 +++++----- registry/auth/silly/access.go | 9 ++------ registry/auth/silly/access_test.go | 11 +++++----- registry/auth/token/accesscontroller.go | 10 ++++----- registry/auth/token/token_test.go | 29 ++++++++++--------------- registry/handlers/app.go | 8 ++++++- 8 files changed, 53 insertions(+), 53 deletions(-) diff --git a/registry/auth/auth.go b/registry/auth/auth.go index 1f28ea85e..0bda67f6c 100644 --- a/registry/auth/auth.go +++ b/registry/auth/auth.go @@ -76,6 +76,12 @@ type Access struct { Action string } +// Grant describes the permitted level of access for an authorized request. +type Grant struct { + User UserInfo // The authenticated user for the request. + Resources []Resource // The list of resources which have been authorized for the request. +} + // Challenge is a special error type which is used for HTTP 401 Unauthorized // responses and is able to write the response with WWW-Authenticate challenge // header values based on the error. @@ -93,15 +99,15 @@ type Challenge interface { // and required access levels for a request. Implementations can support both // complete denial and http authorization challenges. type AccessController interface { - // Authorized returns a nil error if the request is granted access and - // returns a new authorized context. If one or more Access structs are - // provided, the requested access will be compared with what is available - // to the request. Access is denied if the error is non-nil. The error may - // be of type Challenge, in which case the caller may have the Challenge - // handle the request or choose what action to take based on the Challenge - // header or response status. The returned context object should be derived - // from r.Context() and have a "auth.user" value set to a UserInfo struct. - Authorized(r *http.Request, access ...Access) (context.Context, error) + // Authorized determines if the request is granted access. If one or more + // Access structs are provided, the requested access will be compared with + // what is available to the request. + // + // Return a Grant to grant the request access. Return an error to deny + // access. The error may be of type Challenge, in which case the caller may + // have the Challenge handle the request or choose what action to take based + // on the Challenge header or response status. + Authorized(r *http.Request, access ...Access) (*Grant, error) } // CredentialAuthenticator is an object which is able to authenticate credentials diff --git a/registry/auth/htpasswd/access.go b/registry/auth/htpasswd/access.go index a5c89a42d..c8c432653 100644 --- a/registry/auth/htpasswd/access.go +++ b/registry/auth/htpasswd/access.go @@ -49,7 +49,7 @@ func newAccessController(options map[string]interface{}) (auth.AccessController, return &accessController{realm: realm.(string), path: path}, nil } -func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.Access) (context.Context, error) { +func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.Access) (*auth.Grant, error) { username, password, ok := req.BasicAuth() if !ok { return nil, &challenge{ @@ -94,7 +94,7 @@ func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth. } } - return auth.WithUser(req.Context(), auth.UserInfo{Name: username}), nil + return &auth.Grant{User: auth.UserInfo{Name: username}}, nil } // challenge implements the auth.Challenge interface. diff --git a/registry/auth/htpasswd/access_test.go b/registry/auth/htpasswd/access_test.go index ad5e7f70c..01f2ac9d7 100644 --- a/registry/auth/htpasswd/access_test.go +++ b/registry/auth/htpasswd/access_test.go @@ -43,7 +43,7 @@ func TestBasicAccessController(t *testing.T) { userNumber := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - authCtx, err := accessController.Authorized(r) + grant, err := accessController.Authorized(r) if err != nil { switch err := err.(type) { case auth.Challenge: @@ -55,13 +55,12 @@ func TestBasicAccessController(t *testing.T) { } } - userInfo, ok := authCtx.Value(auth.UserKey).(auth.UserInfo) - if !ok { - t.Fatal("basic accessController did not set auth.user context") + if grant == nil { + t.Fatal("basic accessController did not return auth grant") } - if userInfo.Name != testUsers[userNumber] { - t.Fatalf("expected user name %q, got %q", testUsers[userNumber], userInfo.Name) + if grant.User.Name != testUsers[userNumber] { + t.Fatalf("expected user name %q, got %q", testUsers[userNumber], grant.User.Name) } w.WriteHeader(http.StatusNoContent) diff --git a/registry/auth/silly/access.go b/registry/auth/silly/access.go index c8f383e23..1984ba20d 100644 --- a/registry/auth/silly/access.go +++ b/registry/auth/silly/access.go @@ -8,12 +8,10 @@ package silly import ( - "context" "fmt" "net/http" "strings" - "github.com/distribution/distribution/v3/internal/dcontext" "github.com/distribution/distribution/v3/registry/auth" ) @@ -43,7 +41,7 @@ func newAccessController(options map[string]interface{}) (auth.AccessController, // Authorized simply checks for the existence of the authorization header, // responding with a bearer challenge if it doesn't exist. -func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.Access) (context.Context, error) { +func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth.Access) (*auth.Grant, error) { if req.Header.Get("Authorization") == "" { challenge := challenge{ realm: ac.realm, @@ -61,10 +59,7 @@ func (ac *accessController) Authorized(req *http.Request, accessRecords ...auth. return nil, &challenge } - ctx := auth.WithUser(req.Context(), auth.UserInfo{Name: "silly"}) - ctx = dcontext.WithLogger(ctx, dcontext.GetLogger(ctx, auth.UserNameKey, auth.UserKey)) - - return ctx, nil + return &auth.Grant{User: auth.UserInfo{Name: "silly"}}, nil } type challenge struct { diff --git a/registry/auth/silly/access_test.go b/registry/auth/silly/access_test.go index 1a137c715..506af0bde 100644 --- a/registry/auth/silly/access_test.go +++ b/registry/auth/silly/access_test.go @@ -15,7 +15,7 @@ func TestSillyAccessController(t *testing.T) { } server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - authCtx, err := ac.Authorized(r) + grant, err := ac.Authorized(r) if err != nil { switch err := err.(type) { case auth.Challenge: @@ -27,13 +27,12 @@ func TestSillyAccessController(t *testing.T) { } } - userInfo, ok := authCtx.Value(auth.UserKey).(auth.UserInfo) - if !ok { - t.Fatal("silly accessController did not set auth.user context") + if grant == nil { + t.Fatal("silly accessController did not return auth grant") } - if userInfo.Name != "silly" { - t.Fatalf("expected user name %q, got %q", "silly", userInfo.Name) + if grant.User.Name != "silly" { + t.Fatalf("expected user name %q, got %q", "silly", grant.User.Name) } w.WriteHeader(http.StatusNoContent) diff --git a/registry/auth/token/accesscontroller.go b/registry/auth/token/accesscontroller.go index e019d0f51..bed4c827d 100644 --- a/registry/auth/token/accesscontroller.go +++ b/registry/auth/token/accesscontroller.go @@ -1,7 +1,6 @@ package token import ( - "context" "crypto" "crypto/x509" "encoding/json" @@ -291,7 +290,7 @@ func newAccessController(options map[string]interface{}) (auth.AccessController, // Authorized handles checking whether the given request is authorized // for actions on resources described by the given access items. -func (ac *accessController) Authorized(req *http.Request, accessItems ...auth.Access) (context.Context, error) { +func (ac *accessController) Authorized(req *http.Request, accessItems ...auth.Access) (*auth.Grant, error) { challenge := &authChallenge{ realm: ac.realm, autoRedirect: ac.autoRedirect, @@ -332,9 +331,10 @@ func (ac *accessController) Authorized(req *http.Request, accessItems ...auth.Ac } } - ctx := auth.WithResources(req.Context(), claims.resources()) - - return auth.WithUser(ctx, auth.UserInfo{Name: claims.Subject}), nil + return &auth.Grant{ + User: auth.UserInfo{Name: claims.Subject}, + Resources: claims.resources(), + }, nil } // init handles registering the token auth backend. diff --git a/registry/auth/token/token_test.go b/registry/auth/token/token_test.go index 52d34a70f..a96546af3 100644 --- a/registry/auth/token/token_test.go +++ b/registry/auth/token/token_test.go @@ -465,7 +465,7 @@ func TestAccessController(t *testing.T) { Action: "baz", } - authCtx, err := accessController.Authorized(req, testAccess) + grant, err := accessController.Authorized(req, testAccess) challenge, ok := err.(auth.Challenge) if !ok { t.Fatal("accessController did not return a challenge") @@ -475,8 +475,8 @@ func TestAccessController(t *testing.T) { t.Fatalf("accessControler did not get expected error - got %s - expected %s", challenge, ErrTokenRequired) } - if authCtx != nil { - t.Fatalf("expected nil auth context but got %s", authCtx) + if grant != nil { + t.Fatalf("expected nil auth grant but got %#v", grant) } // 2. Supply an invalid token. @@ -500,7 +500,7 @@ func TestAccessController(t *testing.T) { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token.Raw)) - authCtx, err = accessController.Authorized(req, testAccess) + grant, err = accessController.Authorized(req, testAccess) challenge, ok = err.(auth.Challenge) if !ok { t.Fatal("accessController did not return a challenge") @@ -510,8 +510,8 @@ func TestAccessController(t *testing.T) { t.Fatalf("accessControler did not get expected error - got %s - expected %s", challenge, ErrTokenRequired) } - if authCtx != nil { - t.Fatalf("expected nil auth context but got %s", authCtx) + if grant != nil { + t.Fatalf("expected nil auth grant but got %#v", grant) } // create a valid jwk @@ -532,7 +532,7 @@ func TestAccessController(t *testing.T) { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token.Raw)) - authCtx, err = accessController.Authorized(req, testAccess) + grant, err = accessController.Authorized(req, testAccess) challenge, ok = err.(auth.Challenge) if !ok { t.Fatal("accessController did not return a challenge") @@ -542,8 +542,8 @@ func TestAccessController(t *testing.T) { t.Fatalf("accessControler did not get expected error - got %s - expected %s", challenge, ErrInsufficientScope) } - if authCtx != nil { - t.Fatalf("expected nil auth context but got %s", authCtx) + if grant != nil { + t.Fatalf("expected nil auth grant but got %#v", grant) } // 4. Supply the token we need, or deserve, or whatever. @@ -562,18 +562,13 @@ func TestAccessController(t *testing.T) { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token.Raw)) - authCtx, err = accessController.Authorized(req, testAccess) + grant, err = accessController.Authorized(req, testAccess) if err != nil { t.Fatalf("accessController returned unexpected error: %s", err) } - userInfo, ok := authCtx.Value(auth.UserKey).(auth.UserInfo) - if !ok { - t.Fatal("token accessController did not set auth.user context") - } - - if userInfo.Name != "foo" { - t.Fatalf("expected user name %q, got %q", "foo", userInfo.Name) + if grant.User.Name != "foo" { + t.Fatalf("expected user name %q, got %q", "foo", grant.User.Name) } // 5. Supply a token with full admin rights, which is represented as "*". diff --git a/registry/handlers/app.go b/registry/handlers/app.go index 8bb5bbbcd..471f7c169 100644 --- a/registry/handlers/app.go +++ b/registry/handlers/app.go @@ -797,7 +797,7 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont accessRecords = appendCatalogAccessRecord(accessRecords, r) } - ctx, err := app.accessController.Authorized(r.WithContext(context.Context), accessRecords...) + grant, err := app.accessController.Authorized(r.WithContext(context.Context), accessRecords...) if err != nil { switch err := err.(type) { case auth.Challenge: @@ -818,6 +818,12 @@ func (app *App) authorized(w http.ResponseWriter, r *http.Request, context *Cont return err } + if grant == nil { + return fmt.Errorf("access controller returned neither an access grant nor an error") + } + + ctx := auth.WithUser(context.Context, grant.User) + ctx = auth.WithResources(ctx, grant.Resources) dcontext.GetLogger(ctx, auth.UserNameKey).Info("authorized request") // TODO(stevvooe): This pattern needs to be cleaned up a bit. One context