Merge remote-tracking branch 'origin/invalid-cert'

9 files changed, 190 insertions, 58 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 76c5e83..ef99d3d 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -23,6 +23,9 @@ Added
  * Server passwords. Thanks @rbran!
  * Added support for sending and receiving text messages.
  * See a list of occured events with +mumctl events+.
+ * Invalid server certificates are now rejected by default and need to be
+   explicitly allowed either when connecting or permanently per server or
+   globally.
 
 Changed
 ~~~~~~~
diff --git a/documentation/mumdrc.5 b/documentation/mumdrc.5
index ac0a2f5..00e8265 100644
--- a/documentation/mumdrc.5
+++ b/documentation/mumdrc.5
@@ -2,12 +2,12 @@
 .\"     Title: mumdrc
 .\"    Author: [see the "AUTHOR(S)" section]
 .\" Generator: Asciidoctor 2.0.15
-.\"      Date: 2021-04-10
+.\"      Date: 2021-06-08
 .\"    Manual: \ \&
 .\"    Source: \ \&
 .\"  Language: English
 .\"
-.TH "MUMDRC" "5" "2021-04-10" "\ \&" "\ \&"
+.TH "MUMDRC" "5" "2021-06-08" "\ \&" "\ \&"
 .ie \n(.g .ds Aq \(aq
 .el       .ds Aq '
 .ss \n[.ss] 0
@@ -36,6 +36,12 @@ using mumctl(1).
 .sp
 The following configuration values are supported:
 .sp
+accept_all_invalid_certs
+.RS 4
+Whether to connect to a server that supplies an invalid server certificate.
+This is overriden by server\-specific settings. Default false.
+.RE
+.sp
 audio.input_volume
 .RS 4
 Default 1.0.
@@ -73,6 +79,13 @@ password
 .RS 4
 The password to supply to the server. (Optional)
 .RE
+.sp
+accept_invalid_cert
+.RS 4
+Whether to connect to this server even if it supplies an invalid server
+certificate. This overrides the global accept_all_invalid_certs if set.
+Default false.
+.RE
 .SH "AUTHORS"
 .sp
 Gustav Sörnäs and Eskil Queseth.
diff --git a/documentation/mumdrc.txt b/documentation/mumdrc.txt
index ed54b87..0579581 100644
--- a/documentation/mumdrc.txt
+++ b/documentation/mumdrc.txt
@@ -14,6 +14,10 @@ using mumctl(1).
 
 The following configuration values are supported:
 
+accept_all_invalid_certs ::
+    Whether to connect to a server that supplies an invalid server certificate.
+    This is overriden by server-specific settings. Default false.
+
 audio.input_volume ::
     Default 1.0.
 
@@ -38,6 +42,11 @@ username ::
 password ::
     The password to supply to the server. (Optional)
 
+accept_invalid_cert ::
+    Whether to connect to this server even if it supplies an invalid server
+    certificate. This overrides the global accept_all_invalid_certs if set.
+    Default false.
+
 Authors
 -------
 
diff --git a/mumctl/src/main.rs b/mumctl/src/main.rs
index 1fb2a45..5b2bc05 100644
--- a/mumctl/src/main.rs
+++ b/mumctl/src/main.rs
@@ -55,6 +55,8 @@ enum Command {
         password: Option<String>,
         #[structopt(short = "p", long = "port")]
         port: Option<u16>,
+        #[structopt(long = "accept-invalid-cert")]
+        accept_invalid_cert: bool,
     },
     /// Disconnect from the currently connected server
     Disconnect,
@@ -238,10 +240,12 @@ fn match_opt() -> Result<(), Error> {
             username,
             password,
             port,
+            accept_invalid_cert: cli_accept_invalid_cert,
         } => {
             let port = port.unwrap_or(mumlib::DEFAULT_PORT);
 
-            let (host, username, password, port) =
+
+            let (host, username, password, port, server_accept_invalid_cert) =
                 match config.servers.iter().find(|e| e.name == host) {
                     Some(server) => (
                         &server.host,
@@ -252,27 +256,46 @@ fn match_opt() -> Result<(), Error> {
                             .ok_or(CliError::NoUsername)?,
                         server.password.as_ref().or(password.as_ref()),
                         server.port.unwrap_or(port),
+                        server.accept_invalid_cert,
                     ),
                     None => (
                         &host,
                         username.as_ref().ok_or(CliError::NoUsername)?,
                         password.as_ref(),
                         port,
+                        None,
                     ),
                 };
 
+            let config_accept_invalid_cert = server_accept_invalid_cert
+                .or(config.allow_invalid_server_cert);
+            let specified_accept_invalid_cert = cli_accept_invalid_cert || config_accept_invalid_cert.is_some();
+
             let response = send_command(MumCommand::ServerConnect {
                 host: host.to_string(),
                 port,
                 username: username.to_string(),
                 password: password.map(|x| x.to_string()),
-                accept_invalid_cert: true, //TODO
-            })??;
-            if let Some(CommandResponse::ServerConnect { welcome_message, server_state }) = response {
-                parse_state(&server_state);
-                if let Some(message) = welcome_message {
-                    println!("\nWelcome: {}", message);
+                accept_invalid_cert: cli_accept_invalid_cert || config_accept_invalid_cert.unwrap_or(false),
+            })?;
+            match response {
+                Ok(Some(CommandResponse::ServerConnect { welcome_message , server_state })) => {
+                    parse_state(&server_state);
+                    if let Some(message) = welcome_message {
+                        println!("\nWelcome: {}", message);
+                    }
                 }
+                Err(mumlib::error::Error::ServerCertReject) => {
+                    error!("Connection rejected since the server supplied an invalid certificate.");
+                    if !specified_accept_invalid_cert {
+                        eprintln!("help: If you trust this server anyway, you can do any of the following to connect:");
+                        eprintln!("  1. Temporarily trust this server by passing --accept-invalid-cert when connecting.");
+                        eprintln!("  2. Permanently trust this server by setting accept_invalid_cert=true in the server's config.");
+                        eprintln!("  3. Permantently trust all invalid certificates by setting accept_all_invalid_certs=true globally");
+                    }
+                }
+                Ok(other) => unreachable!("Response should only be a ServerConnect or ServerCertReject. Got {:?}", other),
+                Err(e) => return Err(e.into()),
             }
         }
         Command::Disconnect => {
@@ -318,6 +341,11 @@ fn match_opt() -> Result<(), Error> {
                     config.audio.output_volume = Some(volume);
                 }
             }
+            "accept_all_invalid_certs" => {
+                if let Ok(b) = value.parse() {
+                    config.allow_invalid_server_cert = Some(b);
+                }
+            }
             _ => {
                 return Err(CliError::ConfigKeyNotFound(key).into());
             }
@@ -462,7 +490,7 @@ fn match_server_command(server_command: Server, config: &mut Config) -> Result<(
             match (key.as_deref(), value) {
                 (None, _) => {
                     print!(
-                        "{}{}{}{}",
+                        "{}{}{}{}{}",
                         format!("host: {}\n", server.host.to_string()),
                         server
                             .port
@@ -478,6 +506,10 @@ fn match_server_command(server_command: Server, config: &mut Config) -> Result<(
                             .as_ref()
                             .map(|s| format!("password: {}\n", s))
                             .unwrap_or_else(|| "".to_string()),
+                        server
+                            .accept_invalid_cert
+                            .map(|b| format!("accept_invalid_cert: {}\n", if b { "true" } else { "false" }))
+                            .unwrap_or_else(|| "".to_string()),
                     );
                 }
                 (Some("name"), None) => {
@@ -510,6 +542,15 @@ fn match_server_command(server_command: Server, config: &mut Config) -> Result<(
                             .ok_or(CliError::NotSet("password".to_string()))?
                     );
                 }
+                (Some("accept_invalid_cert"), None) => {
+                    println!(
+                        "{}",
+                        server
+                            .accept_invalid_cert
+                            .map(|b| b.to_string())
+                            .ok_or(CliError::NotSet("accept_invalid_cert".to_string()))?
+                    );
+                }
                 (Some("name"), Some(_)) => {
                     return Err(CliError::UseServerRename)?;
                 }
@@ -526,6 +567,12 @@ fn match_server_command(server_command: Server, config: &mut Config) -> Result<(
                     server.password = Some(value);
                     //TODO ask stdin if empty
                 }
+                (Some("accept_invalid_cert"), Some(value)) => {
+                    match value.parse() {
+                        Ok(b) => server.accept_invalid_cert = Some(b),
+                        Err(e) => warn!("{}", e)
+                    }
+                }
                 (Some(_), _) => {
                     return Err(CliError::ConfigKeyNotFound(key.unwrap()))?;
                 }
@@ -555,6 +602,7 @@ fn match_server_command(server_command: Server, config: &mut Config) -> Result<(
                     port,
                     username,
                     password,
+                    accept_invalid_cert: None,
                 });
             }
         }
diff --git a/mumd/src/command.rs b/mumd/src/command.rs
index 410751a..2069178 100644
--- a/mumd/src/command.rs
+++ b/mumd/src/command.rs
@@ -4,10 +4,7 @@ use crate::state::{ExecutionContext, State};
 use log::*;
 use mumble_protocol::{control::ControlPacket, Serverbound};
 use mumlib::command::{Command, CommandResponse};
-use std::sync::{
-    atomic::{AtomicU64, Ordering},
-    Arc, RwLock,
-};
+use std::{rc::Rc, sync::{Arc, RwLock, atomic::{AtomicBool, AtomicU64, Ordering}}};
 use tokio::sync::{mpsc, watch};
 
 pub async fn handle(
@@ -32,16 +29,25 @@ pub async fn handle(
             &mut connection_info_sender,
         );
         match event {
-            ExecutionContext::TcpEventCallback(event, generator) => {
-                tcp_event_queue.register_callback(
-                    event,
-                    Box::new(move |e| {
-                        let response = generator(e);
-                        for response in response {
-                            response_sender.send(response).unwrap();
-                        }
-                    }),
-                );
+            ExecutionContext::TcpEventCallback(callbacks) => {
+                // A shared bool ensures that only one of the supplied callbacks is run.
+                let should_handle = Rc::new(AtomicBool::new(true));
+                for (event, generator) in callbacks {
+                    let should_handle = Rc::clone(&should_handle);
+                    let response_sender = response_sender.clone();
+                    tcp_event_queue.register_callback(
+                        event,
+                        Box::new(move |e| {
+                            // If should_handle == true no other callback has been run yet.
+                            if should_handle.swap(false, Ordering::Relaxed) {
+                                let response = generator(e);
+                                for response in response {
+                                    response_sender.send(response).unwrap();
+                                }
+                            }
+                        }),
+                    );
+                }
             }
             ExecutionContext::TcpEventSubscriber(event, mut handler) => tcp_event_queue
                 .register_subscriber(
diff --git a/mumd/src/network/tcp.rs b/mumd/src/network/tcp.rs
index 4a753bf..f620a32 100644
--- a/mumd/src/network/tcp.rs
+++ b/mumd/src/network/tcp.rs
@@ -1,14 +1,12 @@
+use crate::error::{ServerSendError, TcpError};
 use crate::network::ConnectionInfo;
+use crate::notifications;
 use crate::state::{State, StatePhase};
-use crate::{
-    error::{ServerSendError, TcpError},
-    notifications,
-};
-use log::*;
 
 use futures_util::select;
 use futures_util::stream::{SplitSink, SplitStream, Stream};
 use futures_util::{FutureExt, SinkExt, StreamExt};
+use log::*;
 use mumble_protocol::control::{msgs, ClientControlCodec, ControlCodec, ControlPacket};
 use mumble_protocol::crypt::ClientCryptState;
 use mumble_protocol::voice::VoicePacket;
@@ -36,17 +34,33 @@ type TcpReceiver =
 pub(crate) type TcpEventCallback = Box<dyn FnOnce(TcpEventData)>;
 pub(crate) type TcpEventSubscriber = Box<dyn FnMut(TcpEventData) -> bool>; //the bool indicates if it should be kept or not
 
-#[derive(Debug, Clone, Hash, Eq, PartialEq)]
+/// Why the TCP was disconnected.
+#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)]
+pub enum DisconnectedReason {
+    InvalidTls,
+    User,
+    TcpError,
+}
+
+/// Something a callback can register to. Data is sent via a respective [TcpEventData].
+#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)]
 pub enum TcpEvent {
     Connected,    //fires when the client has connected to a server
-    Disconnected, //fires when the client has disconnected from a server
+    Disconnected(DisconnectedReason), //fires when the client has disconnected from a server
     TextMessage,  //fires when a text message comes in
 }
 
+/// When a [TcpEvent] occurs, this contains the data for the event.
+/// 
+/// The events are picked up by a [crate::state::ExecutionContext].
+/// 
+/// Having two different types might feel a bit confusing. Essentially, a
+/// callback _registers_ to a [TcpEvent] but _takes_ a [TcpEventData] as
+/// parameter.
 #[derive(Clone)]
 pub enum TcpEventData<'a> {
     Connected(Result<&'a msgs::ServerSync, mumlib::Error>),
-    Disconnected,
+    Disconnected(DisconnectedReason),
     TextMessage(&'a msgs::TextMessage),
 }
 
@@ -54,7 +68,7 @@ impl<'a> From<&TcpEventData<'a>> for TcpEvent {
     fn from(t: &TcpEventData) -> Self {
         match t {
             TcpEventData::Connected(_) => TcpEvent::Connected,
-            TcpEventData::Disconnected => TcpEvent::Disconnected,
+            TcpEventData::Disconnected(reason) => TcpEvent::Disconnected(*reason),
             TcpEventData::TextMessage(_) => TcpEvent::TextMessage,
         }
     }
@@ -142,12 +156,25 @@ pub async fn handle(
             }
             return Err(TcpError::NoConnectionInfoReceived);
         };
-        let (mut sink, stream) = connect(
+        let connect_result = connect(
             connection_info.socket_addr,
             connection_info.hostname,
             connection_info.accept_invalid_cert,
         )
-        .await?;
+        .await;
+        
+        let (mut sink, stream) = match connect_result {
+            Ok(ok) => ok,
+            Err(TcpError::TlsConnectError(_)) => {
+                warn!("Invalid TLS");
+                state.read().unwrap().broadcast_phase(StatePhase::Disconnected);
+                event_queue.resolve(TcpEventData::Disconnected(DisconnectedReason::InvalidTls));
+                continue;
+            }
+            Err(e) => {
+                return Err(e);
+            }
+        };
 
         // Handshake (omitting `Version` message for brevity)
         let (username, password) = {
@@ -170,7 +197,7 @@ pub async fn handle(
 
         let phase_watcher_inner = phase_watcher.clone();
 
-        run_until(
+        let result = run_until(
             |phase| matches!(phase, StatePhase::Disconnected),
             async {
                 select! {
@@ -192,9 +219,12 @@ pub async fn handle(
             phase_watcher,
         )
         .await
-        .unwrap_or(Ok(()))?;
+        .unwrap_or(Ok(()));
 
-        event_queue.resolve(TcpEventData::Disconnected);
+        match result {
+            Ok(()) => event_queue.resolve(TcpEventData::Disconnected(DisconnectedReason::User)),
+            Err(_) => event_queue.resolve(TcpEventData::Disconnected(DisconnectedReason::TcpError)),
+        }
 
         debug!("Fully disconnected TCP stream, waiting for new connection info");
     }
diff --git a/mumd/src/state.rs b/mumd/src/state.rs
index d12b5b6..d2d77b1 100644
--- a/mumd/src/state.rs
+++ b/mumd/src/state.rs
@@ -2,7 +2,7 @@ pub mod channel;
 pub mod server;
 pub mod user;
 
-use crate::audio::{AudioInput, AudioOutput, NotificationEvents};
+use crate::{audio::{AudioInput, AudioOutput, NotificationEvents}, network::tcp::DisconnectedReason};
 use crate::error::StateError;
 use crate::network::tcp::{TcpEvent, TcpEventData};
 use crate::network::{ConnectionInfo, VoiceStreamType};
@@ -27,8 +27,10 @@ use std::{
 use tokio::sync::{mpsc, watch};
 
 macro_rules! at {
-    ($event:expr, $generator:expr) => {
-        ExecutionContext::TcpEventCallback($event, Box::new($generator))
+    ( $( $event:expr => $generator:expr ),+ $(,)? ) => {
+        ExecutionContext::TcpEventCallback(vec![
+            $( ($event, Box::new($generator)), )*
+        ])
     };
 }
 
@@ -42,7 +44,7 @@ type Responses = Box<dyn Iterator<Item = mumlib::error::Result<Option<CommandRes
 
 //TODO give me a better name
 pub enum ExecutionContext {
-    TcpEventCallback(TcpEvent, Box<dyn FnOnce(TcpEventData) -> Responses>),
+    TcpEventCallback(Vec<(TcpEvent, Box<dyn FnOnce(TcpEventData) -> Responses>)>),
     TcpEventSubscriber(
         TcpEvent,
         Box<
@@ -606,23 +608,28 @@ pub fn handle_command(
                 )))
                 .unwrap();
             let state = Arc::clone(&og_state);
-            at!(TcpEvent::Connected, move |res| {
-                //runs the closure when the client is connected
-                if let TcpEventData::Connected(res) = res {
-                    Box::new(iter::once(res.map(|msg| {
-                        Some(CommandResponse::ServerConnect {
-                            welcome_message: if msg.has_welcome_text() {
-                                Some(msg.get_welcome_text().to_string())
-                            } else {
-                                None
-                            },
-                            server_state: state.read().unwrap().server.as_ref().unwrap().into(),
-                        })
-                    })))
-                } else {
-                    unreachable!("callback should be provided with a TcpEventData::Connected");
+            at!(
+                TcpEvent::Connected => move |res| {
+                    //runs the closure when the client is connected
+                    if let TcpEventData::Connected(res) = res {
+                        Box::new(iter::once(res.map(|msg| {
+                            Some(CommandResponse::ServerConnect {
+                                welcome_message: if msg.has_welcome_text() {
+                                    Some(msg.get_welcome_text().to_string())
+                                } else {
+                                    None
+                                },
+                                server_state: state.read().unwrap().server.as_ref().unwrap().into(),
+                            })
+                        })))
+                    } else {
+                        unreachable!("callback should be provided with a TcpEventData::Connected");
+                    }
+                },
+                TcpEvent::Disconnected(DisconnectedReason::InvalidTls) => |_| {
+                    Box::new(iter::once(Err(Error::ServerCertReject)))
                 }
-            })
+            )
         }
         Command::ServerDisconnect => {
             if !matches!(*state.phase_receiver().borrow(), StatePhase::Connected(_)) {
diff --git a/mumlib/src/config.rs b/mumlib/src/config.rs
index 1bd3784..3edef37 100644
--- a/mumlib/src/config.rs
+++ b/mumlib/src/config.rs
@@ -10,8 +10,15 @@ use std::path::{Path, PathBuf};
 use toml::value::Array;
 use toml::Value;
 
+/// A TOML-friendly version of [Config].
+///
+/// Values need to be placed before tables due to how TOML works.
 #[derive(Debug, Deserialize, Serialize)]
 struct TOMLConfig {
+    // Values
+    accept_all_invalid_certs: Option<bool>,
+
+    // Tables
     audio: Option<AudioConfig>,
     servers: Option<Array>,
 }
@@ -20,6 +27,10 @@ struct TOMLConfig {
 pub struct Config {
     pub audio: AudioConfig,
     pub servers: Vec<ServerConfig>,
+    /// Whether we allow connecting to servers with invalid server certificates.
+    ///
+    /// None implies false but we can show a better message to the user.
+    pub allow_invalid_server_cert: Option<bool>,
 }
 
 impl Config {
@@ -64,6 +75,7 @@ pub struct ServerConfig {
     pub port: Option<u16>,
     pub username: Option<String>,
     pub password: Option<String>,
+    pub accept_invalid_cert: Option<bool>,
 }
 
 impl ServerConfig {
@@ -105,6 +117,7 @@ impl TryFrom<TOMLConfig> for Config {
                 })
                 .transpose()?
                 .unwrap_or_default(),
+            allow_invalid_server_cert: config.accept_all_invalid_certs,
         })
     }
 }
@@ -125,6 +138,7 @@ impl From<Config> for TOMLConfig {
                     .map(|s| Value::try_from::<ServerConfig>(s).unwrap())
                     .collect(),
             ),
+            accept_all_invalid_certs: config.allow_invalid_server_cert,
         }
     }
 }
diff --git a/mumlib/src/error.rs b/mumlib/src/error.rs
index 2cb3927..30b61ee 100644
--- a/mumlib/src/error.rs
+++ b/mumlib/src/error.rs
@@ -13,6 +13,7 @@ pub enum Error {
     InvalidServerPassword,
     Unimplemented,
     NotConnectedToChannel,
+    ServerCertReject,
 }
 
 impl std::error::Error for Error {}
@@ -30,6 +31,7 @@ impl fmt::Display for Error {
             Error::InvalidServerPassword => write!(f, "Invalid server password"),
             Error::Unimplemented => write!(f, "Unimplemented"),
             Error::NotConnectedToChannel => write!(f, "Not connected to a channel"),
+            Error::ServerCertReject => write!(f, "Invalid server certificate"),
         }
     }
 }