From 9c022f7c2267d0d22d053954b781dab0027ee059 Mon Sep 17 00:00:00 2001 From: Josh Junon Date: Wed, 24 Jan 2024 18:07:08 +0100 Subject: [PATCH] add initial trait types for askpass socket servers --- gitbutler-git/Cargo.toml | 2 +- gitbutler-git/src/backend/cli/executor.rs | 161 +++++++++++++++++++++- 2 files changed, 161 insertions(+), 2 deletions(-) diff --git a/gitbutler-git/Cargo.toml b/gitbutler-git/Cargo.toml index 6a2963e7e..b0561c8c7 100644 --- a/gitbutler-git/Cargo.toml +++ b/gitbutler-git/Cargo.toml @@ -35,4 +35,4 @@ rand = { version = "0.8.5", optional = true } tokio = { workspace = true, features = ["rt", "rt-multi-thread", "process"]} [target."cfg(unix)".dependencies] -nix = { version = "0.27.1", optional = true, features = ["process", "socket"] } +nix = { version = "0.27.1", optional = true, features = ["process", "socket", "user"] } diff --git a/gitbutler-git/src/backend/cli/executor.rs b/gitbutler-git/src/backend/cli/executor.rs index d7a2c66b6..983eea3e6 100644 --- a/gitbutler-git/src/backend/cli/executor.rs +++ b/gitbutler-git/src/backend/cli/executor.rs @@ -1,4 +1,5 @@ use crate::prelude::*; +use core::{future::Future, pin::Pin}; #[cfg(any(test, feature = "tokio"))] pub mod tokio; @@ -8,7 +9,34 @@ pub mod tokio; /// There is no `arg0` passed; it's up to the implementation /// to decide how to execute the command. For example, /// `git status` would be passed as `["status"]`. -pub trait GitExecutor { +/// +/// The executor also provides a means for spinning up +/// ad-hoc socket servers, necessary for the authorization +/// utilities that are passed to Git and SSH to communicate +/// with the host process to exchange credentials. We also +/// implement a simple layer of security over this communication +/// layer to avoid unintended leakage of credentials. +/// +/// Note that this security layer is _not_ impervious +/// to determined attackers. It is merely a means to +/// avoid unintended connections to the socket server +/// or simple, generic attacks. The threat model assumes +/// that more sophisticated attacks targeting the host system +/// are out of scope for this project given that the +/// communication layer is not a far cry from the user +/// inputting the credentials manually, directly into the +/// CLI utility. +/// +/// # Safety +/// +/// This trait is marked as unsafe due to the platform-specific +/// invariants described in [`GitExecutor::create_askpass_server`]. +/// These invariants are not enforced by the typesystem, and while +/// we have some loose checks to ensure that the invariants are upheld, +/// we cannot guarantee that they are upheld in all cases. Thus, it is +/// up to the implementor to ensure that the invariants are upheld. +#[allow(unsafe_code)] +pub unsafe trait GitExecutor { /// The error type returned by this executor, /// specifically in cases where the execution fails. /// @@ -16,6 +44,13 @@ pub trait GitExecutor { /// the exit code is non-zero. type Error: core::error::Error + core::fmt::Debug + Send + Sync + 'static; + /// The type of the handle returned by [`GitExecutor::create_askpass_server`]. + type ServerHandle: AskpassServer + Send + Sync + 'static; + + /// The type of socket passed to the connection handler + /// provided to [`GitExecutor::create_askpass_server`]. + type SocketHandle: Socket + Send + Sync + 'static; + /// Executes the given Git command with the given arguments. /// `git` is never passed as the first argument (arg 0). /// @@ -51,4 +86,128 @@ pub trait GitExecutor { self.execute_raw(&args, Some(envs)).await } + + /// Creates a named pipe server that is compatible with + /// the `askpass` utility (see `bin/askpass.rs` and platform-specific + /// adjacent sources). + /// + /// Servers created should be non-blocking and exist in either + /// another thread or an async task; the method should return + /// immediately with a handle to the server, as described below. + /// + /// ## Unix + /// + /// On Unix-like systems (including MacOS), this is a unix + /// domain socket. The path of the socket is returned as + /// a handle type that is format-able as a string which is + /// passed to the askpass utility as `GITBUTLER_ASKPASS_SOCKET`. + /// + /// The socket itself should be created as read/write for the user + /// with no access to group or everyone (`0600` or `u+rw ag-a`). + /// + /// Further, the callback is invoked with the credentials for + /// the socket upon a connection and is awaited. After the callback + /// returns, the connection must be closed. + /// + /// Upon the handle being dropped, the socket must be closed and + /// the socket file SHOULD be best-effort unlinked. + /// + /// Given that this invariant must be upheld, this method is marked + /// as unsafe. + /// + /// ## Windows + /// + /// On Windows, this is a named pipe. The handle returned must be + /// format-able as a string which is passed to the askpass utility + /// as `GITBUTLER_ASKPASS_SOCKET` and corresponds to the named + /// pipe. + /// + /// The pipe name MUST start with `\.\pipe\LOCAL\`. Given that this + /// invariant must be upheld, this method is marked as unsafe. + /// + /// The callback is invoked with the process ID of the client + /// upon a connection, and is awaited. After the callback returns, + /// the connection must be closed. + /// + /// Upon the handle being dropped, the pipe must be closed. + /// + /// # Safety + /// + /// This method is marked as unsafe due to the platform-specific + /// invariants described above that must be upheld by all implementations. + /// These invariants are not enforced by the typesystem, and while + /// we have some loose checks to ensure that the invariants are upheld, + /// we cannot guarantee that they are upheld in all cases. Thus, it is + /// up to the implementor to ensure that the invariants are upheld. + /// + /// If for some reason these invariants are not possible to uphold, + /// please open an issue on the repository to discuss this issue. + unsafe fn create_askpass_server( + &self, + callback: F, + ) -> Result + where + F: Fn( + Self::SocketHandle, + ) -> Pin< + Box< + dyn Future::SocketHandle>> + + Send + + 'static, + >, + > + Send + + 'static; +} + +/// A handle to a server created by [`GitExecutor::create_askpass_server`]. +/// +/// When formatted as a string, the result should be the connection string +/// necessary for the askpass utility to connect (e.g. a unix domain socket path +/// or a windows named pipe name; see [`GitExecutor::create_askpass_server`] for +/// more information). +/// +/// Upon dropping the handle, the server should be closed. +pub trait AskpassServer: Drop + core::fmt::Display {} + +#[cfg(unix)] +type PidInner = ::nix::unistd::Pid; + +#[cfg(windows)] +type PidInner = u32; + +/// The type of a process ID (platforms-specific) +pub type Pid = PidInner; + +/// The type of a user ID (unix-specific). +#[cfg(unix)] +pub type Uid = ::nix::unistd::Uid; + +/// Platform-specific credentials for a connection to a server created by +/// [`GitExecutor::create_askpass_server`]. This is passed to the callback +/// provided to [`GitExecutor::create_askpass_server`] when a connection +/// is established. +pub trait Socket { + /// The error type returned by I/O operations on this socket. + type Error: core::error::Error + core::fmt::Debug + Send + Sync + 'static; + + /// The process ID of the connecting client. + fn pid(&self) -> Pid; + + /// The user ID of the connecting client. + #[cfg(unix)] + fn uid(&self) -> Uid; + + /// Reads a line from the socket. Must not include the newline. + /// + /// The returned line must not include a newline, and any + /// trailing carriage return (`\r`) must be stripped. + async fn read_line(&mut self) -> Result; + + /// Writes a line to the socket. The write must + /// complete fully before returning (i.e. implementations + /// should use something akin to `write_all`). + /// + /// The input line will not include a newline; one must be + /// added. Newlines should never include a carriage return (`\r`). + async fn write_line(&mut self, line: &str) -> Result<(), Self::Error>; }