fix(tracing): address audit findings

Default to TLS export with Insecure/TLSConfig/Headers options, parent-based
ratio sampling (default 0.1), and no query string or Host in span attributes.
Name spans by route template, record status and panics (re-raised), guard the
tracer with atomic.Pointer, reject double init, add init timeout and attribute
length limit, and move to semconv v1.26.0. Add tracing.insecure and
tracing.sample_rate config keys and tests.
This commit is contained in:
Hein
2026-09-30 13:39:50 +02:00
parent 164ba2b240
commit f9c948ca4e
8 changed files with 356 additions and 46 deletions
+4 -4
View File
@@ -68,7 +68,7 @@ every one of them on the first run:
| `pkg/config` | `*viper.Viper` has no internal lock; `configInstance` singleton | `config.audit.md` findings 1, 2 |
| `pkg/logger` | `Logger`, `errorTracker` globals | `logger.audit.md` finding 1 |
| `pkg/modelregistry` | `defaultRegistry` read by 6 functions without the lock | `modelregistry.audit.md` findings 2, 8 *(fixed 2026-09-30)* |
| `pkg/tracing` | `tracer` global | `tracing.audit.md` finding 5 |
| `pkg/tracing` | `tracer` global | `tracing.audit.md` finding 5 *(fixed 2026-09-30)* |
| `pkg/errortracking` | `sentry.Init` mutates process globals | `errortracking.audit.md` finding 2 |
**Failure scenario.** `pkg/config` finding 1 is the sharpest illustration. A
@@ -162,7 +162,7 @@ The test bodies that exist but are never executed by CI:
| `logger` | 0 | 0 | — |
| `modelregistry` | 1 | ~150 | yes (`-race`) *(added 2026-09-30)* |
| `testmodels` | 0 | 0 | — |
| `tracing` | 0 | 0 | — |
| `tracing` | 1 | ~90 | yes *(added 2026-09-30)* |
**Failure scenario.** `pkg/security` has 6 359 lines of tests — the largest test
body in the repository — and **not one of them runs in CI**. A change that breaks
@@ -307,7 +307,7 @@ package-level variables, and most guard it with nothing:
| `pkg/logger` | `Logger *zap.SugaredLogger` (`logger.go:15`), `errorTracker` (`:16`) | **no** — and `Logger` is exported |
| `pkg/cache` | `defaultCache *Cache` (`cache.go:10`) | **no** |
| `pkg/config` | `configInstance *Manager` (`manager.go:15`) | **no** |
| `pkg/tracing` | `tracer` (`tracing.go:19`) | **no** |
| `pkg/tracing` | `tracer` | **yes** *(fixed 2026-09-30)* — `atomic.Pointer` |
| `pkg/modelregistry` | `defaultRegistry` | **yes** *(fixed 2026-09-30)* — guarded by `registriesMutex`; all access via `GetDefaultRegistry()` |
| `pkg/metrics` | `globalProvider` (`interfaces.go:50-51`) | **yes** — `globalProviderMu sync.RWMutex` |
@@ -369,7 +369,7 @@ to configure otherwise:
| Component | Default | Configurable? | Reference |
|---|---|---|---|
| PostgreSQL | `sslmode: disable` (`config/manager.go:242`) | yes, via config | `config.audit.md` finding 3 |
| OTLP traces | `otlptracegrpc.WithInsecure()` hardcoded (`tracing/tracing.go:41`) | **no** — `Config` has no field for it | `tracing.audit.md` finding 1 |
| OTLP traces | `otlptracegrpc.WithInsecure()` hardcoded (`tracing/tracing.go:41`) *(fixed 2026-09-30: TLS default, `Insecure` opt-in)* | **yes** | `tracing.audit.md` finding 1 |
| Redis (cache) | no `TLSConfig` set | **no** — `RedisConfig` has no TLS field | `cache.audit.md` finding 15 |
| Memcache | no TLS | **no** | `cache.audit.md` finding 15 |
| CORS | `allowed_origins: ["*"]`, `allowed_headers: ["*"]` (`config/manager.go:214-216`) | yes | `config.audit.md` finding 3 |
+21
View File
@@ -36,6 +36,27 @@ by configuration alone.
| 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