diff --git a/CHANGELOG.md b/CHANGELOG.md index a39957220..1e9b4fed2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **Skills**: `[skills.trust] require_integrity_check_on_promote` (default `true`) automatically + arms the per-invocation BLAKE3 integrity re-check (`requires_trust_check`) whenever a skill is + promoted to `trusted`/`verified`, at both the CLI (`zeph skill trust`) and in-session + (`/skill trust`) promotion handlers. Previously the re-check could only be armed manually via + `--require-check`, so an operator who forgot the flag left a promoted skill's tampered-on-disk + `SKILL.md` undetected between promotions (#6087). `--require-check`/`--no-require-check` + (mutually exclusive) continue to override the config default per command; promotion to + `quarantined`/`blocked` leaves `requires_trust_check` untouched. A migration step (80) adds a + commented advisory for the new key to existing configs that already declare `[skills.trust]`. + Self-learning/heuristic auto-promotion and reload trust-assignment are intentionally out of + scope for this change — see the PR description. + ### Testing - `zeph-llm`: reduced the discoverability of bypassing the Claude no-prefill funnel introduced diff --git a/book/src/advanced/skill-trust.md b/book/src/advanced/skill-trust.md index 2ce072e00..765a3eb87 100644 --- a/book/src/advanced/skill-trust.md +++ b/book/src/advanced/skill-trust.md @@ -66,21 +66,41 @@ BLAKE3 hash is recomputed and compared against the stored hash on *every* dispat `invoke_skill`, and `zeph skill invoke`), not just at promotion time. A mismatch aborts the invocation and demotes the skill to `quarantined` immediately, before its body is ever returned. -Arm it with `--require-check` on the trust-setting commands: +### Automatic activation on promotion (default) + +Promoting a skill to `trusted` or `verified` — via `zeph skill trust trusted|verified` or +`/skill trust trusted|verified` — arms `requires_trust_check` **automatically** unless +disabled, per `[skills.trust] require_integrity_check_on_promote` (default `true`). This closes +the "operator forgot to pass `--require-check`" gap: Trusted/Verified is the trust tier whose body +is dispatched verbatim (no sanitization), so it is the choke point where a tampered `SKILL.md` +matters most. + +Promoting to `quarantined` or `blocked` never touches `requires_trust_check` — a previously armed +flag survives a temporary demotion and re-promotion. + +Override per command: ```bash -# CLI +# CLI — force the check on even if the config default is false zeph skill trust my-skill trusted --require-check + +# CLI — skip arming even though the config default is true +zeph skill trust my-skill trusted --no-require-check ``` ```text -# In-session +# In-session — same two overrides /skill trust my-skill trusted --require-check +/skill trust my-skill trusted --no-require-check ``` +`--require-check` and `--no-require-check` are mutually exclusive and always win over the config +default. With neither flag, the command falls back to +`[skills.trust] require_integrity_check_on_promote`. + `/skill trust ` (no level argument) shows the flag's current state as -`requires_trust_check=true|false`. There is no command to clear it yet — the only way is to -manually update the `requires_trust_check` column in the `skill_trust` SQLite table directly. +`requires_trust_check=true|false`. There is no command to clear it once armed — the only way is +to manually update the `requires_trust_check` column in the `skill_trust` SQLite table directly. This is a known usability gap, not a security one (the flag only ever makes enforcement stricter). @@ -100,7 +120,7 @@ External skills installed via `zeph skill install` are stored in `~/.config/zeph |---------|-------------| | `/skill trust` | List all skills with their trust level, source, and hash | | `/skill trust ` | Show trust details for a specific skill | -| `/skill trust ` | Set trust level (`trusted`, `verified`, `quarantined`, `blocked`); append `--require-check` to also arm the [per-invocation integrity re-check](#per-invocation-integrity-re-check) | +| `/skill trust ` | Set trust level (`trusted`, `verified`, `quarantined`, `blocked`). Promotion to `trusted`/`verified` arms the [per-invocation integrity re-check](#per-invocation-integrity-re-check) by default; append `--require-check`/`--no-require-check` to force it on/off | | `/skill block ` | Block a skill (all tool access denied) | | `/skill unblock ` | Unblock a skill (reverts to `quarantined`) | | `/skill install ` | Install an external skill (git URL or local path) with hot reload | @@ -128,6 +148,9 @@ default_level = "quarantined" local_level = "trusted" # Trust level assigned after BLAKE3 hash mismatch on hot-reload hash_mismatch_level = "quarantined" +# Arm the per-invocation integrity re-check by default on promotion to trusted/verified +# (see "Automatic activation on promotion" above) +require_integrity_check_on_promote = true ``` Environment variable overrides: @@ -136,4 +159,5 @@ Environment variable overrides: export ZEPH_SKILLS_TRUST_DEFAULT_LEVEL=quarantined export ZEPH_SKILLS_TRUST_LOCAL_LEVEL=trusted export ZEPH_SKILLS_TRUST_HASH_MISMATCH_LEVEL=quarantined +export ZEPH_SKILLS_TRUST_REQUIRE_INTEGRITY_CHECK_ON_PROMOTE=true ``` diff --git a/book/src/reference/cli.md b/book/src/reference/cli.md index 3fe54774d..c15dc7959 100644 --- a/book/src/reference/cli.md +++ b/book/src/reference/cli.md @@ -91,7 +91,7 @@ Manage external skills. Installed skills are stored in `~/.config/zeph/skills/`. | `skill remove ` | Remove an installed skill by name | | `skill list` | List installed skills with trust level and source metadata | | `skill verify [name]` | Verify BLAKE3 integrity of one or all installed skills | -| `skill trust [level]` | Show or set trust level (`trusted`, `verified`, `quarantined`, `blocked`); add `--require-check` when setting a level to also arm the per-invocation BLAKE3 integrity re-check (see [Skill Trust Levels](../advanced/skill-trust.md#per-invocation-integrity-re-check)) | +| `skill trust [level]` | Show or set trust level (`trusted`, `verified`, `quarantined`, `blocked`). Promoting to `trusted`/`verified` arms the per-invocation BLAKE3 integrity re-check by default (`[skills.trust] require_integrity_check_on_promote`); add `--require-check`/`--no-require-check` to force it on/off (see [Skill Trust Levels](../advanced/skill-trust.md#per-invocation-integrity-re-check)) | | `skill block ` | Block a skill (deny all tool access) | | `skill unblock ` | Unblock a skill (revert to `quarantined`) | | `skill promote-heuristics [--skill ]` | Dry-run: show skills eligible for A6 heuristic → full promotion (requires `[skills.learning.heuristic_promotion_enabled = true]`) | @@ -108,12 +108,12 @@ zeph skill install /path/to/my-skill # List installed skills zeph skill list -# Verify integrity and promote trust +# Verify integrity and promote trust (arms the per-invocation integrity re-check by default) zeph skill verify my-skill zeph skill trust my-skill trusted -# Promote and also arm the per-invocation integrity re-check -zeph skill trust my-skill trusted --require-check +# Promote without arming the per-invocation integrity re-check +zeph skill trust my-skill trusted --no-require-check # Remove a skill zeph skill remove my-skill diff --git a/config/default.toml b/config/default.toml index edced4675..f30e409e5 100644 --- a/config/default.toml +++ b/config/default.toml @@ -295,6 +295,10 @@ local_level = "trusted" hash_mismatch_level = "quarantined" # Scan skill body content for injection patterns at load time (advisory, secure by default) scan_on_load = true +# Arm the per-invocation blake3 integrity re-check when a skill is promoted to trusted/verified +# (secure by default). `--require-check`/`--no-require-check` on `skill trust`/`/skill trust` +# always override this default (#6087). +require_integrity_check_on_promote = true [skills.trust.scanner] # Scan for injection patterns in skill body (advisory, logs warnings) diff --git a/crates/zeph-config/src/migrate/features.rs b/crates/zeph-config/src/migrate/features.rs index 2473bbe6b..563179340 100644 --- a/crates/zeph-config/src/migrate/features.rs +++ b/crates/zeph-config/src/migrate/features.rs @@ -9,7 +9,7 @@ use regex::Regex; -use super::{MigrateError, MigrationResult, insert_after_section}; +use super::{MigrateError, MigrationResult, insert_after_section, section_header_present}; /// Regex matching the `[tui]` section header line (used by step 67). static TUI_HEADER_RE: std::sync::LazyLock = std::sync::LazyLock::new(|| { @@ -846,3 +846,64 @@ pub fn migrate_skills_registry(toml_src: &str) -> Result = std::sync::LazyLock::new(|| { + Regex::new(r"(?m)^[ \t]*\[skills\.trust\][ \t]*(?:#[^\r\n]*)?\r?\n").expect("static pattern") +}); + +/// Step 80 — add a commented `require_integrity_check_on_promote = true` advisory to an +/// existing active `[skills.trust]` table (#6087). +/// +/// The field has `#[serde(default = "default_true")]`, so existing configs already behave as +/// if it were `true` without this migration — this step only surfaces the new key for +/// discoverability on configs that already declare `[skills.trust]` explicitly. No-op when +/// `[skills.trust]` is absent (nothing to annotate) or the key (active or commented) is +/// already present. +/// +/// # Errors +/// +/// Returns [`MigrateError`] if the source is not valid TOML. +pub fn migrate_skill_trust_require_check(toml_src: &str) -> Result { + if !section_header_present(toml_src, "skills.trust") { + return Ok(MigrationResult { + output: toml_src.to_owned(), + changed_count: 0, + sections_changed: Vec::new(), + }); + } + + let already_present = toml_src.lines().any(|l| { + l.trim() + .trim_start_matches('#') + .trim() + .starts_with("require_integrity_check_on_promote") + }); + if already_present || !SKILLS_TRUST_HEADER_RE.is_match(toml_src) { + return Ok(MigrationResult { + output: toml_src.to_owned(), + changed_count: 0, + sections_changed: Vec::new(), + }); + } + + let comment = "# require_integrity_check_on_promote = true # arm per-invocation blake3 \ + re-check on promotion to trusted/verified; override with --no-require-check (#6087)\n"; + let output = SKILLS_TRUST_HEADER_RE + .replacen(toml_src, 1, |caps: ®ex::Captures| { + format!("{}{comment}", &caps[0]) + }) + .into_owned(); + + let changed = output != toml_src; + let changed_count = usize::from(changed); + Ok(MigrationResult { + output, + changed_count, + sections_changed: if changed { + vec!["skills.trust.require_integrity_check_on_promote".to_owned()] + } else { + Vec::new() + }, + }) +} diff --git a/crates/zeph-config/src/migrate/mod.rs b/crates/zeph-config/src/migrate/mod.rs index 29ff0e1e1..4215a4f02 100644 --- a/crates/zeph-config/src/migrate/mod.rs +++ b/crates/zeph-config/src/migrate/mod.rs @@ -25,8 +25,8 @@ pub use features::{ migrate_deep_link_config, migrate_five_signal_config, migrate_goals_config, migrate_knowledge_config, migrate_magic_docs_config, migrate_microcompact_config, migrate_orchestration_asset_sensitivity, migrate_orchestration_persistence, - migrate_skills_registry, migrate_tui_delights, migrate_tui_mouse, migrate_tui_theme_config, - migrate_tui_theme_defaults, + migrate_skill_trust_require_check, migrate_skills_registry, migrate_tui_delights, + migrate_tui_mouse, migrate_tui_theme_config, migrate_tui_theme_defaults, }; pub use infra::*; /// Advisory `GonkaGate` migration is crate-internal (registered via the [`MIGRATIONS`] registry). @@ -618,8 +618,8 @@ use steps::{ MigrateQualityConfig, MigrateSandboxConfig, MigrateSandboxEgressFilter, MigrateSchedulerDaemon, MigrateSecretMaskingConfig, MigrateServeConfig, MigrateSessionPersistProviderOverrides, MigrateSessionPersistenceConfig, MigrateSessionProviderPersistence, MigrateSessionRecapConfig, - MigrateShellCheckpointsConfig, MigrateShellTransactional, MigrateSkillsRegistry, - MigrateSttToProvider, MigrateSupervisorConfig, MigrateTelemetryConfig, + MigrateShellCheckpointsConfig, MigrateShellTransactional, MigrateSkillTrustRequireCheck, + MigrateSkillsRegistry, MigrateSttToProvider, MigrateSupervisorConfig, MigrateTelemetryConfig, MigrateToolsCompressionConfig, MigrateTraceMetadata, MigrateTuiDelights, MigrateTuiMouse, MigrateTuiThemeConfig, MigrateTuiThemeDefaults, MigrateUtilityHighGainTools, MigrateVigilConfig, MigrateWorktreeConfig, MigrateWorktreeGitTimeout, @@ -767,6 +767,9 @@ pub static MIGRATIONS: std::sync::LazyLock> Box::new(MigrateSkillsRegistry), // Step 79 — add shared_db = false advisory to an existing active [durable] table (#5996) Box::new(MigrateDurableSharedDb), + // Step 80 — add require_integrity_check_on_promote advisory to an existing active + // [skills.trust] table (#6087) + Box::new(MigrateSkillTrustRequireCheck), ] }); diff --git a/crates/zeph-config/src/migrate/steps.rs b/crates/zeph-config/src/migrate/steps.rs index bc16b40e6..f2f0c54dd 100644 --- a/crates/zeph-config/src/migrate/steps.rs +++ b/crates/zeph-config/src/migrate/steps.rs @@ -40,7 +40,9 @@ //! step 77 adds a commented `[[acp.auth_clients]]` advisory block (#5868); //! step 78 adds a commented `[skills.registry]` advisory block (spec-045, #5869); //! step 79 adds a commented `shared_db = false` advisory to an existing active `[durable]` -//! table (INV-8 `encryption_gate`, #5996). +//! table (INV-8 `encryption_gate`, #5996); +//! step 80 adds a commented `require_integrity_check_on_promote = true` advisory to an +//! existing active `[skills.trust]` table (#6087). //! //! Each struct is a zero-size type that delegates to the corresponding free function in //! `super`. They exist solely to satisfy the object-safe [`super::Migration`] trait so the @@ -72,12 +74,12 @@ use super::{ migrate_scheduler_daemon_config, migrate_secret_masking_config, migrate_serve_config, migrate_session_persist_provider_overrides, migrate_session_persistence_config, migrate_session_provider_persistence, migrate_session_recap_config, - migrate_shell_checkpoints_config, migrate_shell_transactional, migrate_skills_registry, - migrate_stt_to_provider, migrate_supervisor_config, migrate_telemetry_config, - migrate_tools_compression_config, migrate_trace_metadata, migrate_tui_delights, - migrate_tui_mouse, migrate_tui_theme_config, migrate_tui_theme_defaults, - migrate_utility_high_gain_tools, migrate_vigil_config, migrate_worktree_config, - migrate_worktree_git_timeout, + migrate_shell_checkpoints_config, migrate_shell_transactional, + migrate_skill_trust_require_check, migrate_skills_registry, migrate_stt_to_provider, + migrate_supervisor_config, migrate_telemetry_config, migrate_tools_compression_config, + migrate_trace_metadata, migrate_tui_delights, migrate_tui_mouse, migrate_tui_theme_config, + migrate_tui_theme_defaults, migrate_utility_high_gain_tools, migrate_vigil_config, + migrate_worktree_config, migrate_worktree_git_timeout, }; // ── Wrapper structs for all 73 sequential migration steps ─────────────────────────────────────── @@ -968,3 +970,16 @@ impl Migration for MigrateDurableSharedDb { migrate_durable_shared_db(toml_src) } } + +/// Step 80 — adds a commented `require_integrity_check_on_promote = true` advisory to an +/// existing active `[skills.trust]` table (#6087). +pub(super) struct MigrateSkillTrustRequireCheck; +impl Migration for MigrateSkillTrustRequireCheck { + fn name(&self) -> &'static str { + "migrate_skill_trust_require_check" + } + + fn apply(&self, toml_src: &str) -> Result { + migrate_skill_trust_require_check(toml_src) + } +} diff --git a/crates/zeph-config/src/migrate/tests.rs b/crates/zeph-config/src/migrate/tests.rs index 7ef310c53..714cfc653 100644 --- a/crates/zeph-config/src/migrate/tests.rs +++ b/crates/zeph-config/src/migrate/tests.rs @@ -9,8 +9,8 @@ use super::*; fn migrations_registry_has_all_steps() { assert_eq!( MIGRATIONS.len(), - 79, - "MIGRATIONS registry must contain all 79 sequential steps" + 80, + "MIGRATIONS registry must contain all 80 sequential steps" ); for m in MIGRATIONS.iter() { assert!( @@ -1753,7 +1753,7 @@ fn migrate_focus_auto_consolidate_noop_when_only_commented_section() { #[test] fn registry_has_fifty_entries() { - assert_eq!(MIGRATIONS.len(), 79); + assert_eq!(MIGRATIONS.len(), 80); } #[test] @@ -1872,6 +1872,7 @@ fn registry_preserves_order_matches_dispatch() { "migrate_acp_auth_clients_config", "migrate_skills_registry", "migrate_durable_shared_db", + "migrate_skill_trust_require_check", ]; let actual: Vec<&str> = MIGRATIONS.iter().map(|m| m.name()).collect(); assert_eq!(actual, expected); @@ -4000,3 +4001,83 @@ fn step_79_idempotent_on_own_output() { "output unchanged on second run" ); } + +// ── migrate_skill_trust_require_check tests (step 80, #6087) ───────────────── + +#[test] +fn step_80_adds_commented_advisory_when_skills_trust_active_and_missing_field() { + let src = "[skills.trust]\ndefault_level = \"quarantined\"\nlocal_level = \"trusted\"\n"; + let result = migrate_skill_trust_require_check(src).expect("migrate"); + assert_eq!(result.changed_count, 1); + assert!( + result + .output + .contains("# require_integrity_check_on_promote = true") + ); + assert!( + !result + .output + .contains("\nrequire_integrity_check_on_promote =") + ); + assert!(result.output.contains("default_level = \"quarantined\"")); + assert!(result.output.contains("local_level = \"trusted\"")); + assert_eq!( + result.sections_changed, + vec!["skills.trust.require_integrity_check_on_promote".to_owned()] + ); +} + +#[test] +fn step_80_noop_when_require_integrity_check_on_promote_already_present() { + let src = "[skills.trust]\nrequire_integrity_check_on_promote = false\n"; + let result = migrate_skill_trust_require_check(src).expect("migrate"); + assert_eq!(result.changed_count, 0); + assert_eq!(result.output, src); +} + +#[test] +fn step_80_noop_when_require_integrity_check_on_promote_comment_already_present() { + let src = "[skills.trust]\n# require_integrity_check_on_promote = true\n"; + let result = migrate_skill_trust_require_check(src).expect("migrate"); + assert_eq!(result.changed_count, 0); + assert_eq!(result.output, src); +} + +#[test] +fn step_80_noop_when_skills_trust_section_absent() { + let src = "[skills]\ndefault_level = \"quarantined\"\n"; + let result = migrate_skill_trust_require_check(src).expect("migrate"); + assert_eq!(result.changed_count, 0); + assert_eq!(result.output, src); +} + +#[test] +fn step_80_noop_when_skills_trust_only_commented_section() { + let src = "# [skills.trust]\n# scan_on_load = true\n"; + let result = migrate_skill_trust_require_check(src).expect("migrate"); + assert_eq!(result.changed_count, 0); + assert_eq!(result.output, src); +} + +#[test] +fn step_80_does_not_match_skills_trust_scanner_subtable() { + let src = "[skills.trust.scanner]\ncapability_escalation_check = false\n"; + let result = migrate_skill_trust_require_check(src).expect("migrate"); + assert_eq!( + result.changed_count, 0, + "[skills.trust.scanner] alone must not count as an active [skills.trust] table" + ); +} + +#[test] +fn step_80_idempotent_on_own_output() { + let src = "[skills.trust]\ndefault_level = \"quarantined\"\n"; + let first = migrate_skill_trust_require_check(src).expect("first migrate"); + assert_eq!(first.changed_count, 1); + let second = migrate_skill_trust_require_check(&first.output).expect("second migrate"); + assert_eq!(second.changed_count, 0, "second run must be a no-op"); + assert_eq!( + second.output, first.output, + "output unchanged on second run" + ); +} diff --git a/crates/zeph-config/src/security.rs b/crates/zeph-config/src/security.rs index a6ac06f2b..4512461d5 100644 --- a/crates/zeph-config/src/security.rs +++ b/crates/zeph-config/src/security.rs @@ -128,6 +128,16 @@ pub struct TrustConfig { /// Defaults to `true` (secure by default). #[serde(default = "default_true")] pub scan_on_load: bool, + /// Arm the per-invocation blake3 integrity re-check (`requires_trust_check`) whenever a + /// skill is promoted to `Trusted` or `Verified`. + /// + /// Trusted/Verified bodies are dispatched verbatim (no sanitization), so this is the + /// choke point that matters for tamper detection. `--require-check`/`--no-require-check` + /// on the promoting command always win over this default (#6087). + /// + /// Defaults to `true` (secure by default). + #[serde(default = "default_true")] + pub require_integrity_check_on_promote: bool, /// Fine-grained scanner controls (injection patterns, capability escalation). #[serde(default)] pub scanner: ScannerConfig, @@ -141,6 +151,7 @@ impl Default for TrustConfig { hash_mismatch_level: default_trust_hash_mismatch_level(), bundled_level: default_trust_bundled_level(), scan_on_load: true, + require_integrity_check_on_promote: true, scanner: ScannerConfig::default(), } } @@ -666,12 +677,14 @@ mod tests { hash_mismatch_level: SkillTrustLevel::Quarantined, bundled_level: SkillTrustLevel::Trusted, scan_on_load: false, + require_integrity_check_on_promote: false, scanner: ScannerConfig::default(), }; let toml = toml::to_string(&config).expect("serialize"); let deserialized: TrustConfig = toml::from_str(&toml).expect("deserialize"); assert!(!deserialized.scan_on_load); assert_eq!(deserialized.bundled_level, SkillTrustLevel::Trusted); + assert!(!deserialized.require_integrity_check_on_promote); } #[test] @@ -694,6 +707,26 @@ hash_mismatch_level = "quarantined" assert_eq!(config.bundled_level, SkillTrustLevel::Trusted); } + #[test] + fn trust_config_default_has_require_integrity_check_on_promote_true() { + let config = TrustConfig::default(); + assert!(config.require_integrity_check_on_promote); + } + + #[test] + fn trust_config_missing_require_integrity_check_on_promote_defaults_to_true() { + let toml = r#" +default_level = "quarantined" +local_level = "trusted" +hash_mismatch_level = "quarantined" +"#; + let config: TrustConfig = toml::from_str(toml).expect("deserialize"); + assert!( + config.require_integrity_check_on_promote, + "missing require_integrity_check_on_promote must default to true" + ); + } + #[test] fn trust_config_missing_bundled_level_defaults_to_trusted() { let toml = r#" diff --git a/crates/zeph-core/src/agent/trust_commands.rs b/crates/zeph-core/src/agent/trust_commands.rs index 9977f8f31..807f4f2e5 100644 --- a/crates/zeph-core/src/agent/trust_commands.rs +++ b/crates/zeph-core/src/agent/trust_commands.rs @@ -53,17 +53,31 @@ impl Agent { return Ok(format!("Skill \"{name}\" not found in trust database.")); } let mut output = format!("Trust level for \"{name}\" set to {level}."); - // #6080: `--require-check` arms the per-invocation blake3 integrity - // re-check (`SkillTrustGate::resolve_body`), previously unreachable from - // any production entry point. Scan the whole remaining slice rather than - // indexing a fixed position — a security toggle must not silently fail to - // arm just because the flag isn't the 3rd token (review finding, #6080). - if args[2..].contains(&"--require-check") { - memory.sqlite().set_requires_trust_check(name, true).await?; - let _ = write!( - output, - "\nPer-invocation integrity re-check enabled for \"{name}\"." - ); + // #6087: promotion to Trusted/Verified is the choke point where a + // tampered SKILL.md body would be dispatched verbatim — arm the + // per-invocation blake3 re-check by default there, with + // --require-check/--no-require-check overriding the config default. + // Quarantined/Blocked promotions leave requires_trust_check untouched. + // Scan the whole remaining slice rather than indexing a fixed position — + // a security toggle must not silently fail to arm just because the flag + // isn't the 3rd token (review finding, #6080). + if matches!(level, SkillTrustLevel::Trusted | SkillTrustLevel::Verified) { + let rest = &args[2..]; + let force_on = rest.contains(&"--require-check"); + let force_off = rest.contains(&"--no-require-check"); + let config_default = self + .services + .skill + .trust_config + .require_integrity_check_on_promote; + let arm = crate::resolve_require_check(force_on, force_off, config_default); + if arm { + memory.sqlite().set_requires_trust_check(name, true).await?; + let _ = write!( + output, + "\nPer-invocation integrity re-check enabled for \"{name}\"." + ); + } } Ok(output) } else { @@ -310,7 +324,9 @@ mod tests { } #[tokio::test] - async fn trust_without_flag_leaves_requires_trust_check_false() { + async fn trust_default_config_arms_check_on_promotion_without_flags() { + // #6087: require_integrity_check_on_promote defaults to true, so promoting to + // Trusted with no flags must arm the re-check. let memory = test_memory().await; memory .sqlite() @@ -325,12 +341,53 @@ mod tests { .await .unwrap(); let mut agent = agent_with_memory(memory.clone()); + assert!( + agent + .services + .skill + .trust_config + .require_integrity_check_on_promote, + "default TrustConfig must arm by default" + ); let out = agent .handle_skill_trust_command_as_string(&["git", "trusted"]) .await .unwrap(); assert!(out.contains("Trust level for \"git\" set to trusted")); + assert!(out.contains("Per-invocation integrity re-check enabled")); + + let row = memory + .sqlite() + .load_skill_trust("git") + .await + .unwrap() + .unwrap(); + assert!(row.requires_trust_check); + } + + #[tokio::test] + async fn trust_no_require_check_flag_overrides_config_default() { + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "git", + SkillTrustLevel::Quarantined, + SourceKind::Local, + None, + None, + "hash1", + ) + .await + .unwrap(); + let mut agent = agent_with_memory(memory.clone()); + + let out = agent + .handle_skill_trust_command_as_string(&["git", "trusted", "--no-require-check"]) + .await + .unwrap(); + assert!(out.contains("Trust level for \"git\" set to trusted")); assert!(!out.contains("Per-invocation integrity re-check enabled")); let row = memory @@ -342,6 +399,124 @@ mod tests { assert!(!row.requires_trust_check); } + #[tokio::test] + async fn trust_config_default_false_leaves_check_unarmed_without_flags() { + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "git", + SkillTrustLevel::Quarantined, + SourceKind::Local, + None, + None, + "hash1", + ) + .await + .unwrap(); + let mut agent = agent_with_memory(memory.clone()); + agent + .services + .skill + .trust_config + .require_integrity_check_on_promote = false; + + let out = agent + .handle_skill_trust_command_as_string(&["git", "trusted"]) + .await + .unwrap(); + assert!(out.contains("Trust level for \"git\" set to trusted")); + assert!(!out.contains("Per-invocation integrity re-check enabled")); + + let row = memory + .sqlite() + .load_skill_trust("git") + .await + .unwrap() + .unwrap(); + assert!(!row.requires_trust_check); + } + + #[tokio::test] + async fn trust_require_check_flag_forces_arm_even_when_config_default_false() { + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "git", + SkillTrustLevel::Quarantined, + SourceKind::Local, + None, + None, + "hash1", + ) + .await + .unwrap(); + let mut agent = agent_with_memory(memory.clone()); + agent + .services + .skill + .trust_config + .require_integrity_check_on_promote = false; + + let out = agent + .handle_skill_trust_command_as_string(&["git", "trusted", "--require-check"]) + .await + .unwrap(); + assert!(out.contains("Per-invocation integrity re-check enabled")); + + let row = memory + .sqlite() + .load_skill_trust("git") + .await + .unwrap() + .unwrap(); + assert!(row.requires_trust_check); + } + + #[tokio::test] + async fn trust_promotion_to_quarantined_leaves_requires_trust_check_untouched() { + // Quarantined/Blocked promotions must not be affected by the config default or + // either flag — the re-check only matters for Trusted/Verified skills. + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "git", + SkillTrustLevel::Trusted, + SourceKind::Local, + None, + None, + "hash1", + ) + .await + .unwrap(); + memory + .sqlite() + .set_requires_trust_check("git", true) + .await + .unwrap(); + let mut agent = agent_with_memory(memory.clone()); + + let out = agent + .handle_skill_trust_command_as_string(&["git", "quarantined"]) + .await + .unwrap(); + assert!(out.contains("Trust level for \"git\" set to quarantined")); + assert!(!out.contains("Per-invocation integrity re-check enabled")); + + let row = memory + .sqlite() + .load_skill_trust("git") + .await + .unwrap() + .unwrap(); + assert!( + row.requires_trust_check, + "demoting to quarantined must not clear a previously armed re-check" + ); + } + #[tokio::test] async fn trust_info_display_includes_requires_trust_check() { let memory = test_memory().await; diff --git a/crates/zeph-core/src/lib.rs b/crates/zeph-core/src/lib.rs index b13178e49..da042550f 100644 --- a/crates/zeph-core/src/lib.rs +++ b/crates/zeph-core/src/lib.rs @@ -126,7 +126,7 @@ pub use config::{Config, ConfigError}; pub use runtime_context::RuntimeContext; pub use skill_invoker::{InvokeSkillParams, SkillInvokeExecutor, SkillTrustSnapshot}; pub use skill_loader::SkillLoaderExecutor; -pub use skill_trust_gate::{SkillBodyResolution, SkillTrustGate}; +pub use skill_trust_gate::{SkillBodyResolution, SkillTrustGate, resolve_require_check}; pub use zeph_common::hash::blake3_hex as content_hash; pub use zeph_sanitizer::exfiltration::{ ExfiltrationEvent, ExfiltrationGuard, ExfiltrationGuardConfig, extract_flagged_urls, diff --git a/crates/zeph-core/src/skill_trust_gate.rs b/crates/zeph-core/src/skill_trust_gate.rs index d489d6160..106f56eee 100644 --- a/crates/zeph-core/src/skill_trust_gate.rs +++ b/crates/zeph-core/src/skill_trust_gate.rs @@ -250,6 +250,37 @@ impl SkillTrustGate { } } +/// Single source of truth for the `requires_trust_check` arming decision made on promotion to +/// `Trusted`/`Verified` (#6087). +/// +/// `force_on` (`--require-check`) always wins; otherwise `force_off` (`--no-require-check`) +/// wins; otherwise falls back to `config_default` +/// (`[skills.trust] require_integrity_check_on_promote`). Used identically by the CLI +/// (`zeph skill trust`, binary crate) and in-session (`/skill trust`, +/// `crate::agent::trust_commands`) promotion handlers — both already gate the call on +/// `matches!(level, SkillTrustLevel::Trusted | SkillTrustLevel::Verified)` before consulting +/// this function; promotion to `Quarantined`/`Blocked` must never call it. +/// +/// # Examples +/// +/// ``` +/// use zeph_core::resolve_require_check; +/// +/// assert!(resolve_require_check(true, true, false), "force_on always wins"); +/// assert!(!resolve_require_check(false, true, true), "force_off wins over the default"); +/// assert!(resolve_require_check(false, false, true), "falls back to the config default"); +/// ``` +#[must_use] +pub fn resolve_require_check(force_on: bool, force_off: bool, config_default: bool) -> bool { + if force_on { + true + } else if force_off { + false + } else { + config_default + } +} + #[cfg(test)] mod tests { use std::path::Path; @@ -393,4 +424,31 @@ mod tests { other => panic!("expected Body, got a different variant: {other:?}"), } } + + // ── resolve_require_check (#6087) ──────────────────────────────────────── + + #[test] + fn resolve_require_check_defaults_to_config_when_no_flag_forces_it() { + assert!(resolve_require_check(false, false, true)); + assert!(!resolve_require_check(false, false, false)); + } + + #[test] + fn resolve_require_check_force_on_wins_over_config_default_false() { + assert!(resolve_require_check(true, false, false)); + } + + #[test] + fn resolve_require_check_force_off_wins_over_config_default_true() { + assert!(!resolve_require_check(false, true, true)); + } + + #[test] + fn resolve_require_check_force_on_wins_over_force_off() { + // Both flags present is nonsensical (clap rejects it on the CLI via conflicts_with), + // but the in-session parser has no such enforcement — force_on must still take + // precedence so the decision is total and unambiguous either way. + assert!(resolve_require_check(true, true, false)); + assert!(resolve_require_check(true, true, true)); + } } diff --git a/specs/005-skills/spec.md b/specs/005-skills/spec.md index 954ea7611..7b7a775a9 100644 --- a/specs/005-skills/spec.md +++ b/specs/005-skills/spec.md @@ -569,6 +569,26 @@ blocks invocation. requires_trust_check = true ``` +#### Automatic Activation on Promotion (#6087) + +`requires_trust_check` is armed **automatically** whenever an operator promotes a skill to +`Trusted` or `Verified` — the choke point where a skill's body is dispatched verbatim without +sanitization — via `[skills.trust] require_integrity_check_on_promote` (default `true`). Both +operator-facing promotion handlers apply this: CLI `zeph skill trust trusted|verified` +(`src/commands/skill.rs`) and in-session `/skill trust trusted|verified` +(`crates/zeph-core/src/agent/trust_commands.rs`). `--require-check`/`--no-require-check` +(mutually exclusive) on the promoting command always override the config default. Promotion to +`Quarantined`/`Blocked` leaves `requires_trust_check` untouched — it is irrelevant at those +levels and a previously armed flag must survive a temporary demotion. + +Self-learning/heuristic auto-promotion (`crates/zeph-core/src/agent/learning/trust.rs`) and +reload trust-assignment (`crates/zeph-core/src/agent/skill_reload.rs`) also raise a skill's +trust level to `Trusted`/`Verified` but are **not** operator promotion and do not arm +`requires_trust_check` — the threat this default addresses is an operator promoting a skill +and forgetting to arm the re-check, not autonomous promotion paths. This is an intentional +scope boundary, not an oversight, for #6087; a follow-up issue may be filed to cover the +auto-promotion paths separately. + ### Recursive Nested Skill Discovery (#4682, #4684) `WalkDir`-based discovery replaces the flat `read_dir` loop in the skill scanner. The traversal uses @@ -702,6 +722,8 @@ and emits an advisory `SecurityEvent::SkillAdvisory` with severity and matched p - `sanitize_skill_metadata()` MUST run before EVERY description injection — no bypass path - Blake3 re-hash only applies to skills with `requires_trust_check = true`; normal skills use load-time trust only +- `requires_trust_check` is armed by default on promotion to `Trusted`/`Verified` (`require_integrity_check_on_promote`, default `true`) — NEVER silently leave it off without an explicit `--no-require-check` or config override (#6087) +- Promotion to `Quarantined`/`Blocked` MUST NOT clear `requires_trust_check` — NEVER reset the flag on demotion - Advisory scan result MUST NOT block skill invocation in v1 — advisory only - NEVER store the raw unsanitized description in the system prompt - NEVER proceed when `semantic_scan = true` but `semantic_scan_provider` is empty — return a config error (fail-closed, #4706, #4709) diff --git a/specs/010-security/spec.md b/specs/010-security/spec.md index d5907617c..7dc1a28c4 100644 --- a/specs/010-security/spec.md +++ b/specs/010-security/spec.md @@ -201,6 +201,17 @@ follow-up issue filed with the #3077 PR. - Null bytes, path traversal (`../`), and symlink escapes are caught at load time - Instruction file loading: canonical path must stay within project root +## Skill Trust: Default-On Integrity Re-Check on Promotion (#6087) + +Promoting a skill to `Trusted`/`Verified` — the tier whose body is dispatched verbatim, without +sanitization — arms the per-invocation BLAKE3 integrity re-check (`requires_trust_check`) by +default, per `[skills.trust] require_integrity_check_on_promote` (default `true`). Prior to this, +the re-check could only be armed via an explicit `--require-check` flag (#6080), leaving the +attack the check exists for — a Trusted/Verified skill's `SKILL.md` tampered on disk after +promotion — unprotected by default. See `specs/005-skills/spec.md` §"Automatic Activation on +Promotion" for the full behavior spec and scope boundary (operator promotion only; self-learning +auto-promotion and reload trust-assignment are out of scope for this default). + ## Key Invariants - Secrets never flow through logging, error messages, or debug dumps (redaction applied) diff --git a/src/cli.rs b/src/cli.rs index e1b603a28..dc37e1c58 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -875,9 +875,14 @@ pub(crate) enum SkillCommand { /// Trust level: trusted, verified, quarantined, blocked level: String, /// Enable per-invocation blake3 integrity re-check: re-hash SKILL.md before every - /// dispatch and demote to quarantined on mismatch (#4293, #6080) - #[arg(long)] + /// dispatch and demote to quarantined on mismatch (#4293, #6080). Forces the check on + /// regardless of `[skills.trust] require_integrity_check_on_promote`. + #[arg(long, conflicts_with = "no_require_check")] require_check: bool, + /// Skip arming the per-invocation integrity re-check even if + /// `[skills.trust] require_integrity_check_on_promote` defaults it on (#6087) + #[arg(long)] + no_require_check: bool, }, /// Block a skill Block { @@ -1557,6 +1562,88 @@ mod tests { assert_eq!(err.kind(), clap::error::ErrorKind::InvalidSubcommand); } + // ── skill trust auto-activation CLI parsing (#6087) ────────────────────── + + #[test] + fn cli_parses_skill_trust_with_no_flags() { + use super::{Command, SkillCommand}; + let cli = Cli::try_parse_from(["zeph", "skill", "trust", "git", "trusted"]).unwrap(); + assert!(matches!( + cli.command, + Some(Command::Skill { + command: SkillCommand::Trust { + require_check: false, + no_require_check: false, + .. + } + }) + )); + } + + #[test] + fn cli_parses_skill_trust_require_check() { + use super::{Command, SkillCommand}; + let cli = Cli::try_parse_from([ + "zeph", + "skill", + "trust", + "git", + "trusted", + "--require-check", + ]) + .unwrap(); + assert!(matches!( + cli.command, + Some(Command::Skill { + command: SkillCommand::Trust { + require_check: true, + no_require_check: false, + .. + } + }) + )); + } + + #[test] + fn cli_parses_skill_trust_no_require_check() { + use super::{Command, SkillCommand}; + let cli = Cli::try_parse_from([ + "zeph", + "skill", + "trust", + "git", + "trusted", + "--no-require-check", + ]) + .unwrap(); + assert!(matches!( + cli.command, + Some(Command::Skill { + command: SkillCommand::Trust { + require_check: false, + no_require_check: true, + .. + } + }) + )); + } + + #[test] + fn cli_skill_trust_require_check_and_no_require_check_conflict() { + let err = Cli::try_parse_from([ + "zeph", + "skill", + "trust", + "git", + "trusted", + "--require-check", + "--no-require-check", + ]) + .err() + .unwrap(); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + #[test] fn cli_parses_plugin_search() { use super::{Command, PluginCommand}; diff --git a/src/commands/skill.rs b/src/commands/skill.rs index 65bb92835..08bbea892 100644 --- a/src/commands/skill.rs +++ b/src/commands/skill.rs @@ -203,6 +203,7 @@ pub(crate) async fn handle_skill_command( name, level, require_check, + no_require_check, } => { let trust_level = level.parse::().map_err(|_| { anyhow::anyhow!( @@ -254,15 +255,26 @@ pub(crate) async fn handle_skill_command( } println!("Trust level for \"{name}\" set to {trust_level}."); - // #6080: expose the previously unreachable `requires_trust_check` setter so the - // per-invocation blake3 re-check (`SkillTrustGate::resolve_body`) can actually be - // armed for a skill. - if require_check { - store - .set_requires_trust_check(&name, true) - .await - .map_err(|e| anyhow::anyhow!("{e}"))?; - println!("Per-invocation integrity re-check enabled for \"{name}\"."); + // #6087: promotion to Trusted/Verified is the choke point where a tampered + // SKILL.md body would be dispatched verbatim — arm the re-check by default there, + // with --require-check/--no-require-check overriding the config default. + // Quarantined/Blocked promotions leave requires_trust_check untouched. + if matches!( + trust_level, + zeph_common::SkillTrustLevel::Trusted | zeph_common::SkillTrustLevel::Verified + ) { + let arm = zeph_core::resolve_require_check( + require_check, + no_require_check, + config.skills.trust.require_integrity_check_on_promote, + ); + if arm { + store + .set_requires_trust_check(&name, true) + .await + .map_err(|e| anyhow::anyhow!("{e}"))?; + println!("Per-invocation integrity re-check enabled for \"{name}\"."); + } } } @@ -714,3 +726,133 @@ mod registry_tests { assert!(err.to_string().contains("registry fetch failed")); } } + +// ── `handle_skill_command` Trust promotion (#6087) ─────────────────────────── +// +// Handler-level coverage confirming `SkillCommand::Trust` actually calls the shared +// `zeph_core::resolve_require_check` (not just that the pure function is correct in isolation — +// see `skill_trust_gate::tests::resolve_require_check_*` in `zeph-core`). Isolated from the real +// `~/.config/zeph` vault via a per-test `XDG_CONFIG_HOME`, restored afterward; `#[serial]` +// prevents concurrent env-var mutation across tests in this binary (mirrors the established +// pattern in `src/commands/durable.rs`). +#[allow(unsafe_code)] +#[cfg(test)] +mod trust_promotion_tests { + use serial_test::serial; + + use super::*; + + /// Points `XDG_CONFIG_HOME` (and therefore `managed_skills_dir()`) at a fresh temp dir, + /// writes a config file with an isolated sqlite path, and installs one skill via the real + /// `SkillCommand::Install` path. Returns the config path, the installed skill's name, and + /// the previous `XDG_CONFIG_HOME` value to restore via [`restore_xdg_config_home`]. + async fn install_test_skill( + root: &std::path::Path, + ) -> (std::path::PathBuf, String, Option) { + let prev_xdg = std::env::var("XDG_CONFIG_HOME").ok(); + unsafe { + std::env::set_var("XDG_CONFIG_HOME", root); + } + + let config_path = root.join("config.toml"); + let db_path = root.join("test.db"); + let mut config = zeph_core::config::Config::default(); + config.memory.sqlite_path = db_path.to_string_lossy().into_owned(); + std::fs::write(&config_path, toml::to_string(&config).unwrap()).unwrap(); + + // Source directory basename must match the skill's frontmatter `name` (load_skill_meta + // validates this before install). + let skill_src = root.join("trust-promo-test"); + std::fs::create_dir_all(&skill_src).unwrap(); + std::fs::write( + skill_src.join("SKILL.md"), + "---\nname: trust-promo-test\ndescription: A test skill for #6087.\n---\nbody", + ) + .unwrap(); + + handle_skill_command( + SkillCommand::Install { + source: skill_src.to_string_lossy().into_owned(), + }, + Some(&config_path), + ) + .await + .unwrap(); + + (config_path, "trust-promo-test".to_owned(), prev_xdg) + } + + fn restore_xdg_config_home(prev: Option) { + unsafe { + match prev { + Some(v) => std::env::set_var("XDG_CONFIG_HOME", v), + None => std::env::remove_var("XDG_CONFIG_HOME"), + } + } + } + + #[tokio::test] + #[serial] + async fn trust_promotion_with_no_flags_arms_check_by_default() { + let dir = tempfile::tempdir().unwrap(); + let (config_path, name, prev_xdg) = Box::pin(install_test_skill(dir.path())).await; + + let result = handle_skill_command( + SkillCommand::Trust { + name: name.clone(), + level: "trusted".to_owned(), + require_check: false, + no_require_check: false, + }, + Some(&config_path), + ) + .await; + + let db_path = dir.path().join("test.db"); + let store = zeph_memory::store::SqliteStore::new(db_path.to_str().unwrap()) + .await + .unwrap(); + let row = store.load_skill_trust(&name).await.unwrap(); + + restore_xdg_config_home(prev_xdg); + + result.unwrap(); + assert!( + row.unwrap().requires_trust_check, + "default config (require_integrity_check_on_promote unset -> true) must arm the \ + check via resolve_require_check even with no CLI flags" + ); + } + + #[tokio::test] + #[serial] + async fn trust_promotion_with_no_require_check_flag_overrides_default() { + let dir = tempfile::tempdir().unwrap(); + let (config_path, name, prev_xdg) = Box::pin(install_test_skill(dir.path())).await; + + let result = handle_skill_command( + SkillCommand::Trust { + name: name.clone(), + level: "trusted".to_owned(), + require_check: false, + no_require_check: true, + }, + Some(&config_path), + ) + .await; + + let db_path = dir.path().join("test.db"); + let store = zeph_memory::store::SqliteStore::new(db_path.to_str().unwrap()) + .await + .unwrap(); + let row = store.load_skill_trust(&name).await.unwrap(); + + restore_xdg_config_home(prev_xdg); + + result.unwrap(); + assert!( + !row.unwrap().requires_trust_check, + "--no-require-check must win over the config default via resolve_require_check" + ); + } +} diff --git a/src/init/mod.rs b/src/init/mod.rs index 0337e0e66..187394d39 100644 --- a/src/init/mod.rs +++ b/src/init/mod.rs @@ -156,6 +156,7 @@ pub(crate) struct WizardState { pub(crate) pii_filter_enabled: bool, pub(crate) rate_limit_enabled: bool, pub(crate) skill_scan_on_load: bool, + pub(crate) skill_require_integrity_check_on_promote: bool, pub(crate) skill_cross_session_rollout: bool, pub(crate) skill_min_sessions_before_promote: u32, pub(crate) skill_capability_escalation_check: bool, @@ -425,6 +426,7 @@ impl Default for WizardState { pii_filter_enabled: false, rate_limit_enabled: false, skill_scan_on_load: true, + skill_require_integrity_check_on_promote: true, skill_cross_session_rollout: false, skill_min_sessions_before_promote: 2, skill_capability_escalation_check: false, @@ -1153,6 +1155,8 @@ pub(crate) fn build_config(state: &WizardState) -> Config { .clone_from(&state.sandbox_denied_domains); config.tools.sandbox.fail_if_unavailable = state.sandbox_fail_if_unavailable; config.skills.trust.scan_on_load = state.skill_scan_on_load; + config.skills.trust.require_integrity_check_on_promote = + state.skill_require_integrity_check_on_promote; config.skills.trust.scanner.capability_escalation_check = state.skill_capability_escalation_check; if state.skill_cross_session_rollout { diff --git a/src/init/security.rs b/src/init/security.rs index e8fa7f8a6..93363adf8 100644 --- a/src/init/security.rs +++ b/src/init/security.rs @@ -220,6 +220,12 @@ pub(super) fn step_security(state: &mut WizardState) -> anyhow::Result<()> { ) .default(true) .interact()?; + state.skill_require_integrity_check_on_promote = Confirm::new() + .with_prompt( + "Automatically require a per-invocation integrity re-check when promoting a skill to trusted/verified? (re-hashes SKILL.md before every dispatch; override per-command with --no-require-check; recommended)", + ) + .default(true) + .interact()?; state.skill_capability_escalation_check = Confirm::new() .with_prompt( "Check skill capability escalation on load? (warns if skills declare tools exceeding their trust level)",