Skip to content

Commit 6deb1f0

Browse files
ericcurtinericcurtin
authored
refactor(core): eliminate duplicate utilities across crates (#1381)
Consolidate shared constants and helpers into openshell-core to avoid duplicate definitions scattered across crates. Adds OPENSHELL_SANDBOX to the sandbox_env module so all drivers and the sandbox supervisor reference it via a typed constant rather than string literals, matching the existing pattern for SANDBOX_ID, ENDPOINT, and the other env vars.
1 parent 1c31764 commit 6deb1f0

34 files changed

Lines changed: 424 additions & 594 deletions

File tree

‎crates/openshell-cli/tests/ensure_providers_integration.rs‎

Lines changed: 5 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@
55
//! `--provider` names are auto-created when they match a known provider type,
66
//! pass through when they already exist, and error for unrecognised names.
77
8+
mod helpers;
9+
10+
use helpers::{
11+
EnvVarGuard, build_ca, build_client_cert, build_server_cert, install_rustls_provider,
12+
};
813
use openshell_cli::run;
914
use openshell_cli::tls::TlsOptions;
1015
use openshell_core::proto::open_shell_server::{OpenShell, OpenShellServer};
@@ -23,9 +28,6 @@ use openshell_core::proto::{
2328
SupervisorMessage, UpdateProviderRequest, WatchSandboxRequest,
2429
};
2530
use openshell_core::{ObjectId, ObjectName};
26-
use rcgen::{
27-
BasicConstraints, Certificate, CertificateParams, ExtendedKeyUsagePurpose, IsCa, KeyPair,
28-
};
2931
use std::collections::HashMap;
3032
use std::sync::Arc;
3133
use tempfile::TempDir;
@@ -35,60 +37,6 @@ use tokio_stream::wrappers::TcpListenerStream;
3537
use tonic::transport::{Certificate as TlsCertificate, Identity, Server, ServerTlsConfig};
3638
use tonic::{Response, Status};
3739

38-
// ── EnvVarGuard ──────────────────────────────────────────────────────
39-
40-
// Serialise tests that mutate environment variables so concurrent
41-
// threads don't clobber each other.
42-
static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
43-
44-
struct SavedVar {
45-
key: &'static str,
46-
original: Option<String>,
47-
}
48-
49-
/// Holds the global env lock and restores all modified variables on drop.
50-
struct EnvVarGuard {
51-
vars: Vec<SavedVar>,
52-
_lock: std::sync::MutexGuard<'static, ()>,
53-
}
54-
55-
#[allow(unsafe_code)]
56-
impl EnvVarGuard {
57-
/// Acquire the lock and set one or more environment variables.
58-
fn set(pairs: &[(&'static str, &str)]) -> Self {
59-
let lock = ENV_LOCK
60-
.lock()
61-
.unwrap_or_else(std::sync::PoisonError::into_inner);
62-
let mut vars = Vec::with_capacity(pairs.len());
63-
for &(key, value) in pairs {
64-
let original = std::env::var(key).ok();
65-
unsafe {
66-
std::env::set_var(key, value);
67-
}
68-
vars.push(SavedVar { key, original });
69-
}
70-
Self { vars, _lock: lock }
71-
}
72-
}
73-
74-
#[allow(unsafe_code)]
75-
impl Drop for EnvVarGuard {
76-
fn drop(&mut self) {
77-
for var in &self.vars {
78-
if let Some(value) = &var.original {
79-
unsafe {
80-
std::env::set_var(var.key, value);
81-
}
82-
} else {
83-
unsafe {
84-
std::env::remove_var(var.key);
85-
}
86-
}
87-
}
88-
// _lock drops here, releasing the mutex
89-
}
90-
}
91-
9240
// ── mock OpenShell server ─────────────────────────────────────────────
9341

9442
#[derive(Clone, Default)]
@@ -559,36 +507,6 @@ impl OpenShell for TestOpenShell {
559507
}
560508
}
561509

562-
// ── TLS helpers ──────────────────────────────────────────────────────
563-
564-
fn install_rustls_provider() {
565-
let _ = rustls::crypto::ring::default_provider().install_default();
566-
}
567-
568-
fn build_ca() -> (Certificate, KeyPair) {
569-
let key_pair = KeyPair::generate().unwrap();
570-
let mut params = CertificateParams::new(Vec::<String>::new()).unwrap();
571-
params.is_ca = IsCa::Ca(BasicConstraints::Unconstrained);
572-
let cert = params.self_signed(&key_pair).unwrap();
573-
(cert, key_pair)
574-
}
575-
576-
fn build_server_cert(ca: &Certificate, ca_key: &KeyPair) -> (String, String) {
577-
let key_pair = KeyPair::generate().unwrap();
578-
let mut params = CertificateParams::new(vec!["localhost".to_string()]).unwrap();
579-
params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ServerAuth];
580-
let cert = params.signed_by(&key_pair, ca, ca_key).unwrap();
581-
(cert.pem(), key_pair.serialize_pem())
582-
}
583-
584-
fn build_client_cert(ca: &Certificate, ca_key: &KeyPair) -> (String, String) {
585-
let key_pair = KeyPair::generate().unwrap();
586-
let mut params = CertificateParams::new(Vec::<String>::new()).unwrap();
587-
params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ClientAuth];
588-
let cert = params.signed_by(&key_pair, ca, ca_key).unwrap();
589-
(cert.pem(), key_pair.serialize_pem())
590-
}
591-
592510
// ── test server fixture ──────────────────────────────────────────────
593511

594512
struct TestServer {

‎crates/openshell-cli/tests/helpers/mod.rs‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
//! Shared helpers for CLI integration tests.
5+
//!
6+
//! Include this module from a test file with:
7+
//! ```ignore
8+
//! mod helpers;
9+
//! ```
10+
11+
use rcgen::{
12+
BasicConstraints, Certificate, CertificateParams, ExtendedKeyUsagePurpose, IsCa, KeyPair,
13+
};
14+
15+
// ── EnvVarGuard ──────────────────────────────────────────────────────────────
16+
17+
/// Global mutex that serialises tests which mutate environment variables so
18+
/// concurrent threads don't clobber each other's state.
19+
static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
20+
21+
struct SavedVar {
22+
key: &'static str,
23+
original: Option<String>,
24+
}
25+
26+
/// RAII guard that acquires `ENV_LOCK` and restores all modified environment
27+
/// variables on drop.
28+
pub struct EnvVarGuard {
29+
vars: Vec<SavedVar>,
30+
_lock: std::sync::MutexGuard<'static, ()>,
31+
}
32+
33+
#[allow(dead_code, unsafe_code)]
34+
impl EnvVarGuard {
35+
/// Acquire the global env-var lock and atomically set one or more
36+
/// environment variables. All variables are restored to their prior
37+
/// state (or removed) when the guard is dropped.
38+
pub fn set(pairs: &[(&'static str, &str)]) -> Self {
39+
let lock = ENV_LOCK
40+
.lock()
41+
.unwrap_or_else(std::sync::PoisonError::into_inner);
42+
let mut vars = Vec::with_capacity(pairs.len());
43+
for &(key, value) in pairs {
44+
let original = std::env::var(key).ok();
45+
unsafe {
46+
std::env::set_var(key, value);
47+
}
48+
vars.push(SavedVar { key, original });
49+
}
50+
Self { vars, _lock: lock }
51+
}
52+
}
53+
54+
#[allow(unsafe_code)]
55+
impl Drop for EnvVarGuard {
56+
fn drop(&mut self) {
57+
for var in &self.vars {
58+
if let Some(value) = &var.original {
59+
unsafe {
60+
std::env::set_var(var.key, value);
61+
}
62+
} else {
63+
unsafe {
64+
std::env::remove_var(var.key);
65+
}
66+
}
67+
}
68+
// _lock drops here, releasing the mutex
69+
}
70+
}
71+
72+
// ── TLS helpers ──────────────────────────────────────────────────────────────
73+
74+
/// Install the `rustls` ring crypto provider as the process default.
75+
///
76+
/// Safe to call multiple times — subsequent calls are no-ops.
77+
#[allow(dead_code)]
78+
pub fn install_rustls_provider() {
79+
let _ = rustls::crypto::ring::default_provider().install_default();
80+
}
81+
82+
/// Generate a self-signed CA certificate and its key pair.
83+
#[allow(dead_code)]
84+
pub fn build_ca() -> (Certificate, KeyPair) {
85+
let key_pair = KeyPair::generate().unwrap();
86+
let mut params = CertificateParams::new(Vec::<String>::new()).unwrap();
87+
params.is_ca = IsCa::Ca(BasicConstraints::Unconstrained);
88+
let cert = params.self_signed(&key_pair).unwrap();
89+
(cert, key_pair)
90+
}
91+
92+
/// Generate a server certificate signed by `ca`, valid for `localhost`.
93+
///
94+
/// Returns `(cert_pem, key_pem)`.
95+
#[allow(dead_code)]
96+
pub fn build_server_cert(ca: &Certificate, ca_key: &KeyPair) -> (String, String) {
97+
let key_pair = KeyPair::generate().unwrap();
98+
let mut params = CertificateParams::new(vec!["localhost".to_string()]).unwrap();
99+
params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ServerAuth];
100+
let cert = params.signed_by(&key_pair, ca, ca_key).unwrap();
101+
(cert.pem(), key_pair.serialize_pem())
102+
}
103+
104+
/// Generate a client authentication certificate signed by `ca`.
105+
///
106+
/// Returns `(cert_pem, key_pem)`.
107+
#[allow(dead_code)]
108+
pub fn build_client_cert(ca: &Certificate, ca_key: &KeyPair) -> (String, String) {
109+
let key_pair = KeyPair::generate().unwrap();
110+
let mut params = CertificateParams::new(Vec::<String>::new()).unwrap();
111+
params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ClientAuth];
112+
let cert = params.signed_by(&key_pair, ca, ca_key).unwrap();
113+
(cert.pem(), key_pair.serialize_pem())
114+
}

‎crates/openshell-cli/tests/mtls_integration.rs‎

Lines changed: 7 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
22
// SPDX-License-Identifier: Apache-2.0
33

4+
mod helpers;
5+
6+
use helpers::{
7+
EnvVarGuard, build_ca, build_client_cert, build_server_cert, install_rustls_provider,
8+
};
49
use openshell_cli::tls::{TlsOptions, grpc_client};
510
use openshell_core::proto::{
611
CreateProviderRequest, CreateSshSessionRequest, CreateSshSessionResponse,
@@ -10,9 +15,6 @@ use openshell_core::proto::{
1015
UpdateProviderRequest,
1116
open_shell_server::{OpenShell, OpenShellServer},
1217
};
13-
use rcgen::{
14-
BasicConstraints, Certificate, CertificateParams, ExtendedKeyUsagePurpose, IsCa, KeyPair,
15-
};
1618
use tempfile::tempdir;
1719
use tokio::net::TcpListener;
1820
use tokio::sync::mpsc;
@@ -22,41 +24,6 @@ use tonic::{
2224
transport::{Certificate as TlsCertificate, Identity, Server, ServerTlsConfig},
2325
};
2426

25-
struct EnvVarGuard {
26-
key: &'static str,
27-
original: Option<String>,
28-
}
29-
30-
#[allow(unsafe_code)]
31-
impl EnvVarGuard {
32-
fn set(key: &'static str, value: &str) -> Self {
33-
let original = std::env::var(key).ok();
34-
unsafe {
35-
std::env::set_var(key, value);
36-
}
37-
Self { key, original }
38-
}
39-
}
40-
41-
#[allow(unsafe_code)]
42-
impl Drop for EnvVarGuard {
43-
fn drop(&mut self) {
44-
if let Some(value) = &self.original {
45-
unsafe {
46-
std::env::set_var(self.key, value);
47-
}
48-
} else {
49-
unsafe {
50-
std::env::remove_var(self.key);
51-
}
52-
}
53-
}
54-
}
55-
56-
fn install_rustls_provider() {
57-
let _ = rustls::crypto::ring::default_provider().install_default();
58-
}
59-
6027
#[derive(Clone, Default)]
6128
struct TestOpenShell;
6229

@@ -450,34 +417,6 @@ impl OpenShell for TestOpenShell {
450417
}
451418
}
452419

453-
fn build_ca() -> (Certificate, KeyPair) {
454-
let key_pair = KeyPair::generate().unwrap();
455-
let mut params = CertificateParams::new(Vec::<String>::new()).unwrap();
456-
params.is_ca = IsCa::Ca(BasicConstraints::Unconstrained);
457-
let cert = params.self_signed(&key_pair).unwrap();
458-
(cert, key_pair)
459-
}
460-
461-
fn build_server_cert(ca: &Certificate, ca_key: &KeyPair) -> (String, String) {
462-
let key_pair = KeyPair::generate().unwrap();
463-
let mut params = CertificateParams::new(vec!["localhost".to_string()]).unwrap();
464-
params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ServerAuth];
465-
let cert = params.signed_by(&key_pair, ca, ca_key).unwrap();
466-
let cert_pem = cert.pem();
467-
let key_pem = key_pair.serialize_pem();
468-
(cert_pem, key_pem)
469-
}
470-
471-
fn build_client_cert(ca: &Certificate, ca_key: &KeyPair) -> (String, String) {
472-
let key_pair = KeyPair::generate().unwrap();
473-
let mut params = CertificateParams::new(Vec::<String>::new()).unwrap();
474-
params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ClientAuth];
475-
let cert = params.signed_by(&key_pair, ca, ca_key).unwrap();
476-
let cert_pem = cert.pem();
477-
let key_pem = key_pair.serialize_pem();
478-
(cert_pem, key_pem)
479-
}
480-
481420
async fn run_server(
482421
server_cert: String,
483422
server_key: String,
@@ -544,7 +483,8 @@ async fn cli_requires_client_cert_for_https() {
544483
let dir = tempdir().unwrap();
545484
// Point XDG_CONFIG_HOME at the isolated temp dir so that default_tls_dir
546485
// cannot discover real client certs from the developer's machine.
547-
let _xdg_env = EnvVarGuard::set("XDG_CONFIG_HOME", &dir.path().to_string_lossy());
486+
let xdg_path = dir.path().to_string_lossy();
487+
let _xdg_env = EnvVarGuard::set(&[("XDG_CONFIG_HOME", &xdg_path)]);
548488
let ca_path = dir.path().join("ca.crt");
549489
std::fs::write(&ca_path, ca_cert).unwrap();
550490

0 commit comments

Comments
 (0)