fix: add gpg --export fallback for keyboxd compatibility#84
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughWhen ChangesCertificate loading
Estimated code review effort: 2 (Simple) | ~8 minutes Sequence Diagram(s)sequenceDiagram
participant load_cert
participant gpg_export as gpg export
participant Cert_parser as Cert parser
load_cert->>load_cert: perform keybox lookup
alt no matches
load_cert->>gpg_export: run --export -a spec
gpg_export-->>load_cert: exit status and stdout
alt successful non-empty stdout
load_cert->>Cert_parser: parse exported bytes
Cert_parser-->>load_cert: return Cert
else failure or empty output
load_cert-->>load_cert: log debug details
load_cert-->>load_cert: return existing error
end
end
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gpg/src/keybox.rs (1)
150-155:⚠️ Potential issue | 🟠 MajorMake the fallback run when legacy keybox loading fails.
load_keybox_certs()?returns before Line 155, so keyboxd-only setups with no readable/non-emptypubring.kbxstill never reach the newgpg --exportfallback.🐛 Proposed fix
tracing::debug!("Searching GnuPG keybox"); - let certs = load_keybox_certs()?; - let matches = find_certs_in_keybox(&certs, spec); + let (matches, keybox_error) = match load_keybox_certs() { + Ok(certs) => (find_certs_in_keybox(&certs, spec), None), + Err(e) => { + tracing::debug!(error = %e, "Keybox lookup failed, trying `gpg --export` fallback"); + (Vec::new(), Some(e)) + } + }; if matches.is_empty() { tracing::debug!(spec = spec, "No matching cert in keybox, trying `gpg --export` fallback"); @@ - bail!( - "No matching certificate found for '{}'. Provide a .asc file path or import the key into your keybox.", - spec - ); + if let Some(e) = keybox_error { + bail!( + "No matching certificate found for '{}'. Provide a .asc file path or import the key into your keybox. Keybox lookup also failed: {:#}", + spec, + e + ); + } + + bail!( + "No matching certificate found for '{}'. Provide a .asc file path or import the key into your keybox.", + spec + ); }Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/gpg/src/keybox.rs` around lines 150 - 155, load_keybox_certs()? can return an Err and cause an early return, preventing the intended `gpg --export` fallback when the keybox is absent or unreadable; instead, call load_keybox_certs() without the `?` and match its Result, treating Err (and empty cert list) as an empty certs Vec so execution continues to find_certs_in_keybox(spec) and then triggers the fallback when matches.is_empty(); apply the same pattern to the other occurrence (the later block around the second find_certs_in_keybox call) so both paths fall back to `gpg --export` rather than returning early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/gpg/src/keybox.rs`:
- Around line 157-164: The match arm that handles gpg export currently trusts
non-empty out.stdout and calls Cert::from_bytes; change this to first check
out.status.success() before attempting to parse and only call Cert::from_bytes
when both out.status.success() is true and out.stdout is non-empty (the
Command::new("gpg") invocation/match and Cert::from_bytes call are the targets).
If the status is not success, log or include out.stderr (and the exit status) in
the tracing::debug/error message so failed gpg exports are visible for
debugging, and fall through to the original error path when export failed or
produced no valid output.
---
Outside diff comments:
In `@crates/gpg/src/keybox.rs`:
- Around line 150-155: load_keybox_certs()? can return an Err and cause an early
return, preventing the intended `gpg --export` fallback when the keybox is
absent or unreadable; instead, call load_keybox_certs() without the `?` and
match its Result, treating Err (and empty cert list) as an empty certs Vec so
execution continues to find_certs_in_keybox(spec) and then triggers the fallback
when matches.is_empty(); apply the same pattern to the other occurrence (the
later block around the second find_certs_in_keybox call) so both paths fall back
to `gpg --export` rather than returning early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9779ec24-fc57-4420-8ccc-80b04b7c9764
📒 Files selected for processing (1)
crates/gpg/src/keybox.rs
|
@avivi55, thanks for your contribution! I will take a deeper look during the week. I don't quite like the GPG binary invocation; the whole point of the project is to avoid relying on it, except for challenge-response in air-gapped environments... In the meantime, I approved CI so people can benefit from the Nix cache for running your branch. |
|
Yeah I know, but it seems that sequoia doesn't support the modern way of storing the GPG keys. |
![vasyl[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Seems like a reasonable change Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1c5dbf1 to
35c9dc8
Compare
2 participants
The modern versions of GnuPG (>= 2.4.1) enable use-keyboxd by default. This migrates public key management away from the legacy pubring.kbx file into an internal SQLite database managed exclusively by the keyboxd background daemon.
Thus, because here we only read the on-disk .kbx file, it fails to find newly imported keys or smartcard certificates that are only tracked by keyboxd.
This PR adds a fallback mechanism to the certificate loading process. If a key isn't found in the legacy keybox, the code now shells out to gpg --export -a to request the certificate directly from GnuPG's IPC/daemon. I know it isn't pretty but I don't know how to do this otherwise.
I don't know if this issue is only on my part, but without this I couldn't make my security key appear in my .kbx file.
Summary by CodeRabbit