Merge #99051

99051: admin: statement diagnostics uses correct auth helpers r=knz a=dhartunian

Previously, the statement diagnostics HTTP handler was initialized using the incorrect `ctx` value. This caused the HTTP request context to not be correctly handed down to the handler. Furthermore, the call to `userFromIncomingRPCContext` was incorrect in this scenario since it relied on a gRPC context being populated with HTTP session information, which did not exist. That code sets a `root` user when context is missing because the gRPC handlers are used for inter-node communication *and* HTTP with the DB console and external tools.

This commit attaches the request context to the diagnostics bundle handler correctly, and amends the authorization code to use `userFromHTTPAuthInfoContext` which correctly reads the session cookie info from the request (like many `api/v2` handlers do since those exist outside the gRPC infrastructure).

Resolves #99049

Epic: None

Release note (security update): Previously, users could gain unauthorized access to statement diagnostic bundles they did not create if they requested the bundle through an HTTP request to `/_admin/v1/stmtbundle/<id>` and correctly guessed its (non-secret) ID.  This change locks down this endpoint behind the usual SQL gating that correctly uses the SQL user in the HTTP session as identified by their cookie.

Co-authored-by: David Hartunian <[email protected]>

pkg/server/admin.go

@@ -296,7 +296,15 @@ func (s *adminServer) RegisterGateway(
 			http.Error(w, "invalid id", http.StatusBadRequest)
 			return
 		}
-		s.getStatementBundle(ctx, id, w)
+		// Add default user when running in Insecure mode because we don't
+		// retrieve the user from gRPC metadata (which falls back to `root`)
+		// but from HTTP metadata (which does not).
+		if s.sqlServer.cfg.Insecure {
+			ctx := req.Context()
+			ctx = context.WithValue(ctx, webSessionUserKey{}, username.RootUser)
+			req = req.WithContext(ctx)
+		}
+		s.getStatementBundle(req.Context(), id, w)
 	})
 
 	// Register the endpoints defined in the proto.
@@ -2612,14 +2620,10 @@ func (s *adminServer) QueryPlan(
 // getStatementBundle retrieves the statement bundle with the given id and
 // writes it out as an attachment.
 func (s *adminServer) getStatementBundle(ctx context.Context, id int64, w http.ResponseWriter) {
-	sessionUser, err := userFromIncomingRPCContext(ctx)
-	if err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
-		return
-	}
+	sqlUsername := userFromHTTPAuthInfoContext(ctx)
 	row, err := s.internalExecutor.QueryRowEx(
 		ctx, "admin-stmt-bundle", nil, /* txn */
-		sessiondata.InternalExecutorOverride{User: sessionUser},
+		sessiondata.InternalExecutorOverride{User: sqlUsername},
 		"SELECT bundle_chunks FROM system.statement_diagnostics WHERE id=$1 AND bundle_chunks IS NOT NULL",
 		id,
 	)
@@ -2638,7 +2642,7 @@ func (s *adminServer) getStatementBundle(ctx context.Context, id int64, w http.R
 	for _, chunkID := range chunkIDs {
 		chunkRow, err := s.internalExecutor.QueryRowEx(
 			ctx, "admin-stmt-bundle", nil, /* txn */
-			sessiondata.InternalExecutorOverride{User: sessionUser},
+			sessiondata.InternalExecutorOverride{User: sqlUsername},
 			"SELECT data FROM system.statement_bundle_chunks WHERE id=$1",
 			chunkID,
 		)

pkg/server/admin_test.go

@@ -359,6 +359,43 @@ func generateRandomName() string {
 	return ng.GenerateOne(42)
 }
 
+func TestAdminAPIStatementDiagnosticsBundle(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(context.Background())
+	ts := s.(*TestServer)
+
+	query := "EXPLAIN ANALYZE (DEBUG) SELECT 'secret'"
+	_, err := db.Exec(query)
+	require.NoError(t, err)
+
+	query = "SELECT id FROM system.statement_diagnostics LIMIT 1"
+	idRow, err := db.Query(query)
+	require.NoError(t, err)
+	var diagnosticRow string
+	if idRow.Next() {
+		err = idRow.Scan(&diagnosticRow)
+		require.NoError(t, err)
+	} else {
+		t.Fatal("no results")
+	}
+
+	client, err := ts.GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession)
+	require.NoError(t, err)
+	resp, err := client.Get(ts.AdminURL() + "/_admin/v1/stmtbundle/" + diagnosticRow)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	require.Equal(t, 500, resp.StatusCode)
+
+	adminClient, err := ts.GetAuthenticatedHTTPClient(true, serverutils.SingleTenantSession)
+	require.NoError(t, err)
+	adminResp, err := adminClient.Get(ts.AdminURL() + "/_admin/v1/stmtbundle/" + diagnosticRow)
+	require.NoError(t, err)
+	defer adminResp.Body.Close()
+	require.Equal(t, 200, adminResp.StatusCode)
+}
+
 func TestAdminAPIDatabases(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)