diff --git a/docs/Configuring.md b/docs/Configuring.md index 7be5619d78..9062f88469 100644 --- a/docs/Configuring.md +++ b/docs/Configuring.md @@ -135,7 +135,7 @@ Root level key `server` | `auth.dpop.enforce` | If true, DPoP bindings on Access Tokens are enforced. | `false` | OPENTDF_SERVER_AUTH_DPOP_ENFORCE | | `auth.enforceDPoP` | [DEPRECATED] Use `auth.dpop.enforce`. Still honored: DPoP is enforced when either field is true. | `false` | OPENTDF_SERVER_AUTH_ENFORCEDPOP | | `cryptoProvider` | A list of public/private keypairs and their use. Described [below](#crypto-provider) | empty | | -| `enable_pprof` | Enable golang performance profiling | `false` | OPENTDF_SERVER_ENABLE_PPROF | +| `enable_pprof` | Enable Go performance profiling under `/debug/pprof/`; uses HTTP authentication and authorization when server authentication is enabled, with collection durations capped at 30 seconds | `false` | OPENTDF_SERVER_ENABLE_PPROF | | `grpc.reflection` | The configuration for the grpc server. | `true` | OPENTDF_SERVER_GRPC_REFLECTION | | `public_hostname` | The public facing hostname for the server. | | OPENTDF_SERVER_PUBLIC_HOSTNAME | | `host` | The host address for the server. | `""` | OPENTDF_SERVER_HOST | diff --git a/service/internal/server/server.go b/service/internal/server/server.go index 7303f0afcf..a5c9f43189 100644 --- a/service/internal/server/server.go +++ b/service/internal/server/server.go @@ -11,6 +11,7 @@ import ( "net/http/pprof" "net/textproto" "regexp" + "strconv" "strings" "time" @@ -34,9 +35,11 @@ import ( ) const ( - defaultWriteTimeout time.Duration = 10 * time.Second - defaultReadTimeout time.Duration = 10 * time.Second - shutdownTimeout time.Duration = 5 * time.Second + defaultWriteTimeout time.Duration = 10 * time.Second + defaultReadTimeout time.Duration = 10 * time.Second + shutdownTimeout time.Duration = 5 * time.Second + maxPprofDurationSeconds = 30 + maxPprofFormBodyBytes int64 = 1024 ) type Error string @@ -347,6 +350,16 @@ func newHTTPServer(c Config, connectRPC http.Handler, extraHTTP http.Handler, a httpHandler := extraHTTP + // Keep profiling behind the same authentication and authorization boundary as + // the other HTTP handlers. + if c.EnablePprof { + httpHandler = pprofHandler(httpHandler) + // Need to extend write timeout to collect pprof data. + if c.HTTPServerConfig.WriteTimeout < maxPprofDurationSeconds*time.Second { + c.HTTPServerConfig.WriteTimeout = maxPprofDurationSeconds * time.Second + } + } + // Add authN interceptor to extra handlers. if c.Auth.Enabled { httpHandler = a.MuxHandler(httpHandler) @@ -395,15 +408,6 @@ func newHTTPServer(c Config, connectRPC http.Handler, extraHTTP http.Handler, a httpHandler = corsHandler.Handler(httpHandler) } - // Enable pprof - if c.EnablePprof { - httpHandler = pprofHandler(httpHandler) - // Need to extend write timeout to collect pprof data. - if c.HTTPServerConfig.WriteTimeout < 30*time.Second { - c.HTTPServerConfig.WriteTimeout = 30 * time.Second //nolint:mnd // easier to read that we are overriding the default - } - } - var handler http.Handler if !c.TLS.Enabled { handler = h2c.NewHandler(routeConnectRPCRequests(connectRPC, httpHandler), &http2.Server{}) @@ -447,10 +451,14 @@ func routeConnectRPCRequests(connectRPC http.Handler, httpHandler http.Handler) }) } -// ppprof handler +// pprofHandler routes profiling requests and bounds caller-controlled collection durations. func pprofHandler(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.HasPrefix(r.URL.Path, "/debug/pprof/") { + if supportsPprofDuration(r.URL.Path) && !validatePprofDuration(w, r) { + return + } + switch r.URL.Path { case "/debug/pprof/cmdline": pprof.Cmdline(w, r) @@ -469,6 +477,51 @@ func pprofHandler(h http.Handler) http.Handler { }) } +func supportsPprofDuration(path string) bool { + switch path { + case "/debug/pprof/", "/debug/pprof/cmdline", "/debug/pprof/symbol": + return false + default: + return true + } +} + +func validatePprofDuration(w http.ResponseWriter, r *http.Request) bool { + r.Body = http.MaxBytesReader(w, r.Body, maxPprofFormBodyBytes) + if err := r.ParseForm(); err != nil { + writePprofFormError(w, err) + return false + } + //nolint:gosec // MaxBytesReader bounds the complete request body above. + if err := r.ParseMultipartForm(maxPprofFormBodyBytes); err != nil && !errors.Is(err, http.ErrNotMultipart) { + writePprofFormError(w, err) + return false + } + + seconds := r.FormValue("seconds") + if seconds == "" { + return true + } + + duration, err := strconv.ParseFloat(seconds, 64) + if err == nil && duration > maxPprofDurationSeconds { + http.Error(w, fmt.Sprintf("pprof duration must not exceed %d seconds", maxPprofDurationSeconds), http.StatusBadRequest) + return false + } + + return true +} + +func writePprofFormError(w http.ResponseWriter, err error) { + var maxBytesError *http.MaxBytesError + if errors.As(err, &maxBytesError) { + http.Error(w, "pprof request body too large", http.StatusRequestEntityTooLarge) + return + } + + http.Error(w, "invalid pprof form body", http.StatusBadRequest) +} + func newConnectRPC(c Config, authInts []connect.Interceptor, ints []connect.Interceptor, logger *logger.Logger) (*ConnectRPC, error) { interceptors := make([]connect.HandlerOption, 0) diff --git a/service/internal/server/server_test.go b/service/internal/server/server_test.go index 7f8f9429a9..e5b95f7486 100644 --- a/service/internal/server/server_test.go +++ b/service/internal/server/server_test.go @@ -1,9 +1,13 @@ package server import ( + "bytes" "context" + "fmt" + "mime/multipart" "net/http" "net/http/httptest" + "reflect" "strings" "testing" @@ -80,6 +84,154 @@ func TestMergeStringSlices(t *testing.T) { } } +func TestNewHTTPServer_PprofRequiresAuthentication(t *testing.T) { + server, err := newHTTPServer(Config{ + Auth: auth.Config{Enabled: true}, + EnablePprof: true, + }, http.NotFoundHandler(), http.NotFoundHandler(), &auth.Authentication{}, logger.CreateTestLogger()) + require.NoError(t, err) + + recorder := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, "/debug/pprof/", nil) + server.Handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusUnauthorized, recorder.Code) + assert.Contains(t, recorder.Body.String(), "missing authorization header") +} + +func TestPprofHandler(t *testing.T) { + fallback := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + handler := pprofHandler(fallback) + + tests := []struct { + name string + target string + wantStatus int + }{ + { + name: "serves profiling index", + target: "/debug/pprof/", + wantStatus: http.StatusOK, + }, + { + name: "rejects long CPU profile", + target: "/debug/pprof/profile?seconds=31", + wantStatus: http.StatusBadRequest, + }, + { + name: "rejects long trace", + target: "/debug/pprof/trace?seconds=30.1", + wantStatus: http.StatusBadRequest, + }, + { + name: "rejects long delta profile", + target: "/debug/pprof/goroutine?seconds=60", + wantStatus: http.StatusBadRequest, + }, + { + name: "preserves invalid duration default", + target: "/debug/pprof/?seconds=invalid", + wantStatus: http.StatusOK, + }, + { + name: "passes through non-profiling request", + target: "/healthz", + wantStatus: http.StatusNoContent, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + recorder := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodGet, test.target, nil) + + handler.ServeHTTP(recorder, request) + + assert.Equal(t, test.wantStatus, recorder.Code) + }) + } +} + +func TestPprofHandlerRejectsBodyDuration(t *testing.T) { + fallback := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + handler := pprofHandler(fallback) + + t.Run("URL encoded", func(t *testing.T) { + request := httptest.NewRequest(http.MethodPost, "/debug/pprof/profile", strings.NewReader("seconds=31")) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + }) + + t.Run("multipart", func(t *testing.T) { + var body bytes.Buffer + writer := multipart.NewWriter(&body) + require.NoError(t, writer.WriteField("seconds", "31")) + require.NoError(t, writer.Close()) + + request := httptest.NewRequest(http.MethodPost, "/debug/pprof/trace", &body) + request.Header.Set("Content-Type", writer.FormDataContentType()) + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + }) +} + +func TestPprofHandlerRejectsOversizedBody(t *testing.T) { + fallback := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + handler := pprofHandler(fallback) + + t.Run("URL encoded", func(t *testing.T) { + body := "seconds=1&padding=" + strings.Repeat("x", int(maxPprofFormBodyBytes)) + request := httptest.NewRequest(http.MethodPost, "/debug/pprof/profile", strings.NewReader(body)) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusRequestEntityTooLarge, recorder.Code) + }) + + t.Run("multipart", func(t *testing.T) { + var body bytes.Buffer + writer := multipart.NewWriter(&body) + require.NoError(t, writer.WriteField("seconds", "1")) + require.NoError(t, writer.WriteField("padding", strings.Repeat("x", int(maxPprofFormBodyBytes)))) + require.NoError(t, writer.Close()) + + request := httptest.NewRequest(http.MethodPost, "/debug/pprof/trace", &body) + request.Header.Set("Content-Type", writer.FormDataContentType()) + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusRequestEntityTooLarge, recorder.Code) + }) +} + +func TestPprofHandlerPreservesSymbolPostBody(t *testing.T) { + programCounter := reflect.ValueOf(TestPprofHandlerPreservesSymbolPostBody).Pointer() + request := httptest.NewRequest(http.MethodPost, "/debug/pprof/symbol", strings.NewReader(fmt.Sprintf("%#x", programCounter))) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + recorder := httptest.NewRecorder() + + pprofHandler(http.NotFoundHandler()).ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusOK, recorder.Code) + assert.Contains(t, recorder.Body.String(), "TestPprofHandlerPreservesSymbolPostBody") +} + func Test_OpenTDFServer_RegisterReflectionHandlers_Enabled_RegistersOnlyExternalHandlers(t *testing.T) { server := newReflectionTestServer(t, true)