[PM-14988] Use peercred / GetNamedPipeClientProcessId to gather info about process connecting to ssh agent #12065
Conversation
quexten
changed the title
quexten
changed the title
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #12065 +/- ##
==========================================
- Coverage 33.50% 33.50% -0.01%
==========================================
Files 2897 2897
Lines 90349 90352 +3
Branches 17165 17166 +1
==========================================
Hits 30268 30268
- Misses 57690 57693 +3
Partials 2391 2391 ☔ View full report in Codecov by Sentry. |
|
New Issues
Fixed Issues
|
| #[derive(Clone)] | ||
| pub struct BitwardenDesktopAgent { | ||
| keystore: ssh_agent::KeyStore, | ||
| cancellation_token: CancellationToken, | ||
| show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, | ||
| show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, (String, String))>, |
There was a problem hiding this comment.
I would consider creating a struct type for (String, String), as it is it would be quite easy to accidentally swap them.
There was a problem hiding this comment.
Yeah, had that on my list for later, but I'm adding it in this PR now. Nice!
| #[derive(Clone)] | ||
| pub struct BitwardenDesktopAgent { | ||
| keystore: ssh_agent::KeyStore, | ||
| cancellation_token: CancellationToken, | ||
| show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, | ||
| show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, (String, String))>, | ||
| get_ui_response_rx: Arc<Mutex<tokio::sync::broadcast::Receiver<(u32, bool)>>>, | ||
| request_id: Arc<Mutex<u32>>, | ||
| is_running: Arc<tokio::sync::Mutex<bool>>, |
There was a problem hiding this comment.
This isn't related to this PR, but I just noticed a small nit:
Instead of Arc<tokio::sync::Mutex<bool>>, it's usually best to just use AtomicBool. It's clearer in intent, and makes it impossible to accidentally hold the mutex open for too long
There was a problem hiding this comment.
Does the same go for u32? I.e should request_id use atomicu32?
There was a problem hiding this comment.
Yeah, I'd say it applies to request_id as well. I just noticed that BitwardenDesktopAgent implements clone as it's cloned for every connection, so we should still keep the Arc to make sure there's only one instance of the atomics:
request_id: Arc<AtomicU32>,
is_running: Arc<AtomicBool>,We don't need to make the change in this PR if it's too cumbersome though.
| pub fn get_peer_info(peer_pid: u32) -> Result<PeerInfo, String> { | ||
| let s = System::new_all(); | ||
| if let Some(process) = s.process(Pid::from_u32(peer_pid)) { | ||
| let peer_process_name = match process.name().to_str() { |
There was a problem hiding this comment.
The docs have some warnings about the use of name() on Linux: that it's limited to 15 characters and that it might not match the exe name. They recommend using exe() in most cases. Is this a concern for us?
https://docs.rs/sysinfo/latest/sysinfo/struct.Process.html#method.name
There was a problem hiding this comment.
I'll change it to exe since they recommend it.
However the plan on this long-term is not to use the exe/process if possible, but the app name if available. (I.e instead of "chrome.exe" it would be "Google Chrome", and so on + icon). For ssh running in iTerm2, it would show "iTerm2" since that's the "GUI app" the user uses.
(That's roughly what 1Password does [i think?])
Need to figure out how to get these across the platforms we support though, so that's long-term ;)
There was a problem hiding this comment.
Nice, that sounds good, once this is working, it would be nice to also bring it to the browser and DuckDuckGo integration.
One small question: Is this meant as a more informative dialog for the user rather than a secure attestation of what program is connecting to us? By that I mean, will we try to validate that the chrome.exe/Google Chrome binary is the real thing coming from Google as opposed to a random binary with that name? (Using code signatures maybe?)
There was a problem hiding this comment.
One small question: Is this meant as a more informative dialog for the user rather than a secure attestation of what program is connecting to us? By that I mean, will we try to validate that the chrome.exe/Google Chrome binary is the real thing coming from Google as opposed to a random binary with that name? (Using code signatures maybe?)
For now just informational for the user.
However, I'd like for this to ultimately verify more. From what I gathered, 1P verifies signatures on mac/windows.
On linux, this verifies installation path I.e we could say "If something is installed in the system path, for .deb, flatpak, snap, etc", it is most likely legit, whereas something in ~/Downloads is not. Depending on the use-case, these would be blocked, or at least warned (require fingerprint validation).
And agree on the browser/ddg integration, would be really cool to be able to both give better guarantees, but also to show the connected browsers in the desktop UI.
Build a prototype for extended app info here, to prove it works, and to see constraints: #12166
apps/desktop/desktop_native/core/src/ssh_agent/named_pipe_listener_stream.rs
Outdated
Show resolved
Hide resolved
| let message = (request_id, ("".to_string(), true)); | ||
| let message = SshAgentUIRequest { | ||
| request_id, | ||
| cipher_id: "".to_string(), |
There was a problem hiding this comment.
This looks like it would be better represented by an Option<String>, if the value isn't always present
|
@dani-garcia running |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14988
📔 Objective
We want to be able to make choices about what apps or processes to allow access to the ssh-agent to. Further, we want to show the user which app is requesting access to the ssh key.
On the unix socket, the connecting process is collected via peercred, on windows via GetNamedPipeClientProcessId. These are enriched with process data gathered via the sysinfo crate. Finally, the process name is handed out to the UI for showing what app is requesting access.
At the moment this is just the process, but this can be later enhanced, to map this to a desktop UI application, with a proper application name and icon.
Note
This points the russh-agent crate against an in-development PR branch. We should merge this PR only once the upstream is merged, and this PR references a pinned commit of the main branch.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes