Add PublicEndpoint and PublicEndpointFn options for untrusted trace context - #19
Add PublicEndpoint and PublicEndpointFn options for untrusted trace context#19rintaaaa wants to merge 2 commits into
Conversation
…ontext When serving internet-facing endpoints, the incoming trace context (traceparent/tracestate) comes from untrusted clients. Continuing it as the parent of the server span allows callers to inject arbitrary trace IDs into traces or suppress tracing entirely with a sampled=0 flag. PublicEndpoint starts a new root trace for every request and records the incoming remote span context as a span link instead. PublicEndpointFn allows deciding per request, receiving the extracted remote span context so the decision can also be based on the incoming trace context itself. Semantics follow otelhttp WithPublicEndpoint/WithPublicEndpointFn. Defaults to false to preserve existing behavior and match the OTel instrumentation ecosystem convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aldas
left a comment
There was a problem hiding this comment.
Seems OK but I think 2 fields are bit too much.
| // 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). | ||
| PublicEndpoint bool |
There was a problem hiding this comment.
I think this field is redundant. This can be removed and PublicEndpointFn readme/doc comment could have example for setting always requests as public.
config.PublicEndpointFn = func(c *echo.Context, remote oteltrace.SpanContext) bool { return true }There was a problem hiding this comment.
Fair point — I had included both for parity with otelhttp's WithPublicEndpoint/WithPublicEndpointFn, but since the bool is just sugar for a constant-true fn there's no reason to carry two fields here. Removed PublicEndpoint in 7faf637 and added the always-public example to the PublicEndpointFn doc comment and README.
Per review feedback: the bool field was redundant sugar over PublicEndpointFn returning a constant. Document the always-public case as an example on PublicEndpointFn instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
ok, I propose another change. add field to // ContextSpanStartOptions allows the caller to configure an additional set of trace.SpanStartOptions, which are applied to each new span
ContextSpanStartOptions SpanStartOptionsFuncwhich is type SpanStartOptionsFunc func(c *echo.Context, opts []oteltrace.SpanStartOption) []oteltrace.SpanStartOptionand if it is set we call it in if config.SpanStartOptions != nil {
spanStartOptions = append(spanStartOptions, config.SpanStartOptions...)
}
if config.ContextSpanStartOptions != nil {
spanStartOptions = config.ContextSpanStartOptions(c, spanStartOptions)
}and for untrusted endpoint functionality add utility to create this callback func NewUntrustedEndpointSpanStartOptionsFunc(isUntrustedFunc func(c *echo.Context, remote oteltrace.SpanContext) bool) SpanStartOptionsFunc {
propagator := otel.GetTextMapPropagator() // if you need to use config.Propagators, just copy this func to your project.
return func(c *echo.Context, startOpts []oteltrace.SpanStartOption) []oteltrace.SpanStartOption {
remote := oteltrace.SpanContextFromContext(propagator.Extract(c.Request().Context(), propagation.HeaderCarrier(c.Request().Header)))
if isUntrustedFunc(c, remote) {
startOpts = append(startOpts, oteltrace.WithNewRoot())
// keep the incoming (untrusted) trace context visible by linking it to the new root span
if remote.IsValid() && remote.IsRemote() {
startOpts = append(startOpts, oteltrace.WithLinks(oteltrace.Link{SpanContext: remote}))
}
}
return startOpts
}
}This way we allow other users to customize span start options for their needs with this PR. |
Motivation
Currently the middleware unconditionally trusts the incoming trace context (
traceparent/tracestateheaders) and continues it as the parent of the server span. For internet-facing endpoints receiving requests from untrusted clients, this allows callers to:sampled=0flag (with the defaultParentBasedsampler).This repository already takes a deliberate "do not trust headers by default" stance for
url.schemeandX-Forwarded-For(documented inextrator.go), but the trust decision for incoming trace context was not explicitly addressed anywhere. This PR fills that gap the same wayotelhttpdoes, while keeping the default behavior unchanged.What this PR does
Adds two options to
Config, with semantics equivalent tootelhttp.WithPublicEndpoint/WithPublicEndpointFn:PublicEndpoint bool— when enabled, the incoming trace context is not used as the parent. A new root span (new trace) is started viatrace.WithNewRoot(), and the incoming remote span context, if valid, is recorded as a span link so the caller relationship stays visible.PublicEndpointFn func(c *echo.Context, remote trace.SpanContext) bool— per-request decision, e.g. when the same server serves both internal and public routes. The extracted remote span context is passed as the second argument so the decision can also be based on the incoming trace context itself (this avoids the extraRequest.WithContextallocation otelhttp pays for the same capability). Only called whenPublicEndpointis false.Why default to
falsetruewould silently fragment distributed traces for the most common deployment (internal service-to-service traffic), and would be a behavior change for existing users of released versions.Testing
TestPublicEndpoint— verifies a new trace is started, the span is a root span, the remote context is recorded as a link, and that an unsampled (sampled=0) remote context cannot suppress recording.TestPublicEndpointFn— verifies both branches of the per-request decision and that the fn can inspect the remote span context.make check(staticcheck, golint, vet, gosec, race) passes.🤖 Generated with Claude Code