fix: correctness and fail-closed gaps - #63
Open
araujof wants to merge 20 commits into
Open
Conversation
Narrowing a whole-valued double to an int broke float arithmetic: a confidence of exactly 1.0 became int 1, so `confidence * 100.0` failed with "no such overload" and denied the maximum-confidence case while allowing lower ones. CEL already compares an int literal against a double operand, so the narrowing bought nothing. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The collision check was exact-match, so an inline `package authz.exceptions` slipped past a global `package authz` and still fed the `data.authz.*` subtree that the global rule reads. The check is now prefix-aware on a path boundary, so `data.authz` still does not collide with `data.authznext`. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The derived Debug reached `DecodingKeySource::Secret`, so a `{:?}` of the
resolver printed HMAC secrets in plaintext. For HS* those are signing keys,
which is token-forgery material. KeyStore and TrustedIssuer already redact
theirs; the resolver now matches.
The auth scheme is also case-insensitive per RFC 9110, so a `bearer <token>`
header no longer falls through to the parser as a malformed token.
Ported from praxis-proxy#54.
Signed-off-by: Shane Utt <shaneutt@linux.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
A timeout mapped to Ok(false), so the refresh-failed alarm never fired for the most likely failure under an overloaded backend. A persistently timing out EXPIRE would silently stop the sliding TTL and let session taint expire mid-session with no signal. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The append-only check ignored authorization_details, ttl_seconds, and timestamp, so an existing hop could have its RFC 9396 grant rewritten wider or its lifetime extended and still pass as an append. DelegationHop already documents that each hop's details must be structurally narrowed. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…tion The sequential phase halted only when a violation was present, so a blocking plugin that set continue_processing=false without one fell through to the modification path. The concurrent phase already synthesizes concurrent_deny for the same case. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The change gate compared only security, delegation, and raw_credentials, so a route whose sole mutation was a header rewrite or a custom-extension write reported unchanged and the edit was dropped before the merge. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
AttenuationConfig accepted unknown fields silently, so a typo in an attenuation: block dropped that constraint and minted a broader token than the route author wrote. The invoker comment claiming unknown keys flow through to the plugin was wrong and is corrected. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
DelegationKey is a struct, so the delegated_tokens map could not round trip through serde_json. Any serialization carrying a minted token failed. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
A plugin holding append_labels but not read_labels sees an empty filtered label set, so its returned set is never a superset of canonical and the whole edit was discarded, silently disabling write-only tainting and DLP plugins. Monotonicity is already structural here because the returned labels are folded in rather than assigned, and the laundering check that needs capability context lives in the executor's labels_ok. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
A panicking branch contributed no taints and no Halt, so a branch that would have denied was silently dropped and the parallel block continued. In a policy engine that is a fail-open, so a panic now halts the block. This reverses the earlier choice to keep sibling branches running, and drops the comment that claimed the panic was logged when the crate has no tracing dep. Ported from praxis-proxy#54. Signed-off-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
RouteIdentityStep documents that a step name must match a top-level plugins: entry registered under identity.resolve, but nothing enforced it. An unresolvable name found no entry at dispatch and was dropped with no error, so the route ran with that authentication step missing. Global, group, and route authentication share one shape, so one check covers all three. Reworked from praxis-proxy#54 for the renamed authentication and bundles fields. Co-authored-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
A `result:` naming the same field twice must not keep one pipeline silently, since dropping a redact for a passthrough leaks the field. Every route reaches RouteYaml through a serde_yaml::Value and serde_yaml rejects a repeated key there, so the property already holds; praxis-proxy#54 proposed a custom deserializer for it, which the current parse path makes unnecessary. Pinned because a refactor deserializing RouteYaml from a string would be last-wins. Co-authored-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`result.rows.ssn | redact` silently redacted nothing when `rows` was an array: the dotted helpers required every segment to be an object, so the walk fell off at the array and the pipeline never ran. A rule that reads as redacting every row's SSN passed them all through, which is a PII fail-open. Paths now expand to one concrete path per array element before the pipeline runs, and the dotted read, write, and remove helpers follow numeric segments so the expansion resolves. A terminal array is still one path, so whole-value pipelines keep their semantics. Expansion is bounded in depth and width, and an over-large shape denies rather than half-redacting the head and passing the tail through. The same fan-out applies to `do:`-embedded field ops. Reworked from praxis-proxy#54 against the current route and evaluator shape. Co-authored-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The elicitation id is a single flat bag key and a present id means "already dispatched, poll it", so two elicit steps in one phase collide inside a single request: the first dispatches and writes the key, the second reads it, skips its own dispatch, and adopts the first's verdict. A `require_approval` written after a `confirm` never reaches an approver, and which step wins is only evaluation order. The config expresses two independent gates the engine can enforce as one, so it is rejected at load. The parser checks each block and the visitor rechecks the fully-stacked route, which is where a global or group elicit combined with a route one first becomes visible. Reworked from praxis-proxy#54 for the renamed phase fields. Co-authored-by: Shane Utt <shaneutt@linux.com> Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Drop em dashes and trim the wordier doc blocks across the ported commits. Comment-only, except for one real fix: `get_dotted` is public and its doc linked `expand_field_paths`, which is `pub(crate)`, so rustdoc under `-D warnings` failed. That link is now plain code formatting. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Two fixtures named an `authentication:` step whose plugin was absent from `plugins:`, which the new load-time check refuses. Both were under-specified rather than wrong to reject: engine.rs resolves an authentication step by finding a matching entry and drops it with no else branch, so such a route would resolve no identity plugin and authenticate with nothing. delegation_identity_warning also needed an inert factory, since a declared plugin's `kind:` must resolve to one. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Missed running fmt after the previous commit. No behavior change. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Comment-only: no code or doctest lines change. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Name the fields `chain_extends` compares, since the bug it fixed was three of them missing and an unnamed set invites dropping one again. Restore the `DeclassifierToken` requirement and a line on each label test saying what regressed, so neither reads as redundant later. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
araujof
changed the title
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
3 participants
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
This fixes correctness and fail-closed behavior across policy evaluation, credential handling, runtime merging, field transforms, configuration, and orchestration. It carries forward the parts of #54 that still apply after #55 and #56, with regression coverage for each fix.
Changes
confidence * 100.0to evaluate correctly.Debugoutput and stripsBearercase-insensitively, as required by RFC 9110.authorization_details,ttl_seconds, andtimestamp, preventing an existing hop from being widened.httpandcustom, preserving changes such as header rewrites during route merging.AttenuationConfigrejects unknown keys so misspelled constraints cannot produce broader tokens.delegated_tokensuses a structured key.labels_ok.authenticationstep names.result.rows.ssn | redactupdate every matching element. Oversized structures fail closed rather than being partially transformed.Behavior changes
Two changes deserve closer review:
Scope
Changes from #54 that are already covered on main are not included: parser shorthand support from #55,
RouteCacheKeytags made obsolete by route resolution changes, and the concurrent-path synthesized denial. YAML duplicate keys are already rejected by the current parser, so that case only adds a regression test.