From 23bda8ea4febac7939fa6f8efe0e39e2b3737325 Mon Sep 17 00:00:00 2001 From: Zeeshan Ali Khan Date: Thu, 3 Oct 2024 21:46:58 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82=20zb:=20Only=20support=20one=20aut?= =?UTF-8?q?hentication=20method=20at=20a=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that we're down to only two authentication mechanisms with one of them being no-authentication, this really makes sense since we can just autodetect what authentication method to use for a specific socket type on a specific platform. This also simplifies the handshake logic and will allow us to pipeline the whole client-side handshake in the future, when we can drop the xdg-dbus-proxy [workarounds]. As they say in the famous Queen song: Hey! One man, one goal Ha, one mission One heart, one soul Just one solution One flash of light Yeah, one God, one vision One flesh, one bone, one true religion One voice, one hope, one real decision Whoa, whoa, whoa, whoa, whoa, whoa Give me one vision, yeah Fixes #731. [workarounds]: https://github.com/dbus2/zbus/issues/781 --- zbus/src/connection/builder.rs | 14 +++--- zbus/src/connection/handshake/client.rs | 65 ++++++++++++------------- zbus/src/connection/handshake/common.rs | 25 +++------- zbus/src/connection/handshake/mod.rs | 14 +++--- zbus/src/connection/handshake/server.rs | 42 +++++++--------- 5 files changed, 71 insertions(+), 89 deletions(-) diff --git a/zbus/src/connection/builder.rs b/zbus/src/connection/builder.rs index dd81bf100..a20dc1ddd 100644 --- a/zbus/src/connection/builder.rs +++ b/zbus/src/connection/builder.rs @@ -7,7 +7,7 @@ use std::net::TcpStream; #[cfg(all(unix, not(feature = "tokio")))] use std::os::unix::net::UnixStream; use std::{ - collections::{HashMap, HashSet, VecDeque}, + collections::{HashMap, HashSet}, vec, }; #[cfg(feature = "tokio")] @@ -68,7 +68,7 @@ pub struct Builder<'a> { internal_executor: bool, interfaces: Interfaces<'a>, names: HashSet>, - auth_mechanisms: Option>, + auth_mechanism: Option, #[cfg(feature = "bus-impl")] unique_name: Option>, } @@ -190,7 +190,7 @@ impl<'a> Builder<'a> { /// Specify the mechanism to use during authentication. pub fn auth_mechanism(mut self, auth_mechanism: AuthMechanism) -> Self { - self.auth_mechanisms = Some(VecDeque::from(vec![auth_mechanism])); + self.auth_mechanism = Some(auth_mechanism); self } @@ -381,7 +381,7 @@ impl<'a> Builder<'a> { match self.guid { None => { // SASL Handshake - Authenticated::client(stream, server_guid, self.auth_mechanisms, is_bus_conn) + Authenticated::client(stream, server_guid, self.auth_mechanism, is_bus_conn) .await? } Some(guid) => { @@ -402,7 +402,7 @@ impl<'a> Builder<'a> { client_uid, #[cfg(windows)] client_sid, - self.auth_mechanisms, + self.auth_mechanism, unique_name, ) .await? @@ -410,7 +410,7 @@ impl<'a> Builder<'a> { } #[cfg(not(feature = "p2p"))] - Authenticated::client(stream, server_guid, self.auth_mechanisms, is_bus_conn).await? + Authenticated::client(stream, server_guid, self.auth_mechanism, is_bus_conn).await? }; // SAFETY: `Authenticated` is always built with these fields set to `Some`. @@ -468,7 +468,7 @@ impl<'a> Builder<'a> { internal_executor: true, interfaces: HashMap::new(), names: HashSet::new(), - auth_mechanisms: None, + auth_mechanism: None, #[cfg(feature = "bus-impl")] unique_name: None, } diff --git a/zbus/src/connection/handshake/client.rs b/zbus/src/connection/handshake/client.rs index c85010df2..6cf3269a9 100644 --- a/zbus/src/connection/handshake/client.rs +++ b/zbus/src/connection/handshake/client.rs @@ -1,6 +1,5 @@ use async_trait::async_trait; -use std::collections::VecDeque; -use tracing::{debug, instrument, trace, warn}; +use tracing::{instrument, trace, warn}; use crate::{conn::socket::ReadHalf, is_flatpak, names::OwnedUniqueName, Message}; @@ -24,19 +23,14 @@ impl Client { /// Start a handshake on this client socket pub fn new( socket: BoxedSplit, - mechanisms: Option>, + mechanism: Option, server_guid: Option, bus: bool, ) -> Client { - let mechanisms = mechanisms.unwrap_or_else(|| { - let mut mechanisms = VecDeque::new(); - mechanisms.push_back(AuthMechanism::External); - mechanisms.push_back(AuthMechanism::Anonymous); - mechanisms - }); + let mechanism = mechanism.unwrap_or_else(|| socket.read().auth_mechanism()); Client { - common: Common::new(socket, mechanisms), + common: Common::new(socket, mechanism), server_guid, bus, } @@ -84,32 +78,37 @@ impl Client { /// Perform the authentication handshake with the server. #[instrument(skip(self))] async fn authenticate(&mut self) -> Result<()> { - loop { - let mechanism = self.common.next_mechanism()?; - trace!("Trying {mechanism} mechanism"); - let auth_cmd = match mechanism { - AuthMechanism::Anonymous => Command::Auth(Some(mechanism), Some("zbus".into())), - AuthMechanism::External => { - Command::Auth(Some(mechanism), Some(sasl_auth_id()?.into_bytes())) - } - }; - self.common.write_command(auth_cmd).await?; + let mechanism = self.common.mechanism(); + trace!("Trying {mechanism} mechanism"); + let auth_cmd = match mechanism { + AuthMechanism::Anonymous => Command::Auth(Some(mechanism), Some("zbus".into())), + AuthMechanism::External => { + Command::Auth(Some(mechanism), Some(sasl_auth_id()?.into_bytes())) + } + }; + self.common.write_command(auth_cmd).await?; - match self.common.read_command().await? { - Command::Ok(guid) => { - trace!("Received OK from server"); - self.set_guid(guid)?; + match self.common.read_command().await? { + Command::Ok(guid) => { + trace!("Received OK from server"); + self.set_guid(guid)?; - return Ok(()); - } - Command::Rejected(_) => debug!("{mechanism} rejected by the server"), - Command::Error(e) => debug!("Received error from server: {e}"), - cmd => { - return Err(Error::Handshake(format!( - "Unexpected command from server: {cmd}" - ))) - } + Ok(()) + } + Command::Rejected(accepted) => { + let list = accepted + .iter() + .map(|m| m.to_string()) + .collect::>() + .join(", "); + Err(Error::Handshake(format!( + "{mechanism} rejected by the server. Accepted mechanisms: [{list}]" + ))) } + Command::Error(e) => Err(Error::Handshake(format!("Received error from server: {e}"))), + cmd => Err(Error::Handshake(format!( + "Unexpected command from server: {cmd}" + ))), } } diff --git a/zbus/src/connection/handshake/common.rs b/zbus/src/connection/handshake/common.rs index 9c5a4d327..d87f15037 100644 --- a/zbus/src/connection/handshake/common.rs +++ b/zbus/src/connection/handshake/common.rs @@ -1,4 +1,3 @@ -use std::collections::VecDeque; use tracing::{instrument, trace}; use super::{AuthMechanism, BoxedSplit, Command}; @@ -12,21 +11,20 @@ pub(super) struct Common { #[cfg(unix)] received_fds: Vec, cap_unix_fd: bool, - // the current AUTH mechanism is front, ordered by priority - mechanisms: VecDeque, + mechanism: AuthMechanism, first_command: bool, } impl Common { /// Start a handshake on this client socket - pub fn new(socket: BoxedSplit, mechanisms: VecDeque) -> Self { + pub fn new(socket: BoxedSplit, mechanism: AuthMechanism) -> Self { Self { socket, recv_buffer: Vec::new(), #[cfg(unix)] received_fds: Vec::new(), cap_unix_fd: false, - mechanisms, + mechanism, first_command: true, } } @@ -44,9 +42,8 @@ impl Common { self.cap_unix_fd = cap_unix_fd; } - #[cfg(feature = "p2p")] - pub fn mechanisms(&self) -> &VecDeque { - &self.mechanisms + pub fn mechanism(&self) -> AuthMechanism { + self.mechanism } pub fn into_components(self) -> IntoComponentsReturn { @@ -56,7 +53,7 @@ impl Common { #[cfg(unix)] self.received_fds, self.cap_unix_fd, - self.mechanisms, + self.mechanism, ) } @@ -175,12 +172,6 @@ impl Common { Ok(commands) } - - pub fn next_mechanism(&mut self) -> Result { - self.mechanisms - .pop_front() - .ok_or_else(|| Error::Handshake("Exhausted available AUTH mechanisms".into())) - } } #[cfg(unix)] @@ -189,7 +180,7 @@ type IntoComponentsReturn = ( Vec, Vec, bool, - VecDeque, + AuthMechanism, ); #[cfg(not(unix))] -type IntoComponentsReturn = (BoxedSplit, Vec, bool, VecDeque); +type IntoComponentsReturn = (BoxedSplit, Vec, bool, AuthMechanism); diff --git a/zbus/src/connection/handshake/mod.rs b/zbus/src/connection/handshake/mod.rs index 56a665cbd..b54d4d021 100644 --- a/zbus/src/connection/handshake/mod.rs +++ b/zbus/src/connection/handshake/mod.rs @@ -8,7 +8,7 @@ mod server; use async_trait::async_trait; #[cfg(unix)] use nix::unistd::Uid; -use std::{collections::VecDeque, fmt::Debug}; +use std::fmt::Debug; use zbus_names::OwnedUniqueName; #[cfg(windows)] @@ -51,10 +51,10 @@ impl Authenticated { pub async fn client( socket: BoxedSplit, server_guid: Option, - mechanisms: Option>, + mechanism: Option, bus: bool, ) -> Result { - Client::new(socket, mechanisms, server_guid, bus) + Client::new(socket, mechanism, server_guid, bus) .perform() .await } @@ -68,7 +68,7 @@ impl Authenticated { guid: OwnedGuid, #[cfg(unix)] client_uid: Option, #[cfg(windows)] client_sid: Option, - auth_mechanisms: Option>, + auth_mechanism: Option, unique_name: Option, ) -> Result { Server::new( @@ -78,7 +78,7 @@ impl Authenticated { client_uid, #[cfg(windows)] client_sid, - auth_mechanisms, + auth_mechanism, unique_name, )? .perform() @@ -250,7 +250,7 @@ mod tests { p1.into(), Guid::generate().into(), Some(Uid::effective().into()), - Some(vec![AuthMechanism::Anonymous].into()), + Some(AuthMechanism::Anonymous), None, ) .unwrap(); @@ -267,7 +267,7 @@ mod tests { p1.into(), Guid::generate().into(), Some(Uid::effective().into()), - Some(vec![AuthMechanism::Anonymous].into()), + Some(AuthMechanism::Anonymous), None, ) .unwrap(); diff --git a/zbus/src/connection/handshake/server.rs b/zbus/src/connection/handshake/server.rs index 518a556f7..3480311af 100644 --- a/zbus/src/connection/handshake/server.rs +++ b/zbus/src/connection/handshake/server.rs @@ -1,5 +1,4 @@ use async_trait::async_trait; -use std::collections::VecDeque; use tracing::{instrument, trace}; use crate::names::OwnedUniqueName; @@ -41,21 +40,13 @@ impl Server { guid: OwnedGuid, #[cfg(unix)] client_uid: Option, #[cfg(windows)] client_sid: Option, - mechanisms: Option>, + mechanism: Option, unique_name: Option, ) -> Result { - let mechanisms = match mechanisms { - Some(mechanisms) => mechanisms, - None => { - let mut mechanisms = VecDeque::new(); - mechanisms.push_back(AuthMechanism::External); - - mechanisms - } - }; + let mechanism = mechanism.unwrap_or_else(|| socket.read().auth_mechanism()); Ok(Server { - common: Common::new(socket, mechanisms), + common: Common::new(socket, mechanism), step: ServerHandshakeStep::WaitingForAuth, #[cfg(unix)] client_uid, @@ -111,8 +102,7 @@ impl Server { #[instrument(skip(self))] async fn rejected_error(&mut self) -> Result<()> { - let mechanisms = self.common.mechanisms().iter().cloned().collect(); - let cmd = Command::Rejected(mechanisms); + let cmd = Command::Rejected(vec![self.common.mechanism()]); trace!("Sending authentication error"); self.common.write_command(cmd).await?; self.step = ServerHandshakeStep::WaitingForAuth; @@ -141,22 +131,24 @@ impl Server { trace!("Waiting for authentication"); let reply = self.common.read_command().await?; match reply { - Command::Auth(mech, resp) => { - let mech = mech.filter(|m| self.common.mechanisms().contains(m)); + Command::Auth(requested_mech, resp) => { + let mech = self.common.mechanism(); + if requested_mech != Some(mech) { + self.rejected_error().await?; - match (mech, &resp) { - (Some(mech), None) => { + return Ok(()); + } + + match &resp { + None => { trace!("Sending data request"); self.common.write_command(Command::Data(None)).await?; self.step = ServerHandshakeStep::WaitingForData(mech); } - (Some(AuthMechanism::Anonymous), Some(_)) => { - self.auth_ok().await?; - } - (Some(AuthMechanism::External), Some(sasl_id)) => { - self.check_external_auth(sasl_id).await?; - } - _ => self.rejected_error().await?, + Some(sasl_id) => match mech { + AuthMechanism::Anonymous => self.auth_ok().await?, + AuthMechanism::External => self.check_external_auth(sasl_id).await?, + }, } } Command::Cancel | Command::Error(_) => {