feat: configurable claim mapping for the JWT identity plugin - #31
Conversation
A path addresses a claim value by dot-separated segments. Only `.` and `\` are special, so `cognito:groups` and a URL-named Auth0 claim are reachable, the latter by escaping its dots. Malformed paths are rejected at parse time, which is construction time for a configured map. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…token corpus 38 token shapes paired with the identity they produce, drawn from Keycloak, Auth0, Cognito, SPIRE and Kubernetes documentation, plus the shapes a reimplementation is most likely to get wrong: a string where an array is expected, an empty array that must not fall through, a non-string element, and a null claim. Each entry records its provenance as a data field. Structural checks pin the coverage: every role, both branches of every fallback, all three aud shapes, and a declining entry per role. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
A field is written as a path, a list of candidates, or an object carrying `paths` plus `merge`, `split` and `on_missing`. Compilation parses every path and checks every field name against its role's own set, so a typo, a malformed path, or `merge: union` on a field holding one value fails at plugin construction with the field named. Field shapes are read from the JSON value rather than through an untagged enum, whose one error text would name neither the field nor the path. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
One resolution routine for all three roles. A candidate that is present but the wrong shape counts as not resolving, so the fallback chain continues; a claim holding an empty array does resolve, so it stops the chain and is reported as empty rather than missing. The claims bag excludes the registered JWT claims plus every single-segment declared path, which reproduces the Rust mapper's static reserved lists, and a nested path leaves its parent policy-visible. `claims.include` can restore any name, which is what makes gating on the issuer expressible. The SPIFFE prefix is checked per candidate and has no config surface. Misses aggregate into one debug event per call naming every path tried, distinct from the event for a field that resolved to nothing. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Four embedded JSON maps behind one registry table, so a preset cannot go missing at deploy and a preset added without a test is not possible. An unknown name lists every valid one. Each provider preset declares a candidate only where the provider mints the claim, and its description names what it omits: Keycloak's groups claim holds realm roles rather than groups, Auth0 forbids a bare roles claim so roles are per-deployment namespaced, and Cognito's roles claims hold IAM ARNs. Filling a field with the wrong concept is worse than leaving it empty, because the operator has no reason to look, so each omission is pinned by a test. `split` applies to a string, not to an array's elements: an array already says where its elements end, and splitting them would change what a claim carrying an element with a space in it produces. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
… claim_map `claim_mapper` now names any shipped preset instead of only `standard`, and a new `claim_map` field takes an inline map. Setting both is a config error rather than a precedence rule. An absent setting still resolves to the standard preset, so a deployment that changes nothing sees what it saw. The section matching the configured role is required at construction, so a map paired with the wrong role fails at startup rather than denying every request. The three mapping denials keep their code and stop naming `sub` and `client_id` as though they were fixed, pointing at the debug diagnostics instead. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Every corpus entry maps through both paths and must agree field for field, including the whole claims bag and the order of the client fields, where the declaration-order and no-deduplication choices are the only observable ones. A token the Rust mapper declines must be declined by the preset too. Dropping array_only, reordering the client anchor candidates, dropping the scope split, and dropping the aud candidate each fail the gate, so it is not passing vacuously. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Thirteen tests through the real resolver on signed tokens: nested and per-client roles through a union map, an escaped URL claim name alongside the unescaped path that resolves nothing, cognito:groups without escaping, a delimited scope split and unsplit, a preset by name, iss made policy-visible, a mistyped path both permissive and fatal, the SPIFFE prefix per candidate, and construction failures reaching the host through the factory. The rustdoc carries the quoting trap: the plugin receives JSON, so a double-quoted YAML scalar needs doubled backslashes where a plain or single-quoted one does not. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The preset diverged from the Rust standard mapper on two token shapes the corpus never reached, both a claim carrying the wrong JSON type as the second candidate of a chain. An array-valued `scope` granted its elements as permissions where the mapper, reading `scope` through a string accessor, grants none. That affected `subject.permissions` and `client.authorized_scopes`, so a token could gain permissions it did not have before. The grammar had `array_only` with no inverse, so no preset could express the accessor: `string_only` is that mirror, and the two together are rejected because nothing would satisfy them. A non-string `client_id` fell through to `azp` where the mapper declines, because the mapper picks the first anchor key that *exists* and only then requires a string of it. `stop_if_present` expresses that chain, and the universal present-but-unusable-keeps-looking rule stays the default, which is what the collection chains and the SPIFFE chain need. Also: a mapped workload trust domain is now required to equal the SPIFFE ID authority rather than replacing it unchecked, and falls back to derivation when its path resolves nothing. A section declaring no path for its anchor warns at load and names the anchor in its miss event, instead of denying every token while the log the denial points at says nothing. A shape sweep now enumerates every JSON shape each mapped claim can hold, and every pair across each fallback, 1040 cases. It fails on either parity break; the 46-entry corpus did not. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
… shape sweep The trust-domain disagreement warning is the one diagnostic carrying a value read from the token rather than from operator config, and it was formatted for Display, so a claim holding a newline could forge a log line. It is Debug-escaped now, with a test asserting no raw newline reaches the record. The shape sweep had no SPIFFE-shaped string, so every workload case declined on both sides and agreed vacuously: 169 of its pairs proved nothing about the role whose invariants matter most. Adding a SPIFFE ID and a non-SPIFFE URI takes the sweep to 1350 cases and makes the workload half real. Also corrects a rustdoc claim that all three candidate flags are rejected on a field holding one value. Only the shape flags are, and only they should be: the standard preset sets `stop_if_present` on its client anchor, which is such a field, so the documented rule would not have compiled. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…e the trust domain Review found the trust domain and the chain flag had each acquired a second, incoherent meaning, so both are narrowed rather than documented. A mapped workload trust domain could only be redundant or fatal: required to equal the SPIFFE authority, a differing string denied every token while a wrong-typed one was ignored. It is no longer mappable. The authority is derived from the identity, always, which is what makes a policy gating the trust boundary read it off the identity it belongs to. `stop_if_present` on a collection truncated a `merge: union` chain, dropped the candidates behind it, and reported the stop as an ordinary miss with a truncated path list. It is rejected there. Each flag is now valid exactly where it means something: the shape flags on a field holding a collection, the chain flag on one holding a single value. The undeclared-anchor warning fired once per request for a condition that is static; it is emitted once at construction and the anchor is still named in the per-request miss event the denial points at. All four presets now decline a present-but-unusable client anchor rather than three of them falling through to `azp`. Two tests close a gap that let stripping `string_only` from any provider preset pass the whole suite. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…nfig Every field in both structs is optional or defaulted, so a misspelling deserialized to the default and took effect silently. `claim_maps` left the resolver on the standard preset while the operator believed their map was live, and a misspelled `audiences` was worse than confusing: the field defaults to an empty list, and an empty list turns audience validation off, so a typo silently accepted tokens minted for any audience. `deny_unknown_fields` on both, which is the reason the policy parser already carries it. **Breaking** for a config that passes a key this plugin does not read: it now fails at load naming the key, where before it was ignored. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…uals Tests for what was unexercised: the remaining malformed-candidate shapes, a malformed `claims` block, `on_missing: deny` on a field holding one value, and Keycloak's camelCase `clientId` tail candidate. Each provider preset's candidates are now pinned in order, which only the standard preset had through its parity gate. Fallback coverage is asserted on the claims an entry carries rather than on its name, with each chain's accept rule written down, so an entry edited until it no longer reaches its branch fails instead of passing quietly. A test tying `SCALAR_FIELDS` to the fields the mapper resolves as one value found a dead `trust_domain` entry left behind when that field stopped being mappable. A malformed `claims` block now names the field, matching every other error in the module. Diagnostic field lists are emitted as arrays rather than pre-joined strings, so a JSON subscriber does not have to re-split them. Path rendering moved off the resolving path: a field's candidates are counted while resolving and rendered only when a miss is recorded, so escaping no longer costs a request that resolved. `SplitMode` is `#[non_exhaustive]`, which its own documentation already assumed. The raw preset table and the authored field types are no longer public, since nothing outside the crate can use them. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The overrides lived inside `claim_map`, which is mutually exclusive with `claim_mapper`, so gating on `iss` meant copying a whole shipped preset into an inline map to add two lines. They are now a plugin-level `claims:` field, a sibling of both mapper fields, so a preset and an inline map reach them the same way and there is no precedence rule to learn. The alternative was to let a `claim_map` carrying only a `claims` block sit alongside `claim_mapper` as an overlay, which makes the same field mean either a whole map or a fragment depending on its contents. The bag is a separate output from the typed fields, so it reads as its own setting. **Breaking** for a map written against the previous shape: a `claims` block inside `claim_map` is now rejected, and the error lists the sections a map may declare. Also extracts the end-to-end harness into `tests/common/`. The keypair, minter, config builder and pipeline call were duplicated between two suites; there is now one copy. `jwks_url_e2e` keeps its own keypairs deliberately, since it exercises rotation and multiple key ids, which a process-global key cannot express. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Merging main brought a stricter clippy set and a spell checker. Three fixes in this crate: `assert!(x.is_ok())` on the default preset lookup becomes `expect`, which is what `assertions_on_result_states` asks for and reads better anyway. Two test fixtures used deliberately misspelled strings that the spell checker reads as mistakes in the source rather than as the fixtures they are. A mistyped path now misspells the leaf segment instead of `access`, and a misspelled config key is `excludes` rather than a misspelled `exclude`. Both still test what they tested. Also corrects `unparseable` in a doc line, the one pre-existing hit the checker finds in this crate. The others it reports are in crates this branch does not touch. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The three entries for this work ran long and buried the breaking parts in prose. Shortened to the style the rest of the file uses, and the breaking changes moved to their own section. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
9b18ee5 to
d7b34d5
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Review: configurable claim mapping for the JWT identity plugin
Thorough review of all 20 changed files (~6,500 lines added). The design is sound: hand-rolled config parsing over serde_json::Value produces targeted error messages that name the field and path, the compilation step catches every misconfiguration at startup, and the standard-preset equivalence suite with its corpus and shape sweep is an unusually strong compatibility guarantee.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 2 |
No blocking issues found. Two observations below.
The authority carries the trust domain and SPIFFE requires it, so `spiffe:///ns/default/sa/agent` named no trust boundary yet still became a workload identity whose trust domain was the empty string. Both mappers now share one check, so they cannot drift on what a SPIFFE ID is. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Validation returned on the first claim found in both `exclude` and `include`, so an operator with two of them fixed one, restarted, and met the next. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…cache Callsite interest is cached process-wide, so `with_default` per test did not own whether an event fired: a parallel test installing its own subscriber rebuilt the cache and could leave the callsite disabled between the `debug!` and the assertion. One subscriber, installed once and always interested, with a thread-local sink per test. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
terylt
left a comment
There was a problem hiding this comment.
Overall, nice work!
Here are some comments:
Four findings follow, ordered by how much they matter.
1. merge: union does not dedupe, and it shows up on client fields
builtins/plugins/identity-jwt/src/configured_mapper.rs:142
resolve_collection concatenates the values from each path under merge: union without deduplicating. For subject fields this is invisible because they land in HashSets downstream. ClientExtension.roles, .permissions, .teams, .authorized_scopes and .authorized_audiences are Vec<String>, so the duplicates survive.
The union example documented in this PR is enough to trigger it. With realm_access.roles unioned against resource_access.<api>.roles, and a role name present in both (common in Keycloak), client.roles comes out as ["admin", "viewer", "admin"].
Policy evaluation is not affected, since the APL bag re-collects into a HashSet. What is affected is anything reading ClientExtension directly: audit logs, serialized output, other plugins. The other awkward part is that the same config behaves differently depending on role, deduped for user, not deduped for client.
Dedupe in resolve_collection while preserving first-seen order.
2. Dotted paths in claims.exclude / claims.include are accepted and match nothing
builtins/plugins/identity-jwt/src/claim_map_config.rs:305
ClaimsOverrides::validate only checks that exclude and include do not overlap. The entries themselves are never validated as plain top-level claim names.
Every other path-shaped field in this config uses dotted syntax, so an operator writing claims: {exclude: [realm_access.roles]} to keep a sensitive nested claim out of the policy-visible bag is a plausible mistake rather than a contrived one. I confirmed it: the config loads without complaint, the entry matches nothing, and the entire realm_access object stays in subject.claims.
Rejecting an entry that contains an unescaped ., or at minimum warning at load time, matches the stance this PR already takes elsewhere. deny_unknown_fields and the undeclared-anchor warning both exist to stop misconfiguration from going quiet, and this is the same class of problem.
3. claims overrides are silently ignored for role: workload
builtins/plugins/identity-jwt/src/resolver.rs:257
with_claims attaches the overrides regardless of the configured role, but WorkloadIdentity has no claims bag, so claims_bag is never called on that path. A config with role: workload and claims: {include: [iss]} builds without error and does nothing at all.
The construction-time warn! a few lines below for a missing anchor is the precedent. Same treatment fits here.
4. Diagnostic strings are built before the level check
builtins/plugins/identity-jwt/src/configured_mapper.rs:275
Diagnostics::emit, and record / record_scalar_miss feeding paths_tried at :234 and :253, render their strings unconditionally. The comment says the escaping cost never lands on a resolving request, which assumes a miss is rare, but a miss is the common case.
A plain {sub, email} token under the default standard preset misses roles, permissions and teams. That is 5 ClaimPath::to_string() renderings into paths_tried, 3 Vec<String>, and 3 more format! plus join strings in emit, all thrown away when the subscriber sits at info. Every request pays it.
Gate on tracing::enabled!(Level::DEBUG), or defer the rendering into a lazily formatted field. I have not benchmarked this, so the size of the win is unmeasured, but the allocations are unconditional either way.
A `merge: union` field concatenated its candidates, so a value two of them carry arrived twice. The subject fields are sets downstream and swallowed it, but the client fields are `Vec`s, so the same map behaved differently per role and the duplicate reached audit logs and serialized output. Keycloak naming one role under both `realm_access` and `resource_access.<client>` is the ordinary case. Deduped in the engine, first-seen order kept. `first_match` is left alone: duplicates there are the claim's own content. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Both override lists take top-level claim names, because the claims bag is
keyed by name. Nothing said so: every other path-shaped field in this config
takes dotted syntax, so `claims: {exclude: [realm_access.roles]}` loaded
without complaint, matched nothing, and left the claim visible to policy.
The names are parsed at load now, the same way a path is, so a dotted entry
fails there and a claim whose own name holds a dot is reachable by writing
`\.`. The parsed names reach the mapper, so nothing parses per request.
A `role: caller_workload` resolver has no claims bag at all, so the setting
is inert there. It warns at load, which is how the undeclared anchor one
field over is handled.
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`Diagnostics` rendered every path a field tried and formatted the event
strings before the subscriber's level was consulted. The comment justified
it by a miss being rare, which it is not: a plain `{sub, email}` token under
the standard preset misses `roles`, `permissions` and `teams`, so every
request paid five path renderings and three `format!`s that were then
dropped.
The level is checked once per mapping call. The `on_missing: deny`
bookkeeping stays ungated, since it decides the answer rather than what
gets logged.
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Addressed! Thanks! |
|
Re-reviewed. All comments addressed. A couple of small comments below but I will approve! All four fixes are correct. Notes on each, then two small things that are not blocking. Union dedupe
Worth recording why this is safe for the parity story: no shipped preset uses Claims override namesThis went further than the finding asked for, and in a good direction. A dotted entry is refused at load, and The overlap check now runs on the unescaped names, so a conflict written two different ways is still caught. That is a small improvement over what was there before. Workload warningGated on exactly the right role. I confirmed by reading the mapper that Diagnostics gatingSafe. One caveat I did not chase far: Two small things
|
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Summary
Adds claim mapping in the JWT identity plugin as configuration. It was fixed in
StandardClaimMap, andclaim_mapperaccepted only"standard", so an operator whose IdP nests roles underrealm_access.rolesor namespaces them behind a URL had to patch the crate and rebuild.Closes: #27
Two ways to configure it, mutually exclusive:
Paths use dots for nesting.
\.escapes a literal dot, so a claim namedhttps://my-app.example.com/rolesis reachable. Colons are literals and need no escaping.Compatibility
A config that does not change produces the identity it produced before.
StandardClaimMapis untouched and the tests use it as the reference:Both fail if a preset candidate is changed. Review found two places where the preset and the old mapper disagreed, an array-valued
scopeand a non-stringclient_id. The sweep exists because the hand-written corpus missed both.Breaking changes
config:and in atrusted_issuersentry are rejected. Both structs default every field, so a misspelling took effect silently. A misspelledaudienceswas the worst case, because an empty audience list turns audience checking off.claimsis a top-level field, not part ofclaim_map.trust_domainis no longer mappable. It is the SPIFFE ID's authority, so it is derived from the identity.New capability
claims: {include: [iss]}makes the issuing IdP readable from a policy. Subject claims are the only route from a JWT claim to a policy and registered claims were always dropped, so a deployment with several trusted issuers could not gate on which one signed the token.Presets
Each preset declares a candidate only where the provider mints the claim, and its
descriptionrecords what it leaves out:groupsclaim holds realm rolescognito:rolesholds IAM role ARNsTests assert those fields stay empty, so adding a candidate later cannot pass quietly.
Errors
Misconfiguration fails when the plugin is built, not on the first request: both mapper fields set, an unknown preset name, a malformed path (named with its field), a map with no section for its role, a flag on a field where it has no meaning. A section with no path for its anchor logs a warning at startup, since it would deny every token.
At runtime a field whose paths all miss stays empty and logs one debug event naming the paths tried.
on_missing: denyturns that intoauth.mapping_failed, the existing code. Thespiffe://check on workload identities has no config surface that disables it.