Merge pull request #50489 from bdarnell/backport19.2-50483

release-19.2: server: Apply cookie auth to /debug/ endponts

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)
 					}

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)
 	}

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.