diff --git a/pkg/acceptance/debug_remote_test.go b/pkg/acceptance/debug_remote_test.go index b118313bd9f0..997c5d3cc82c 100644 --- a/pkg/acceptance/debug_remote_test.go +++ b/pkg/acceptance/debug_remote_test.go @@ -15,6 +15,7 @@ import ( gosql "database/sql" "fmt" "net/http" + "strings" "testing" "github.com/cockroachdb/cockroach/pkg/acceptance/cluster" @@ -46,6 +47,12 @@ func testDebugRemote(t *testing.T) { } defer db.Close() + stdout, stderr, err := l.ExecCLI(ctx, 0, []string{"auth-session", "login", "root", "--only-cookie"}) + if err != nil { + t.Fatalf("auth-session failed: %s\nstdout: %s\nstderr: %s\n", err, stdout, stderr) + } + cookie := strings.Trim(stdout, "\n") + testCases := []struct { remoteDebug string status int @@ -73,7 +80,12 @@ func testDebugRemote(t *testing.T) { "/debug/logspy?duration=1ns", } { t.Run(url, func(t *testing.T) { - resp, err := cluster.HTTPClient.Get(l.URL(ctx, 0) + url) + req, err := http.NewRequest("GET", l.URL(ctx, 0)+url, nil) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Cookie", cookie) + resp, err := cluster.HTTPClient.Do(req) if err != nil { t.Fatalf("%d: %v", i, err) } diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go index 5ae896953202..e7ee1c8edcda 100644 --- a/pkg/server/admin_test.go +++ b/pkg/server/admin_test.go @@ -199,18 +199,70 @@ func TestAdminDebugTrace(t *testing.T) { } } +func TestAdminDebugAuth(t *testing.T) { + defer leaktest.AfterTest(t)() + s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(context.Background()) + ts := s.(*TestServer) + + url := debugURL(s) + + // Unauthenticated. + client, err := ts.GetHTTPClient() + if err != nil { + t.Fatal(err) + } + resp, err := client.Get(url) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("expected status code %d; got %d", http.StatusUnauthorized, resp.StatusCode) + } + + // Authenticated as non-admin. + client, err = ts.GetAuthenticatedHTTPClient(false) + if err != nil { + t.Fatal(err) + } + resp, err = client.Get(url) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("expected status code %d; got %d", http.StatusUnauthorized, resp.StatusCode) + } + + // Authenticated as admin. + client, err = ts.GetAuthenticatedHTTPClient(true) + if err != nil { + t.Fatal(err) + } + resp, err = client.Get(url) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("expected status code %d; got %d", http.StatusOK, resp.StatusCode) + } +} + // TestAdminDebugRedirect verifies that the /debug/ endpoint is redirected to on // incorrect /debug/ paths. func TestAdminDebugRedirect(t *testing.T) { defer leaktest.AfterTest(t)() s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) - defer s.Stopper().Stop(context.TODO()) + defer s.Stopper().Stop(context.Background()) + ts := s.(*TestServer) expURL := debugURL(s) origURL := expURL + "incorrect" - // There are no particular permissions on admin endpoints, TestUser is fine. - client, err := testutils.NewTestBaseContext(TestUser).GetHTTPClient() + // Must be admin to access debug endpoints + client, err := ts.GetAdminAuthenticatedHTTPClient() if err != nil { t.Fatal(err) } diff --git a/pkg/server/server.go b/pkg/server/server.go index 34317daf0180..f18b2142a47e 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -84,6 +84,7 @@ import ( opentracing "github.com/opentracing/opentracing-go" "github.com/pkg/errors" "google.golang.org/grpc" + "google.golang.org/grpc/metadata" ) var ( @@ -1158,13 +1159,6 @@ func (s *Server) Start(ctx context.Context) error { } } - // Enable the debug endpoints first to provide an earlier window into what's - // going on with the node in advance of exporting node functionality. - // - // TODO(marc): when cookie-based authentication exists, apply it to all web - // endpoints. - s.mux.Handle(debug.Endpoint, debug.NewServer(s.st)) - // Initialize grpc-gateway mux and context in order to get the /health // endpoint working even before the node has fully initialized. jsonpb := &protoutil.JSONPb{ @@ -1593,8 +1587,36 @@ func (s *Server) Start(ctx context.Context) error { // The /login endpoint is, by definition, available pre-authentication. s.mux.Handle(loginPath, gwMux) s.mux.Handle(logoutPath, authHandler) + // The /_status/vars endpoint is not authenticated either. Useful for monitoring. s.mux.Handle(statusVars, http.HandlerFunc(s.status.handleVars)) + + // Register debugging endpoints. + var debugHandler http.Handler = debug.NewServer(s.st) + if s.cfg.RequireWebSession() { + origDebug := debugHandler + // TODO(bdarnell): Refactor our authentication stack. + // authenticationMux guarantees that we have a non-empty user + // session, but our machinery for verifying the roles of a user + // lives on adminServer and is tied to GRPC metadata. + debugHandler = newAuthenticationMux(s.authentication, http.HandlerFunc( + func(w http.ResponseWriter, req *http.Request) { + md := forwardAuthenticationMetadata(req.Context(), req) + authCtx := metadata.NewIncomingContext(req.Context(), md) + _, err := s.admin.requireAdminUser(authCtx) + if err == errInsufficientPrivilege { + http.Error(w, "admin privilege required", http.StatusUnauthorized) + return + } else if err != nil { + log.Infof(authCtx, "web session error: %s", err) + http.Error(w, "error checking authentication", http.StatusInternalServerError) + return + } + origDebug.ServeHTTP(w, req) + })) + } + s.mux.Handle(debug.Endpoint, debugHandler) + log.Event(ctx, "added http endpoints") // Attempt to upgrade cluster version.