From c9a5295e08fd212975a7e0df9202b82ed6c3d746 Mon Sep 17 00:00:00 2001 From: Chetan baliyan Date: Sun, 29 Dec 2024 16:35:46 +0530 Subject: [PATCH] fix: address review feedback and add test for read/write functionality --- crates/sshx-server/src/web/socket.rs | 9 ++- crates/sshx-server/tests/common/mod.rs | 8 ++- crates/sshx-server/tests/snapshot.rs | 4 +- crates/sshx-server/tests/with_client.rs | 79 +++++++++++++++++++++---- crates/sshx/src/controller.rs | 20 ++++--- crates/sshx/src/main.rs | 10 ++-- src/lib/Session.svelte | 29 ++------- 7 files changed, 102 insertions(+), 57 deletions(-) diff --git a/crates/sshx-server/src/web/socket.rs b/crates/sshx-server/src/web/socket.rs index 5928c70..413ae56 100644 --- a/crates/sshx-server/src/web/socket.rs +++ b/crates/sshx-server/src/web/socket.rs @@ -11,7 +11,7 @@ use bytes::Bytes; use futures_util::SinkExt; use sshx_core::proto::{server_update::ServerMessage, NewShell, TerminalInput, TerminalSize}; use sshx_core::Sid; -use subtle::ConstantTimeEq; +use subtle::{Choice, ConstantTimeEq}; use tokio::sync::mpsc; use tokio_stream::StreamExt; use tracing::{error, info_span, warn, Instrument}; @@ -98,9 +98,8 @@ async fn handle_socket(socket: &mut WebSocket, session: Arc) -> Result< let (user_guard, _) = match recv(socket).await? { Some(WsClient::Authenticate(bytes, write_password_bytes)) => { - // `ct_eq` returns a `Choice`, and `unwrap_u8()` converts it to 1 (equal) or 0 - // (not equal). - if bytes.ct_eq(metadata.encrypted_zeros.as_ref()).unwrap_u8() != 1 { + // Constant-time comparison of bytes, converting Choice to bool + if !>::into(bytes.ct_eq(metadata.encrypted_zeros.as_ref())) { send(socket, WsServer::InvalidAuth()).await?; return Ok(()); } @@ -112,7 +111,7 @@ async fn handle_socket(socket: &mut WebSocket, session: Arc) -> Result< // Both password provided and stored, validate they match using constant-time // comparison. (Some(provided_password), Some(stored_password)) => { - if provided_password.ct_eq(stored_password).unwrap_u8() != 1 { + if !>::into(provided_password.ct_eq(stored_password)) { send(socket, WsServer::InvalidAuth()).await?; return Ok(()); } diff --git a/crates/sshx-server/tests/common/mod.rs b/crates/sshx-server/tests/common/mod.rs index a4892d4..594fd3d 100644 --- a/crates/sshx-server/tests/common/mod.rs +++ b/crates/sshx-server/tests/common/mod.rs @@ -82,6 +82,7 @@ impl Drop for TestServer { pub struct ClientSocket { inner: WebSocketStream>, encrypt: Encrypt, + write_encrypt: Option, pub user_id: Uid, pub users: BTreeMap, @@ -93,13 +94,14 @@ pub struct ClientSocket { impl ClientSocket { /// Connect to a WebSocket endpoint. - pub async fn connect(uri: &str, key: &str) -> Result { + pub async fn connect(uri: &str, key: &str, write_password: Option<&str>) -> Result { let (stream, resp) = tokio_tungstenite::connect_async(uri).await?; ensure!(resp.status() == StatusCode::SWITCHING_PROTOCOLS); let mut this = Self { inner: stream, encrypt: Encrypt::new(key), + write_encrypt: write_password.map(Encrypt::new), user_id: Uid(0), users: BTreeMap::new(), shells: BTreeMap::new(), @@ -113,7 +115,9 @@ impl ClientSocket { async fn authenticate(&mut self) { let encrypted_zeros = self.encrypt.zeros().into(); - self.send(WsClient::Authenticate(encrypted_zeros, None)) + let write_zeros = self.write_encrypt.as_ref().map(|e| e.zeros().into()); + + self.send(WsClient::Authenticate(encrypted_zeros, write_zeros)) .await; } diff --git a/crates/sshx-server/tests/snapshot.rs b/crates/sshx-server/tests/snapshot.rs index 0ab727e..dde5f6e 100644 --- a/crates/sshx-server/tests/snapshot.rs +++ b/crates/sshx-server/tests/snapshot.rs @@ -21,7 +21,7 @@ async fn test_basic_restore() -> Result<()> { let key = controller.encryption_key().to_owned(); tokio::spawn(async move { controller.run().await }); - let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key).await?; + let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key, None).await?; s.flush().await; assert_eq!(s.user_id, Uid(1)); @@ -47,7 +47,7 @@ async fn test_basic_restore() -> Result<()> { .state() .insert(&name, Arc::new(Session::restore(&data)?)); - let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key).await?; + let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key, None).await?; s.send(WsClient::Subscribe(Sid(1), 0)).await; s.flush().await; diff --git a/crates/sshx-server/tests/with_client.rs b/crates/sshx-server/tests/with_client.rs index a9ee955..50f13c2 100644 --- a/crates/sshx-server/tests/with_client.rs +++ b/crates/sshx-server/tests/with_client.rs @@ -57,9 +57,11 @@ async fn test_ws_missing() -> Result<()> { let server = TestServer::new().await; let bad_endpoint = format!("ws://{}/not/an/endpoint", server.local_addr()); - assert!(ClientSocket::connect(&bad_endpoint, "").await.is_err()); + assert!(ClientSocket::connect(&bad_endpoint, "", None) + .await + .is_err()); - let mut s = ClientSocket::connect(&server.ws_endpoint("foobar"), "").await?; + let mut s = ClientSocket::connect(&server.ws_endpoint("foobar"), "", None).await?; s.expect_close(4404).await; Ok(()) @@ -74,7 +76,7 @@ async fn test_ws_basic() -> Result<()> { let key = controller.encryption_key().to_owned(); tokio::spawn(async move { controller.run().await }); - let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key).await?; + let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key, None).await?; s.flush().await; assert_eq!(s.user_id, Uid(1)); @@ -106,7 +108,7 @@ async fn test_ws_resize() -> Result<()> { let key = controller.encryption_key().to_owned(); tokio::spawn(async move { controller.run().await }); - let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key).await?; + let mut s = ClientSocket::connect(&server.ws_endpoint(&name), &key, None).await?; s.send(WsClient::Move(Sid(1), None)).await; // error: does not exist yet! s.flush().await; @@ -151,16 +153,16 @@ async fn test_users_join() -> Result<()> { tokio::spawn(async move { controller.run().await }); let endpoint = server.ws_endpoint(&name); - let mut s1 = ClientSocket::connect(&endpoint, &key).await?; + let mut s1 = ClientSocket::connect(&endpoint, &key, None).await?; s1.flush().await; assert_eq!(s1.users.len(), 1); - let mut s2 = ClientSocket::connect(&endpoint, &key).await?; + let mut s2 = ClientSocket::connect(&endpoint, &key, None).await?; s2.flush().await; assert_eq!(s2.users.len(), 2); drop(s2); - let mut s3 = ClientSocket::connect(&endpoint, &key).await?; + let mut s3 = ClientSocket::connect(&endpoint, &key, None).await?; s3.flush().await; assert_eq!(s3.users.len(), 2); @@ -180,7 +182,7 @@ async fn test_users_metadata() -> Result<()> { tokio::spawn(async move { controller.run().await }); let endpoint = server.ws_endpoint(&name); - let mut s = ClientSocket::connect(&endpoint, &key).await?; + let mut s = ClientSocket::connect(&endpoint, &key, None).await?; s.flush().await; assert_eq!(s.users.len(), 1); assert_eq!(s.users.get(&s.user_id).unwrap().cursor, None); @@ -205,8 +207,8 @@ async fn test_chat_messages() -> Result<()> { tokio::spawn(async move { controller.run().await }); let endpoint = server.ws_endpoint(&name); - let mut s1 = ClientSocket::connect(&endpoint, &key).await?; - let mut s2 = ClientSocket::connect(&endpoint, &key).await?; + let mut s1 = ClientSocket::connect(&endpoint, &key, None).await?; + let mut s2 = ClientSocket::connect(&endpoint, &key, None).await?; s1.send(WsClient::SetName("billy".into())).await; s1.send(WsClient::Chat("hello there!".into())).await; @@ -219,10 +221,65 @@ async fn test_chat_messages() -> Result<()> { (s1.user_id, "billy".into(), "hello there!".into()) ); - let mut s3 = ClientSocket::connect(&endpoint, &key).await?; + let mut s3 = ClientSocket::connect(&endpoint, &key, None).await?; s3.flush().await; assert_eq!(s1.messages.len(), 1); assert_eq!(s3.messages.len(), 0); Ok(()) } + +#[tokio::test] +async fn test_read_write_permissions() -> Result<()> { + let server = TestServer::new().await; + + // create controller with read-only mode enabled + let mut controller = Controller::new(&server.endpoint(), "", Runner::Echo, true).await?; + let name = controller.name().to_owned(); + let key = controller.encryption_key().to_owned(); + let write_url = controller + .write_url() + .expect("Should have write URL when enable_readers is true") + .clone(); + + tokio::spawn(async move { controller.run().await }); + + let write_password = write_url + .split(',') + .nth(1) + .expect("Write URL should contain password"); + + // connect with write access + let mut writer = + ClientSocket::connect(&server.ws_endpoint(&name), &key, Some(write_password)).await?; + writer.flush().await; + + // test write permissions + writer.send(WsClient::Create(0, 0)).await; + writer.flush().await; + assert_eq!( + writer.shells.len(), + 1, + "Writer should be able to create a shell" + ); + assert!(writer.errors.is_empty(), "Writer should not receive errors"); + + // connect with read-only access + let mut reader = ClientSocket::connect(&server.ws_endpoint(&name), &key, None).await?; + reader.flush().await; + + // test read-only restrictions + reader.send(WsClient::Create(0, 0)).await; + reader.flush().await; + assert!( + !reader.errors.is_empty(), + "Reader should receive an error when attempting to create shell" + ); + assert_eq!( + reader.shells.len(), + 1, + "Reader should still see the existing shell" + ); + + Ok(()) +} diff --git a/crates/sshx/src/controller.rs b/crates/sshx/src/controller.rs index e2d3894..85e659d 100644 --- a/crates/sshx/src/controller.rs +++ b/crates/sshx/src/controller.rs @@ -55,23 +55,27 @@ impl Controller { ) -> Result { debug!(%origin, "connecting to server"); let encryption_key = rand_alphanumeric(14); // 83.3 bits of entropy - let write_password = rand_alphanumeric(14); // 83.3 bits of entropy let kdf_task = { let encryption_key = encryption_key.clone(); task::spawn_blocking(move || Encrypt::new(&encryption_key)) }; - let kdf_write_password_task = { - let write_password = write_password.clone(); - task::spawn_blocking(move || Encrypt::new(&write_password)) + let (write_password, kdf_write_password_task) = if enable_readers { + let write_password = rand_alphanumeric(14); // 83.3 bits of entropy + let task = { + let write_password = write_password.clone(); + task::spawn_blocking(move || Encrypt::new(&write_password)) + }; + (Some(write_password), Some(task)) + } else { + (None, None) }; let mut client = Self::connect(origin).await?; let encrypt = kdf_task.await?; - let encrypt_write_password = kdf_write_password_task.await?; - let encrypted_write_zeros = if enable_readers { - Some(encrypt_write_password.zeros().into()) + let encrypted_write_zeros = if let Some(task) = kdf_write_password_task { + Some(task.await?.zeros().into()) } else { None }; @@ -86,7 +90,7 @@ impl Controller { let mut resp = client.open(req).await?.into_inner(); resp.url = resp.url + "#" + &encryption_key; - let write_url = if enable_readers { + let write_url = if let (true, Some(write_password)) = (enable_readers, write_password) { Some(resp.url.clone() + "," + &write_password) } else { None diff --git a/crates/sshx/src/main.rs b/crates/sshx/src/main.rs index a169749..34ab6eb 100644 --- a/crates/sshx/src/main.rs +++ b/crates/sshx/src/main.rs @@ -43,10 +43,10 @@ fn print_greeting(shell: &str, controller: &Controller) { r#" {sshx} {version} - {arr} Read-only link: {link_v} - {arr} Writable link: {link_e} - {arr} Shell: {shell_v} - "#, + {arr} Read-only link: {link_v} + {arr} Writable link: {link_e} + {arr} Shell: {shell_v} +"#, sshx = Green.bold().paint("sshx"), version = Green.paint(&version_str), arr = Green.paint("➜"), @@ -63,7 +63,7 @@ fn print_greeting(shell: &str, controller: &Controller) { {arr} Link: {link_v} {arr} Shell: {shell_v} - "#, +"#, sshx = Green.bold().paint("sshx"), version = Green.paint(&version_str), arr = Green.paint("➜"), diff --git a/src/lib/Session.svelte b/src/lib/Session.svelte index a4fac5a..05318d6 100644 --- a/src/lib/Session.svelte +++ b/src/lib/Session.svelte @@ -28,6 +28,7 @@ import { TouchZoom, INITIAL_ZOOM } from "./action/touchZoom"; import { arrangeNewTerminal } from "./arrange"; import { settings } from "./settings"; + import { EyeIcon } from "svelte-feather-icons"; export let id: string; @@ -93,7 +94,6 @@ } let encrypt: Encrypt; - let write_password_encrypt: Encrypt | null = null; let srocket: Srocket | null = null; let connected = false; @@ -135,12 +135,9 @@ encrypt = await Encrypt.new(key); const encryptedZeros = await encrypt.zeros(); - write_password_encrypt = writePassword - ? await Encrypt.new(writePassword) - : null; - const writeEncryptedZeros = write_password_encrypt - ? await write_password_encrypt.zeros() - : null; + const writeEncryptedZeros = writePassword + ? await (await Encrypt.new(writePassword)).zeros() + : null; srocket = new Srocket(`/api/s/${id}`, { onMessage(message) { @@ -473,23 +470,7 @@
- + Read-only
{/if}