From af6514b0a7779f265c6ee23f25740b6dcf77578b Mon Sep 17 00:00:00 2001 From: Santamaura Date: Mon, 1 Aug 2022 10:44:35 -0400 Subject: [PATCH] server: fix tenant auth on statements http endpoint This patch resolves a panic due to failed authorization by adding in the missing fields cluster.Settings and makePlanner function to the privilegechecker in tenant.go. Resolves #85404 Release note (bug fix): fixes a panic when loading tenant http endpoints for statement stats --- .../serverccl/statusccl/tenant_status_test.go | 19 +++++++++++++++++ .../serverccl/statusccl/tenant_test_utils.go | 4 ++++ pkg/server/admin.go | 10 +++++++-- pkg/server/server.go | 2 +- pkg/server/tenant.go | 21 +++++++++++++++++-- 5 files changed, 51 insertions(+), 5 deletions(-) diff --git a/pkg/ccl/serverccl/statusccl/tenant_status_test.go b/pkg/ccl/serverccl/statusccl/tenant_status_test.go index b55afb746b58..3cb32acc9587 100644 --- a/pkg/ccl/serverccl/statusccl/tenant_status_test.go +++ b/pkg/ccl/serverccl/statusccl/tenant_status_test.go @@ -105,6 +105,10 @@ func TestTenantStatusAPI(t *testing.T) { t.Run("tenant_ranges", func(t *testing.T) { testTenantRangesRPC(ctx, t, testHelper) }) + + t.Run("tenant_auth_statement", func(t *testing.T) { + testTenantAuthOnStatements(ctx, t, testHelper) + }) } func TestTenantCannotSeeNonTenantStats(t *testing.T) { @@ -1179,6 +1183,21 @@ func testTenantRangesRPC(_ context.Context, t *testing.T, helper *tenantTestHelp }) } +func testTenantAuthOnStatements(ctx context.Context, t *testing.T, helper *tenantTestHelper) { + client := helper.testCluster().tenantHTTPClient(t, 1, false) + defer client.Close() + err := client.GetJSONChecked("/_status/statements", &serverpb.StatementsResponse{}) + // Should return an error because the user is not admin and doesn't have any system + // privileges. + require.Error(t, err) + + // Once user has been granted the required system privilege there should be no error. + grantStmt := `GRANT SYSTEM VIEWACTIVITY TO authentic_user_noadmin;` + helper.tenantTestCluster.tenantConn(0).Exec(t, grantStmt) + err = client.GetJSONChecked("/_status/statements", &serverpb.StatementsResponse{}) + require.NoError(t, err) +} + // assertStartKeyInRange compares the pretty printed startKey with the provided // tenantPrefix key, ensuring that the startKey starts with the tenantPrefix. func assertStartKeyInRange(t *testing.T, startKey string, tenantPrefix roachpb.Key) { diff --git a/pkg/ccl/serverccl/statusccl/tenant_test_utils.go b/pkg/ccl/serverccl/statusccl/tenant_test_utils.go index 71c3b9064cd7..fcdbf58d50b0 100644 --- a/pkg/ccl/serverccl/statusccl/tenant_test_utils.go +++ b/pkg/ccl/serverccl/statusccl/tenant_test_utils.go @@ -217,6 +217,10 @@ func (c *httpClient) GetJSON(path string, response protoutil.Message) { require.NoError(c.t, err) } +func (c *httpClient) GetJSONChecked(path string, response protoutil.Message) error { + return httputil.GetJSON(c.client, c.baseURL+path, response) +} + func (c *httpClient) PostJSON(path string, request protoutil.Message, response protoutil.Message) { err := c.PostJSONChecked(path, request, response) require.NoError(c.t, err) diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 0f10702fa5dc..e998981b28d8 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -3420,8 +3420,14 @@ func (s *adminServer) dialNode( // adminPrivilegeChecker is a helper struct to check whether given usernames // have admin privileges. type adminPrivilegeChecker struct { - ie *sql.InternalExecutor - st *cluster.Settings + ie *sql.InternalExecutor + st *cluster.Settings + // makePlanner is a function that calls NewInternalPlanner + // to make a planner outside of the sql package. This is a hack + // to get around a Go package dependency cycle. See comment + // in pkg/scheduledjobs/env.go on planHookMaker. It should + // be cast to AuthorizationAccessor in order to use + // privilege checking functions. makePlanner func(opName string) (interface{}, func()) } diff --git a/pkg/server/server.go b/pkg/server/server.go index 97e4f5e43c0e..bcc2bb61b16c 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -716,7 +716,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) { lateBoundServer := &Server{} // TODO(tbg): give adminServer only what it needs (and avoid circular deps). - adminAuthzCheck := &adminPrivilegeChecker{ie: internalExecutor, st: st} + adminAuthzCheck := &adminPrivilegeChecker{ie: internalExecutor, st: st, makePlanner: nil} sAdmin := newAdminServer(lateBoundServer, adminAuthzCheck, internalExecutor) // These callbacks help us avoid a dependency on gossip in httpServer. diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go index e7011608995b..34ff963a6665 100644 --- a/pkg/server/tenant.go +++ b/pkg/server/tenant.go @@ -33,6 +33,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/rpc/nodedialer" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/debug" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/status" @@ -41,6 +42,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/flowinfra" "github.com/cockroachdb/cockroach/pkg/sql/optionalnodeliveness" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/sql/sqlinstance" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness" "github.com/cockroachdb/cockroach/pkg/util" @@ -210,7 +212,7 @@ func startTenantInternal( // going to assume that the tenant status server won't require // the SQL server object. tenantStatusServer := newTenantStatusServer( - baseCfg.AmbientCtx, &adminPrivilegeChecker{ie: args.circularInternalExecutor}, + baseCfg.AmbientCtx, nil, args.sessionRegistry, args.closedSessionCache, args.flowScheduler, baseCfg.Settings, nil, args.rpcContext, args.stopper, ) @@ -220,6 +222,22 @@ func startTenantInternal( if err != nil { return nil, nil, nil, "", "", err } + adminAuthzCheck := &adminPrivilegeChecker{ + ie: s.execCfg.InternalExecutor, + st: args.Settings, + makePlanner: func(opName string) (interface{}, func()) { + txn := args.db.NewTxn(ctx, "check-system-privilege") + return sql.NewInternalPlanner( + opName, + txn, + username.RootUserName(), + &sql.MemoryMetrics{}, + s.execCfg, + sessiondatapb.SessionData{}, + ) + }, + } + tenantStatusServer.privilegeChecker = adminAuthzCheck tenantStatusServer.sqlServer = s drainServer = newDrainServer(baseCfg, args.stopper, args.grpc, s) @@ -260,7 +278,6 @@ func startTenantInternal( } debugServer := debug.NewServer(baseCfg.AmbientCtx, args.Settings, s.pgServer.HBADebugFn(), s.execCfg.SQLStatusServer) - adminAuthzCheck := &adminPrivilegeChecker{ie: s.execCfg.InternalExecutor} parseNodeIDFn := func(s string) (roachpb.NodeID, bool, error) { return roachpb.NodeID(0), false, errors.New("tenants cannot proxy to KV Nodes")