From 0db60c57f49e238f54445dd3304b03adc57ba91a Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Mon, 6 Feb 2023 17:50:18 -0500 Subject: [PATCH] admin: statement diagnostics uses correct auth helpers 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/` 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. --- pkg/server/admin.go | 20 ++++++++++++-------- pkg/server/admin_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 6fed5a53fb33..cb29d6144129 100644 --- a/pkg/server/admin.go +++ b/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, ) diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go index 7b79fe77e90d..8bcd19b75580 100644 --- a/pkg/server/admin_test.go +++ b/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)