diff --git a/admin.go b/admin.go index 3f78a7e96..14174559e 100644 --- a/admin.go +++ b/admin.go @@ -680,6 +680,10 @@ func (ctx Context) IdentityCredentials(logger *zap.Logger) ([]tls.Certificate, e // trust, and then looks for a matching, authorized public key that is allowed to access // the defined path(s) using the defined method(s). func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error { + // the same public key may appear in more than one access control entry; + // the request is allowed if any entry containing the key permits it + var keyRecognized, pathAllowedSomewhere bool + for _, chain := range r.TLS.VerifiedChains { for _, peerCert := range chain { for _, adminAccess := range remote.AccessControl { @@ -692,18 +696,15 @@ func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error { if !ok || !comparer.Equal(allowedKey) { continue } + keyRecognized = true // key recognized; make sure its HTTP request is permitted + // by at least one of the permission entries (if none are + // configured, all permissions are granted) + if len(adminAccess.Permissions) == 0 { + return nil + } for _, accessPerm := range adminAccess.Permissions { - // verify method - methodFound := accessPerm.Methods == nil || slices.Contains(accessPerm.Methods, r.Method) - if !methodFound { - return APIError{ - HTTPStatus: http.StatusForbidden, - Message: "not authorized to use this method", - } - } - // verify path pathFound := accessPerm.Paths == nil for _, allowedPath := range accessPerm.Paths { @@ -713,20 +714,40 @@ func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error { } } if !pathFound { - return APIError{ - HTTPStatus: http.StatusForbidden, - Message: "not authorized to access this path", - } + continue + } + pathAllowedSomewhere = true + + // verify method + if accessPerm.Methods == nil || slices.Contains(accessPerm.Methods, r.Method) { + // public key authorized, method and path allowed + return nil } } - // public key authorized, method and path allowed - return nil + // this entry does not permit the request; other entries + // containing the same key may + break } } } } + if keyRecognized { + // no entry allows this request; if the path is allowed by + // some entry, the method is what's missing + if pathAllowedSomewhere { + return APIError{ + HTTPStatus: http.StatusForbidden, + Message: "not authorized to use this method", + } + } + return APIError{ + HTTPStatus: http.StatusForbidden, + Message: "not authorized to access this path", + } + } + // in theory, this should never happen; with an unverified chain, the TLS server // should not accept the connection in the first place, and the acceptable cert // pool is configured using the same list of public keys we verify against diff --git a/admin_test.go b/admin_test.go index 3d70baf6c..a4b8da073 100644 --- a/admin_test.go +++ b/admin_test.go @@ -805,6 +805,141 @@ func TestRemoteAdminAccessControlPathSegmentMatching(t *testing.T) { } } +func TestRemoteAdminAccessControlMultiplePermissions(t *testing.T) { + const authorizedKey testAdminPublicKey = "authorized" + peerCert := &x509.Certificate{PublicKey: authorizedKey} + + tests := []struct { + name string + permissions []AdminPermissions + // extraAccess adds further access control entries holding the same key + extraAccess [][]AdminPermissions + method string + requestPath string + wantErr string + }{ + { + name: "no permissions grants all", + permissions: nil, + method: http.MethodDelete, + requestPath: "/config/", + }, + { + name: "first entry matches", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + {Methods: []string{http.MethodPost}, Paths: []string{"/load"}}, + }, + method: http.MethodGet, + requestPath: "/config/", + }, + { + name: "second entry matches", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + {Methods: []string{http.MethodPost}, Paths: []string{"/load"}}, + }, + method: http.MethodPost, + requestPath: "/load", + }, + { + name: "same path different methods across entries", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + {Methods: []string{http.MethodPatch}, Paths: []string{"/config/"}}, + }, + method: http.MethodPatch, + requestPath: "/config/", + }, + { + name: "method and path from different entries", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + {Methods: []string{http.MethodPost}, Paths: []string{"/load"}}, + }, + method: http.MethodPost, + requestPath: "/config/", + wantErr: "not authorized to use this method", + }, + { + name: "path allowed by no entry", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + {Methods: []string{http.MethodPost}, Paths: []string{"/load"}}, + }, + method: http.MethodGet, + requestPath: "/stop", + wantErr: "not authorized to access this path", + }, + { + name: "later access entry with the same key matches", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + }, + extraAccess: [][]AdminPermissions{ + {{Methods: []string{http.MethodPost}, Paths: []string{"/load"}}}, + }, + method: http.MethodPost, + requestPath: "/load", + }, + { + name: "no access entry with the same key matches", + permissions: []AdminPermissions{ + {Methods: []string{http.MethodGet}, Paths: []string{"/config/"}}, + }, + extraAccess: [][]AdminPermissions{ + {{Methods: []string{http.MethodGet}, Paths: []string{"/load"}}}, + }, + method: http.MethodPost, + requestPath: "/load", + wantErr: "not authorized to use this method", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + remote := RemoteAdmin{ + AccessControl: []*AdminAccess{ + { + Permissions: test.permissions, + publicKeys: []crypto.PublicKey{authorizedKey}, + }, + }, + } + for _, permissions := range test.extraAccess { + remote.AccessControl = append(remote.AccessControl, &AdminAccess{ + Permissions: permissions, + publicKeys: []crypto.PublicKey{authorizedKey}, + }) + } + + req := httptest.NewRequest(test.method, "https://localhost:2021"+test.requestPath, nil) + req.TLS = &tls.ConnectionState{ + VerifiedChains: [][]*x509.Certificate{{peerCert}}, + } + + err := remote.enforceAccessControls(req) + if test.wantErr == "" { + if err != nil { + t.Errorf("%s %s: expected no error, got %v", test.method, test.requestPath, err) + } + return + } + + var apiErr APIError + if !errors.As(err, &apiErr) { + t.Fatalf("%s %s: expected APIError, got %T: %v", test.method, test.requestPath, err, err) + } + if apiErr.HTTPStatus != http.StatusForbidden { + t.Errorf("%s %s: expected HTTP status %d, got %d", test.method, test.requestPath, http.StatusForbidden, apiErr.HTTPStatus) + } + if apiErr.Message != test.wantErr { + t.Errorf("%s %s: expected message %q, got %q", test.method, test.requestPath, test.wantErr, apiErr.Message) + } + }) + } +} + func TestReplaceRemoteAdminServer(t *testing.T) { const testCert = `MIIDCTCCAfGgAwIBAgIUXsqJ1mY8pKlHQtI3HJ23x2eZPqwwDQYJKoZIhvcNAQEL BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTIzMDEwMTAwMDAwMFoXDTI0MDEw