From c86ea9eb0e0134de1b053f0829f4a2705615f3a7 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Mon, 10 Aug 2026 09:42:53 +0000 Subject: [PATCH] Fix pass Git config and lazy OTP commits --- apps/cli/src/main.rs | 62 +++++++++++--- crates/storage/src/git.rs | 12 +-- crates/storage/src/otp.rs | 77 ++++++++++++++++++ .../tests/fixtures/compatibility/README.md | 5 ++ .../compatibility/pass-git-config.ini | 3 + crates/storage/tests/git_embedded.rs | 4 + crates/storage/tests/otp.rs | 81 ++++++++++++++++++- docs/git-synchronization.md | 9 ++- 8 files changed, 231 insertions(+), 22 deletions(-) create mode 100644 crates/storage/tests/fixtures/compatibility/pass-git-config.ini diff --git a/apps/cli/src/main.rs b/apps/cli/src/main.rs index e79d8b8..6082505 100644 --- a/apps/cli/src/main.rs +++ b/apps/cli/src/main.rs @@ -832,16 +832,10 @@ fn execute_otp timestamp, Err(error) => return operation_error(stderr, error), }; - let mut committer = - match automatic_committer(&repository, request.entry.trim_end_matches('/')) { - Ok(committer) => committer, - Err(error) => return operation_error(stderr, error), - }; - let outcome = - match service.code(&request.entry, timestamp, None, secrets, &mut committer) { - Ok(outcome) => outcome, - Err(error) => return operation_error(stderr, error), - }; + let outcome = match service.code_automatic(&request.entry, timestamp, None, secrets) { + Ok(outcome) => outcome, + Err(error) => return operation_error(stderr, error), + }; if request.clipboard { match presentation.clipboard( outcome.code(), @@ -1488,6 +1482,7 @@ mod tests { error::Error, ffi::OsString, fs, + io::Write as _, sync::{Arc, Mutex}, }; @@ -1502,7 +1497,7 @@ mod tests { }, config::Config, crypto::KeyInfo, - git::GitCredentialProvider as _, + git::{GitCredentialProvider as _, GitIdentity, GitRepository}, otp::OtpInput, presentation::{ClipboardTimeout, QrMatrix}, repository::{EntryPath, Repository, SecretBytes}, @@ -2097,12 +2092,28 @@ mod tests { let fixtures = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join("../../crates/storage/tests/fixtures/compatibility"); let temporary = tempfile::tempdir()?; + let vault = temporary.path().join("vault"); + fs::create_dir_all(vault.join("otp"))?; + for path in [".gpg-id", ".gpg-id.sig", "otp/totp.gpg"] { + fs::copy(fixtures.join("stores/basic").join(path), vault.join(path))?; + } + let repository = Repository::open(&vault)?; + let identity = GitIdentity::ironstorage(); + let git = GitRepository::init(&repository, identity.clone())?; + let commits_before = git.log(None)?.len(); + drop(git); + let git_config = vault.join(".git/config"); + let mut config_file = fs::OpenOptions::new().append(true).open(&git_config)?; + config_file.write_all(&fs::read(fixtures.join("pass-git-config.ini"))?)?; + drop(config_file); + let config_before = fs::read(&git_config)?; + let ciphertext_before = fs::read(vault.join("otp/totp.gpg"))?; let config_path = temporary.path().join("config.toml"); fs::write( &config_path, format!( "vault = {:?}\ndefault_key = {:?}\nkey_material = {:?}\nclipboard_timeout_seconds = 1\n", - fixtures.join("stores/basic"), + vault, FINGERPRINT, fixtures.join("keys"), ), @@ -2114,6 +2125,29 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); + assert_eq!( + execute_secure_with_services( + &config, + &CommandRequest::Show(ShowRequest { + entry: Some("otp/totp".to_owned()), + presentation: Presentation::Terminal, + }), + &mut secrets, + &mut presentation, + &mut interaction, + || Ok(59), + &mut stdout, + &mut stderr, + ) + .expect("memory output cannot fail"), + EXIT_SUCCESS + ); + assert!( + stdout + .windows(b"otpauth://totp".len()) + .any(|part| part == b"otpauth://totp") + ); + stdout.clear(); assert_eq!( execute_secure_with_services( &config, @@ -2136,6 +2170,10 @@ mod tests { assert!(code.iter().all(u8::is_ascii_digit)); assert!(!stdout.windows(code.len()).any(|part| part == code)); assert!(stderr.is_empty()); + assert_eq!(fs::read(vault.join("otp/totp.gpg"))?, ciphertext_before); + assert_eq!(fs::read(&git_config)?, config_before); + let git = GitRepository::open(&repository, identity)?; + assert_eq!(git.log(None)?.len(), commits_before); stdout.clear(); let uri = fs::read(fixtures.join("expected/basic/otp/totp.txt"))?; diff --git a/crates/storage/src/git.rs b/crates/storage/src/git.rs index 0fa3612..90e33bc 100644 --- a/crates/storage/src/git.rs +++ b/crates/storage/src/git.rs @@ -2476,20 +2476,16 @@ fn validate_local_config_security(config: &gix_config::File) -> Result<(), GitEr let name = section.header().name().to_str_lossy().to_ascii_lowercase(); if matches!( name.as_str(), - "include" - | "includeif" - | "credential" - | "filter" - | "diff" - | "merge" - | "protocol" - | "url" + "include" | "includeif" | "credential" | "filter" | "merge" | "protocol" | "url" ) { return Err(GitError::InvalidRepository(format!( "unsafe Git configuration section: {name}" ))); } } + // Upstream `pass git init` installs diff.gpg.binary and + // diff.gpg.textconv. IronStorage parses blobs and renders diffs itself, so + // these passive settings are preserved but never evaluated or executed. for key in [ "core.hooksPath", "core.sshCommand", diff --git a/crates/storage/src/otp.rs b/crates/storage/src/otp.rs index 154228a..13d2fe3 100644 --- a/crates/storage/src/otp.rs +++ b/crates/storage/src/otp.rs @@ -11,6 +11,7 @@ use zeroize::Zeroize as _; use crate::{ command::{OtpAppendRequest, OtpInputSource, OtpInsertRequest}, crypto::{CryptoError, KeyStore, SecretProvider}, + git::{AutomaticEntryCommitter, GitError, GitIdentity}, recipient::{RecipientPolicyError, RecipientPolicyManager, SigningPolicy}, repository::{EncryptedEntry, EntryPath, Repository, RepositoryError, SecretBytes}, write::{EntryAction, EntryCommit, EntryCommitError, EntryCommitter, OverwriteDecision}, @@ -613,12 +614,80 @@ impl<'a> OtpService<'a> { provider: &mut impl SecretProvider, committer: &mut impl EntryCommitter, ) -> Result { + let (path, original, plaintext, range, uri) = self.load_code_entry(entry, provider)?; + self.finish_code( + path, + original, + plaintext, + range, + uri, + unix_seconds, + signing, + committer, + ) + } + + /// Generate a code with storage-owned lazy Git selection. TOTP is + /// read-only and never opens Git; HOTP opens the innermost repository only + /// after the token has been decrypted and identified as counter based. + pub fn code_automatic( + &self, + entry: &str, + unix_seconds: u64, + signing: Option<&SigningPolicy>, + provider: &mut impl SecretProvider, + ) -> Result { + let (path, original, plaintext, range, uri) = self.load_code_entry(entry, provider)?; + if uri.kind() == OtpKind::Totp { + return Ok(OtpCodeOutcome { + code: uri.code_at(unix_seconds)?, + counter: None, + }); + } + let entry = path.to_string(); + let mut committer = AutomaticEntryCommitter::for_entry( + self.repository, + &entry, + GitIdentity::ironstorage(), + )?; + self.finish_code( + path, + original, + plaintext, + range, + uri, + unix_seconds, + signing, + &mut committer, + ) + } + + fn load_code_entry( + &self, + entry: &str, + provider: &mut impl SecretProvider, + ) -> Result<(EntryPath, EncryptedEntry, SecretBytes, Range, OtpUri), OtpError> { let path = parse_entry(entry)?; let original = self.repository.read_entry(&path)?; let plaintext = self.keys.decrypt(&original, provider)?; let (range, uri) = find_uri(&plaintext, &path)?.ok_or_else(|| OtpError::MissingUri { entry: path.clone(), })?; + Ok((path, original, plaintext, range, uri)) + } + + #[allow(clippy::too_many_arguments)] + fn finish_code( + &self, + path: EntryPath, + original: EncryptedEntry, + plaintext: SecretBytes, + range: Range, + uri: OtpUri, + unix_seconds: u64, + signing: Option<&SigningPolicy>, + committer: &mut impl EntryCommitter, + ) -> Result { match uri.kind() { OtpKind::Totp => Ok(OtpCodeOutcome { code: uri.code_at(unix_seconds)?, @@ -722,6 +791,7 @@ pub enum OtpError { Repository(RepositoryError), Crypto(CryptoError), RecipientPolicy(RecipientPolicyError), + Git(GitError), InvalidUri, InvalidScheme, UnsupportedType, @@ -771,6 +841,7 @@ impl fmt::Display for OtpError { Self::Repository(error) => error.fmt(formatter), Self::Crypto(error) => error.fmt(formatter), Self::RecipientPolicy(error) => error.fmt(formatter), + Self::Git(error) => error.fmt(formatter), Self::InvalidUri => formatter.write_str("OTP key URI is not valid UTF-8 URI text"), Self::InvalidScheme => formatter.write_str("OTP key URI must use the otpauth scheme"), Self::UnsupportedType => formatter.write_str("OTP key URI type must be totp or hotp"), @@ -848,6 +919,12 @@ impl From for OtpError { } } +impl From for OtpError { + fn from(error: GitError) -> Self { + Self::Git(error) + } +} + fn validate_input(input: &[u8]) -> Result<(), OtpError> { if input.is_empty() { Err(OtpError::EmptyInput) diff --git a/crates/storage/tests/fixtures/compatibility/README.md b/crates/storage/tests/fixtures/compatibility/README.md index 003c97d..8e7498a 100644 --- a/crates/storage/tests/fixtures/compatibility/README.md +++ b/crates/storage/tests/fixtures/compatibility/README.md @@ -30,6 +30,11 @@ password-store tree, and the expected plaintext. It deliberately lives outside the reproducible generator's `stores/`, `expected/`, and `repositories/` directories so a refresh cannot silently replace genuine GnuPG producer output. +`pass-git-config.ini` records the local diff driver written by upstream +`pass git init`. IronStorage preserves this configuration for repository +compatibility, but its embedded Rust diff implementation never evaluates the +driver or launches the configured GnuPG command. + Rust tests parse keys, verify signatures, decrypt every entry, compare exact plaintext bytes, validate ciphertext digests, validate Git objects and refs, and materialize isolated stores and repositories. They never invoke `pass`, diff --git a/crates/storage/tests/fixtures/compatibility/pass-git-config.ini b/crates/storage/tests/fixtures/compatibility/pass-git-config.ini new file mode 100644 index 0000000..8a81d48 --- /dev/null +++ b/crates/storage/tests/fixtures/compatibility/pass-git-config.ini @@ -0,0 +1,3 @@ +[diff "gpg"] + binary = true + textconv = gpg -d --no-tty --batch --quiet --yes diff --git a/crates/storage/tests/git_embedded.rs b/crates/storage/tests/git_embedded.rs index 42725be..79d0f0a 100644 --- a/crates/storage/tests/git_embedded.rs +++ b/crates/storage/tests/git_embedded.rs @@ -134,6 +134,10 @@ fn remotes_and_config_are_local_https_only() -> TestResult { git.config_set("user.name", "Local User")?; assert_eq!(git.config_get("user.name")?.as_deref(), Some("Local User")); assert!(git.config_set("credential.helper", "evil").is_err()); + assert!( + git.config_set("diff.gpg.textconv", "external-helper") + .is_err() + ); git.remove_remote("origin")?; assert!(git.remotes().is_empty()); Ok(()) diff --git a/crates/storage/tests/otp.rs b/crates/storage/tests/otp.rs index c3b98fd..9399afc 100644 --- a/crates/storage/tests/otp.rs +++ b/crates/storage/tests/otp.rs @@ -2,12 +2,13 @@ mod support; -use std::collections::BTreeMap; +use std::{collections::BTreeMap, fs, io::Write as _}; use data_encoding::BASE32_NOPAD; use ironstorage::{ command::{OtpAppendRequest, OtpInputSource, OtpInsertRequest}, crypto::{KeyInfo, KeyStore, SecretProvider, SecretProviderError}, + git::{GitIdentity, GitRepository}, otp::{OtpAlgorithm, OtpError, OtpInput, OtpKind, OtpService, OtpUri}, recipient::RecipientPolicyManager, repository::{EntryPath, Repository, SecretBytes}, @@ -454,6 +455,84 @@ fn malformed_ambiguous_declined_and_commit_failures_never_mutate() -> TestResult Ok(()) } +#[test] +fn automatic_code_supports_pass_diff_config_and_opens_git_only_for_hotp() -> TestResult { + let fixture = FixtureSet::load()?; + let store = fixture.materialize_store("basic")?; + let repository = Repository::open(store.path())?; + let identity = GitIdentity::ironstorage(); + let git = GitRepository::init(&repository, identity.clone())?; + let initial_commits = git.log(None)?.len(); + drop(git); + let config_path = store.path().join(".git/config"); + let mut config = fs::OpenOptions::new().append(true).open(&config_path)?; + config.write_all(&fixture.read("pass-git-config.ini")?)?; + drop(config); + let original_config = fs::read(&config_path)?; + + let keys = KeyStore::load(fixture.path("keys"))?; + let service = OtpService::new(&repository, &keys); + let mut provider = FixtureSecrets::all(&fixture); + let totp_path = EntryPath::parse("otp/totp")?; + let totp_before = repository.read_entry(&totp_path)?; + let totp = service.code_automatic("otp/totp", 59, None, &mut provider)?; + assert_eq!(totp.counter(), None); + assert_eq!(repository.read_entry(&totp_path)?, totp_before); + let git = GitRepository::open(&repository, identity.clone())?; + assert_eq!(git.log(None)?.len(), initial_commits); + drop(git); + + let hotp = service.code_automatic("otp/hotp", 0, None, &mut provider)?; + assert_eq!(hotp.counter(), Some(1)); + assert_eq!(service.uri("otp/hotp", &mut provider)?.counter(), Some(1)); + let git = GitRepository::open(&repository, identity)?; + assert_eq!(git.log(None)?.len(), initial_commits + 1); + assert_eq!( + git.log(Some(1))?[0].message(), + "Increment HOTP counter for otp/hotp." + ); + assert_eq!(fs::read(config_path)?, original_config); + Ok(()) +} + +#[test] +fn read_only_totp_ignores_git_that_mutations_correctly_reject() -> TestResult { + let fixture = FixtureSet::load()?; + let store = fixture.materialize_store("basic")?; + let repository = Repository::open(store.path())?; + GitRepository::init(&repository, GitIdentity::ironstorage())?; + let config_path = store.path().join(".git/config"); + let mut config = fs::OpenOptions::new().append(true).open(&config_path)?; + config.write_all(b"[filter \"unsafe\"]\n\tclean = external-helper\n")?; + drop(config); + + let keys = KeyStore::load(fixture.path("keys"))?; + let service = OtpService::new(&repository, &keys); + let mut provider = FixtureSecrets::all(&fixture); + let totp_path = EntryPath::parse("otp/totp")?; + let totp_before = repository.read_entry(&totp_path)?; + assert!( + service + .code_automatic("otp/totp", 59, None, &mut provider) + .is_ok() + ); + assert_eq!(repository.read_entry(&totp_path)?, totp_before); + + let hotp_path = EntryPath::parse("otp/hotp")?; + let hotp_before = repository.read_entry(&hotp_path)?; + let head_before = fs::read(store.path().join(".git/refs/heads/main"))?; + assert!(matches!( + service.code_automatic("otp/hotp", 0, None, &mut provider), + Err(OtpError::Git(_)) + )); + assert_eq!(repository.read_entry(&hotp_path)?, hotp_before); + assert_eq!( + fs::read(store.path().join(".git/refs/heads/main"))?, + head_before + ); + Ok(()) +} + fn token(secret: &[u8], algorithm: &str) -> Result { OtpUri::parse_str(&format!( "otpauth://totp/RFC6238?secret={}&algorithm={algorithm}&digits=8&period=30", diff --git a/docs/git-synchronization.md b/docs/git-synchronization.md index 12af823..a4fe2e3 100644 --- a/docs/git-synchronization.md +++ b/docs/git-synchronization.md @@ -3,7 +3,14 @@ All Git behavior is implemented in `crates/storage`. IronStorage never launches `git`, a credential helper, an SSH client, a hook, a filter, or a merge driver. Repositories are opened with isolated configuration and environment access; -repository-local configuration that could name an executable is rejected. +repository-local configuration that can affect an operation is rejected. +Upstream `pass git init` deliberately writes `diff.gpg.binary` and a +`diff.gpg.textconv` GnuPG command. IronStorage preserves these passive keys for +compatibility but never evaluates them: it reads Git blobs and renders decrypted +diffs through Rust storage APIs. Frontends cannot add or change diff drivers. +This behavior follows the +[upstream password-store initialization](https://git.zx2c4.com/password-store/tree/src/password-store.sh) +while keeping runtime helper execution disabled. `GitRepository` initializes and opens password-store worktrees, selects the innermost repository for a nested entry, and implements status, log, diff, add,