Merge cockroachdb#85407

85407: server: fix tenant auth on statements http endpoint r=Santamaura a=Santamaura 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 cockroachdb#85404 Release note (bug fix): fixes a panic when loading tenant http endpoints for statement stats After changes on multi-tenant: Without system privileges: ![Screen Shot 2022-08-01 at 5 28 02 PM](https://user-images.githubusercontent.com/17861665/182249941-29cb6f7d-3e38-47e3-9349-296486239029.png) With `VIEWACTIVITY`: ![Screen Shot 2022-08-01 at 5 28 33 PM](https://user-images.githubusercontent.com/17861665/182249905-9000b851-ac65-460a-9065-ba2f239eed39.png) Co-authored-by: Santamaura <[email protected]>
irfansharif · Aug 8, 2022 · 1ff642c · 1ff642c
2 parents b667254 + af6514b
commit 1ff642c
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 5 deletions.
diff --git 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
@@ -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
@@ -3422,8 +3422,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
@@ -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
@@ -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")