From d31d1f1550f561a674626414173769695f5a5980 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Mon, 15 Jun 2026 22:26:43 -0400 Subject: [PATCH 1/3] feat(auth): add per-user sandbox ownership enforcement Add an `auth/ownership.rs` module that stamps an `openshell.ai/owner` label on sandboxes at creation time (from the verified OIDC identity) and gates access on subsequent operations. Key behaviors: - CreateSandbox: strips client-supplied reserved keys, stamps owner - ListSandboxes: injects owner filter into the label selector - Get/Delete/Watch/Exec/SSH/Service handlers: verify caller owns the sandbox before proceeding - Admin bypass: callers with the configured admin role skip ownership - Backward compat: sandboxes without owner label (pre-existing) and anonymous/LocalDev principals (OIDC disabled) are allowed through Addresses kagenti/kagenti#1976 task A. Signed-off-by: Paolo Dettori Assisted-By: Claude (Anthropic AI) Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/mod.rs | 1 + crates/openshell-server/src/auth/ownership.rs | 380 ++++++++++++++++++ crates/openshell-server/src/grpc/sandbox.rs | 98 ++++- crates/openshell-server/src/grpc/service.rs | 22 + 4 files changed, 493 insertions(+), 8 deletions(-) create mode 100644 crates/openshell-server/src/auth/ownership.rs diff --git a/crates/openshell-server/src/auth/mod.rs b/crates/openshell-server/src/auth/mod.rs index cbf3b94d9..6b77980a0 100644 --- a/crates/openshell-server/src/auth/mod.rs +++ b/crates/openshell-server/src/auth/mod.rs @@ -16,6 +16,7 @@ pub mod identity; pub mod k8s_sa; pub mod method_authz; pub mod oidc; +pub mod ownership; pub mod principal; pub mod sandbox_jwt; pub mod sandbox_methods; diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs new file mode 100644 index 000000000..60e04380f --- /dev/null +++ b/crates/openshell-server/src/auth/ownership.rs @@ -0,0 +1,380 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Per-user sandbox ownership enforcement. +//! +//! Stamps an `openshell.ai/owner` label on sandboxes at creation time (from the +//! verified caller identity) and gates access on subsequent operations. Admin +//! callers bypass the ownership check. + +use std::collections::HashMap; + +use tonic::Status; + +use super::identity::Identity; +use super::principal::Principal; + +/// Reserved label key for sandbox ownership. Server-set, never client-controlled. +pub const OWNER_LABEL: &str = "openshell.ai/owner"; + +/// Reserved label key prefix. All keys starting with this are stripped from +/// client-supplied labels on create. +const RESERVED_PREFIX: &str = "openshell.ai/"; + +/// Sanitize an identity subject into a valid Kubernetes label value. +/// +/// K8s label values: alphanumeric, `-`, `_`, `.`, max 63 chars, must start and +/// end with alphanumeric. Keycloak UUID subjects (`sub`) satisfy this directly. +/// For non-conforming subjects, we produce a hex-encoded SHA-256 truncated to 63 +/// chars (always alphanumeric). +pub fn sanitize_subject(subject: &str) -> Result { + if subject.is_empty() { + return Err(Status::unauthenticated( + "identity subject is empty; cannot determine sandbox owner", + )); + } + + if is_valid_label_value(subject) { + return Ok(subject.to_string()); + } + + // Fallback: hex SHA-256 truncated to 63 chars (always valid). + use sha2::{Digest, Sha256}; + use std::fmt::Write; + let hash = Sha256::digest(subject.as_bytes()); + let mut hex = String::with_capacity(64); + for byte in &hash { + let _ = write!(hex, "{byte:02x}"); + } + hex.truncate(63); + Ok(hex) +} + +/// Strip all reserved `openshell.ai/` keys from client-supplied labels and stamp +/// the owner label from the caller's identity. +/// +/// Returns `UNAUTHENTICATED` if the principal is `Anonymous` (ownership requires +/// a verified identity). +pub fn stamp_owner( + labels: &mut HashMap, + principal: Option<&Principal>, +) -> Result<(), Status> { + let identity = require_identity(principal)?; + labels.retain(|key, _| !key.starts_with(RESERVED_PREFIX)); + let owner_value = sanitize_subject(&identity.subject)?; + labels.insert(OWNER_LABEL.to_string(), owner_value); + Ok(()) +} + +/// Check that the caller owns the sandbox (or is an admin). +/// +/// Returns `PERMISSION_DENIED` if the sandbox has an owner label that doesn't +/// match the caller's identity subject. +/// +/// When `admin_role` is non-empty and the caller has that role, the check is +/// bypassed (admins operate across all users' sandboxes). +/// +/// When the sandbox has no owner label (pre-existing sandboxes created before +/// this feature), access is allowed to maintain backward compatibility. +pub fn check_owner( + sandbox_labels: &HashMap, + principal: Option<&Principal>, + admin_role: &str, +) -> Result<(), Status> { + let Some(owner_value) = sandbox_labels.get(OWNER_LABEL) else { + // No owner label → legacy sandbox, allow access. + return Ok(()); + }; + + let identity = match principal_identity(principal) { + Some(id) => id, + None => { + // Anonymous caller on an owned sandbox → deny. + return Err(Status::permission_denied( + "sandbox is owned; authenticated identity required", + )); + } + }; + + // Admin bypass. + if !admin_role.is_empty() && identity.roles.iter().any(|r| r == admin_role) { + return Ok(()); + } + + let caller_value = sanitize_subject(&identity.subject)?; + if caller_value != *owner_value { + return Err(Status::permission_denied( + "you do not own this sandbox", + )); + } + + Ok(()) +} + +/// AND the caller's owner selector into an existing label selector string. +/// +/// Returns the augmented selector. When the principal is `Anonymous` and no OIDC +/// is configured, returns the selector unchanged (backward compat). +pub fn owner_selector( + principal: Option<&Principal>, + existing_selector: &str, + admin_role: &str, +) -> Result { + let identity = match principal_identity(principal) { + Some(id) => id, + None => { + // Anonymous → no owner filtering (backward compat). + return Ok(existing_selector.to_string()); + } + }; + + // Admin bypass — see all sandboxes. + if !admin_role.is_empty() && identity.roles.iter().any(|r| r == admin_role) { + return Ok(existing_selector.to_string()); + } + + let owner_value = sanitize_subject(&identity.subject)?; + let owner_filter = format!("{OWNER_LABEL}={owner_value}"); + + if existing_selector.is_empty() { + Ok(owner_filter) + } else { + Ok(format!("{existing_selector},{owner_filter}")) + } +} + +/// Extract identity from principal, returning `UNAUTHENTICATED` if anonymous. +fn require_identity(principal: Option<&Principal>) -> Result<&Identity, Status> { + match principal { + Some(Principal::User(user)) => Ok(&user.identity), + Some(Principal::Sandbox(_)) | Some(Principal::Anonymous) | None => Err( + Status::unauthenticated("authenticated user identity required for sandbox operations"), + ), + } +} + +/// Extract identity without failing — returns `None` for anonymous/sandbox/missing. +fn principal_identity(principal: Option<&Principal>) -> Option<&Identity> { + match principal { + Some(Principal::User(user)) => Some(&user.identity), + _ => None, + } +} + +fn is_valid_label_value(value: &str) -> bool { + if value.is_empty() || value.len() > 63 { + return false; + } + let bytes = value.as_bytes(); + if !bytes[0].is_ascii_alphanumeric() || !bytes[bytes.len() - 1].is_ascii_alphanumeric() { + return false; + } + value + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use crate::auth::identity::IdentityProvider; + use crate::auth::principal::UserPrincipal; + + fn user_principal(subject: &str, roles: &[&str]) -> Principal { + Principal::User(UserPrincipal { + identity: Identity { + subject: subject.to_string(), + display_name: None, + roles: roles.iter().map(|r| (*r).to_string()).collect(), + scopes: vec![], + provider: IdentityProvider::Oidc, + }, + }) + } + + fn admin_principal() -> Principal { + user_principal("admin-uuid", &["openshell-admin", "openshell-user"]) + } + + fn alice() -> Principal { + user_principal("alice-uuid-1234", &["openshell-user"]) + } + + fn bob() -> Principal { + user_principal("bob-uuid-5678", &["openshell-user"]) + } + + // ---- sanitize_subject ---- + + #[test] + fn sanitize_uuid_passthrough() { + let result = sanitize_subject("550e8400-e29b-41d4-a716-446655440000").unwrap(); + assert_eq!(result, "550e8400-e29b-41d4-a716-446655440000"); + } + + #[test] + fn sanitize_simple_alphanumeric() { + assert_eq!(sanitize_subject("user123").unwrap(), "user123"); + } + + #[test] + fn sanitize_email_hashes() { + // Email contains '@' which is invalid for labels → hashed + let result = sanitize_subject("alice@corp.com").unwrap(); + assert_ne!(result, "alice@corp.com"); + assert!(result.len() <= 63); + assert!(result.chars().all(|c| c.is_ascii_hexdigit())); + } + + #[test] + fn sanitize_empty_subject_fails() { + assert!(sanitize_subject("").is_err()); + } + + #[test] + fn sanitize_long_subject_hashes() { + let long = "a".repeat(100); + let result = sanitize_subject(&long).unwrap(); + assert!(result.len() <= 63); + } + + // ---- stamp_owner ---- + + #[test] + fn stamp_owner_sets_label() { + let principal = alice(); + let mut labels = HashMap::new(); + labels.insert("env".to_string(), "dev".to_string()); + + stamp_owner(&mut labels, Some(&principal)).unwrap(); + + assert_eq!(labels.get(OWNER_LABEL).unwrap(), "alice-uuid-1234"); + assert_eq!(labels.get("env").unwrap(), "dev"); + } + + #[test] + fn stamp_owner_strips_reserved_keys() { + let principal = alice(); + let mut labels = HashMap::new(); + labels.insert("openshell.ai/owner".to_string(), "spoofed".to_string()); + labels.insert("openshell.ai/custom".to_string(), "sneaky".to_string()); + labels.insert("safe-key".to_string(), "kept".to_string()); + + stamp_owner(&mut labels, Some(&principal)).unwrap(); + + assert_eq!(labels.get(OWNER_LABEL).unwrap(), "alice-uuid-1234"); + assert!(!labels.contains_key("openshell.ai/custom")); + assert_eq!(labels.get("safe-key").unwrap(), "kept"); + } + + #[test] + fn stamp_owner_rejects_anonymous() { + let mut labels = HashMap::new(); + let result = stamp_owner(&mut labels, Some(&Principal::Anonymous)); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().code(), tonic::Code::Unauthenticated); + } + + #[test] + fn stamp_owner_rejects_none() { + let mut labels = HashMap::new(); + let result = stamp_owner(&mut labels, None); + assert!(result.is_err()); + } + + // ---- check_owner ---- + + #[test] + fn check_owner_allows_owner() { + let mut labels = HashMap::new(); + labels.insert(OWNER_LABEL.to_string(), "alice-uuid-1234".to_string()); + + let principal = alice(); + assert!(check_owner(&labels, Some(&principal), "openshell-admin").is_ok()); + } + + #[test] + fn check_owner_denies_non_owner() { + let mut labels = HashMap::new(); + labels.insert(OWNER_LABEL.to_string(), "alice-uuid-1234".to_string()); + + let principal = bob(); + let result = check_owner(&labels, Some(&principal), "openshell-admin"); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().code(), tonic::Code::PermissionDenied); + } + + #[test] + fn check_owner_admin_bypass() { + let mut labels = HashMap::new(); + labels.insert(OWNER_LABEL.to_string(), "alice-uuid-1234".to_string()); + + let principal = admin_principal(); + assert!(check_owner(&labels, Some(&principal), "openshell-admin").is_ok()); + } + + #[test] + fn check_owner_no_label_allows_access() { + let labels = HashMap::new(); + let principal = bob(); + assert!(check_owner(&labels, Some(&principal), "openshell-admin").is_ok()); + } + + #[test] + fn check_owner_anonymous_on_owned_sandbox_denied() { + let mut labels = HashMap::new(); + labels.insert(OWNER_LABEL.to_string(), "alice-uuid-1234".to_string()); + + let result = check_owner(&labels, Some(&Principal::Anonymous), "openshell-admin"); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().code(), tonic::Code::PermissionDenied); + } + + #[test] + fn check_owner_none_principal_on_owned_sandbox_denied() { + let mut labels = HashMap::new(); + labels.insert(OWNER_LABEL.to_string(), "alice-uuid-1234".to_string()); + + let result = check_owner(&labels, None, "openshell-admin"); + assert!(result.is_err()); + } + + // ---- owner_selector ---- + + #[test] + fn owner_selector_empty_base() { + let principal = alice(); + let result = owner_selector(Some(&principal), "", "openshell-admin").unwrap(); + assert_eq!(result, "openshell.ai/owner=alice-uuid-1234"); + } + + #[test] + fn owner_selector_appends_to_existing() { + let principal = alice(); + let result = owner_selector(Some(&principal), "env=dev", "openshell-admin").unwrap(); + assert_eq!(result, "env=dev,openshell.ai/owner=alice-uuid-1234"); + } + + #[test] + fn owner_selector_admin_sees_all() { + let principal = admin_principal(); + let result = owner_selector(Some(&principal), "env=dev", "openshell-admin").unwrap(); + assert_eq!(result, "env=dev"); + } + + #[test] + fn owner_selector_anonymous_no_filter() { + let result = owner_selector(Some(&Principal::Anonymous), "env=dev", "openshell-admin").unwrap(); + assert_eq!(result, "env=dev"); + } + + #[test] + fn owner_selector_none_no_filter() { + let result = owner_selector(None, "", "openshell-admin").unwrap(); + assert_eq!(result, ""); + } +} diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index e60ce3995..f16d334ad 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -54,6 +54,31 @@ use crate::persistence::current_time_ms; const TCP_FORWARD_CHUNK_SIZE: usize = 64 * 1024; +// --------------------------------------------------------------------------- +// Ownership helpers +// --------------------------------------------------------------------------- + +/// Get the configured admin role name (empty string when OIDC is not configured). +fn admin_role_name(state: &ServerState) -> &str { + state + .config + .oidc + .as_ref() + .map_or("", |oidc| &oidc.admin_role) +} + +/// Verify the caller owns the sandbox. Extracts labels from sandbox metadata and +/// delegates to the ownership module. +pub(super) fn check_sandbox_owner( + sandbox: &Sandbox, + principal: Option<&crate::auth::principal::Principal>, + state: &ServerState, +) -> Result<(), Status> { + let empty = std::collections::HashMap::new(); + let labels = sandbox.metadata.as_ref().map_or(&empty, |m| &m.labels); + crate::auth::ownership::check_owner(labels, principal, admin_role_name(state)) +} + // --------------------------------------------------------------------------- // Sandbox lifecycle handlers // --------------------------------------------------------------------------- @@ -118,7 +143,8 @@ async fn handle_create_sandbox_inner( state: &Arc, request: Request, ) -> Result, Status> { - let request = request.into_inner(); + let principal = request.extensions().get::().cloned(); + let mut request = request.into_inner(); let spec = request .spec .ok_or_else(|| Status::invalid_argument("spec is required"))?; @@ -126,6 +152,13 @@ async fn handle_create_sandbox_inner( // Validate field sizes before any I/O (fail fast on oversized payloads). validate_sandbox_spec(&request.name, &spec)?; + // Stamp ownership: strip reserved keys and set owner from verified identity. + // When OIDC is not configured (principal is None), skip ownership stamping + // to maintain backward compatibility with single-user local dev. + if principal.is_some() { + crate::auth::ownership::stamp_owner(&mut request.labels, principal.as_ref())?; + } + // Validate labels (keys and values must meet Kubernetes requirements). for (key, value) in &request.labels { crate::grpc::validation::validate_label_key(key)?; @@ -227,6 +260,7 @@ pub(super) async fn handle_get_sandbox( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let name = request.into_inner().name; if name.is_empty() { return Err(Status::invalid_argument("name is required")); @@ -239,6 +273,7 @@ pub(super) async fn handle_get_sandbox( .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))?; let sandbox = sandbox.ok_or_else(|| Status::not_found("sandbox not found"))?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; Ok(Response::new(SandboxResponse { sandbox: Some(sandbox), })) @@ -248,20 +283,29 @@ pub(super) async fn handle_list_sandboxes( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let request = request.into_inner(); let limit = clamp_limit(request.limit, 100, MAX_PAGE_SIZE); - let sandboxes: Vec = if request.label_selector.is_empty() { + // Inject owner filter so users only see their own sandboxes. + let admin_role = admin_role_name(state); + let effective_selector = crate::auth::ownership::owner_selector( + principal.as_ref(), + &request.label_selector, + admin_role, + )?; + + let sandboxes: Vec = if effective_selector.is_empty() { state .store .list_messages(limit, request.offset) .await .map_err(|e| Status::internal(format!("list sandboxes failed: {e}")))? } else { - crate::grpc::validation::validate_label_selector(&request.label_selector)?; + crate::grpc::validation::validate_label_selector(&effective_selector)?; state .store - .list_messages_with_selector(&request.label_selector, limit, request.offset) + .list_messages_with_selector(&effective_selector, limit, request.offset) .await .map_err(|e| Status::internal(format!("list sandboxes with selector failed: {e}")))? }; @@ -273,7 +317,9 @@ pub(super) async fn handle_list_sandbox_providers( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let sandbox = sandbox_by_name(state, &request.into_inner().sandbox_name).await?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; let providers = providers_for_sandbox(state, &sandbox).await?; Ok(Response::new(ListSandboxProvidersResponse { providers })) } @@ -282,6 +328,7 @@ pub(super) async fn handle_attach_sandbox_provider( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let request = request.into_inner(); if request.provider_name.is_empty() { return Err(Status::invalid_argument("provider_name is required")); @@ -312,6 +359,7 @@ pub(super) async fn handle_attach_sandbox_provider( let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; let sandbox = sandbox_by_name(state, &request.sandbox_name).await?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; let sandbox_id = sandbox .metadata .as_ref() @@ -396,6 +444,7 @@ pub(super) async fn handle_detach_sandbox_provider( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let request = request.into_inner(); if request.provider_name.is_empty() { return Err(Status::invalid_argument("provider_name is required")); @@ -412,6 +461,7 @@ pub(super) async fn handle_detach_sandbox_provider( let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await; let sandbox = sandbox_by_name(state, &request.sandbox_name).await?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; let sandbox_id = sandbox .metadata .as_ref() @@ -488,18 +538,24 @@ async fn handle_delete_sandbox_inner( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let name = request.into_inner().name; if name.is_empty() { return Err(Status::invalid_argument("name is required")); } - let sandbox_id = state + // Ownership check: fetch sandbox, verify caller owns it before deleting. + let sandbox = state .store .get_message_by_name::(&name) .await - .ok() - .flatten() - .map(|sandbox| sandbox.object_id().to_string()); + .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))?; + + if let Some(ref sb) = sandbox { + check_sandbox_owner(sb, principal.as_ref(), state)?; + } + + let sandbox_id = sandbox.map(|sb| sb.object_id().to_string()); let deleted = state.compute.delete_sandbox(&name).await?; if deleted && let Some(sandbox_id) = sandbox_id { state.telemetry.end_sandbox_session(&sandbox_id); @@ -567,12 +623,22 @@ pub(super) async fn handle_watch_sandbox( state: &Arc, request: Request, ) -> Result>>, Status> { + let principal = request.extensions().get::().cloned(); let req = request.into_inner(); if req.id.is_empty() { return Err(Status::invalid_argument("id is required")); } let sandbox_id = req.id.clone(); + // Ownership check before subscribing to any buses. + let sandbox = state + .store + .get_message::(&sandbox_id) + .await + .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? + .ok_or_else(|| Status::not_found("sandbox not found"))?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; + let follow_status = req.follow_status; let follow_logs = req.follow_logs; let follow_events = req.follow_events; @@ -797,6 +863,7 @@ pub(super) async fn handle_exec_sandbox( ) -> Result>>, Status> { use openshell_core::ObjectId; + let principal = request.extensions().get::().cloned(); let req = request.into_inner(); if req.sandbox_id.is_empty() { return Err(Status::invalid_argument("sandbox_id is required")); @@ -817,6 +884,7 @@ pub(super) async fn handle_exec_sandbox( .await .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); @@ -912,6 +980,7 @@ pub(super) async fn handle_forward_tcp( >, Status, > { + let principal = request.extensions().get::().cloned(); let mut inbound = request.into_inner(); let first = inbound .message() @@ -931,6 +1000,7 @@ pub(super) async fn handle_forward_tcp( .await .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); @@ -1245,6 +1315,7 @@ pub(super) async fn handle_exec_sandbox_interactive( ) -> Result>>, Status> { use openshell_core::ObjectId; + let principal = request.extensions().get::().cloned(); let mut input_stream = request.into_inner(); let first_msg = input_stream @@ -1260,6 +1331,7 @@ pub(super) async fn handle_exec_sandbox_interactive( .await .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); @@ -1322,6 +1394,7 @@ pub(super) async fn handle_create_ssh_session( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let req = request.into_inner(); if req.sandbox_id.is_empty() { return Err(Status::invalid_argument("sandbox_id is required")); @@ -1333,6 +1406,7 @@ pub(super) async fn handle_create_ssh_session( .await .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); @@ -1398,6 +1472,7 @@ pub(super) async fn handle_revoke_ssh_session( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let token = request.into_inner().token; if token.is_empty() { return Err(Status::invalid_argument("token is required")); @@ -1413,6 +1488,13 @@ pub(super) async fn handle_revoke_ssh_session( return Ok(Response::new(RevokeSshSessionResponse { revoked: false })); }; + // Ownership check: look up the sandbox referenced by this session. + if !session.sandbox_id.is_empty() { + if let Ok(Some(sandbox)) = state.store.get_message::(&session.sandbox_id).await { + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; + } + } + let resource_version = session .metadata .as_ref() diff --git a/crates/openshell-server/src/grpc/service.rs b/crates/openshell-server/src/grpc/service.rs index 246d639be..eee0c47f1 100644 --- a/crates/openshell-server/src/grpc/service.rs +++ b/crates/openshell-server/src/grpc/service.rs @@ -25,6 +25,7 @@ pub(super) async fn handle_expose_service( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let req = request.into_inner(); validate_endpoint_name("sandbox", &req.sandbox, MAX_SANDBOX_NAME_LEN)?; validate_optional_endpoint_name("service", &req.service, MAX_SERVICE_NAME_LEN)?; @@ -38,6 +39,7 @@ pub(super) async fn handle_expose_service( .await .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; + crate::grpc::sandbox::check_sandbox_owner(&sandbox, principal.as_ref(), state)?; let now = crate::persistence::current_time_ms(); let key = service_routing::endpoint_key(&req.sandbox, &req.service); @@ -128,10 +130,20 @@ pub(super) async fn handle_get_service( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let req = request.into_inner(); validate_endpoint_name("sandbox", &req.sandbox, MAX_SANDBOX_NAME_LEN)?; validate_optional_endpoint_name("service", &req.service, MAX_SERVICE_NAME_LEN)?; + // Ownership check: verify caller owns the referenced sandbox. + let sandbox = state + .store + .get_message_by_name::(&req.sandbox) + .await + .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? + .ok_or_else(|| Status::not_found("sandbox not found"))?; + crate::grpc::sandbox::check_sandbox_owner(&sandbox, principal.as_ref(), state)?; + let endpoint = get_service_endpoint(state, &req.sandbox, &req.service) .await? .ok_or_else(|| Status::not_found("service endpoint not found"))?; @@ -171,10 +183,20 @@ pub(super) async fn handle_delete_service( state: &Arc, request: Request, ) -> Result, Status> { + let principal = request.extensions().get::().cloned(); let req = request.into_inner(); validate_endpoint_name("sandbox", &req.sandbox, MAX_SANDBOX_NAME_LEN)?; validate_optional_endpoint_name("service", &req.service, MAX_SERVICE_NAME_LEN)?; + // Ownership check: verify caller owns the referenced sandbox. + let sandbox = state + .store + .get_message_by_name::(&req.sandbox) + .await + .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? + .ok_or_else(|| Status::not_found("sandbox not found"))?; + crate::grpc::sandbox::check_sandbox_owner(&sandbox, principal.as_ref(), state)?; + let endpoint = get_service_endpoint(state, &req.sandbox, &req.service).await?; let Some(endpoint) = endpoint else { return Ok(Response::new(DeleteServiceResponse { deleted: false })); From 5fc83f408a925a7aed5f58760af83acb48864eb5 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Mon, 15 Jun 2026 22:40:54 -0400 Subject: [PATCH 2/3] style: apply rustfmt to ownership handler code Break long method chains across multiple lines to satisfy the project's rustfmt configuration. Signed-off-by: Paolo Dettori Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 7 +- crates/openshell-server/src/grpc/sandbox.rs | 71 +++++++++++++++---- crates/openshell-server/src/grpc/service.rs | 15 +++- 3 files changed, 72 insertions(+), 21 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index 60e04380f..f5b829575 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -103,9 +103,7 @@ pub fn check_owner( let caller_value = sanitize_subject(&identity.subject)?; if caller_value != *owner_value { - return Err(Status::permission_denied( - "you do not own this sandbox", - )); + return Err(Status::permission_denied("you do not own this sandbox")); } Ok(()) @@ -368,7 +366,8 @@ mod tests { #[test] fn owner_selector_anonymous_no_filter() { - let result = owner_selector(Some(&Principal::Anonymous), "env=dev", "openshell-admin").unwrap(); + let result = + owner_selector(Some(&Principal::Anonymous), "env=dev", "openshell-admin").unwrap(); assert_eq!(result, "env=dev"); } diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index f16d334ad..13afe803b 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -143,7 +143,10 @@ async fn handle_create_sandbox_inner( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let mut request = request.into_inner(); let spec = request .spec @@ -260,7 +263,10 @@ pub(super) async fn handle_get_sandbox( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let name = request.into_inner().name; if name.is_empty() { return Err(Status::invalid_argument("name is required")); @@ -283,7 +289,10 @@ pub(super) async fn handle_list_sandboxes( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); let limit = clamp_limit(request.limit, 100, MAX_PAGE_SIZE); @@ -317,7 +326,10 @@ pub(super) async fn handle_list_sandbox_providers( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let sandbox = sandbox_by_name(state, &request.into_inner().sandbox_name).await?; check_sandbox_owner(&sandbox, principal.as_ref(), state)?; let providers = providers_for_sandbox(state, &sandbox).await?; @@ -328,7 +340,10 @@ pub(super) async fn handle_attach_sandbox_provider( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); if request.provider_name.is_empty() { return Err(Status::invalid_argument("provider_name is required")); @@ -444,7 +459,10 @@ pub(super) async fn handle_detach_sandbox_provider( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let request = request.into_inner(); if request.provider_name.is_empty() { return Err(Status::invalid_argument("provider_name is required")); @@ -538,7 +556,10 @@ async fn handle_delete_sandbox_inner( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let name = request.into_inner().name; if name.is_empty() { return Err(Status::invalid_argument("name is required")); @@ -623,7 +644,10 @@ pub(super) async fn handle_watch_sandbox( state: &Arc, request: Request, ) -> Result>>, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); if req.id.is_empty() { return Err(Status::invalid_argument("id is required")); @@ -863,7 +887,10 @@ pub(super) async fn handle_exec_sandbox( ) -> Result>>, Status> { use openshell_core::ObjectId; - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); if req.sandbox_id.is_empty() { return Err(Status::invalid_argument("sandbox_id is required")); @@ -980,7 +1007,10 @@ pub(super) async fn handle_forward_tcp( >, Status, > { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let mut inbound = request.into_inner(); let first = inbound .message() @@ -1315,7 +1345,10 @@ pub(super) async fn handle_exec_sandbox_interactive( ) -> Result>>, Status> { use openshell_core::ObjectId; - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let mut input_stream = request.into_inner(); let first_msg = input_stream @@ -1394,7 +1427,10 @@ pub(super) async fn handle_create_ssh_session( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); if req.sandbox_id.is_empty() { return Err(Status::invalid_argument("sandbox_id is required")); @@ -1472,7 +1508,10 @@ pub(super) async fn handle_revoke_ssh_session( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let token = request.into_inner().token; if token.is_empty() { return Err(Status::invalid_argument("token is required")); @@ -1490,7 +1529,11 @@ pub(super) async fn handle_revoke_ssh_session( // Ownership check: look up the sandbox referenced by this session. if !session.sandbox_id.is_empty() { - if let Ok(Some(sandbox)) = state.store.get_message::(&session.sandbox_id).await { + if let Ok(Some(sandbox)) = state + .store + .get_message::(&session.sandbox_id) + .await + { check_sandbox_owner(&sandbox, principal.as_ref(), state)?; } } diff --git a/crates/openshell-server/src/grpc/service.rs b/crates/openshell-server/src/grpc/service.rs index eee0c47f1..6557f6537 100644 --- a/crates/openshell-server/src/grpc/service.rs +++ b/crates/openshell-server/src/grpc/service.rs @@ -25,7 +25,10 @@ pub(super) async fn handle_expose_service( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); validate_endpoint_name("sandbox", &req.sandbox, MAX_SANDBOX_NAME_LEN)?; validate_optional_endpoint_name("service", &req.service, MAX_SERVICE_NAME_LEN)?; @@ -130,7 +133,10 @@ pub(super) async fn handle_get_service( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); validate_endpoint_name("sandbox", &req.sandbox, MAX_SANDBOX_NAME_LEN)?; validate_optional_endpoint_name("service", &req.service, MAX_SERVICE_NAME_LEN)?; @@ -183,7 +189,10 @@ pub(super) async fn handle_delete_service( state: &Arc, request: Request, ) -> Result, Status> { - let principal = request.extensions().get::().cloned(); + let principal = request + .extensions() + .get::() + .cloned(); let req = request.into_inner(); validate_endpoint_name("sandbox", &req.sandbox, MAX_SANDBOX_NAME_LEN)?; validate_optional_endpoint_name("service", &req.service, MAX_SERVICE_NAME_LEN)?; From f2fa5bd671ff1621f7be1e6796d6c166204714c1 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Mon, 15 Jun 2026 22:47:05 -0400 Subject: [PATCH 3/3] fix: resolve clippy lints in ownership module - Move `use sha2` and `use std::fmt::Write` to module top (items-after-statements) - Add `#![allow(clippy::result_large_err)]` (consistent with other handlers) - Use `let...else` instead of match for early returns (manual-let-else) - Nest or-patterns in require_identity (unnested-or-patterns) - Collapse nested if in revoke_ssh handler (collapsible-if) Signed-off-by: Paolo Dettori Signed-off-by: Paolo Dettori --- crates/openshell-server/src/auth/ownership.rs | 32 ++++++++----------- crates/openshell-server/src/grpc/sandbox.rs | 9 +++--- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/crates/openshell-server/src/auth/ownership.rs b/crates/openshell-server/src/auth/ownership.rs index f5b829575..6fb32a3c9 100644 --- a/crates/openshell-server/src/auth/ownership.rs +++ b/crates/openshell-server/src/auth/ownership.rs @@ -7,8 +7,12 @@ //! verified caller identity) and gates access on subsequent operations. Admin //! callers bypass the ownership check. +#![allow(clippy::result_large_err)] + use std::collections::HashMap; +use std::fmt::Write; +use sha2::{Digest, Sha256}; use tonic::Status; use super::identity::Identity; @@ -39,8 +43,6 @@ pub fn sanitize_subject(subject: &str) -> Result { } // Fallback: hex SHA-256 truncated to 63 chars (always valid). - use sha2::{Digest, Sha256}; - use std::fmt::Write; let hash = Sha256::digest(subject.as_bytes()); let mut hex = String::with_capacity(64); for byte in &hash { @@ -86,14 +88,10 @@ pub fn check_owner( return Ok(()); }; - let identity = match principal_identity(principal) { - Some(id) => id, - None => { - // Anonymous caller on an owned sandbox → deny. - return Err(Status::permission_denied( - "sandbox is owned; authenticated identity required", - )); - } + let Some(identity) = principal_identity(principal) else { + return Err(Status::permission_denied( + "sandbox is owned; authenticated identity required", + )); }; // Admin bypass. @@ -118,12 +116,8 @@ pub fn owner_selector( existing_selector: &str, admin_role: &str, ) -> Result { - let identity = match principal_identity(principal) { - Some(id) => id, - None => { - // Anonymous → no owner filtering (backward compat). - return Ok(existing_selector.to_string()); - } + let Some(identity) = principal_identity(principal) else { + return Ok(existing_selector.to_string()); }; // Admin bypass — see all sandboxes. @@ -145,9 +139,9 @@ pub fn owner_selector( fn require_identity(principal: Option<&Principal>) -> Result<&Identity, Status> { match principal { Some(Principal::User(user)) => Ok(&user.identity), - Some(Principal::Sandbox(_)) | Some(Principal::Anonymous) | None => Err( - Status::unauthenticated("authenticated user identity required for sandbox operations"), - ), + Some(Principal::Sandbox(_) | Principal::Anonymous) | None => Err(Status::unauthenticated( + "authenticated user identity required for sandbox operations", + )), } } diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 13afe803b..1114709ea 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -1528,14 +1528,13 @@ pub(super) async fn handle_revoke_ssh_session( }; // Ownership check: look up the sandbox referenced by this session. - if !session.sandbox_id.is_empty() { - if let Ok(Some(sandbox)) = state + if !session.sandbox_id.is_empty() + && let Ok(Some(sandbox)) = state .store .get_message::(&session.sandbox_id) .await - { - check_sandbox_owner(&sandbox, principal.as_ref(), state)?; - } + { + check_sandbox_owner(&sandbox, principal.as_ref(), state)?; } let resource_version = session