From 182f1b5e89f17e00f19520177af4a5ef01e2e6bf Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 7 Sep 2026 16:49:49 -0400 Subject: [PATCH] Reuse Bitwarden listing within each manifest resolution Resolve name and project references through one lazy snapshot of parsed lookup metadata. UUID references skip listing, each mapping still fetches its value, and every resolution starts with fresh lookup state. Add CLI regression coverage for bounded backend calls, mixed references, source recordings, fresh values, fail-closed errors, and child contracts. Clarify the listing lifetime and remaining get overhead in the README. Validation: - Regression failed before the change: 2 listings instead of 1. - Targeted CLI tests, cargo check --all-targets, cargo test, cargo fmt --check, and cargo clippy --all-targets -- -D warnings pass. - Standards and Spec reviews: 0 findings each. Live macOS comparison using the same 23-name/2-UUID manifest and timing wrapper, two runs per binary, reversing order on the second pair: - Installed v0.4.21: 12.946/13.173s, 23 list + 25 get calls. - Optimized local build: 6.530/5.171s, 1 list + 25 get calls. - Remaining get calls took 5.207/4.024s; startup is not at bare AGY parity. Only command verbs, durations, and status were recorded, never secrets. An extra uninstrumented baseline failed then succeeded on retry; it is excluded from the comparison above. Fixes #100 --- README.md | 9 +- src/backend.rs | 43 +++++-- tests/cli_bitwarden_lookup.rs | 205 ++++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 15 deletions(-) create mode 100644 tests/cli_bitwarden_lookup.rs diff --git a/README.md b/README.md index faea606..5271041 100644 --- a/README.md +++ b/README.md @@ -767,9 +767,12 @@ Wrong token types fail with errors such as “Doesn't contain a decryption key. | `name:` | `OPENAI_API_KEY=name:openai-api-key` | | `project:/` | `OPENAI_API_KEY=project:tools/openai-api-key` | -Names resolve via `bws secret list` once per process. Prefer -`vaulted-agent secrets list` / `vaulted-agent setup` over raw `bws` so auth -matches launches. +Names and project-qualified references share one `bws secret list` per manifest +resolution. UUID-only and empty manifests need no listing. Lookup metadata is +kept only for that resolution; each launch fetches current vault state and still +runs one `bws secret get` per mapping. No lookup data or secret values are cached +between launches. Prefer `vaulted-agent secrets list` / `vaulted-agent setup` +over raw `bws` so auth matches launches. **Refs file (what setup / refresh write).** After listing secrets, setup (or `va refresh`) can write a **refs file** under `/etc/vaulted-agent/manifests/` diff --git a/src/backend.rs b/src/backend.rs index 158894c..dcf3e32 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -94,8 +94,7 @@ pub fn parse_bws_list_json(list_json: &str) -> Result Result { - let rows = parse_bws_list_json(list_json)?; +fn parse_bws_ref(rows: &[(String, String, String)], r: &str) -> Result { if let Some(want_name) = r.strip_prefix("name:") { let matches: Vec<_> = rows.iter().filter(|(_, k, _)| k == want_name).collect(); if matches.is_empty() { @@ -121,13 +120,30 @@ fn parse_bws_ref(list_json: &str, r: &str) -> Result { Err(Error::Message(format!("bad bitwarden ref {r}"))) } -fn bws_resolve_ref_to_id(token: &ManagerToken, r: &str) -> Result { - let bare = r.strip_prefix("uuid:").unwrap_or(r); - if is_uuid(bare) { - return Ok(bare.to_string()); +/// Lookup metadata lives only for one resolution; UUIDs need no listing. +struct BwsRefResolver<'a> { + token: &'a ManagerToken, + rows: Option>, +} + +impl<'a> BwsRefResolver<'a> { + fn new(token: &'a ManagerToken) -> Self { + Self { token, rows: None } + } + + fn resolve_id(&mut self, r: &str) -> Result { + let bare = r.strip_prefix("uuid:").unwrap_or(r); + if is_uuid(bare) { + return Ok(bare.to_string()); + } + let rows = match self.rows { + Some(ref rows) => rows, + None => self + .rows + .insert(parse_bws_list_json(&bws_list_json(self.token)?)?), + }; + parse_bws_ref(rows, r) } - let list = bws_list_json(token)?; - parse_bws_ref(&list, r) } /// Extract secret value from `bws secret get --output json`. @@ -151,7 +167,7 @@ fn bws_get_value(token: &ManagerToken, id: &str) -> Result { /// Resolve a bitwarden ref to secret id (for secrets get). pub fn bws_resolve_ref(token: &ManagerToken, r: &str) -> Result { - bws_resolve_ref_to_id(token, r) + BwsRefResolver::new(token).resolve_id(r) } /// Fetch secret value by id (for secrets get). @@ -165,9 +181,11 @@ pub fn resolve_bitwarden( ) -> Result> { let pairs: Vec<(String, String)> = validate_manifest_file(manifest, Backend::Bitwarden)?; let mut out = HashMap::new(); + let mut resolver = BwsRefResolver::new(token); for (var, r) in pairs { - let id = - bws_resolve_ref_to_id(token, &r).map_err(|e| name_the_manifest(manifest, &var, e))?; + let id = resolver + .resolve_id(&r) + .map_err(|e| name_the_manifest(manifest, &var, e))?; let value = bws_get_value(token, &id)?; out.insert(var, SecretValue::new(value)); } @@ -765,7 +783,8 @@ mod tests { #[test] fn parse_name_ref() { let j = r#"[{"id":"id1","key":"openai-api-key","project":{"name":"tools"}}]"#; - assert_eq!(parse_bws_ref(j, "name:openai-api-key").unwrap(), "id1"); + let rows = parse_bws_list_json(j).unwrap(); + assert_eq!(parse_bws_ref(&rows, "name:openai-api-key").unwrap(), "id1"); } #[test] diff --git a/tests/cli_bitwarden_lookup.rs b/tests/cli_bitwarden_lookup.rs new file mode 100644 index 0000000..f8c0f66 --- /dev/null +++ b/tests/cli_bitwarden_lookup.rs @@ -0,0 +1,205 @@ +//! Issue #100: bound remote lookup work at the CLI acceptance seam. + +mod common; + +use common::CliSeam; +use std::fs; +use std::process::Output; + +const FIRST: &str = "12345678-1234-5678-9012-123456789001"; +const SECOND: &str = "12345678-1234-5678-9012-123456789002"; +const THIRD: &str = "12345678-1234-5678-9012-123456789003"; + +fn harness(manifest: &str) -> CliSeam { + let seam = CliSeam::new(); + fs::write( + seam.config_dir.join("harnesses.d/agy.conf"), + "backend = bitwarden\nmanifest = test.refs\ncommand = agy\nworkdir = caller\n", + ) + .unwrap(); + fs::write(seam.config_dir.join("manifests/test.refs"), manifest).unwrap(); + fs::write( + seam.config_dir.join("bws.env"), + "BWS_ACCESS_TOKEN=synthetic-manager-token\n", + ) + .unwrap(); + seam.install_stub_agent("agy"); + fs::write(seam.root.join("calls"), "").unwrap(); + fs::write( + seam.root.join("vault.json"), + serde_json::json!([ + {"id": FIRST, "key": "first", "project": {"name": "tools"}, "value": "first-value"}, + {"id": SECOND, "key": "second", "project": {"name": "tools"}, "value": "second-value"}, + {"id": THIRD, "key": "second", "project": {"name": "other"}, "value": "other-project-value"} + ]) + .to_string(), + ) + .unwrap(); + // A fake external CLI, not a mock of a launcher module. Record verbs only. + seam.write_executable( + "bws", + r#"#!/usr/bin/env python3 +import json, os, sys +from pathlib import Path +root = Path(__file__).resolve().parent.parent +assert os.environ.get('BWS_ACCESS_TOKEN') == 'synthetic-manager-token' +assert sys.argv[1] == 'secret' +verb = sys.argv[2] +with (root / 'calls').open('a') as log: + log.write(verb + '\n') +if (root / ('fail-' + verb)).exists(): + sys.exit('fixture forced ' + verb + ' failure') +rows = json.loads((root / 'vault.json').read_text()) +if verb == 'list': + print(json.dumps(rows)) +elif verb == 'get': + row = next(row for row in rows if row['id'] == sys.argv[3]) + print(json.dumps(row)) +else: + sys.exit('unexpected fake bws command') +"#, + ); + seam +} + +fn launch(seam: &CliSeam) -> Output { + seam.vaulted_agent() + .env("VAULTED_AGENT_AUTH_MODE", "file") + .args(["agy", "--conversation", "conv-123"]) + .output() + .expect("launch") +} + +fn assert_success(out: &Output) { + assert!( + out.status.success(), + "stderr={}", + String::from_utf8_lossy(&out.stderr) + ); +} + +fn calls(seam: &CliSeam, verb: &str) -> usize { + fs::read_to_string(seam.root.join("calls")) + .unwrap() + .lines() + .filter(|line| *line == verb) + .count() +} + +#[test] +fn mixed_refs_share_one_listing_and_preserve_the_child_contract() { + let seam = harness(&format!( + "BARE={FIRST}\nNAMED=name:first # uuid:{SECOND}\n\ + QUALIFIED=project:tools/second\nPREFIXED=uuid:{SECOND}\n" + )); + let out = launch(&seam); + assert_success(&out); + let child = seam.read_stub_record("agy"); + for expected in [ + "ENV BARE=first-value", + "ENV NAMED=first-value", + "ENV QUALIFIED=second-value", + "ENV PREFIXED=second-value", + ] { + assert!(child.contains(expected), "missing {expected}: {child}"); + } + assert_eq!(child.lines().next(), Some("ARGV: --conversation conv-123")); + let cwd = child + .lines() + .find_map(|line| line.strip_prefix("ENV PWD=")) + .unwrap(); + assert_eq!( + fs::canonicalize(cwd).unwrap(), + fs::canonicalize(&seam.work_dir).unwrap() + ); + assert!(!child.contains("BWS_ACCESS_TOKEN=")); + assert!(!child.contains("OP_SERVICE_ACCOUNT_TOKEN=")); + assert!(out.stdout.is_empty()); + assert!(out.stderr.is_empty()); + assert_eq!(calls(&seam, "get"), 4); + assert_eq!(calls(&seam, "list"), 1, "listing repeated for named refs"); +} + +#[test] +fn uuid_only_and_empty_manifests_do_not_list() { + for (manifest, gets) in [ + (format!("BARE={FIRST}\nPREFIXED=uuid:{SECOND}\n"), 2), + ("# No references\n".to_string(), 0), + ] { + let seam = harness(&manifest); + // Listing must not even be attempted, including when it is unavailable. + fs::write(seam.root.join("fail-list"), "").unwrap(); + assert_success(&launch(&seam)); + assert_eq!(calls(&seam, "list"), 0); + assert_eq!(calls(&seam, "get"), gets); + } +} + +#[test] +fn each_launch_reads_current_lookup_metadata_and_values() { + let seam = harness("NAMED=name:first\nQUALIFIED=project:tools/second\n"); + assert_success(&launch(&seam)); + assert!(seam + .read_stub_record("agy") + .contains("ENV NAMED=first-value")); + + fs::write( + seam.root.join("vault.json"), + serde_json::json!([ + {"id": THIRD, "key": "first", "value": "new-first-value"}, + {"id": SECOND, "key": "second", "project": {"name": "tools"}, "value": "rotated-second-value"} + ]) + .to_string(), + ) + .unwrap(); + assert_success(&launch(&seam)); + let child = seam.read_stub_record("agy"); + assert!(child.contains("ENV NAMED=new-first-value")); + assert!(child.contains("ENV QUALIFIED=rotated-second-value")); + assert_eq!(calls(&seam, "list"), 2); + assert_eq!(calls(&seam, "get"), 4); +} + +#[test] +fn unresolved_refs_fail_closed_even_with_source_recordings() { + for (reference, error, lists, gets) in [ + ( + format!("name:missing # uuid:{SECOND}"), + "no secret matched", + 1, + 1, + ), + ( + format!("name:second # uuid:{SECOND}"), + "multiple secrets named", + 1, + 1, + ), + ("project:missing/second".into(), "no secret matched", 1, 1), + ("uuid:not-a-uuid".into(), "uuid: value is not a UUID", 0, 0), + ] { + let seam = harness(&format!("GOOD=name:first\nBAD={reference}\n")); + let out = launch(&seam); + assert!(!out.status.success(), "accepted {reference}"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains(error), "{stderr}"); + assert!(!seam.work_dir.join("agy.record").exists()); + assert_eq!(calls(&seam, "list"), lists); + assert_eq!(calls(&seam, "get"), gets); + } +} + +#[test] +fn backend_failures_do_not_start_the_child() { + for (verb, gets) in [("list", 0), ("get", 1)] { + let seam = harness("NAMED=name:first\nQUALIFIED=project:tools/second\n"); + fs::write(seam.root.join(format!("fail-{verb}")), "").unwrap(); + let out = launch(&seam); + assert!(!out.status.success()); + assert!(String::from_utf8_lossy(&out.stderr) + .contains(&format!("fixture forced {verb} failure"))); + assert!(!seam.work_dir.join("agy.record").exists()); + assert_eq!(calls(&seam, "list"), 1); + assert_eq!(calls(&seam, "get"), gets); + } +}