docs(cmf): settle the extensions bag contract - #59
Conversation
The twelve CMF slots had an implicit per-type empty/absent rule and a claims gap that only lived next to extractors. Document the contract, assert it in the bridge, and move present-empty sets into the PDP agreement subset so APL, CEL, cedar-direct, and OPA deny the same way. Signed-off-by: mkoushni <mkoushni@redhat.com>
araujof
added
documentation
Keep the praxis-proxy#18 changelog bullet and take the assertions entry from praxis-proxy#56. Signed-off-by: mkoushni <mkoushni@redhat.com>
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Thorough documentation and differential testing of the CMF bag contract. The normative doc, bridge tests, and pdp-diff cases are consistent and well-structured. Two convention items in the new bridge tests.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 2 |
| bag | ||
| } | ||
|
|
||
| /// The per-type contract in `docs/cmf-extensions.md`: inside a present |
There was a problem hiding this comment.
[Medium] Project conventions prohibit doc comments on #[test] functions ("Do not add doc comments (///) or regular comments (//) on test functions"). Remove the /// block — the function name already documents its intent.
|
|
||
| let bag = bag_of(ext); | ||
|
|
||
| // StringSet: present and empty. |
There was a problem hiding this comment.
[Medium] Lines 177, 201, 228, 234, and 242 use inline comments as section headings inside this test body. Project conventions require assertion messages or tracing calls instead of inline comments in test function bodies. Convert each section heading to a tracing::debug! call (e.g. tracing::debug!("StringSet: present and empty");) or remove them — the assertion messages on each check already explain the intent.
|
hi @mkoushni Overall, nice work... just a few small things and I think I'll create a couple of related issues. Blocking: the APL row of the missing-key tableThis is the only place the document misdescribes the code. The rest is accurate 1.
2. The row should name negation. Suggested replacement for the row:
Worth a test, since the point of the document is that it is checkable. Asks, not blockers3. The The cost is CEL-specific. On a bridge-built bag with a security slot and no Please record that in the row as a known sharp edge rather than as a rule whose 4. 5. The
6.
7. Follow-ups, fine to defer8. Nothing binds the catalog to the bridge. The new test walks default 9. The 10. |
terylt
left a comment
terylt
left a comment
There was a problem hiding this comment.
requesting some small changes above ^
Keep the CMF extensions-bag changelog under Unreleased; take 0.2.0 wording for assertions and token cache. Signed-off-by: mkoushni <mkoushni@redhat.com>
Name negation on the APL missing-key row, record the CEL authenticated sharp edge, and catalog unbridged security.objects / security.data. Omitted claim scalars agree on Deny, so they leave the allowlist. Signed-off-by: mkoushni <mkoushni@redhat.com>
Keep the CMF extensions-bag changelog under Unreleased alongside the PPE documentation entry from praxis-proxy#82. Signed-off-by: mkoushni <mkoushni@redhat.com>
araujof
left a comment
araujof
left a comment
There was a problem hiding this comment.
Suggestions for the absent-key contract, all verified together on 4216a6a: cargo fmt --check clean, praxis-policy-apl-core 352/352.
Two things in the APL row of the missing-key table:
-
deny when not authenticateddoes not parse.notis reserved for thenot inphrase (parser.rsreturnsNOT_IS_RESERVEDin predicate position), and awhen:clause hands its value straight toparse_predicate(parser.rs:1923), sowhen: not authenticatedfails at config load.docs/content/apl/apl-grammar.mdalready states the rule, and line 36 of this file already uses!authenticated. -
The row omits two forms that are also true on an absent key:
!(...)around any condition, andX not in Y(InSetresolves to false, then negates). That makes the "omitted claim scalar agrees on the verdict (all Deny)" claim false for!=andnot in. Measured on an empty bag:
| Rule | APL | CEL / OPA / cedar-direct |
|---|---|---|
require(claim.tenant != "acme") |
Allow | Deny |
require(subject.type not in blocked_types) |
Allow | Deny |
Denylists are the shape that hits it.
One note on the harness half: AllowlistEntry has cedar, cel and opa and no apl field, and assert_apl runs only on AgreeAllow / AgreeDeny, so this split cannot be recorded as a case today. Adding apl: Outcome would make it executable; fine to defer alongside #78 as long as the doc no longer claims agreement the harness does not check.
|
|
||
| | Engine | Missing key | Empty `StringSet` | | ||
| |---|---|---| | ||
| | APL | false for presence, equality, membership, and order; `!=` is true (an absent key is not equal to a value, matching `!(x == y)`). Negation is the other exception: `!key` is true (`deny when not authenticated` is the idiom). | `contains` / `in` is false | |
There was a problem hiding this comment.
not authenticated does not parse, and !(...) / not in are missing from the true-on-absent list.
| | APL | false for presence, equality, membership, and order; `!=` is true (an absent key is not equal to a value, matching `!(x == y)`). Negation is the other exception: `!key` is true (`deny when not authenticated` is the idiom). | `contains` / `in` is false | | |
| | APL | false for presence, equality, membership, and order. Every negated form is true: `!=`, `!key`, `!(...)`, and `not in`. Negation is spelled `!`; `not` is reserved for the `not in` phrase, so the idiom is `!authenticated`. | `contains` / `in` is false | |
| | APL | false for presence, equality, membership, and order; `!=` is true (an absent key is not equal to a value, matching `!(x == y)`). Negation is the other exception: `!key` is true (`deny when not authenticated` is the idiom). | `contains` / `in` is false | | ||
| | CEL | evaluation error; default `OnError::Deny` turns it into a denial that reports a key error, not a policy false | `in` is false | | ||
| | cedar-direct | empty `roles` / `permissions` / `teams` / `claims` on the principal so those names exist; no `subject.id` is a dispatch error | `contains` is false | | ||
| | OPA | undefined; without `default allow := false` the query is a default deny | `in` is false | |
There was a problem hiding this comment.
Adds the verdict split after the table, so the fail-open direction is stated rather than implied.
| | OPA | undefined; without `default allow := false` the query is a default deny | `in` is false | | |
| | OPA | undefined; without `default allow := false` the query is a default deny | `in` is false | | |
| The negated forms are the one place the four split on the **verdict**, not just | |
| the cause. On an empty bag: | |
| | Rule | APL | CEL / OPA / cedar-direct | | |
| |---|---|---| | |
| | `require(claim.tenant == "acme")` | Deny | Deny | | |
| | `require(subject.roles contains "hr")` | Deny | Deny | | |
| | `require(claim.tenant != "acme")` | **Allow** | Deny | | |
| | `require(subject.type not in blocked_types)` | **Allow** | Deny | | |
| A denylist is the shape that hits this. Guard the key: | |
| `require(exists(claim.tenant) & claim.tenant != "acme")` denies when it is absent. |
| bridge filled both from the same set. A policy written against an **omitted | ||
| claim scalar** agrees on the verdict (all Deny) and is an `AgreeDeny` in | ||
| `ppe-pdp-diff`; the cause still differs. A flattened bool whose namespace was |
There was a problem hiding this comment.
Scope the agreement claim to == / order / membership probes.
| bridge filled both from the same set. A policy written against an **omitted | |
| claim scalar** agrees on the verdict (all Deny) and is an `AgreeDeny` in | |
| `ppe-pdp-diff`; the cause still differs. A flattened bool whose namespace was | |
| bridge filled both from the same set. A policy that probes an **omitted | |
| claim scalar** with `==`, order, or membership agrees on the verdict (all Deny) | |
| and is an `AgreeDeny` in `ppe-pdp-diff`; the cause still differs. A `!=` or | |
| `not in` probe does not agree; see above. A flattened bool whose namespace was |
| fn missing_key_matches_cmf_extensions_table() { | ||
| // `docs/cmf-extensions.md` "What each decision point does with a | ||
| // missing key" — APL row. Presence, equality, membership, and | ||
| // order are false; `!=` is true; `!key` is true. |
There was a problem hiding this comment.
| // order are false; `!=` is true; `!key` is true. | |
| // order are false; every negated form is true. |
| ); | ||
| let rule = crate::parser::parse_rule("require(authenticated)", "test") | ||
| .expect("require(authenticated) parses"); | ||
| assert!( | ||
| matches!(evaluate_rules(&[rule], &bag), Decision::Deny { .. }), | ||
| "require(authenticated) fires on an empty bag" | ||
| ); |
There was a problem hiding this comment.
Pins not in and !(...), plus the rule-level Allow so the fail-open direction is asserted rather than assumed.
| ); | |
| let rule = crate::parser::parse_rule("require(authenticated)", "test") | |
| .expect("require(authenticated) parses"); | |
| assert!( | |
| matches!(evaluate_rules(&[rule], &bag), Decision::Deny { .. }), | |
| "require(authenticated) fires on an empty bag" | |
| ); | |
| ); | |
| assert!( | |
| eval_pred("subject.type not in blocked_types", &bag), | |
| "`not in` on an absent set key is true" | |
| ); | |
| assert!( | |
| eval_pred(r#"!(subject.id == "alice")"#, &bag), | |
| "`!(...)` on an absent key is true" | |
| ); | |
| let rule = crate::parser::parse_rule("require(authenticated)", "test") | |
| .expect("require(authenticated) parses"); | |
| assert!( | |
| matches!(evaluate_rules(&[rule], &bag), Decision::Deny { .. }), | |
| "require(authenticated) fires on an empty bag" | |
| ); | |
| let denylist = | |
| crate::parser::parse_rule("require(subject.type not in blocked_types)", "test") | |
| .expect("denylist require parses"); | |
| assert_eq!( | |
| evaluate_rules(&[denylist], &bag), | |
| Decision::Allow, | |
| "an unguarded denylist require allows on an absent key" | |
| ); |
araujof
left a comment
araujof
left a comment
There was a problem hiding this comment.
Nice work! Just a few minor suggestions, otherwise good to go.
- Consider moving
docs/cmf-extensions.mdtodocs/content/cmf-extensions.md, and add a link to it fromdocs/content/extensions.md. docs/cmf-extensions.mdomits APL cases where a missing key makes!=,not in, or some negated conditions true. Narrow the “all four deny” claim and add differential tests for!=andnot in.deny when not authenticateddoes not parse. See inlined patches and comments.- Do not recommend
has(role.hr): when no role keys exist, CEL has norolenamespace and returns an error. Use the always-presentsubject.rolesset. - List the exact
argsandresultkeys emitted for top-level scalar and array payloads, not onlyargs.*andresult.*. - Document cedar-direct’s missing
subject.typebehavior. It defaults toUser, while bridged values are lowercase, so type-scoped policies can miss the principal. - The claim that CEL cannot guard missing authentication is too broad.
has(subject.id)works when the subject namespace exists; only a completely absent subject leaves no guard. - Update the changelog for the removed public
BAG_WORKLOAD_PREFIXconstant and the changed capability mappings.
|
Nice work! Here is my re-review: Fix before mergeThe changelog only has an Added bullet. The second commit removes The PR body has the same gap: the Changes section lists docs, Still open, not blockingThe original-vs-flattened section states the five pairs but not the rule. Nine collections are set-only ( |
Move the catalog under docs/content, pin APL != / not in splits in ppe-pdp-diff, and document cedar-direct's User default versus lowercase bridged types. Signed-off-by: mkoushni <mkoushni@redhat.com>
|
Docs. docs/cmf-extensions.md moved to docs/content/cmf-extensions.md, with links from extensions.md, cmf.md, and the architecture index. APL missing-key contract. The “all four Deny” claim is limited to presence, equality, membership, and order. Negated forms (!=, !key, !(...), not in) are true. The idiom is !authenticated — deny when not authenticated does not parse. ppe-pdp-diff now has missing-claim-not-eq and missing-not-in. On not in, OPA Allows with APL (not of undefined is true); CEL and cedar-direct Deny. That is recorded instead of claiming all three PDPs Deny. CEL. has(role.hr) is no longer recommended when the role namespace is absent; use subject.roles. has(subject.id) is the guard when the subject namespace exists; only a completely absent subject leaves CEL with no authentication guard. Payloads. Top-level scalars and scalar arrays land on the bare keys args / result, not only args.* / result.*. Tests pin that. cedar-direct. A missing subject.type defaults to PascalCase User; the bridge writes lowercase (user), so type-scoped policies can miss that principal. Changelog. Unreleased now records the BAG_WORKLOAD_PREFIX removal and the read_labels / read_workload mapping changes. |
Hosts look at the changelog for public-surface changes; read_labels and read_workload advertised prefixes the extractors never wrote. Also state that only five collections flatten and the rest are set-only by design. Signed-off-by: mkoushni <mkoushni@redhat.com>
|
There was a problem hiding this comment.
One nit, not blocking
crates/ppe-apl-cmf/src/payload.rs,assert!(!bag.contains("args."))is vacuous.AttributeBag::containsis an exact key lookup, so a key namedargs.can never exist. If the intent is noargs.*children, usebag.iter()orlen() == 1.
Approving. Fine to fold in on merge or leave to #77.
AttributeBag::contains is an exact lookup, so !contains("args.") never
caught children. len() == 1 is the real check.
Signed-off-by: mkoushni <mkoushni@redhat.com>
|
test(cmf): assert scalar payloads occupy a single bag key fix: len() == 1 |
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Re-Review
Re-review after 6 new commits. One new convention finding not covered by the previous review.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
| // `docs/content/cmf-extensions.md` "What each decision point does with a | ||
| // missing key" — APL row. Presence, equality, membership, and | ||
| // order are false. Every negated form is true: `!=`, `!key`, | ||
| // `!(...)`, and `not in`. |
There was a problem hiding this comment.
[Medium] This 4-line block comment inside the test body violates the convention: "Never use inline comments inside test function bodies. All explanatory text must be either an assertion message or a tracing::debug! call." The function name missing_key_matches_cmf_extensions_table already documents the intent. Move the doc reference into a tracing::debug! call, e.g.:
tracing::debug!("docs/content/cmf-extensions.md — APL missing-key row: presence/equality/membership/order are false; negated forms are true");5 participants
Summary
subject.claimsis not a bag key (docs/content/cmf-extensions.md).capability_namespaceswith the keys the extractors actually write; drop the unused publicBAG_WORKLOAD_PREFIX.ppe-pdp-diffmoves present-empty sets and omitted claim scalars (==/ order / membership) into the agreement subset. APL!=/not inon a missing key are allowlisted splits. A flattened bool with no namespace, and a missingsubject.id, stay on the allowlist.Refs #18. Present-empty sets and omitted claim scalars agree on the verdict for presence, equality, membership, and order. Genuinely absent flattened-bool namespaces and a missing principal still diverge (CEL eval error / Cedar dispatch). Making those agree needs CEL root seeding, load-time validation via
Program::references(), and a generated Cedar schema — follow-up to file.Changes
Added
docs/content/cmf-extensions.md— normative bag contract:StringSetpresent-empty; optional String/Int/Float omitted; flattened member bools presence-only; non-option scalars always written)role.*/perm.*/team.*/client.role.*/client.perm.*(the set is primary; the five flattened collections exist because that idiom predates the sets; nine other collections are set-only)subject.claimsmap in the bag (would need a sixthAttributeValuevariant and would duplicate flattenedclaim.*)!=/not inand cedar-direct'sUserdefault vs lowercase bridgedsubject.typeextract_extensionsslots, keys, and typesargs/args.<dotted>,result/result.<dotted>,data/data.<dotted>;raw_credentialsandcandidate_constraintstay out of the bag)docs/content/extensions.md,docs/content/cmf.md, anddocs/content/index.md.ppe-apl-cmfsrc/lib.rs— rustdoc points atdocs/content/cmf-extensions.md.src/extensions_bridge.rs— rustdoc onextract_extensionsrestates the per-type contract.present_slots_follow_the_absent_value_contract— present empty slots emit empty sets, omit optionals, skip flattened bools, write non-option zeros/false, and never emit asubject.claimsparent key.original_set_and_flattened_bools_stay_paired— a member in the set also appears as the flattened bool, and both stay in lockstep.src/security.rs— extractors writecaller_workload.*/this_workload.*andsecurity.labels; noworkload.*.src/payload.rs— top-level scalar and scalar-array payloads land on the bare keysargs/result.src/capability_namespaces.rs—read_labelsunlockssecurity.labels;read_workloadunlockscaller_workload.*/this_workload.*(was empty /workload.*at 0.2.0).src/constants.rs— removed publicBAG_WORKLOAD_PREFIX.ppe-apl-coresrc/evaluator.rs—missing_key_matches_cmf_extensions_tablepins APL missing-key semantics, including!=,!key,!(...), andnot in.ppe-pdp-diffCargo.toml/Cargo.lock— dev-deps onpraxis-policy-apl-cmfandpraxis-policy-coreso cases can build bags throughextract_extensions.README.md— present-emptyStringSetis in the semantic subset; omitted claim==/ order isAgreeDeny;!=/not inare allowlisted; pointer todocs/content/cmf-extensions.md.src/cases.rsCase.apl_rule+with_aplso agreement cases also run the native APL evaluator.alice_via_bridge()— bag fromextract_extensions, not a hand-built set.empty-set/bridge-empty-teams/bridge-empty-roles—AgreeDeny.missing-claim-string/missing-claim-int— omitted scalars,AgreeDeny.missing-claim-not-eq/missing-not-in— APL Allows on a missing key; other engines as allowlisted.src/allowlist.rs—missing-collection/missing-subject-idremain;missing-claim-not-eq/missing-not-inadded;empty-setremoved (now subset).src/lib.rs—assert_aplonAgreeAllow/AgreeDenyand on allowlist rows that name an APL verdict.cedar-directsubject.typedefaults to PascalCaseUserwhile the bridge writes lowercase.Changelog
read_labels/read_workloadprefixes now match the extractors.BAG_WORKLOAD_PREFIX.Test plan
make lintcargo test -p praxis-policy-apl-cmfcargo test -p praxis-policy-pdp-diffmake coverage(95.74% lines, floor 95%)