feat: add host-controlled trust-class policy engine#3043
Conversation
…licy-engine # Conflicts: # Cargo.toml
There was a problem hiding this comment.
Code Review
This pull request introduces the ironclaw_trust crate, which implements a host-controlled trust policy engine for IronClaw Reborn. It defines the EffectiveTrustClass to manage privileged trust ceilings, establishes an InvalidationBus for handling trust-change events, and provides a layered policy evaluation mechanism. My feedback suggests optimizing the authority_changed function to avoid unnecessary heap allocations and improving the testability of HostTrustPolicy by abstracting the time provider instead of using Utc::now() directly.
- Fix default_decision docstring/code mismatch — LocalManifest now drops to Sandbox (matching the docstring intent); other origins keep UserTrusted. - Gate fixtures module behind a `test-fixtures` Cargo feature; integration test target opts in via required-features. Production builds cannot import the privileged-trust constructors. - Add AdminConfig::remove, plus module-level docs spelling out the mutation→InvalidationBus.publish contract on every mutator. - Beef up SignedRegistry with a trusted_signers map + SignerEntry struct (still inert in PR1b — the seam is now defensible). - Add LocalDevOverride placeholder source — fourth source named in #3012. - Thread max_resource_ceiling through SourceMatch / BundledEntry / AdminEntry / SignerEntry — no longer dead state. - Remove unused TrustError::SourceRejected variant; document InvariantViolation as the future-extension surface. - T9: add static_assertions::assert_not_impl_any!(EffectiveTrustClass: DeserializeOwned) for the compile-time AC #1 guarantee. - T6: rename misleading curr_removed → curr_unchanged, add real removal case + reorder case, document over-firing as deliberate. - Add lib.rs unit tests so bare `cargo test -p ironclaw_trust` runs smoke checks instead of silently passing 0. Verification: cargo fmt, cargo clippy --all --all-features, and cargo test -p ironclaw_host_api -p ironclaw_trust -p ironclaw_architecture --features ironclaw_trust/test-fixtures all pass (46/46). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…+ injectable clock - authority_changed: replace sort+collect with bidirectional iter().all(contains) + length guard. Allocation-free, faster on small authority lists, better for WASM. Set semantics preserved; reorder of equal sets remains retainable; the length guard catches multiset cases like [a,a] vs [a,b]. (gemini-code-assist comment on invalidation.rs) - HostTrustPolicy::evaluate: inject a Clock instead of calling Utc::now() directly. New `clock` module with `Clock` trait, `SystemClock` (default production wiring), and `FixedClock` test fixture. Existing `HostTrustPolicy::new` is non-breaking — it constructs a SystemClock internally. Tests can use `HostTrustPolicy::with_clock` for deterministic audit-replay / golden-file scenarios. New contract test `evaluate_uses_injected_clock_for_evaluated_at` locks in the guarantee. (gemini-code-assist comment on policy.rs) Verification: cargo fmt clean, cargo clippy --all --all-features -D warnings clean, cargo test -p ironclaw_host_api -p ironclaw_trust -p ironclaw_architecture --features ironclaw_trust/test-fixtures → 47/47. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fix(trust): commits in this branch trip the regression-check workflow's IS_FIX heuristic, but the workflow's tests-detection step fails to see the 24 added test markers in the diff on CI even though they are visible locally — likely an actions/checkout vs pull-request merge-commit interaction. This empty commit's body carries the explicit [skip-regression-check] directive that the workflow honors so the false-positive failure unblocks merge. The PR also has the matching label applied. Tests added in this PR (visible locally via `git diff origin/reborn-integration...HEAD -U0 -- '*.rs' | \ grep -E '^\+.*(#\[test\]|#\[cfg\(test\)\]|mod tests)'`): 24 added test markers across host_api_contract.rs (+4), policy_contract.rs (+15), and ironclaw_trust/src/lib.rs (+2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
left a comment
There was a problem hiding this comment.
Strong PR overall. This is exactly the host-controlled trust-class policy engine I argued for in #issuecomment-4328881303, and the type-level requested/effective split is the right answer to the manifest self-assertion gap I flagged on PR1. Worth pulling out what works before raising concerns:
What's working well:
RequestedTrustClass(freely deserializable) vsEffectiveTrustClass(newtype withpub(crate)privileged constructors) is precisely the boundary the freeze packet was missing. The compile-timeassert_not_impl_any!(EffectiveTrustClass: serde::de::DeserializeOwned)pins the invariant against future refactor drift.test-fixturesCargo feature gating is exactly the right level of paranoia for the privileged constructors. Production builds physically cannot enable it without an explicit Cargo.toml change.TrustProvenancerecorded on every decision gives audit a real story for why a decision came back the way it did.Ok(None)(source did not recognize) vsErr(real evaluation failure) is a clean trait contract.- Digest pinning in
BundledRegistry::evaluatecorrectly implements AC #7: a drift returnsOk(None), falls through to default downgrade. SignedRegistryreturningOk(None)until verification lands is the safe default — won't accidentally trust on the basis of a self-declaredsignerfield.- Clock injection (
HostTrustPolicy::with_clock) makes a security-critical path deterministic. - The doc-level orchestration contract on
BundledRegistry/AdminConfigmutators ("publish before next dispatch") is loud and explicit.
The two gemini comments are misreads worth dismissing: authority_changed does not sort (it's iter().all(contains) bidirectional, as the doc explains), and evaluate does use the injected Clock trait, not Utc::now() directly.
Concerns
CRITICAL — AdminConfig matches by package_id alone across every PackageSource
// AdminConfig matches any source — operators may elevate a package
// installed from any origin.
let Some(entry) = entries.get(&input.identity.package_id) else {
return Ok(None);
};AdminEntry carries package_id + effective_trust + allowed_effects + max_resource_ceiling — no digest, no signer, no PackageSource constraint. So if an operator blesses package_id = "operator_blessed" at FirstParty, then any package with that name from any source — including a LocalManifest controlled by an unprivileged user — gets FirstParty effective trust. T13 demonstrates exactly this path: local_manifest_identity("operator_blessed") resolves to TrustClass::FirstParty via AdminConfig.
PackageId::new only validates the character set; it's not authenticated. So a user-installed LocalManifest shadowing an admin-blessed identifier escalates to FirstParty.
BundledRegistry doesn't have this problem because (a) it gates on PackageSource::Bundled and (b) it has digest pinning. AdminConfig has neither.
Suggested fix (any one):
AdminEntryrequiresdigestand/orsigner, andAdminConfig::evaluateenforces them; orAdminEntrycarries an explicitPackageSourcediscriminant the entry must match; orAdminConfig::evaluaterejectsPackageSource::LocalManifestpackages by default, with operator opt-in for "I really do mean to elevate this local-manifest package."
The current doc comment ("operators may elevate a package installed from any origin") names the design but understates the consequence. As long as operators are forced to use globally unique identifiers, this is "fine in production" — but the structure leaves the foot gun in the same crate that just type-locked the rest of the surface.
HIGH — InvalidationBus orchestration is enforced only by caller discipline
The contract is correct and the docs are loud, but there's no type-level coupling between mutating a BundledRegistry/AdminConfig/SignedRegistry and publishing a TrustChange. A future caller can call registry.remove(pid) and forget the bus.publish(...) step. AC #6 silently breaks. The compiler won't catch it.
PR3's ironclaw_authorization wiring is where this matters most. Worth either:
- adding a
RegistryMutator<'a, 'b> { registry: &'a Registry, bus: &'b InvalidationBus, prev: TrustDecision }lifetime-bound wrapper that publishes atomically on drop or commit; or - collapsing
upsert/removeinto a single API that returns the previous entry so the caller cannot forget the previous-state half of the contract.
For PR1b this is acceptable because no caller exists yet, but the PR3 review should specifically check this orchestration. Today's docs describe the rule; nothing enforces it.
HIGH — default_decision for unmatched Bundled / Registry is UserTrusted, not Sandbox
PackageSource::Bundled | PackageSource::Registry { .. } | PackageSource::Admin => {
EffectiveTrustClass::user_trusted()
}Two specific concerns:
- Unmatched
Bundledshouldn't exist. "Bundled" means compiled into the host binary. If a Bundled package is missing fromBundledRegistry, something is misconfigured. Falling soft toUserTrustedcovers the bug; falling closed toSandbox(or returning a hard error, as the doc anticipates for PR3) makes it audible. - Unmatched
Registryis more dangerous.Registry { url }is a remote source.SignedRegistry::evaluatereturnsOk(None)until verification lands. So an unsigned remote package today getsUserTrustedauthority via no verification at all. That's fail-open in a security-critical surface. SuggestSandboxfor unmatchedRegistryuntil signature verification ships.
MEDIUM — TrustChange with previous == current (no-op or upgrade) is publishable
InvalidationBus::publish runs every listener regardless of whether the change is a downgrade, no-op, or upgrade. The doc emphasizes the downgrade case, but listeners that read "TrustChange ⇒ invalidate active grants" will incorrectly invalidate on benign upgrades or no-ops.
Suggest a TrustChange::is_downgrade(&self) -> bool and either a guard in publish or stronger listener-side documentation. Or define the type to require previous != current at construction.
MEDIUM — authority_changed over-fires on duplicate entries
Capability authority is conceptually a set, but the function compares &[CapabilityId] slices. The doc explains why the length guard is needed ([a, a, b] vs [a, b]), but treating those two as different forces grant reissue when the content is identical. Two callers with the same effective authority but different list-canonicalization will trigger unnecessary reissues.
Suggest typing previous_authority as HashSet<CapabilityId> (or BTreeSet for determinism), or canonicalizing before comparison.
LOW — LocalDevOverride has unreachable interior
enabled: bool is private with no setter; overrides: RwLock<HashMap<...>> is #[allow(dead_code)]. PR1b ships a structural seam that can't be exercised even by tests. Consider:
- gating the whole struct behind a future
dev-overridefeature, or - exposing a
pub(crate)test-fixtures setter so the seam is at least exercisable in the test suite.
The current shape works as a forward-compat placeholder, but it's dead code today.
LOW — Missing privileged round-trip serialization test
EffectiveTrustClass does implement Serialize (for audit envelopes). There's a smoke test serializing user_trusted(), but nothing pins the wire shape for first_party() / system() (which can be constructed via the test-fixtures feature). One assertion that serde_json::to_value(effective_first_party_for_test()) produces json!("first_party") would lock the audit shape against accidental rename.
Verdict
Approve with one CRITICAL follow-up (the AdminConfig cross-source name-collision attack surface) and the orchestration concerns flagged for PR3 wiring. The substrate is correct, the type discipline is the strongest in the Reborn stack so far, and the test coverage maps cleanly to the AC matrix. The AdminConfig issue is the only one that genuinely changes the security posture; the rest are quality-of-implementation items that can land as follow-ups.
Happy to file individual issues for the critical and HIGH items if useful.
|
I think the requested/effective trust split is directionally right. The PR gives us a useful type-level boundary between untrusted manifest input and host-approved effective trust. I still think a few contract details should be clarified before downstream Reborn crates build against this. What existing Reborn docs already establishThe broader model is already documented pretty well:
So the concern is not with the high-level direction. The concern is that this PR introduces the concrete trust-policy substrate, and a few source-of-truth relationships are still implicit. 1. Please make the exact evaluation contract explicitThe main missing piece for me is a small policy matrix for: The code changes that make this important are:
Could the PR docs or crate docs define, per policy source:
Without this table, downstream crates could reasonably disagree about whether requested trust is a cap, a claim, or just audit metadata. 2. Please define
|
Address the review items spanning security correctness, API discipline, and contract documentation from the cross-crate review of PR3043 (Henry's review). Security fixes: - AdminConfig: bind elevation to (package_id, source, digest) so a LocalManifest cannot shadow an admin-blessed Bundled id. The highest-risk path lives behind AdminEntry::for_local_manifest so every elevation of a user-writable origin is greppable. Closes the cross-source shadowing footgun documented in T13b/T13c/T13d. - default_decision: fail-closed across every PackageSource. An unmatched Registry url no longer picks up UserTrusted by self- declaration; unmatched Bundled / Admin also drop to Sandbox. T15 pins the contract. API tightening: - HostTrustPolicy::mutate_with is the only public runtime-mutation path; per-source upsert/remove are pub(crate) and reachable only through SourceMutators inside a mutate_with closure. AC #6 (trust changes invalidate before next dispatch) is now a compile-time guarantee rather than caller discipline. T14 family. - TrustChange::new(...) -> Option<Self> filters no-ops at construction; is_downgrade / is_upgrade / is_kind_change helpers backed by EffectiveTrustClass::authority_level let listeners be selective rather than over-revoking on benign upgrades. InvalidationBus::publish drops no-ops as defense-in-depth (debug_assert in dev). T16 family. - TrustChange.previous_authority / TrustPolicyInput.requested_authority / authority_changed / grant_retention_eligible typed as BTreeSet<CapabilityId>, not Vec / slice. Closes the [a, a, b] vs [a, b] over-fire at the type boundary. Required PartialOrd/Ord on string-id newtypes in host_api (additive change). - LocalDevOverride exposes is_enabled / override_count accessors and a test-fixtures-only enabled_for_test constructor; inert contract is now testable via T17. - EffectiveTrustClass canonical wire shapes pinned for all four variants (T18) so audit envelopes survive serde renames. Documentation: - New crates/ironclaw_trust/CONTRACT.md (421 lines) — cross-crate contract co-located with the code. Covers evaluation matrix (per-source match keys; requested_trust audit-only; mismatch = fall through; first-match-wins), PackageIdentity scope, requested vs effective trust split, mutation orchestration, set-typed authority, and built-in tool migration intent with V1 vs post-migration axis mapping. - crates/ironclaw_host_api/src/trust.rs: type-level docs on PackageIdentity / RequestedTrustClass / module level answer PackageIdentity scope and CapabilityDescriptor.trust_ceiling reconciliation at the source. Manifest \`trust = "..."\` mapping table inline. - CLAUDE.md and lib.rs point at CONTRACT.md. Verification: cargo fmt clean cargo clippy --all-features --tests -D warnings clean 30 trust contract tests + 2 lib smoke tests + host_api tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| pub fn with_entries<I: IntoIterator<Item = AdminEntry>>(entries: I) -> Self { |
There was a problem hiding this comment.
High Severity — AdminConfig is keyed only by PackageId, despite the contract binding trust to (package_id, source).
PackageIdentity explicitly documents that same-package_id packages from different PackageSources are distinct trust subjects, and the evaluator checks entry.source != input.identity.source. But the backing map stores only one AdminEntry per PackageId:
HashMap<PackageId, AdminEntry>That means an admin config containing both for_bundled("foo", ...) and for_registry("foo", ...) silently drops whichever entry was inserted first, and admin_remove(&PackageId) removes the whole id regardless of source. This breaks the source-pin invariant and can also cause untracked trust changes for the overwritten/removed source.
Please key admin entries by the full trust subject (at least (PackageId, PackageSource)) and make admin_upsert / admin_remove source-aware. Add a regression test with two same-id entries for different sources and verify both evaluate independently.
| let mutators = SourceMutators { | ||
| sources: &self.sources, | ||
| }; | ||
| let result = f(&mutators)?; |
There was a problem hiding this comment.
High Severity — mutate_with can change policy state and then skip invalidation entirely when the closure returns an error.
The closure receives live in-place mutators. If it performs a downgrade/removal and then returns Err, this ? exits before the post-evaluation and before bus.publish runs. The policy state remains changed, but active grants/leases are never told about it. The new test t14e_mutate_with_short_circuits_on_closure_error codifies exactly this unsafe behavior.
A concrete scenario: admin_remove() succeeds, a later operation in the same closure fails, mutate_with returns Err, and the package has now fallen from FirstParty to default Sandbox without a TrustChange. Stale privileged grants can survive under the old ceiling.
Please make mutation transactional/queued, roll back on closure errors, or at minimum detect that a mutation occurred and still post-evaluate/publish any authority reduction before returning the closure error.
| }; | ||
| let result = f(&mutators)?; | ||
|
|
||
| let curr = self.evaluate(&probe)?; |
There was a problem hiding this comment.
High Severity — there is no synchronization that enforces “publish before any subsequent evaluate returns the new decision.”
mutate_with pre-evaluates, runs in-place source mutations, post-evaluates, then publishes. The source mutators only take per-source write locks and release them before mutate_with post-evaluates and publishes. A concurrent thread can call evaluate() after the source mutation is visible but before bus.publish(change) runs, observe the new lower trust decision, and proceed before grants have been invalidated.
That violates the core fail-closed contract in this PR: trust downgrade/revocation must invalidate active grants before any further side effect can run under stale authority.
Please add a policy-level synchronization/epoch mechanism that spans pre-evaluate → mutation → post-evaluate → publish, or otherwise make evaluate/dispatch wait until the corresponding invalidation has completed.
| // is the canonical filter, so a closure that mutates without | ||
| // changing this identity's effective trust class produces no | ||
| // publish. | ||
| if let Some(change) = TrustChange::new( |
There was a problem hiding this comment.
High Severity — invalidation only considers effective_trust, not reductions in the authority ceiling.
TrustChange::new is called with only prev.effective_trust and curr.effective_trust. If an entry keeps the same trust class but removes allowed_effects or lowers max_resource_ceiling, no event is published. Existing grants issued under the old ceiling can therefore survive after the policy has reduced what may be granted.
This is not just metadata: AuthorityCeiling is the cap downstream authorization is expected to intersect with each CapabilityGrant. A same-class reduction from [ReadFilesystem, WriteFilesystem] to [ReadFilesystem], or a lower resource ceiling, needs the same fail-closed invalidation behavior as a trust downgrade.
Please compare the full authority ceiling (or emit a separate authority-ceiling change event) and publish invalidation whenever allowed effects/resource ceilings shrink, even if the trust class is unchanged. Add a test that mutates an entry from the same EffectiveTrustClass with fewer effects and verifies the bus fires.
| # Off by default — production builds must NEVER enable this. Tests opt in | ||
| # via `cargo test --features test-fixtures` (or `--features | ||
| # ironclaw_trust/test-fixtures` from the workspace root). | ||
| test-fixtures = [] |
There was a problem hiding this comment.
High Severity — test-fixtures exposes privileged effective-trust constructors as a normal Cargo feature.
The PR states production builds cannot import privileged constructors, but test-fixtures is a regular additive feature that makes pub mod fixtures available. Any workspace crate or downstream build that enables ironclaw_trust/test-fixtures can call effective_system_for_test() outside tests and produce privileged EffectiveTrustClass values without policy evaluation.
Because the central invariant is “privileged effective trust only comes from the policy engine,” this should not be a public feature surface. Prefer keeping privileged tests inside crate-internal #[cfg(test)] modules, or add a hard compile-time guard that prevents building non-test targets with test-fixtures enabled.
|
I think the mechanics here are right, but I’m worried the naming may blur two different Reborn concepts.
Would it be clearer to name this The important distinction I want to preserve is:
I don’t think this is a functional blocker, but the naming may make future wiring into |
|
I like the intent of §9 — built-ins should eventually move from implicit A few concerns:
Could we either mark this table as illustrative/non-exhaustive, or reshape it as a provider-package mapping rather than a tool mapping? For example: That would better preserve the axes we’ve been separating: |
Summary
RequestedTrustClass(manifest input, freely deserializable) fromEffectiveTrustClass(policy output) at the type level — privileged variants are crate-private constructors.ironclaw_trustcrate:TrustPolicytrait +HostTrustPolicyengine with layeredPolicySources (BundledRegistry+AdminConfigfunctional,SignedRegistry+LocalDevOverrideinterface seams), plusInvalidationBusfor synchronous trust-change fan-out.static_assertions::assert_not_impl_any!(EffectiveTrustClass: DeserializeOwned).ironclaw_trustfrom depending on dispatcher / capability host / runtimes / approvals / etc.Change Type
Linked Issue
Closes #3012. Originally meant to land before PR3 #2999 (auth control), but #2999 already merged into
reborn-integration; PR1b still applies becauseironclaw_authorizationdoes not yet consumeEffectiveTrustClass— a small follow-up PR will wire them together.Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test -p ironclaw_host_api -p ironclaw_trust -p ironclaw_architecture --features ironclaw_trust/test-fixtures→ 46/46 (1 boundary + 29 host_api + 2 lib smoke + 14 contract). The--features ironclaw_trust/test-fixturesflag is required for the contract test target — see "Test fixtures gated behind feature flag" below.--libtests: all pass except 4 pre-existing failures inironclaw/ironclaw_engine(verified to fail on basereborn-integrationHEAD too — unrelated to PR1b)cargo test --features integration— N/A (no DB or integration code)Acceptance-criteria coverage matrix
assert_not_impl_any!(EffectiveTrustClass: DeserializeOwned)at top ofpolicy_contract.rs. Runtime: T9, T10, H1.default_decision(LocalManifest → Sandbox, others → UserTrusted)InvalidationBussynchronous fan-out + mutator-publish docs onBundledRegistry/AdminConfig/SignedRegistryidentity_changed/grant_retention_eligiblehelpersFakeAuthorizer)Suggested-tests coverage
FakeAuthorizer)FakeGrantStorelistener)Security Impact
Security-critical change. Enforces the type-level guarantee that user-installed manifests cannot self-promote to privileged effective trust:
EffectiveTrustClass::FirstParty/Systemconstructors arepub(crate)— onlyHostTrustPolicy::evaluatecan produce them.EffectiveTrustClasshas noDeserializeimpl; statically asserted at compile time viastatic_assertions.host_api::TrustClassprivileged variants reject serde input (#[serde(skip_deserializing)]).RequestedTrustClass(untrusted input) andEffectiveTrustClass(policy output) are distinct types with noFrom/TryFrombetween them.TrustChangesynchronously onInvalidationBusbefore any subsequentevaluate()returns the lower decision — fail-closed.BundledRegistry/AdminConfig/SignedRegistrycarry explicit doc-level mutation→publish contracts; module docs explain the orchestration pattern.test-fixturesCargo feature — production builds cannot import the privileged-trust constructors.ironclaw_host_api); enforced byreborn_crate_dependency_boundaries_holdtest.Database Impact
None.
Blast Radius
ironclaw_trust(foundation level, depends only onironclaw_host_api).ironclaw_host_api:trustmodule:RequestedTrustClass,PackageIdentity,PackageSourcePackageIdnewtype inids.rs(one line, uses existingstring_id!macro)TrustClassto clarify it is the effective ceilingironclaw_architecture/tests/reborn_dependency_boundaries.rs.Rollback Plan
Revert this PR cleanly. The
ironclaw_trustcrate has no consumers yet — PR3'sironclaw_authorizationcrate (#2999) does not yet integrate the policy engine. Rollback isgit revert <merge>plus removingcrates/ironclaw_trustfrom the workspacemembersarray; no migration or data impact.Review Follow-Through
EffectiveTrustClasswrapshost_api::TrustClass(variantsSandbox/UserTrusted/FirstParty/System) rather than introducing a new enum with the issue's suggestedUntrusted/ThirdParty/FirstParty/System. The issue explicitly permits this ("Implementation does not need to use these exact names").Sandbox≡Untrusted,UserTrusted≡ThirdPartysemantically.crates/ironclaw_trust/src/fixtures.rsis#[cfg(any(test, feature = "test-fixtures"))] #[doc(hidden)]. The contract test target setsrequired-features = ["test-fixtures"]. Production builds cannot import the privileged-trust constructors. To run the full contract suite locally:cargo test -p ironclaw_trust --features test-fixtures(or--features ironclaw_trust/test-fixturesfrom workspace root).trusted_signersmap +SignerEntry), LocalDevOverride (interface seam, inert in PR1b).BundledRegistry::upsert/remove,AdminConfig::upsert/remove,SignedRegistry::upsert/remove) carry explicit doc-level contracts requiring the caller to publish aTrustChangeon the relevantInvalidationBus. PR3's grant-store wiring will own this orchestration; T7 / T8 already exercise the contract.EffectiveTrustClass+InvalidationBusintoironclaw_authorization(PR3's auth-control crate). PR1b deliberately stops at the contract boundary so the wiring can be reviewed independently.Review track: C (security/runtime/DB/CI) — adds a security-critical type-system boundary to the foundation layer.