ResolveSpec/audit/pkg/tracing.audit.md

# Audit — `pkg/tracing`

- **Date:** 2026-09-29
- **Scope:** `pkg/tracing/tracing.go` (146 LOC, 1 file, **no tests**)
- **Axes:** thread locking/waiting · slowness · security · panic handling & logging
- **Threat model:** hostile internet client. Span names and attributes here are built directly from
  request-controlled data (method, path, full URL, Host header).

## Summary

A thin OpenTelemetry wrapper: `InitTracer`, an HTTP middleware, and helpers. The abstraction is
fine; the **hardcoded choices** are the problem. Three of them are not configurable at all and each
is wrong for a production, internet-facing deployment:

- `otlptracegrpc.WithInsecure()` — trace export is **plaintext**, with a source comment admitting it.
- `sdktrace.AlwaysSample()` — **100% of requests** are traced, with no sampling knob in config.
- `semconv.HTTPURLKey.String(r.URL.String())` — the **full URL including query string** is exported.

Combined: every request's full URL is shipped unencrypted to a collector, and an attacker sets the
export volume. Span names are also built from raw paths, giving unbounded cardinality.

`config.TracingConfig` (`pkg/config/config.go:86-91`) exposes only `Enabled`, `ServiceName`,
`ServiceVersion` and `Endpoint` — there is no field for TLS or sample rate, so these cannot be fixed
by configuration alone.

| # | Severity | Axis | Finding |
|---|----------|------|---------|
| 1 | **High** | Security | `WithInsecure()` hardcoded — traces exported in plaintext, not configurable |
| 2 | **High** | Security | Full URL **including query string** exported as a span attribute |
| 3 | **High** | Slowness | `AlwaysSample()` hardcoded — 100% trace volume, attacker-controlled, no sampling config |
| 4 | Medium | Slowness | Span name is `method + " " + r.URL.Path` — unbounded cardinality from raw path IDs |
| 5 | Medium | Locking | `tracer` global written by `InitTracer`, read unsynchronised by `Middleware`/`StartSpan` |
| 6 | Medium | Observability | `Middleware` records no HTTP status and no error status — spans never show failures |
| 7 | Medium | Panic | `Middleware` does not recover; a downstream panic leaves the span unmarked (`Unset` status) |
| 8 | Low | Slowness | `InitTracer` has no timeout/deadline on exporter or resource creation |
| 9 | Low | Maintenance | `semconv/v1.4.0` (2021) — deprecated attribute names modern collectors no longer index |
| 10 | Low | Security | `SetAttributes`/`AddEvent` pass caller data through with no size or cardinality limit |

## Resolution (2026-09-30)

Fixed in `pkg/tracing/tracing.go`, `pkg/config` (`TracingConfig`, defaults), the package README, and new
`pkg/tracing/tracing_test.go` (passes).

| # | Status | What changed |
|---|--------|--------------|
| 1 | **Fixed** | TLS is the default. `Config` gains `Insecure`, `TLSConfig` and `Headers` (OTLP auth). `tracing.insecure` added to `pkg/config`. **Breaking:** plaintext collectors now need `Insecure: true`. |
| 2 | **Fixed** | Query string and `Host` are no longer exported; attributes are method, `url.path`, scheme, `http.route`, status. `TLSConfig` has no config-file key (code only). |
| 3 | **Fixed** | `ParentBased(TraceIDRatioBased(rate))`; `SampleRate` defaults to 0.1, validated to [0,1]; `tracing.sample_rate` added to `pkg/config`. |
| 4 | **Fixed** | Span name is `METHOD <route template>` from `Request.Pattern`, `<unmatched>` otherwise. `MiddlewareWithRoute(fn)` supports other routers. |
| 5 | **Fixed** | `tracer` is an `atomic.Pointer`; a second `InitTracer` returns an error; the shutdown func resets state. |
| 6 | **Fixed** | Response writer wrapped; `http.response.status_code` recorded, 5xx sets Error status. Preserves `Flush`/`Unwrap`. |
| 7 | **Fixed** | Panics are recorded (`RecordError`, Error status) and re-raised so the panic middleware still responds. Must be installed inside the panic middleware; actual order in `pkg/server` not verified. |
| 8 | **Fixed** | `InitTracerContext(ctx, cfg)` with `InitTimeout` (default 10s); `InitTracer` retained as a wrapper. Exporter is shut down if resource creation fails. |
| 9 | **Fixed** | Moved to `semconv/v1.26.0`. |
| 10 | **Fixed** | `AttributeValueLengthLimit` set via `WithRawSpanLimits` (`AttributeValueLimit`, default 1024). |

Tests added: query redaction and route naming, unmatched route, 5xx status, panic recorded and re-raised,
double-init and invalid sample rate.

---

## Findings

### 1. `WithInsecure()` hardcoded (High, Security)

`tracing.go:38-42`

```go
client := otlptracegrpc.NewClient(
	otlptracegrpc.WithEndpoint(config.Endpoint),
	otlptracegrpc.WithInsecure(), // Use WithTLSCredentials in production
)
```

The comment names the fix and the code does not implement it, and — critically — `Config`
(`tracing.go:21-27`) has no field to express it:

```go
type Config struct {
	ServiceName    string
	ServiceVersion string
	Endpoint       string
	Enabled        bool
}
```

So there is **no supported way** to enable TLS on trace export short of editing this file. Every
span — carrying the full request URL per finding 2 — crosses the network in cleartext, and the
collector endpoint is unauthenticated (no OTLP headers/bearer token option either), so anything that
can reach it can also *inject* fabricated spans.

**Recommendation:** add `Insecure bool`, `TLSConfig *tls.Config` and `Headers map[string]string` to
`Config` (and the matching `tracing.*` keys to `pkg/config`), default to TLS on, and require an
explicit opt-in for insecure. Wire `otlptracegrpc.WithTLSCredentials` / `WithHeaders`.

### 2. Full URL with query string exported (High, Security)

`tracing.go:95-103`

```go
ctx, span := tracer.Start(ctx, r.Method+" "+r.URL.Path,
	trace.WithSpanKind(trace.SpanKindServer),
	trace.WithAttributes(
		semconv.HTTPMethodKey.String(r.Method),
		semconv.HTTPURLKey.String(r.URL.String()),   // <- full URL, query string included
		semconv.HTTPTargetKey.String(r.URL.Path),
		semconv.HTTPSchemeKey.String(r.URL.Scheme),
		semconv.NetHostNameKey.String(r.Host),
	),
)
```

`r.URL.String()` includes `RawQuery`. For this API the query string is where the interesting data
lives: filter expressions, column lists, and — for any client that passes credentials as a query
parameter (`?api_key=`, `?token=`, signed-URL style parameters) — secrets. All of it lands in the
tracing backend, and per finding 1 it gets there in plaintext.

Note `HTTPTargetKey` is also set to `r.URL.Path`, so the *useful* part is already captured
separately; `HTTPURLKey` adds only the sensitive part.

Secondary: `r.Host` comes from the `Host` header, which is client-controlled and unvalidated here —
so an attacker can pollute the `net.host.name` dimension with arbitrary values (cardinality blowup,
and log/dashboard spoofing).

**Recommendation:** export a redacted URL (scheme + host + path, query keys only or dropped
entirely). OTel's own guidance is to strip or redact query parameters for exactly this reason.

### 3. `AlwaysSample()` hardcoded (High, Slowness)

`tracing.go:61-65`

```go
tp := sdktrace.NewTracerProvider(
	sdktrace.WithBatcher(exporter),
	sdktrace.WithResource(res),
	sdktrace.WithSampler(sdktrace.AlwaysSample()),
)
```

Every request produces a recorded, exported span. There is no `SampleRate` in `Config` and no
`tracing.sample_rate` key in `pkg/config/manager.go`'s defaults, so this is not tunable.

Under the hostile-client threat model the request rate — and therefore the span rate, the batch
queue pressure, the serialisation cost and the outbound bandwidth — is set by the attacker. Each
request pays span allocation, attribute encoding (including the full URL string), and a share of
batch export. When the batch queue fills, the SDK drops spans, so a flood also destroys the
observability you need to see the flood.

**Recommendation:** default to `sdktrace.ParentBased(sdktrace.TraceIDRatioBased(rate))` with a
configurable rate (e.g. 0.01–0.1), keeping `AlwaysSample` available for development. `ParentBased`
also means an upstream sampling decision is respected, which `AlwaysSample` currently overrides.

### 4. Unbounded span-name cardinality (Medium, Slowness)

`tracing.go:95` — the span name is `r.Method + " " + r.URL.Path`.

This API's paths embed identifiers (`/api/public/employees/7f3c…`, `/api/<schema>/<entity>/<id>`), so
each distinct ID becomes a distinct span name. Consequences:

- Tracing backends index on span name; unbounded distinct names is the classic cardinality-explosion
  cost bomb (and in some backends, a hard limit that starts rejecting data).
- It violates the OTel HTTP convention, which requires a **low-cardinality route template**
  (`GET /api/{schema}/{entity}/{id}`), with the concrete value in `http.route`/attributes.
- It is attacker-driven: requests to random paths — including 404s — each mint a new span name.

**Recommendation:** derive the name from the matched route pattern. `pkg/server`'s router
(chi/mux/gin, see `audit/pkg/server.audit.md`) exposes the route template after matching; use it, and
place this middleware after the router so the pattern is available. Fall back to
`r.Method + " " + "<unmatched>"` rather than the raw path.

### 5. Unsynchronised `tracer` global (Medium, Locking)

`tracing.go:19`

```go
var tracer trace.Tracer
```

Written at `tracing.go:77` (`tracer = tp.Tracer(config.ServiceName)`), read at `tracing.go:86`,
`:95` (`Middleware`) and `:116`, `:119` (`StartSpan`). No mutex, no `atomic.Value`.

Same pattern as `pkg/logger`'s `Logger` global (see `audit/pkg/logger.audit.md` finding 1). Benign if
`InitTracer` runs once before any request is served; a race the moment tracing is re-initialised at
runtime. `InitTracer` is exported and callable at any time, and calling it twice also leaks the
first `TracerProvider` (nothing shuts it down) — its batch processor goroutine and gRPC connection
stay alive for the life of the process.

Note the nil checks at `:86` and `:116` are the read-half of the race: a goroutine can observe a
non-nil-but-torn interface value.

**Recommendation:** `atomic.Pointer` or a `sync.Once`-guarded init; return an error from a second
`InitTracer` call, or shut down the previous provider first.

### 6. No HTTP status or error status on spans (Medium, Observability)

`Middleware` (`tracing.go:84-112`) never wraps `w`, so it cannot observe the status code. It sets no
`semconv.HTTPStatusCodeKey` and never calls `span.SetStatus`. Every span therefore has status
`Unset`, which tracing backends render as "OK".

The practical effect: you cannot find failing requests in the traces. A 500-storm and a healthy
period look identical in the span data, which defeats the main reason to run tracing on an
internet-facing service.

**Recommendation:** wrap the `ResponseWriter` to capture the status, set
`semconv.HTTPStatusCodeKey.Int(status)`, and `span.SetStatus(codes.Error, ...)` for 5xx.

### 7. No panic handling in the middleware (Medium, Panic handling)

`Middleware` has `defer span.End()` (`tracing.go:106`) but no `recover()`. If `next.ServeHTTP`
panics:

- The `defer span.End()` **does** run, so no span is leaked — that part is correct.
- But the span is ended with status `Unset` and no exception event, so the panic is invisible in the
  trace. The one place a trace would be most valuable records nothing.
- The panic propagates up to whichever handler is outermost. Whether that is
  `pkg/middleware/panic.go` depends on middleware ordering — if `tracing.Middleware` is installed
  *outside* the panic middleware, the panic escapes to `net/http`'s per-connection recovery, which
  kills the connection and logs to the default logger, bypassing `pkg/logger` and the error tracker
  entirely. See `audit/pkg/middleware.audit.md` and `audit/pkg/server.audit.md` for the actual order.

This package does not import `pkg/logger` at all, so nothing here can be logged.

**Recommendation:** recover, record `span.RecordError` + `span.SetStatus(codes.Error, …)`, then
re-panic so the dedicated panic middleware still handles the response. Document the required
middleware order.

### 8. No deadline on initialisation (Low, Slowness)

`tracing.go:36` uses `ctx := context.Background()` for both `otlptrace.New` (`:44`) and
`resource.New` (`:51`). `otlptracegrpc` does not block on connect by default, so this is unlikely to
hang today — but `resource.New` with detectors can perform network calls (cloud metadata endpoints),
and an unreachable metadata service is a classic multi-second startup stall. `InitTracer` should
accept a `context.Context` from the caller so startup has a deadline.

### 9. `semconv/v1.4.0` (Low, Maintenance)

`tracing.go:15` pins the 2021 semantic conventions. `http.method`, `http.url`, `http.target`,
`http.scheme`, `net.host.name` were all renamed in v1.20+ (`http.request.method`, `url.full`,
`url.path`, `url.scheme`, `server.address`). Current collectors, dashboards and backend
auto-instrumentation views key off the new names, so these spans will not populate standard HTTP
dashboards.

### 10. No limits on caller-supplied span data (Low, Security)

`StartSpan`, `AddEvent`, `SetAttributes` (`tracing.go:115-145`) forward caller attributes verbatim.
If any caller passes request-derived values (a filter expression, a row payload), span size is
attacker-influenced. The SDK's default limits (128 attributes, 128 events) cap the count but not the
*value* length — a 1 MB string attribute is accepted.

**Recommendation:** set explicit `sdktrace.WithSpanLimits` including `AttributeValueLengthLimit`.

---

## What looks right

- **Disabled path is genuinely free.** `InitTracer` with `Enabled: false` (`tracing.go:31-34`)
  returns a no-op shutdown func and never builds an exporter, so a disabled deployment pays nothing
  and cannot leak.
- **Nil-tracer guards everywhere.** `Middleware` (`tracing.go:86-89`) passes through untouched and
  `StartSpan` (`tracing.go:116-118`) returns the incoming context plus the context's (no-op) span.
  So a partially-initialised process degrades safely rather than nil-panicking — a pattern
  `pkg/logger` gets right too.
- **Context propagation is correct.** `Extract` from `propagation.HeaderCarrier(r.Header)`
  (`tracing.go:92`), a composite `TraceContext` + `Baggage` propagator (`tracing.go:72-75`), and
  `r = r.WithContext(ctx)` (`tracing.go:109`) before calling `next` — the span context actually
  reaches downstream handlers, which is the part most hand-rolled middlewares get wrong.
- `SpanKindServer` is set correctly (`tracing.go:96`).
- `WithBatcher` rather than a simple/sync span processor (`tracing.go:62`) — export does not block
  the request path.
- `InitTracer` returns `tp.Shutdown` (`tracing.go:80`), giving the caller a real flush-on-shutdown
  hook with a caller-supplied context, which is better than the fixed-timeout pattern in
  `pkg/errortracking` (see that audit, finding 3).
- `RecordError` nil-guards (`tracing.go:140-143`) so `RecordError(ctx, nil)` is a no-op.

## Suggested follow-up

1. Extend `Config` with `Insecure`, TLS credentials, OTLP headers and `SampleRate`; add the matching
   keys to `pkg/config` (findings 1, 3). These cannot be fixed without an API change, so they should
   go together.
2. Redact the query string from exported attributes (finding 2).
3. Move to route-template span names, which requires positioning the middleware after routing
   (finding 4).
4. Capture status code and panics in the middleware (findings 6, 7).
5. Guard the `tracer` global (finding 5) and upgrade `semconv` (finding 9).
6. Add tests: this package has none. A tracetest/in-memory exporter makes assertions on span name,
   attributes and status straightforward, and would have caught findings 2, 4 and 6.