diff --git a/README.md b/README.md index 86fa5e7..cdfbf8f 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,36 @@ e.Use(echootel.NewMiddlewareWithConfig(echootel.Config{ })) ``` +### Public (internet-facing) endpoints + +By default, the middleware trusts the incoming trace context (e.g. `traceparent` header) and continues +that trace as the parent of the server span. For endpoints exposed to untrusted clients this allows +callers to inject arbitrary trace IDs into your traces or suppress tracing entirely with a +`sampled=0` flag. + +Set `PublicEndpointFn` to start a new trace instead. The incoming trace context, +if present, is recorded as a span link rather than being used as the parent. To treat every +request as public: + +```go +e.Use(echootel.NewMiddlewareWithConfig(echootel.Config{ + PublicEndpointFn: func(c *echo.Context, remote trace.SpanContext) bool { return true }, +})) +``` + +The decision is made per request, so the same server can serve both internal and public routes + +```go +e.Use(echootel.NewMiddlewareWithConfig(echootel.Config{ + PublicEndpointFn: func(c *echo.Context, remote trace.SpanContext) bool { + return !strings.HasPrefix(c.Request().URL.Path, "/internal/") + }, +})) +``` + +The second argument is the remote span context extracted from the incoming request, so the +decision can also be based on the incoming trace context itself. + Retrieving the tracer from the Echo context ```go tp, err := echo.ContextGet[trace.Tracer](c, echootel.TracerKey) diff --git a/otel.go b/otel.go index bd90e01..2691769 100644 --- a/otel.go +++ b/otel.go @@ -48,6 +48,25 @@ type Config struct { // Skipper defines a function to skip middleware. Skipper middleware.Skipper + // PublicEndpointFn decides per request whether it should be handled as a public + // (internet-facing) endpoint receiving requests from untrusted clients. + // + // When the function returns true, the incoming trace context (e.g. `traceparent`/`tracestate` + // headers) is not used as the parent of the server span. Instead, a new root span (new trace) + // is started and the incoming remote span context, if valid, is recorded as a span link. This + // prevents untrusted clients from injecting arbitrary trace IDs into your traces or + // influencing the sampling decision (e.g. suppressing tracing with a `sampled=0` flag). + // + // To treat every request as public: + // + // config.PublicEndpointFn = func(c *echo.Context, remote oteltrace.SpanContext) bool { return true } + // + // The remote span context extracted from the incoming request by Propagators is passed as + // the second argument. It can be invalid (see trace.SpanContext.IsValid) when the request + // carries no trace context. This allows, for example, trusting only trace contexts that + // originate from known internal systems. + PublicEndpointFn func(c *echo.Context, remote oteltrace.SpanContext) bool + // OnNextError is used to specify how errors returned from the next middleware / handler are handled. OnNextError OnErrorFunc @@ -190,8 +209,18 @@ func (config Config) ToMiddleware() (echo.MiddlewareFunc, error) { spanStartOptions = append(spanStartOptions, config.SpanStartOptions...) } + ctx := config.Propagators.Extract(request.Context(), propagation.HeaderCarrier(request.Header)) + remote := oteltrace.SpanContextFromContext(ctx) + if config.PublicEndpointFn != nil && config.PublicEndpointFn(c, remote) { + spanStartOptions = append(spanStartOptions, oteltrace.WithNewRoot()) + // keep the incoming (untrusted) trace context visible by linking it to the new root span + if remote.IsValid() && remote.IsRemote() { + spanStartOptions = append(spanStartOptions, oteltrace.WithLinks(oteltrace.Link{SpanContext: remote})) + } + } + ctx, span := tracer.Start( - config.Propagators.Extract(request.Context(), propagation.HeaderCarrier(request.Header)), + ctx, SpanNameFormatter(ev), spanStartOptions..., ) diff --git a/otel_test.go b/otel_test.go index b4ccb73..fb11b12 100644 --- a/otel_test.go +++ b/otel_test.go @@ -105,6 +105,142 @@ func TestPropagationWithCustomPropagators(t *testing.T) { assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler") } +func TestPublicEndpointFnAlwaysPublic(t *testing.T) { + tests := []struct { + name string + traceFlags trace.TraceFlags + }{ + { + name: "sampled remote trace context is not used as parent", + traceFlags: trace.FlagsSampled, + }, + { + name: "unsampled remote trace context can not suppress tracing", + traceFlags: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter)) + prop := propagation.TraceContext{} + + remoteSc := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: trace.TraceID{0x01}, + SpanID: trace.SpanID{0x01}, + TraceFlags: tt.traceFlags, + }) + + e := echo.New() + e.Use(NewMiddlewareWithConfig(Config{ + ServerName: "foobar", + TracerProvider: tp, + Propagators: prop, + PublicEndpointFn: func(c *echo.Context, remote trace.SpanContext) bool { return true }, + })) + e.GET("/user/:id", func(c *echo.Context) error { + return c.NoContent(http.StatusOK) + }) + + r := httptest.NewRequest(http.MethodGet, "/user/123", http.NoBody) + prop.Inject(trace.ContextWithRemoteSpanContext(t.Context(), remoteSc), propagation.HeaderCarrier(r.Header)) + w := httptest.NewRecorder() + e.ServeHTTP(w, r) + + assert.Equal(t, http.StatusOK, w.Result().StatusCode) + + spans := exporter.GetSpans() + if assert.Len(t, spans, 1, "span must be recorded regardless of the remote sampling flag") { + span := spans[0] + assert.NotEqual(t, remoteSc.TraceID(), span.SpanContext.TraceID(), "span must start a new trace") + assert.False(t, span.Parent.IsValid(), "span must be a root span") + if assert.Len(t, span.Links, 1, "remote trace context must be recorded as a span link") { + assert.Equal(t, remoteSc.TraceID(), span.Links[0].SpanContext.TraceID()) + assert.Equal(t, remoteSc.SpanID(), span.Links[0].SpanContext.SpanID()) + } + } + }) + } +} + +func TestPublicEndpointFn(t *testing.T) { + remoteSc := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: trace.TraceID{0x01}, + SpanID: trace.SpanID{0x01}, + TraceFlags: trace.FlagsSampled, + }) + + tests := []struct { + name string + fn func(c *echo.Context, remote trace.SpanContext) bool + expectNewTrace bool + }{ + { + name: "fn returns true, remote trace context is linked instead of continued", + fn: func(c *echo.Context, remote trace.SpanContext) bool { return true }, + expectNewTrace: true, + }, + { + name: "fn returns false, remote trace context is continued", + fn: func(c *echo.Context, remote trace.SpanContext) bool { return false }, + expectNewTrace: false, + }, + { + name: "fn can inspect the remote trace context", + fn: func(c *echo.Context, remote trace.SpanContext) bool { + // continue the trace only when it comes from a trusted trace ID + return remote.TraceID() != (trace.TraceID{0x01}) + }, + expectNewTrace: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter)) + prop := propagation.TraceContext{} + + e := echo.New() + e.Use(NewMiddlewareWithConfig(Config{ + ServerName: "foobar", + TracerProvider: tp, + Propagators: prop, + PublicEndpointFn: tt.fn, + })) + e.GET("/user/:id", func(c *echo.Context) error { + return c.NoContent(http.StatusOK) + }) + + r := httptest.NewRequest(http.MethodGet, "/user/123", http.NoBody) + prop.Inject(trace.ContextWithRemoteSpanContext(t.Context(), remoteSc), propagation.HeaderCarrier(r.Header)) + w := httptest.NewRecorder() + e.ServeHTTP(w, r) + + assert.Equal(t, http.StatusOK, w.Result().StatusCode) + + spans := exporter.GetSpans() + if !assert.Len(t, spans, 1) { + return + } + span := spans[0] + if tt.expectNewTrace { + assert.NotEqual(t, remoteSc.TraceID(), span.SpanContext.TraceID(), "span must start a new trace") + assert.False(t, span.Parent.IsValid(), "span must be a root span") + if assert.Len(t, span.Links, 1, "remote trace context must be recorded as a span link") { + assert.Equal(t, remoteSc.TraceID(), span.Links[0].SpanContext.TraceID()) + assert.Equal(t, remoteSc.SpanID(), span.Links[0].SpanContext.SpanID()) + } + } else { + assert.Equal(t, remoteSc.TraceID(), span.SpanContext.TraceID(), "span must continue the remote trace") + assert.Equal(t, remoteSc.SpanID(), span.Parent.SpanID(), "remote span must be the parent") + assert.Empty(t, span.Links) + } + }) + } +} + func TestSkipper(t *testing.T) { r := httptest.NewRequest(http.MethodGet, "/ping", http.NoBody) w := httptest.NewRecorder()