Skip to content

Commit

Permalink
admin: statement diagnostics uses correct auth helpers
Browse files Browse the repository at this point in the history
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 cockroachdb#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.
  • Loading branch information
dhartunian committed Mar 20, 2023
1 parent 54129eb commit 6d7107f
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 8 deletions.
23 changes: 15 additions & 8 deletions pkg/server/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,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.server.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.
Expand Down Expand Up @@ -2474,14 +2482,13 @@ 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 := userFromContext(ctx)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
// This endpoint uses `getSQLUsername` to get its user metadata
// due to the fact that it is implemented as a raw HTTP handler
// instead of a gRPC handler.
sqlUsername := getSQLUsername(ctx)
row, err := s.server.sqlServer.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,
)
Expand All @@ -2500,7 +2507,7 @@ func (s *adminServer) getStatementBundle(ctx context.Context, id int64, w http.R
for _, chunkID := range chunkIDs {
chunkRow, err := s.server.sqlServer.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,
)
Expand Down
37 changes: 37 additions & 0 deletions pkg/server/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,43 @@ func TestAdminDebugRedirect(t *testing.T) {
}
}

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)
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)
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)
Expand Down

0 comments on commit 6d7107f

Please sign in to comment.