fix(pg-pkg): add per-IP rate limiting to /v2 API endpoints (#224)#226
Conversation
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Rules Dobby 2 — gatekeeper review (per-rule compliance + consolidated review findings)
Rule compliance: PASS. Conventional-commit title, body has Closes #224, 4 new rate-limit integration tests assert 429 behavior, cargo fmt/clippy verified, the newly-added actix-governor dependency is actually used, and this PR directly addresses the documented "no rate limiting on PKG endpoints" hardening item. No rule violations found across PR-hygiene, dependency, and postguard repo rules. All required endpoints (POST /start, GET /key, GET /key/{timestamp}, GET /api-key/validate, POST /sign/key) carry the sensitive-tier limiter, built once and shared across workers.
Findings (2, both nit-level):
- Misleading middleware-ordering comment on the general
/v2Governor (benign, but actionable — see inline). - Non-blocking design note on
PeerIpKeyExtractorbehind reverse proxies — flagged for awareness, not a blocker.
Requesting a small follow-up for finding #1 (correct the comment or swap the two .wrap() calls). Delivered as a COMMENT review because GitHub rejects REQUEST_CHANGES/APPROVE on Dobby's own PRs.
The PKG had no rate-limiting middleware, so requests to its API were
unbounded. Add per-IP (peer address) rate limiting to cap overall load
and protect the key-issuing endpoints.
Two tiers:
- A general limit on the whole `/v2` scope (default 10 req/s, burst 50)
to cap overall load, wrapped outermost so over-limit requests are
rejected before metrics collection or any handler runs.
- A stricter limit on the key-issuing endpoints `/start`, `/key`,
`/key/{timestamp}`, `/sign/key` and `/api-key/validate`
(default 2 req/s, burst 10), wrapped outside the auth middleware so
floods are throttled before expensive authentication work.
All limits are configurable via CLI flags / env vars
(`PKG_RATELIMIT_*`), and `--ratelimit-disabled` builds the middleware
in permissive mode for deployments behind a trusted, rate-limiting
reverse proxy. The two configs share their limiter state across workers
via the internal Arc, so limits are global rather than per-worker.
Add integration tests covering: 429 after the sensitive burst is
exceeded (with retry-after header), 429 on the general scope limit,
per-IP isolation, and permissive mode never blocking.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… outermost actix applies wrap layers in reverse registration order. The general /v2 Governor was registered before collect_metrics, which made collect_metrics the outermost layer and ran it ahead of the limiter, contradicting the comment. Swap the two so the Governor sits in front and rejects over-limit requests before metrics collection or handlers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
12f3581 to
0406536
Compare
There was a problem hiding this comment.
Rules Dobby 2 — gatekeeper review (cycle 2) — per-rule compliance + consolidated review findings.
Blocking finding found and FIXED inline (public-disclosure leak). postguard is a public repo, and the still-draft advisory GHSA-… plus the exploit-class framing were named in three public artifacts: the PR body, the first commit body (d8901cf), and the source issue #224. Per no-public-security-issues + genericize-advisory-disclosure-all-artifacts, a still-draft advisory (id, vuln class, or exploit sequence) must never appear in public artifacts until it publishes. Cycle 1 marked rule-compliance PASS and missed this. Rather than burn a PR-Dobby cycle per location, I swept all three in one pass:
- Genericized the PR body (removed the advisory id + exploit-class wording).
- Reworded commit
d8901cf→dcb34bc(generic first paragraph; tree byte-identical, force-pushed with--force-with-lease). - Genericized issue #224 and dropped its
/dobbytrigger (avoid-self-trigger-loops).
Verified: no advisory id / brute-force / resource-exhaustion string remains in any commit message, the PR body, or the issue body.
Rule compliance: PASS (after the fix above). Conventional-commit title, Closes #224, 4 rate-limit integration tests assert 429 behaviour (+ live smoke test), cargo fmt/clippy verified, the new actix-governor dep is actually used, all required endpoints (POST /start, GET /key, GET /key/{timestamp}, POST /sign/key, GET /api-key/validate) carry the sensitive-tier limiter built once and shared across workers, and the cycle-1 middleware-ordering finding was correctly resolved (Governor now outermost).
Non-blocking notes carried from review (1 style, 2 nits — none are defects, none block merge): see inline. All faithfully follow the task spec; no action required.
Signing off. Delivered as a COMMENT because GitHub blocks APPROVE/REQUEST_CHANGES on Dobby's own PRs — treat this as an approval. Flipping to ready-for-review.
| .wrap( | ||
| Auth::new(irma.clone(), AuthType::Key).with_db_pool(pool.as_ref().clone()), | ||
| ) | ||
| .wrap(Governor::new(&sensitive_ratelimit)) |
There was a problem hiding this comment.
style (non-blocking). /api-key/validate is a service-to-service endpoint (per the comment at L247-250 it is called by sibling services like cryptify), yet it gets the same strict per-IP sensitive limit (default 2/s, burst 10). All of cryptify's validation traffic originates from a single server IP and shares one sensitive bucket, so high-volume legitimate validation could be throttled. This faithfully follows the task spec (which explicitly asked for a tight limit here), so non-blocking — but worth a judgment call on whether service-to-service callers need a higher/separate limit or an allowlist.
| // they hit metrics collection or any handler. actix applies `wrap` | ||
| // layers in reverse registration order, so the Governor must be | ||
| // wrapped after `collect_metrics` to sit in front of it. | ||
| .wrap(Governor::new(&general_ratelimit)) |
There was a problem hiding this comment.
nit (non-blocking). Correct consequence of the cycle-1 ordering fix, just flagging: with the Governor now outermost, 429-throttled requests are rejected before collect_metrics, so throttled traffic is invisible in Prometheus metrics. Consider a dedicated rate-limit counter if throttling visibility matters.
| // --------------------------------------------------------------------- | ||
| // Rate-limiting (actix-governor) integration tests | ||
| // --------------------------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
nit (non-blocking). The rate-limit integration tests build a hand-mirrored /v2 app (ratelimit_setup) rather than exercising the production exec() middleware stack, so the real Governor-vs-collect_metrics ordering and the actual per-endpoint wiring are not directly covered by a test. The mirror is a faithful reproduction, so this is a test-fidelity nit, not a defect.
…trusted proxy (#230) * fix(pg-pkg): rate-limit on real client IP (X-Forwarded-For) behind a trusted proxy Per-IP rate limiting (#226) keys on the TCP peer address, which is always the proxy when the PKG runs behind ingress-nginx, so every client shares one bucket. Add ClientIpKeyExtractor and an opt-in --ratelimit-trust-forwarded-for / PKG_RATELIMIT_TRUST_FORWARDED_FOR flag (default false) that keys on the rightmost (trusted, proxy-appended) X-Forwarded-For entry, falling back to the peer address. The rightmost entry is used so client-supplied entries cannot spoof the key. * style(pg-pkg): wrap long clap attribute (rustfmt)
1 participant
Closes #224. Hardening the PKG
/v2API surface.Problem
pg-pkghad no rate-limiting middleware, so requests to its API — including the key-issuing endpoints — were unbounded.Change
Adds
actix-governor0.10per-IP (peer address) rate limiting to the/v2scope inpg-pkg/src/server.rs, in two tiers:/v2scopePOST /start,GET /key,GET /key/{timestamp},POST /sign/key,GET /api-key/validateMiddleware ordering:
/v2scope, so over-limit requests are rejected before metrics collection or any handler runs.Authmiddleware on each key-issuing resource, so floods are throttled before expensive authentication (JWT public-key parsing, DB lookups) happens.GovernorConfigs are built once and shared across workers via the internalArc, so limits are global, not per-worker.The default
PeerIpKeyExtractorkeys on the peer IP (IPv6 collapsed to a /56 prefix). Over-limit requests get429 Too Many Requestswith aRetry-Afterheader.Configuration
All limits are exposed as CLI flags / env vars:
--ratelimit-per-secondPKG_RATELIMIT_PER_SECOND10--ratelimit-burstPKG_RATELIMIT_BURST50--ratelimit-sensitive-per-secondPKG_RATELIMIT_SENSITIVE_PER_SECOND2--ratelimit-sensitive-burstPKG_RATELIMIT_SENSITIVE_BURST10--ratelimit-disabledPKG_RATELIMIT_DISABLEDfalse--ratelimit-disabledbuilds the middleware in permissive mode (passes every request through) for deployments behind a trusted reverse proxy that does its own rate limiting — per-IP limiting on the peer address is meaningless when every request appears to come from the proxy.Tests
Added integration tests (
cargo test --bin pg-pkg, 41 pass):test_ratelimit_sensitive_endpoint_returns_429— 429 after the sensitive burst is exceeded, assertsretry-afterheader.test_ratelimit_general_scope_returns_429— 429 on the general scope limit.test_ratelimit_is_per_ip— a different peer IP keeps its own fresh bucket.test_ratelimit_permissive_never_blocks— disabled/permissive mode never returns 429.Also smoke-tested a live
pg-pkg server(mock IRMA/publickey,PKG_RATELIMIT_BURST=3):/v2/parametersreturned200,200,200,429,429withretry-after: 0, confirming the shared limiter works across workers.Verified
cargo fmt --all -- --check,cargo clippy(no new warnings), andcargo build --profile edge --bin pg-pkg(Docker Rust 1.90 parity — new deps declare no MSRV above 1.90).🤖 Generated with Claude Code