From 850c38784a4eb8d98495ec2e93e7144d4f0d2c16 Mon Sep 17 00:00:00 2001 From: Drew Newberry Date: Sun, 15 Mar 2026 14:10:39 -0700 Subject: [PATCH] fix(core): harden file permissions for user config directory Sensitive files in the OpenShell config directory (~/.config/openshell/) were created with default umask permissions, making them world-readable. This is a security concern for mTLS private keys (tls.key) and auth tokens. - Add centralized paths module to openshell-core with permission helpers (create_dir_restricted 0o700, set_file_owner_only 0o600) - Set 0o600 on tls.key in mtls.rs (was inheriting umask, typically 0o644) - Set 0o700 on all config directories (mtls/, gateways/, forwards/, etc.) - Migrate legacy cf_token files to new edge_token path with proper perms - Deduplicate xdg_config_dir() from 6 locations into openshell-core --- Cargo.lock | 2 + crates/openshell-bootstrap/Cargo.toml | 1 + crates/openshell-bootstrap/src/edge_token.rs | 46 +++-- crates/openshell-bootstrap/src/metadata.rs | 19 +-- crates/openshell-bootstrap/src/mtls.rs | 15 +- crates/openshell-bootstrap/src/paths.rs | 13 +- crates/openshell-cli/src/ssh.rs | 16 +- crates/openshell-cli/src/tls.rs | 8 +- crates/openshell-core/Cargo.toml | 3 + crates/openshell-core/src/forward.rs | 15 +- crates/openshell-core/src/lib.rs | 1 + crates/openshell-core/src/paths.rs | 166 +++++++++++++++++++ crates/openshell-tui/src/lib.rs | 5 +- 13 files changed, 227 insertions(+), 83 deletions(-) create mode 100644 crates/openshell-core/src/paths.rs diff --git a/Cargo.lock b/Cargo.lock index 3978e183..f4bc2e4d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2810,6 +2810,7 @@ dependencies = [ "bytes", "futures", "miette", + "openshell-core", "rcgen", "serde", "serde_json", @@ -2874,6 +2875,7 @@ dependencies = [ "protobuf-src", "serde", "serde_json", + "tempfile", "thiserror 2.0.18", "tonic", "tonic-build", diff --git a/crates/openshell-bootstrap/Cargo.toml b/crates/openshell-bootstrap/Cargo.toml index c090a7f0..ab57ad57 100644 --- a/crates/openshell-bootstrap/Cargo.toml +++ b/crates/openshell-bootstrap/Cargo.toml @@ -10,6 +10,7 @@ repository.workspace = true rust-version.workspace = true [dependencies] +openshell-core = { path = "../openshell-core" } base64 = "0.22" bollard = { version = "0.20", features = ["ssh"] } bytes = { workspace = true } diff --git a/crates/openshell-bootstrap/src/edge_token.rs b/crates/openshell-bootstrap/src/edge_token.rs index 12b294d3..7b1892d1 100644 --- a/crates/openshell-bootstrap/src/edge_token.rs +++ b/crates/openshell-bootstrap/src/edge_token.rs @@ -9,6 +9,7 @@ use crate::paths::gateways_dir; use miette::{IntoDiagnostic, Result, WrapErr}; +use openshell_core::paths::{ensure_parent_dir_restricted, set_file_owner_only}; use std::path::PathBuf; /// Path to the stored edge auth token for a gateway. @@ -24,22 +25,12 @@ fn legacy_token_path(gateway_name: &str) -> Result { /// Store an edge authentication token for a gateway. pub fn store_edge_token(gateway_name: &str, token: &str) -> Result<()> { let path = edge_token_path(gateway_name)?; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .into_diagnostic() - .wrap_err_with(|| format!("failed to create {}", parent.display()))?; - } + ensure_parent_dir_restricted(&path)?; std::fs::write(&path, token) .into_diagnostic() .wrap_err_with(|| format!("failed to write edge token to {}", path.display()))?; // Restrict permissions to owner-only (0600). - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .into_diagnostic() - .wrap_err("failed to set token file permissions")?; - } + set_file_owner_only(&path)?; Ok(()) } @@ -47,15 +38,34 @@ pub fn store_edge_token(gateway_name: &str, token: &str) -> Result<()> { /// /// Returns `None` if no token file exists or the file is empty. /// Falls back to the legacy `cf_token` path for backwards compatibility. +/// When loading from the legacy path, migrates the token to the new path +/// with proper permissions. pub fn load_edge_token(gateway_name: &str) -> Option { - // Try the new path first, then fall back to legacy. - let path = edge_token_path(gateway_name) + // Try the new path first. + if let Some(path) = edge_token_path(gateway_name).ok().filter(|p| p.exists()) { + let contents = std::fs::read_to_string(&path).ok()?; + let token = contents.trim().to_string(); + if !token.is_empty() { + return Some(token); + } + } + + // Fall back to the legacy cf_token path. + let legacy_path = legacy_token_path(gateway_name) .ok() - .filter(|p| p.exists()) - .or_else(|| legacy_token_path(gateway_name).ok().filter(|p| p.exists()))?; - let contents = std::fs::read_to_string(&path).ok()?; + .filter(|p| p.exists())?; + let contents = std::fs::read_to_string(&legacy_path).ok()?; let token = contents.trim().to_string(); - if token.is_empty() { None } else { Some(token) } + if token.is_empty() { + return None; + } + + // Migrate: write to new path with proper permissions, then remove legacy. + if store_edge_token(gateway_name, &token).is_ok() { + let _ = std::fs::remove_file(&legacy_path); + } + + Some(token) } /// Remove a stored edge authentication token. diff --git a/crates/openshell-bootstrap/src/metadata.rs b/crates/openshell-bootstrap/src/metadata.rs index 12e1b98a..bd49ba8c 100644 --- a/crates/openshell-bootstrap/src/metadata.rs +++ b/crates/openshell-bootstrap/src/metadata.rs @@ -4,6 +4,7 @@ use crate::RemoteOptions; use crate::paths::{active_gateway_path, gateways_dir, last_sandbox_path}; use miette::{IntoDiagnostic, Result, WrapErr}; +use openshell_core::paths::ensure_parent_dir_restricted; use serde::{Deserialize, Serialize}; use std::path::PathBuf; @@ -205,11 +206,7 @@ pub fn resolve_ssh_hostname(host: &str) -> String { pub fn store_gateway_metadata(name: &str, metadata: &GatewayMetadata) -> Result<()> { let path = stored_metadata_path(name)?; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .into_diagnostic() - .wrap_err_with(|| format!("failed to create {}", parent.display()))?; - } + ensure_parent_dir_restricted(&path)?; let contents = serde_json::to_string_pretty(metadata) .into_diagnostic() .wrap_err("failed to serialize gateway metadata")?; @@ -237,11 +234,7 @@ pub fn get_gateway_metadata(name: &str) -> Option { /// Save the active gateway name to persistent storage. pub fn save_active_gateway(name: &str) -> Result<()> { let path = active_gateway_path()?; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .into_diagnostic() - .wrap_err_with(|| format!("failed to create {}", parent.display()))?; - } + ensure_parent_dir_restricted(&path)?; std::fs::write(&path, name) .into_diagnostic() .wrap_err_with(|| format!("failed to write active gateway to {}", path.display()))?; @@ -261,11 +254,7 @@ pub fn load_active_gateway() -> Option { /// Save the last-used sandbox name for a gateway to persistent storage. pub fn save_last_sandbox(gateway: &str, sandbox: &str) -> Result<()> { let path = last_sandbox_path(gateway)?; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .into_diagnostic() - .wrap_err_with(|| format!("failed to create {}", parent.display()))?; - } + ensure_parent_dir_restricted(&path)?; std::fs::write(&path, sandbox) .into_diagnostic() .wrap_err_with(|| format!("failed to write last sandbox to {}", path.display()))?; diff --git a/crates/openshell-bootstrap/src/mtls.rs b/crates/openshell-bootstrap/src/mtls.rs index 3767a639..0c802e63 100644 --- a/crates/openshell-bootstrap/src/mtls.rs +++ b/crates/openshell-bootstrap/src/mtls.rs @@ -1,15 +1,16 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::paths::xdg_config_dir; use crate::pki::PkiBundle; use miette::{IntoDiagnostic, Result}; +use openshell_core::paths::{create_dir_restricted, set_file_owner_only, xdg_config_dir}; use std::path::PathBuf; /// Store the PKI bundle's client materials (ca.crt, tls.crt, tls.key) to the /// local filesystem so the CLI can use them for mTLS connections. /// /// Files are written atomically: temp dir -> validate -> rename over target. +/// Directories are created with `0o700` and `tls.key` is set to `0o600`. pub fn store_pki_bundle(name: &str, bundle: &PkiBundle) -> Result<()> { let dir = cli_mtls_dir(name)?; let temp_dir = cli_mtls_temp_dir(name)?; @@ -21,9 +22,9 @@ pub fn store_pki_bundle(name: &str, bundle: &PkiBundle) -> Result<()> { .map_err(|e| e.wrap_err(format!("failed to remove {}", temp_dir.display())))?; } - std::fs::create_dir_all(&temp_dir) - .into_diagnostic() - .map_err(|e| e.wrap_err(format!("failed to create {}", temp_dir.display())))?; + // Create the temp dir with restricted permissions so the private key + // is never world-readable, even momentarily. + create_dir_restricted(&temp_dir)?; std::fs::write(temp_dir.join("ca.crt"), &bundle.ca_cert_pem) .into_diagnostic() @@ -35,6 +36,9 @@ pub fn store_pki_bundle(name: &str, bundle: &PkiBundle) -> Result<()> { .into_diagnostic() .map_err(|e| e.wrap_err("failed to write tls.key"))?; + // Restrict the private key to owner-only. + set_file_owner_only(&temp_dir.join("tls.key"))?; + validate_cli_mtls_bundle_dir(&temp_dir)?; let had_backup = if dir.exists() { @@ -61,6 +65,9 @@ pub fn store_pki_bundle(name: &str, bundle: &PkiBundle) -> Result<()> { return Err(err); } + // Ensure the final directory also has restricted permissions after rename. + create_dir_restricted(&dir)?; + if had_backup { std::fs::remove_dir_all(&backup_dir) .into_diagnostic() diff --git a/crates/openshell-bootstrap/src/paths.rs b/crates/openshell-bootstrap/src/paths.rs index 84bd06e3..cd3cb769 100644 --- a/crates/openshell-bootstrap/src/paths.rs +++ b/crates/openshell-bootstrap/src/paths.rs @@ -1,19 +1,10 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use miette::{IntoDiagnostic, Result, WrapErr}; +use miette::Result; +use openshell_core::paths::xdg_config_dir; use std::path::PathBuf; -pub fn xdg_config_dir() -> Result { - if let Ok(path) = std::env::var("XDG_CONFIG_HOME") { - return Ok(PathBuf::from(path)); - } - let home = std::env::var("HOME") - .into_diagnostic() - .wrap_err("HOME is not set")?; - Ok(PathBuf::from(home).join(".config")) -} - /// Path to the file that stores the active gateway name. /// /// Location: `$XDG_CONFIG_HOME/openshell/active_gateway` diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index 7f8e7905..4b284bff 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -773,15 +773,9 @@ fn render_ssh_config(gateway: &str, name: &str) -> String { } fn openshell_ssh_config_path() -> Result { - let base = if let Ok(path) = std::env::var("XDG_CONFIG_HOME") { - PathBuf::from(path) - } else { - let home = std::env::var("HOME") - .into_diagnostic() - .wrap_err("HOME is not set")?; - PathBuf::from(home).join(".config") - }; - Ok(base.join("openshell").join("ssh_config")) + Ok(openshell_core::paths::xdg_config_dir()? + .join("openshell") + .join("ssh_config")) } fn user_ssh_config_path() -> Result { @@ -905,9 +899,7 @@ pub fn install_ssh_config(gateway: &str, name: &str) -> Result { ensure_openshell_include(&main_config, &managed_config)?; if let Some(parent) = managed_config.parent() { - fs::create_dir_all(parent) - .into_diagnostic() - .wrap_err("failed to create OpenShell config directory")?; + openshell_core::paths::create_dir_restricted(parent)?; } let alias = host_alias(name); diff --git a/crates/openshell-cli/src/tls.rs b/crates/openshell-cli/src/tls.rs index f24e9366..5543a081 100644 --- a/crates/openshell-cli/src/tls.rs +++ b/crates/openshell-cli/src/tls.rs @@ -150,13 +150,7 @@ fn sanitize_name(value: &str) -> String { } fn xdg_config_dir() -> Result { - if let Ok(path) = std::env::var("XDG_CONFIG_HOME") { - return Ok(PathBuf::from(path)); - } - let home = std::env::var("HOME") - .into_diagnostic() - .wrap_err("HOME is not set")?; - Ok(PathBuf::from(home).join(".config")) + openshell_core::paths::xdg_config_dir() } pub fn require_tls_materials(server: &str, tls: &TlsOptions) -> Result { diff --git a/crates/openshell-core/Cargo.toml b/crates/openshell-core/Cargo.toml index 6adf8039..eeedd11a 100644 --- a/crates/openshell-core/Cargo.toml +++ b/crates/openshell-core/Cargo.toml @@ -24,5 +24,8 @@ url = { workspace = true } tonic-build = { workspace = true } protobuf-src = { workspace = true } +[dev-dependencies] +tempfile = "3" + [lints] workspace = true diff --git a/crates/openshell-core/src/forward.rs b/crates/openshell-core/src/forward.rs index 142252e5..c7b63fef 100644 --- a/crates/openshell-core/src/forward.rs +++ b/crates/openshell-core/src/forward.rs @@ -6,6 +6,7 @@ //! Used by both the CLI (`openshell-cli`) and the TUI (`openshell-tui`) to //! start, stop, list, and track background SSH port forwards. +use crate::paths::{create_dir_restricted, xdg_config_dir}; use miette::{IntoDiagnostic, Result, WrapErr}; use std::net::TcpListener; use std::path::PathBuf; @@ -17,15 +18,7 @@ use std::process::Command; /// Base directory for forward PID files. pub fn forward_pid_dir() -> Result { - let base = if let Ok(path) = std::env::var("XDG_CONFIG_HOME") { - PathBuf::from(path) - } else { - let home = std::env::var("HOME") - .into_diagnostic() - .wrap_err("HOME is not set")?; - PathBuf::from(home).join(".config") - }; - Ok(base.join("openshell").join("forwards")) + Ok(xdg_config_dir()?.join("openshell").join("forwards")) } /// PID file path for a specific sandbox + port forward. @@ -44,9 +37,7 @@ pub fn write_forward_pid( bind_addr: &str, ) -> Result<()> { let dir = forward_pid_dir()?; - std::fs::create_dir_all(&dir) - .into_diagnostic() - .wrap_err("failed to create forwards directory")?; + create_dir_restricted(&dir)?; let path = forward_pid_path(name, port)?; std::fs::write(&path, format!("{pid}\t{sandbox_id}\t{bind_addr}")) .into_diagnostic() diff --git a/crates/openshell-core/src/lib.rs b/crates/openshell-core/src/lib.rs index 4c91a91d..9cf0d620 100644 --- a/crates/openshell-core/src/lib.rs +++ b/crates/openshell-core/src/lib.rs @@ -13,6 +13,7 @@ pub mod config; pub mod error; pub mod forward; pub mod inference; +pub mod paths; pub mod proto; pub use config::{Config, TlsConfig}; diff --git a/crates/openshell-core/src/paths.rs b/crates/openshell-core/src/paths.rs new file mode 100644 index 00000000..bd9ce23d --- /dev/null +++ b/crates/openshell-core/src/paths.rs @@ -0,0 +1,166 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Centralized XDG config directory resolution and permission helpers. +//! +//! All `OpenShell` crates should use [`xdg_config_dir`] from this module instead +//! of reimplementing the XDG lookup. The permission helpers ensure that +//! sensitive files (private keys, tokens) and the directories containing them +//! are created with restrictive modes. + +use miette::{IntoDiagnostic, Result, WrapErr}; +use std::path::{Path, PathBuf}; + +/// Resolve the XDG config base directory. +/// +/// Returns `$XDG_CONFIG_HOME` if set, otherwise `$HOME/.config`. +pub fn xdg_config_dir() -> Result { + if let Ok(path) = std::env::var("XDG_CONFIG_HOME") { + return Ok(PathBuf::from(path)); + } + let home = std::env::var("HOME") + .into_diagnostic() + .wrap_err("HOME is not set")?; + Ok(PathBuf::from(home).join(".config")) +} + +/// The top-level `OpenShell` config directory: `$XDG_CONFIG_HOME/openshell/`. +pub fn openshell_config_dir() -> Result { + Ok(xdg_config_dir()?.join("openshell")) +} + +/// Create a directory (and parents) with owner-only permissions (`0o700`) on +/// Unix. On non-Unix platforms, falls back to default permissions. +/// +/// This should be used for any directory that contains sensitive material +/// (tokens, private keys, certificates). +pub fn create_dir_restricted(path: &Path) -> Result<()> { + std::fs::create_dir_all(path) + .into_diagnostic() + .wrap_err_with(|| format!("failed to create {}", path.display()))?; + set_dir_owner_only(path)?; + Ok(()) +} + +/// Set a directory to owner-only access (`0o700`) on Unix. +/// +/// No-op on non-Unix platforms. +pub fn set_dir_owner_only(path: &Path) -> Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) + .into_diagnostic() + .wrap_err_with(|| format!("failed to set permissions on {}", path.display()))?; + } + #[cfg(not(unix))] + let _ = path; + Ok(()) +} + +/// Set a file to owner-only read/write (`0o600`) on Unix. +/// +/// No-op on non-Unix platforms. +pub fn set_file_owner_only(path: &Path) -> Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + .into_diagnostic() + .wrap_err_with(|| format!("failed to set permissions on {}", path.display()))?; + } + #[cfg(not(unix))] + let _ = path; + Ok(()) +} + +/// Ensure the parent directory of `path` exists with restricted permissions. +/// +/// Equivalent to `create_dir_restricted(path.parent())` but handles the case +/// where `path` has no parent gracefully. +pub fn ensure_parent_dir_restricted(path: &Path) -> Result<()> { + if let Some(parent) = path.parent() { + create_dir_restricted(parent)?; + } + Ok(()) +} + +/// Check whether a file has permissions that are too open (group/other readable). +/// +/// Returns `true` if the file has group or other read/write/execute bits set. +/// Always returns `false` on non-Unix platforms. +#[cfg(unix)] +pub fn is_file_permissions_too_open(path: &Path) -> bool { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(path) + .map(|m| m.permissions().mode() & 0o077 != 0) + .unwrap_or(false) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn xdg_config_dir_respects_env() { + // This test checks the logic — actual env var mutation is unsafe so + // we rely on the integration tests in openshell-bootstrap for full + // round-trip testing. + let result = xdg_config_dir(); + assert!(result.is_ok()); + } + + #[test] + fn openshell_config_dir_appends_openshell() { + let dir = openshell_config_dir().unwrap(); + assert!( + dir.ends_with("openshell"), + "expected path ending with 'openshell', got: {dir:?}" + ); + } + + #[cfg(unix)] + #[test] + fn create_dir_restricted_sets_0o700() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("restricted"); + create_dir_restricted(&dir).unwrap(); + let mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o700, "expected 0700, got {mode:04o}"); + } + + #[cfg(unix)] + #[test] + fn set_file_owner_only_sets_0o600() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("secret"); + std::fs::write(&file, "secret-data").unwrap(); + set_file_owner_only(&file).unwrap(); + let mode = std::fs::metadata(&file).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "expected 0600, got {mode:04o}"); + } + + #[cfg(unix)] + #[test] + fn is_file_permissions_too_open_detects_world_readable() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("open-file"); + std::fs::write(&file, "data").unwrap(); + std::fs::set_permissions(&file, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert!(is_file_permissions_too_open(&file)); + } + + #[cfg(unix)] + #[test] + fn is_file_permissions_too_open_accepts_restricted() { + use std::os::unix::fs::PermissionsExt; + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("restricted-file"); + std::fs::write(&file, "data").unwrap(); + std::fs::set_permissions(&file, std::fs::Permissions::from_mode(0o600)).unwrap(); + assert!(!is_file_permissions_too_open(&file)); + } +} diff --git a/crates/openshell-tui/src/lib.rs b/crates/openshell-tui/src/lib.rs index f0891079..e0b36f94 100644 --- a/crates/openshell-tui/src/lib.rs +++ b/crates/openshell-tui/src/lib.rs @@ -473,10 +473,7 @@ async fn connect_to_gateway(name: &str, endpoint: &str) -> Result { /// Resolve the mTLS cert directory for a gateway. fn gateway_mtls_dir(name: &str) -> Option { - let config_dir = std::env::var("XDG_CONFIG_HOME") - .map(PathBuf::from) - .or_else(|_| std::env::var("HOME").map(|h| PathBuf::from(h).join(".config"))) - .ok()?; + let config_dir = openshell_core::paths::xdg_config_dir().ok()?; Some( config_dir .join("openshell")