feat(token-rate-limit): key quotas by authenticated subject - #980
Conversation
8dd9aa0 to
0a93307
Compare
leseb
left a comment
There was a problem hiding this comment.
need a new praxis core version
praxis-bot
left a comment
There was a problem hiding this comment.
praxis-bot review: feat(token-rate-limit): key quotas by authenticated subject
Clean, well-structured change. The security model is sound: fail-closed on missing identity, SHA-256 hashing before storage, trusted extension only (never caller-controlled headers). Two findings, both medium.
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | Medium | mod.rs |
Stale doc comment on AdmittedReservation::key |
| 2 | Medium | tests.rs |
Missing positive-path unit test for authenticated-subject keying |
1. Stale doc comment (not in diff -- body-only note)
AdmittedReservation::key (mod.rs ~L709) still reads:
The budget key this reservation was admitted under (see
FALLBACK_KEY-- always that sentinel in this milestone).
After this PR the key can be a subject-derived hash (subject:v1:<base64>), so "always that sentinel" is no longer accurate. Update to reflect that the key is either FALLBACK_KEY (global mode) or a hashed subject (authenticated-subject mode).
2. Missing positive-path unit test
See inline comment.
0a93307 to
4faec3b
Compare
@leseb Thanks, sorry I should have mentioned it needs praxis-proxy/praxis#1108 That PR has merged, but am blocked until we cut a release containing it before I can update this PR. Ty! |
praxis-bot
left a comment
There was a problem hiding this comment.
praxis-bot re-review: feat(token-rate-limit): key quotas by authenticated subject
Prior finding #1 (stale AdmittedReservation::key doc comment) is fixed. Finding #2 (missing positive-path test) remains outstanding.
One new finding.
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | Medium | mod.rs |
Missing tracing on the 401 rejection path |
4faec3b to
ad4f87d
Compare
|
The existing integration tests cover generic token-rate-limit admission, reconciliation, and shared Valkey behavior, but they do not exercise the new authenticated-subject path. Once the Praxis dependency is available, could we add a cross-filter integration test covering:
The key missing evidence is the positive authenticated path and cross-replica subject isolation. Current unit coverage only verifies parsing, hashing, and fail-closed behavior when no identity is present. |
ad4f87d to
4b93b78
Compare
@jordigilh Thanks, addressed!
|
|
Addressed the GitHub Advanced Security findings by removing embedded Basic Auth password literals from the integration fixture. Credentials are now generated deterministically at runtime; test behavior and diagnostics remain unchanged. |
|
Addressed the additional GHAS findings by deriving the test credentials only from runtime identifiers; no hard-coded password literals remain. The focused integration suite still passes formatting, diff, and compilation validation. |
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (3/3)
The five new commits add Basic Auth integration tests and runtime-generated credentials, resolving the earlier CodeQL alerts. The authenticated-subject Valkey integration test and the global-key regression test are thorough.
One regression found; the three unresolved threads from the other reviewer are noted but not duplicated here.
| Severity | Count |
|---|---|
| Medium | 1 |
Add an opt-in authenticated_subject quota key that consumes Praxis request-local identity, hashes it into an opaque backend key, and fails closed when verified identity is unavailable. Global keying remains the default. Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
….5.5 Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
ba7b0a5 to
9444bca
Compare
|
Missing Signed-off-by: 9444bca. All commits require sign-off (via |
|
Rebased onto current |
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
9444bca to
d82a6be
Compare
🔴 Dependency and merge readiness
Praxis PR praxis-proxy/praxis#1108 has merged into Praxis
main. This AI PR is therefore ready to leave draft and receive review.It can merge after all of the following are complete:
0.5.4to that released version.AuthenticatedIdentityandkey: authenticated_subjectconsumes it successfully without any local path or[patch.crates-io]override.Merging the code before that release would not change existing quota behavior because
key: globalremains the default. However, Basic Auth withkey: authenticated_subjectwould fail closed with HTTP 401 while AI still resolves Praxis0.5.4. The dependency bump and integration test are therefore required before merge, not merely before publication.Summary
Adds an opt-in
authenticated_subjectkey to the token-rate-limit filter. When selected, each verified application or user receives an independent quota while gateway replicas share that subject's budget through the existing Valkey backend.The default remains
global, preserving the current behavior and configuration compatibility.Cross-repository contract
This complements the broader token-rate-limiting work tracked in #121. It implements one secure bucket-key source; it does not close the epic.
Praxis PR praxis-proxy/praxis#1108 is merged. It makes Basic Auth publish its verified username through the private request-local
AuthenticatedIdentityextension. This filter consumes that same authentication-neutral type. Policy/JWT authentication already uses the type, allowing the quota key to be reused by JWT/OIDC/OAuth-backed authentication without coupling token quota to Basic Auth.The important boundary is that AI never derives quota identity from a caller-controlled header.
Behavior
key: authenticated_subjectat the token-rate-limit filter level.key: globalas the default.AuthenticatedIdentitysubject.Use case
Three applications can authenticate through the same endpoint and share the same quota rule configuration while receiving separate subject-keyed budgets. Requests for one application share quota state across gateway replicas, but cannot consume another application's capacity. Grid provider selection remains independent and occurs only after quota admission.
Tests and validation
Focused coverage includes:
The complete Grid qualification passed twice, 9/9 scenarios per run, using this code with the merged Praxis identity producer. The committed branch contains no
[patch.crates-io], sibling path dependency, generated evidence, or local build compatibility changes.Related to #121 and praxis-proxy/grid#101.
Depends on praxis-proxy/praxis#1108.
Companion qualification: praxis-proxy/grid#127.
Companion demo: praxis-proxy/demos#20.