fix(http): serve /status/diagnostics only on the internal interface

docs/pages/deployment/monitoring.rst

@@ -87,6 +87,10 @@ Basic diagnostics
     this page is intended to be read by humans, not machines.
     all but the ``status`` entry are related to V5 functionality (gRPC network, VDRv1 and VCRv1 APIs).
 
+.. note::
+
+    this endpoint is only served on the internal interface. It discloses the exact software version, the node's network peers and data store counts, so requests arriving through the public interface are refused with HTTP 403. The ``/status`` endpoint (without ``/diagnostics``) remains available on both interfaces as a liveness signal.
+
 Returns the status of the various services in ``yaml`` format:
 
 .. code-block:: text

docs/pages/release_notes.rst

@@ -14,6 +14,7 @@ Unreleased
 * #4233: ``request-credential`` API gains an optional ``credential_request_params`` JSON object overlaid on top of the OpenID4VCI Credential Request body sent to the issuer. Lets the wallet talk to issuers that accept additional fields, or to override the credential request entirely.
 
 ## Security
+* #4442: ``/status/diagnostics`` is now only served on the internal interface; requests through the public interface are refused with HTTP 403 and an explanatory message. The endpoint discloses the exact software version, the gRPC peer list and data store counts, and the deployment documentation has always listed it as internal. ``/status`` remains available on both interfaces as a liveness signal, so external load balancers and uptime monitors probing ``/status`` are unaffected. If an external monitor probes ``/status/diagnostics``, point it at ``/status`` or at the internal interface instead. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4442
 * #4421: Stop reflecting fetched HTTP response bodies in API responses. The OAuth2 and OpenID4VCI callback handlers no longer place a remote endpoint's response body or error text into the returned ``error_description``, the did:web resolver no longer returns the fetched document body in its parse error, and the Discovery Service client no longer includes the remote server's error response in errors returned through the discovery APIs. Such content is now logged (truncated) for diagnostics instead. Static context such as the endpoint that failed is retained. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4421
 * #4420: Harden the strict-mode HTTP client against SSRF. In strict mode the client now refuses at connect time to reach non-public addresses (loopback, private/RFC1918, unique local, link-local and unspecified), checked against the resolved IP so DNS-rebinding cannot bypass it, and refuses to follow a redirect that downgrades from HTTPS to HTTP. Cloud provider metadata endpoints are always blocked, following the OWASP SSRF prevention cheat sheet. Deployments that legitimately reach a private address for an internal flow (such as an internal credential offering or OAuth user flow) can permit specific ranges with ``http.client.allowedinternalcidrs``; publicly routable ranges that are internal-only can additionally be blocked with ``http.client.deniedcidrs``, which takes precedence. Reported by @raysabee, fixed by @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4420
 

http/engine.go

@@ -42,6 +42,20 @@ import (
 
 const moduleName = "HTTP"
 
+// diagnosticsPath is the status endpoint that reports software version, git commit, peer list
+// and store counts. It is only meant to be available on the internal interface, but it shares
+// its first path segment with /status, which deliberately stays public as a liveness signal
+// for load balancers and uptime monitors.
+//
+// Interface binding (MultiEcho) resolves on the first path segment only and rejects subpath
+// binds, so /status and /status/diagnostics cannot be bound to different interfaces through
+// the bind table: the route is registered on every interface that serves /status. Therefore
+// internalOnlyMiddleware refuses requests that reach this path through any interface other
+// than the internal one. If the bind table ever learns longest-prefix matching so subpaths
+// can be bound to their own interface, this path should become a regular internal-only bind
+// and the middleware can be removed.
+const diagnosticsPath = StatusPath + "/diagnostics"
+
 // New returns a new HTTP engine. The callback is called when an HTTP interface shuts down unexpectedly.
 func New(serverShutdownCb func(), signingKeyResolver cryptoEngine.KeyResolver) *Engine {
 	return &Engine{
@@ -97,9 +111,52 @@ func (h *Engine) Configure(serverConfig core.ServerConfig) error {
 	h.applyTracingMiddleware(h.server)
 	h.applyRateLimiterMiddleware(h.server, serverConfig)
 	h.applyLoggerMiddleware(h.server, []string{MetricsPath, StatusPath, HealthPath}, h.config.Log)
+	diagnosticsGuard, err := internalOnlyMiddleware(h.config.Internal.Address, diagnosticsPath)
+	if err != nil {
+		return err
+	}
+	h.server.Use(diagnosticsGuard)
 	return h.applyAuthMiddleware(h.server, InternalPath, h.config.Internal.Auth)
 }
 
+// internalOnlyMiddleware returns middleware that refuses requests to the given path (and its
+// subpaths) unless the connection was accepted on the interface the internal API is bound to.
+//
+// How the interface is determined: Go's net/http server stores the connection's local address
+// (the address the listener accepted on) in the request context under http.LocalAddrContextKey.
+// Every HTTP interface listens on its own port, since binding the same port twice fails at
+// startup, so comparing the local address port against the internal interface's configured
+// port identifies the interface the request arrived on. When the operator configures the
+// public and internal interface to the same address there is only one server and the ports
+// always match, which honors that (explicitly configured) setup. The comparison uses only the
+// port, not the host: the configured bind host (e.g. ":8081" or "0.0.0.0:8081") does not have
+// to equal the connection's concrete local IP.
+//
+// Requests are refused with 403 rather than 404: the endpoint's existence is public knowledge
+// (the node is open source), so a 404 would hide nothing from an attacker, while the explicit
+// message tells an operator whose external monitor probes this path exactly why it broke.
+// When the local address is missing from the request context or unparseable, the request is
+// refused (fail closed); this cannot happen with Go's net/http server, which always sets it.
+func internalOnlyMiddleware(internalAddress string, path string) (echo.MiddlewareFunc, error) {
+	_, internalPort, err := net.SplitHostPort(internalAddress)
+	if err != nil {
+		return nil, fmt.Errorf("invalid internal address %s: %w", internalAddress, err)
+	}
+	return func(next echo.HandlerFunc) echo.HandlerFunc {
+		return func(c echo.Context) error {
+			if !matchesPath(c.Request().URL.Path, path) {
+				return next(c)
+			}
+			if localAddr, ok := c.Request().Context().Value(http.LocalAddrContextKey).(net.Addr); ok {
+				if _, port, err := net.SplitHostPort(localAddr.String()); err == nil && port == internalPort {
+					return next(c)
+				}
+			}
+			return echo.NewHTTPError(http.StatusForbidden, "only available on the internal interface")
+		}
+	}, nil
+}
+
 func (h *Engine) configureClient(serverConfig core.ServerConfig) error {
 	client.StrictMode = serverConfig.Strictmode
 	if err := client.SetAllowedNonPublicCIDRs(h.config.Client.AllowedInternalCIDRs); err != nil {

http/engine_test.go

@@ -387,6 +387,49 @@ func TestEngine_LoggingMiddleware(t *testing.T) {
 	})
 }
 
+func TestEngine_DiagnosticsInternalOnly(t *testing.T) {
+	// Configure sets the package-global client strict mode flag; restore it afterwards.
+	oldStrictMode := client.StrictMode
+	t.Cleanup(func() { client.StrictMode = oldStrictMode })
+	noop := func() {}
+	engine := New(noop, nil)
+	engine.config = createTestConfig()
+	require.NoError(t, engine.Configure(*core.NewServerConfig()))
+	// Simulate the routes the status engine registers under /status.
+	engine.Router().GET("/status", func(c echo.Context) error {
+		return c.String(http.StatusOK, "OK")
+	})
+	engine.Router().GET("/status/diagnostics", func(c echo.Context) error {
+		return c.String(http.StatusOK, "diagnostics")
+	})
+	require.NoError(t, engine.Start())
+	defer engine.Shutdown()
+	assertServerStarted(t, engine.config.Public.Address)
+	assertServerStarted(t, engine.config.Internal.Address)
+
+	t.Run("/status is available on the public interface", func(t *testing.T) {
+		response, err := http.Get("http://" + engine.config.Public.Address + "/status")
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusOK, response.StatusCode)
+	})
+	t.Run("/status/diagnostics is available on the internal interface", func(t *testing.T) {
+		response, err := http.Get("http://" + engine.config.Internal.Address + "/status/diagnostics")
+		require.NoError(t, err)
+		require.Equal(t, http.StatusOK, response.StatusCode)
+		body, err := io.ReadAll(response.Body)
+		require.NoError(t, err)
+		assert.Equal(t, "diagnostics", string(body))
+	})
+	t.Run("/status/diagnostics is forbidden on the public interface", func(t *testing.T) {
+		response, err := http.Get("http://" + engine.config.Public.Address + "/status/diagnostics")
+		require.NoError(t, err)
+		body, err := io.ReadAll(response.Body)
+		require.NoError(t, err)
+		assert.Equal(t, http.StatusForbidden, response.StatusCode)
+		assert.NotContains(t, string(body), "diagnostics")
+	})
+}
+
 func assertServerStarted(t *testing.T, address string) {
 	t.Helper()
 	var err error