feat(get): resolve leased credentials from fnox get#338
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR wires Key changes:
The one remaining concern is that Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant get.rs as fnox get
participant lease.rs as lease.rs
participant Ledger as LeaseLedger
participant Provider as Encryption Provider
participant Backend as Lease Backend API
User->>get.rs: fnox get AWS_ACCESS_KEY_ID
get.rs->>get.rs: produces_env_var() config lookup
Note right of get.rs: Pure in-memory check,<br/>no network I/O
get.rs->>Ledger: lock + load (short lock)
Ledger-->>get.rs: CachedEntry (or None)
Note right of get.rs: Lock released immediately
alt Plaintext cached entry
get.rs-->>User: return credential (fast path, no network)
else Encrypted cached entry
get.rs->>get.rs: consumed_env_vars filter
get.rs->>get.rs: resolve_secrets_batch (needed secrets only)
get.rs->>get.rs: set_secrets_as_env
get.rs->>Provider: find_encryption_provider + decrypt
alt Decryption succeeds
Provider-->>get.rs: decrypted credentials
get.rs-->>User: return credential
else Decryption fails / No cached entry
get.rs->>get.rs: check_prerequisites
get.rs->>lease.rs: resolve_lease(skip_cache=true)
lease.rs->>Backend: backend.create_lease()
Backend-->>lease.rs: Lease result
lease.rs->>Provider: cache_credentials (encrypt)
Provider-->>lease.rs: encrypted creds (or None on failure)
lease.rs->>Ledger: lock + load + record_lease + save (short lock)
lease.rs-->>get.rs: credentials
get.rs-->>User: return credential
end
end
|
There was a problem hiding this comment.
Code Review
This pull request effectively adds support for resolving leased credentials via fnox get, improving code reuse by extracting resolve_lease and enabling new functionality with produced_env_vars in the LeaseBackend trait. However, it introduces serious security risks due to the lack of a trust mechanism for project-local configuration files, which could lead to arbitrary code execution via the Command lease backend or theft of Vault tokens. It is strongly recommended to implement a trust mechanism (e.g., fnox trust) that requires explicit user approval of configuration files. Additionally, I suggest refactoring the produced_env_vars logic to LeaseBackendConfig for better code structure and error handling, and improving documentation around a limitation of this feature with the command backend.
| pub async fn resolve_lease( | ||
| name: &str, | ||
| lease_config: &crate::lease_backends::LeaseBackendConfig, | ||
| config: &Config, | ||
| profile: &str, | ||
| project_dir: &Path, | ||
| ledger: &mut LeaseLedger, | ||
| prereq_missing: Option<&str>, | ||
| ) -> Result<IndexMap<String, String>> { | ||
| let config_hash = lease_config.config_hash(); | ||
| if let Some(cached_lease) = ledger.find_reusable(name, &config_hash) | ||
| && let Some(ref cached_creds) = cached_lease.cached_credentials | ||
| { | ||
| if let Some(ref enc_provider_name) = cached_lease.encryption_provider { | ||
| match find_encryption_provider(config, profile).await { | ||
| EncryptionProviderResult::Available(found_name, provider) | ||
| if found_name == *enc_provider_name => | ||
| { | ||
| match decrypt_credentials(provider.as_ref(), cached_creds).await { | ||
| Ok(decrypted) => { | ||
| tracing::debug!( | ||
| "Reusing cached encrypted lease '{}' for backend '{}'", | ||
| cached_lease.lease_id, | ||
| name | ||
| ); | ||
| return Ok(decrypted); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Failed to decrypt cached lease '{}': {}, creating fresh lease", | ||
| cached_lease.lease_id, | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| _ => { | ||
| tracing::warn!( | ||
| "Encryption provider '{}' not available for cached lease '{}', creating fresh lease", | ||
| enc_provider_name, | ||
| cached_lease.lease_id | ||
| ); | ||
| } | ||
| } | ||
| } else { | ||
| tracing::debug!( | ||
| "Reusing cached plaintext lease '{}' for backend '{}'", | ||
| cached_lease.lease_id, | ||
| name | ||
| ); | ||
| return Ok(cached_creds.clone()); | ||
| } | ||
| } | ||
|
|
||
| if let Some(missing) = prereq_missing { | ||
| return Err(FnoxError::Config(format!( | ||
| "Lease '{}': cached credentials could not be decrypted and \ | ||
| prerequisites are missing: {}\n\ | ||
| Run 'fnox lease create -i {}' to set up credentials interactively.", | ||
| name, missing, name | ||
| ))); | ||
| } | ||
| let backend = lease_config.create_backend()?; | ||
|
|
||
| let duration_str = lease_config.duration().unwrap_or(DEFAULT_LEASE_DURATION); | ||
| let duration = parse_duration(duration_str)?; | ||
|
|
||
| let max_duration = backend.max_lease_duration(); | ||
| if duration > max_duration { | ||
| return Err(FnoxError::Config(format!( | ||
| "Lease duration '{}' for '{}' exceeds maximum {:?}", | ||
| duration_str, name, max_duration | ||
| ))); | ||
| } | ||
|
|
||
| let label = format!("fnox-{}", name); | ||
| let result = create_and_record_lease( | ||
| backend.as_ref(), | ||
| name, | ||
| &label, | ||
| duration, | ||
| config_hash, | ||
| config, | ||
| profile, | ||
| ledger, | ||
| project_dir, | ||
| ) | ||
| .await?; | ||
|
|
||
| tracing::debug!( | ||
| "Created lease '{}' for backend '{}' (expires {:?})", | ||
| result.lease_id, | ||
| name, | ||
| result.expires_at | ||
| ); | ||
|
|
||
| Ok(result.credentials) | ||
| } |
There was a problem hiding this comment.
The resolve_lease function, which has been moved to this file and is now used by both fnox exec and fnox get, facilitates the execution of arbitrary shell commands when the Command lease backend is configured. Since fnox automatically loads and merges configuration files from the current directory tree without any trust mechanism or user confirmation, an attacker can achieve Remote Code Execution (RCE) by placing a malicious .fnox.toml file in a directory. When a user runs fnox exec or fnox get in that directory, the create_command specified in the malicious configuration will be executed.
| fn produced_env_vars(&self) -> Vec<String> { | ||
| self.env_map.values().cloned().collect() | ||
| } |
There was a problem hiding this comment.
The implementation of produced_env_vars for the Vault lease backend enables fnox get to automatically resolve Vault leases when a matching environment variable is requested. If an attacker provides a malicious configuration file with a Vault lease backend pointing to an attacker-controlled server, fnox get will send the user's VAULT_TOKEN in the X-Vault-Token header to that server. This allows an attacker to steal Vault credentials from users who run fnox get in a directory containing a malicious configuration.
| let matching_lease = leases.iter().find(|(_, lease_config)| { | ||
| let backend = match lease_config.create_backend() { | ||
| Ok(b) => b, | ||
| Err(_) => return false, | ||
| }; | ||
| backend.produced_env_vars().contains(&self.key) | ||
| }); |
There was a problem hiding this comment.
The current approach of creating a LeaseBackend instance inside the find closure has a couple of downsides:
- It's potentially inefficient, as
create_backend()might do non-trivial work. - It silently swallows errors from
create_backend(), which can hide configuration issues.
A cleaner approach would be to move the logic for determining produced environment variables from the LeaseBackend trait to the LeaseBackendConfig enum. This would centralize the logic and avoid instantiating a backend just to check its produced keys.
This would involve:
- Adding a
produced_env_vars(&self) -> Vec<String>method to theLeaseBackendConfigenum. - Removing
produced_env_vars()from theLeaseBackendtrait and its implementations. - Updating this
findcall to uselease_config.produced_env_vars(), which would simplify it to a single line and make it more robust:
let matching_lease = leases.iter().find(|(_, lease_config)| {
lease_config.produced_env_vars().contains(&self.key)
});| /// Environment variable keys this backend produces | ||
| fn produced_env_vars(&self) -> Vec<String>; |
There was a problem hiding this comment.
The command backend returns an empty Vec for produced_env_vars, which means fnox get will not be able to resolve credentials from it. This is a reasonable limitation since the keys are dynamic, but it should be clearly documented in the trait definition for produced_env_vars so that future implementers and users are aware of this behavior.
| /// Environment variable keys this backend produces | |
| fn produced_env_vars(&self) -> Vec<String>; | |
| /// Environment variable keys this backend produces. | |
| /// | |
| /// This allows `fnox get` to resolve leased credentials by key. Backends with | |
| /// dynamic keys (like the `command` backend) should return an empty vector, | |
| /// which means `fnox get` will not be able to resolve their credentials. | |
| fn produced_env_vars(&self) -> Vec<String>; |
|
bugbot run |
Previously `fnox get` only resolved provider secrets, ignoring lease backends entirely. If a key like CLOUDFLARE_API_TOKEN was produced by a lease backend, `fnox get` would fall through to the provider lookup and either return the master secret or error. This adds `produced_env_vars()` to the `LeaseBackend` trait so each backend declares which env var keys it produces. `fnox get` now checks lease backends first and resolves the lease (with caching) before falling back to provider secrets. Also extracts `resolve_lease` from `exec.rs` into `lease.rs` so it can be shared between `fnox exec` and `fnox get`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move produced_env_vars() from LeaseBackend trait to LeaseBackendConfig so key matching works without instantiating backends (fixes ordering bug where env vars weren't set before create_backend, and silently swallowed backend construction errors) - Move set_secrets_as_env before the lease find so fnox-managed secrets (e.g. VAULT_ADDR) are available for prerequisite checks - Apply --base64-decode to lease-resolved values (was silently ignored) - Add label_prefix to resolve_lease so exec uses "fnox-exec-<name>" and get uses "fnox-get-<name>" for distinguishable audit logs - Add doc comment noting command backend returns empty produced_env_vars Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the matching_lease lookup before resolve_secrets_batch so non-lease keys skip secret resolution entirely. Since produced_env_vars() is on the config (not the backend), no env vars or network calls are needed for the match check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of resolving all profile secrets, filter to only those matching the backend's required_env_vars (e.g. CLOUDFLARE_API_TOKEN, VAULT_ADDR). Avoids unnecessary provider calls and spurious failures from unrelated unreachable secrets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use rfind() instead of find() to match exec's last-wins semantics when multiple leases produce the same key - Error instead of silently falling through when lease claims to produce a key but returned credentials don't contain it - Document command backend produced_env_vars limitation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
required_env_vars() under-enumerates credential aliases (e.g. FNOX_VAULT_TOKEN vs VAULT_TOKEN, CF_API_TOKEN vs CLOUDFLARE_API_TOKEN), causing silent failures when secrets are stored under alternate names. Resolve all profile secrets like exec does for correctness. Also fix clippy::too_many_arguments on resolve_lease. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ced_env_vars - Add consumed_env_vars() that enumerates all env var names a backend may read at runtime, including aliases (FNOX_VAULT_TOKEN, CF_API_TOKEN, GCP_SERVICE_ACCOUNT_KEY, etc.). Use this to filter which profile secrets to resolve, avoiding unnecessary network calls to unrelated providers while still covering every alias. - Change produced_env_vars() to return Vec<&str> instead of Vec<String> to avoid per-call allocations in the rfind loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rs to backends - Check as_file on profile secret config when returning lease-resolved credentials, matching exec's behavior - Move CONSUMED_ENV_VARS constants into each backend module to centralize env var knowledge per backend - consumed_env_vars() now returns &'static [&'static str] (zero alloc) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sumed vars The AWS SDK respects these env vars to locate non-default credential files. Without them, fnox get with a lease backend would fail to find credentials when a user stores these as profile secrets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
95055b0 to
7ca203e
Compare
Define PRODUCED_ENV_VARS constant in aws_sts.rs rather than hardcoding the env var names in mod.rs, consistent with how CONSUMED_ENV_VARS is already defined per-backend. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…c produces_env_var Reorder lock acquisition in resolve_from_lease so the ledger lock is held before set_secrets_as_env mutates process environment variables. Replace produced_env_vars() (which allocated a Vec per call) with produces_env_var() for zero-allocation key matching in the rfind loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion # Conflicts: # src/lease_backends/aws_sts.rs # src/lease_backends/azure_token.rs # src/lease_backends/command.rs # src/lease_backends/gcp_iam.rs # src/lease_backends/vault.rs
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| let mut temp_env_guard = lease::TempEnvGuard::default(); | ||
| let _temp_files = | ||
| lease::set_secrets_as_env(&resolved_secrets, &needed_secrets, &mut temp_env_guard)?; |
There was a problem hiding this comment.
Consumed env vars filter excludes encryption provider credentials
Medium Severity
The secret injection in resolve_from_lease filters profile secrets to only those matching the lease backend's consumed_env_vars(). However, the code's own comments (lines 152-154) state env vars are injected for two purposes: (1) the encryption provider and (2) the backend SDK. When the encryption provider differs from the lease backend (e.g., a Cloudflare lease with Vault as the encryption provider), credentials like VAULT_TOKEN won't be injected because they aren't in Cloudflare's CONSUMED_ENV_VARS. This causes resolve_cached_entry to fail to decrypt cached credentials, producing a confusing hard error. In contrast, exec.rs injects all resolved secrets, so it works correctly in the same scenario.
Additional Locations (1)
…dle empty consumed set - Add VAULT_NAMESPACE to Vault backend's CONSUMED_ENV_VARS so fnox get injects it when stored as a profile secret - Fall back to unwrap_or_default when consumed_env_vars is empty, since backends using ambient credentials don't need profile secrets and shouldn't fail on transient config read errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| Ensure the encryption provider is reachable or run \ | ||
| 'fnox lease create -i {}' to refresh credentials.", | ||
| name, name | ||
| ))); |
There was a problem hiding this comment.
Encrypted cache failure blocks get but not exec
Medium Severity
When an encrypted cached lease entry exists but decryption fails, resolve_from_lease returns a hard error instead of falling through to create a fresh lease. In contrast, resolve_lease (used by fnox exec) falls through from try_cached_credentials returning None and successfully creates a fresh lease. This means fnox exec succeeds while fnox get for the same key fails when the encryption provider is temporarily unavailable. The comment justifies this as avoiding a "cycle," but create_and_record_lease handles encryption failure gracefully by storing (None, None) for cached credentials, so fresh credentials are still returned successfully.
Additional Locations (1)
…iour Remove the hard error when encrypted cache decryption fails — instead fall through to create a fresh lease, matching how resolve_lease (used by exec) handles the same case. create_and_record_lease handles encryption failure gracefully by storing (None, None). Also document why check_prerequisites is called after set_secrets_as_env (profile secrets are already injected, matching exec.rs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add VAULT_CACERT, VAULT_CAPATH, VAULT_SKIP_VERIFY, VAULT_CLIENT_CERT, VAULT_CLIENT_KEY, and VAULT_TLS_SERVER_NAME so fnox get injects them when stored as profile secrets, matching fnox exec behaviour. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clarify that a missing key in the runtime credential map is a backend contract violation, not a static config error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lter Add LeaseContractViolation error variant for backend contract violations instead of reusing FnoxError::Config. When a cached lease entry uses an encryption provider, include that provider's env_dependencies in the consumed set so credentials like VAULT_TOKEN get resolved and injected before decryption is attempted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…firmed needed When no consumed var is present as a profile secret (e.g. GitHub App with private_key_file, Azure with az CLI auth), a transient secrets-config read failure no longer aborts the command. Also add doc note explaining resolve_cached_entry plaintext branch reachability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Look up the default encryption provider's env_dependencies unconditionally (not gated on cached_entry existence) so that create_and_record_lease can encrypt credentials on a cold start. Previously, encryption provider credentials like VAULT_TOKEN were only injected when an encrypted cached entry already existed, causing a permanent cache-miss cycle. Also simplify get_secrets error handling: use consumed.is_empty() to decide between unwrap_or_default and hard ?, avoiding a double-call pattern that could silently swallow read errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Distinguish Vault/configurable-path backends (where a missing key is likely a server/config mismatch) from static backends (where it may indicate a code bug). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the single resolve_lease call (which held the ledger lock across async decryption and network calls) with inlined logic that acquires short-lived locks only for synchronous ledger reads and writes: 1. TOCTOU cache re-check: sync read under lock, release, async decrypt 2. Lease creation: async network call with no lock held 3. Credential encryption: async, no lock held 4. Ledger write: sync add+save under short lock This prevents concurrent fnox get/exec calls in the same project from serializing on a single lock for the duration of network I/O. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
| } | ||
|
|
||
| self.extract_key_from_creds(name, result.credentials, all_secrets) |
There was a problem hiding this comment.
Duplicated lease creation logic diverges from shared function
Medium Severity
The resolve_from_lease method in get.rs duplicates the lease creation, duration validation, credential encryption, and ledger recording logic that already exists in create_and_record_lease and resolve_lease in lease.rs. The resolve_lease docstring claims it is "Shared between fnox exec and fnox get", but get.rs never calls it. Any future bug fix or behavior change to lease creation logic needs to be applied in both places, creating a maintenance risk and divergence hazard.
Additional Locations (1)
…lication Refactor resolve_lease to manage its own ledger locks with minimal scope instead of requiring callers to hold a lock across async operations: 1. TOCTOU cache check: short lock for sync read, release, async decrypt 2. Lease creation: async network call with no lock held 3. Credential encryption: async, no lock held 4. Ledger write: short lock for sync add+save This removes the duplicated lease creation logic from get.rs (now calls resolve_lease again) and fixes exec.rs which also held a lock across all resolve_lease calls in its loop. Remove now-unused try_cached_credentials. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ve_lease Address three review items: 1. Extract record_lease() helper for the shared add+save+warn-on-error pattern, used by both create_and_record_lease and resolve_lease. Eliminates the duplicated LeaseRecord construction and save logic. 2. Add skip_cache parameter to resolve_lease. When true (used by get.rs), the TOCTOU cache check is skipped since the caller already performed the full cache lookup and decryption attempt with encryption-provider credentials injected. Avoids a redundant network round-trip to the encryption provider on cache-miss. 3. Add comment in exec.rs noting the intentional per-lease lock scope. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
produced_env_vars()to theLeaseBackendtrait so each backend declares which env var keys it producesfnox getnow checks lease backends first — if the requested key matches a lease backend's output, it resolves the lease (with caching) before falling back to provider secretsresolve_leasefromexec.rsintolease.rsas a shared function for bothfnox execandfnox getTest plan
fnox get AWS_ACCESS_KEY_IDreturns the leased credential (not the master secret) when an AWS STS lease is configuredfnox get SOME_PROVIDER_SECRETstill works for non-lease keysfnox execstill resolves leases correctly (regression check)fnox get🤖 Generated with Claude Code
Note
Medium Risk
Changes touch credential leasing, caching/encryption, and environment injection paths used by
fnox exec/get, so regressions could affect secret retrieval or lease reuse behaviour, though the scope is localized and mostly refactoring/extension.Overview
fnox getnow detects when the requested key is produced by a configured lease backend and resolves that lease (reusing cached credentials when possible) before falling back to normal secret/provider resolution, including honoringas_fileoutput.Lease handling is refactored: the lease resolution/caching logic previously embedded in
execis moved intolease.rsas a sharedresolve_leasethat uses short-lived ledger locks (read → unlock → async work → write) to reduce cross-process contention, andfnox getinjects only the backend/encryption-provider consumed secrets into the environment when needed.Backends now declare which env vars they may consume (and AWS STS which it produces), and a new
FnoxError::LeaseContractViolationsurfaces when a backend claims to produce a key but does not return it at runtime; housekeeping updates includehk.pkllinter tweaks.Written by Cursor Bugbot for commit f6736ad. This will update automatically on new commits. Configure here.