feat: add privacy-safe scan telemetry - #45
Merged
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Compatibility | 1 medium |
| Security | 6 high |
| Performance | 2 medium |
🟢 Metrics 88 complexity · 12 duplication
Metric Results Complexity 88 Duplication 12
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR implements the requested privacy-safe scan telemetry; however, it is currently not up to standards due to critical security and stability risks.
Major issues that must be addressed before merging include:
- A security vulnerability in the mTLS middleware where the authentication fails open if the proxy shared secret is missing, allowing for header-injection attacks.
- An unbounded memory growth risk in the resolver's ScanCounter, which could lead to OOM crashes under high load or targeted attacks.
- Logic flaws in telemetry retry handling that create 'poison batches,' potentially blocking all subsequent telemetry flushes if a single malformed batch is rejected by the vault with a 4xx error.
- RFC 4514 non-compliance in the Distinguished Name (DN) parser, which will cause legitimate certificates with escaped commas to be rejected.
About this PR
- The telemetry implementation introduces several systemic risks to the resolver's availability. Specifically, the combination of unbounded in-memory maps and 'poison batch' retry logic creates a scenario where the service could easily crash or cease telemetry reporting during period of high traffic or upstream vault instability.
Test suggestions
- Verify the internal scan-ingest endpoint rejects requests without a valid client certificate.
- Verify that GS1 redirects (/01/{gtin}) do not increment the scan counter.
- Verify that QR image renders increment a separate bucket and are not summed into scans.
- Verify the database 'variant' CHECK constraint prevents invalid resolution types (e.g., 'qr') from entering the scan table.
- Verify the resolver's merge-back logic correctly retains counts if a flush to the node fails.
- Verify the daily retention prune task removes data older than the 730-day horizon.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
# Conflicts: # crates/dpp-dal/tests/pg_integration.rs # crates/dpp-node/src/boot/tasks.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds privacy-safe scan telemetry: aggregate counts of how often each passport is resolved, surfaced to the operator — without recording anything about the person who scanned.
The public resolver has no database (it is kept stateless for an edge target), so it counts resolutions in memory and periodically flushes aggregates to the node's internal, mTLS-gated ingest endpoint. The node persists them and the operator reads them back through the API, the CLI, and the interactive console.
Design
ops/pg/0025_scan_telemetry.sql): two upserted counter tables keyed only by(passport, day, surface). There is no column for an IP, user agent, session, or per-event row — the schema itself is the privacy policy. ACHECK (variant IN ('html','json'))structurally prevents a QR render being written as a resolution; retention is a rolling 24-month prune (the app role gets a scopedDELETE).POST /vault/internal/scan-batch): mounted off both the public and Bearer trees, gated by client-certificate mTLS (CN=odal-resolver). The mTLS middleware was promoted from the identity service intodpp-common::mtlsand parameterised by allowed CN, so there is one audited implementation (identity is now a thin shim)./dpp/{id}html/json), with QR-image renders tracked as a separate metric and the GS1/01/{gtin}redirect deliberately not counted (its followed terminal view is). Off entirely unlessSCAN_INGEST_URLis configured.GET …/dpp/{id}/stats+GET …/stats;odal stats/odal passport stats <id>(--days,--json); and an interactive "Scan telemetry" entry in the console (operator rollup + per-passport).Key decisions
Testing
resolver_e2e: the counting model end to end — GS1 redirect not double-counted, QR not counted as a scan.scan_ingest_route: the internal endpoint rejects a caller with no client certificate.just test-pg, realpostgres:17): migration applies, upsert/stats/prune and thevariantCHECK all pass.html=3, json=2(5 resolutions),qrRenders=1, GS1 redirect uncounted, read back viaodal stats.Full
just checkis green.Out of scope (separate repos / branches)
client_auth+ on-VM cert provisioning that terminates the mTLS in production (locally the flush usesMTLS_ALLOW_INSECURE=true).Reviewer notes
SCAN_INGEST_URL/SCAN_FLUSH_INTERVAL_SECS/SCAN_FLUSH_CLIENT_IDENTITY; node reuses identity'sMTLS_*. Documented in.env.example.0025is additive; the app role's newDELETEgrant is scoped to the two telemetry tables only.