From 31f9b9bdbb919f91f69917cccd6663e229040bd2 Mon Sep 17 00:00:00 2001 From: Zhidong Peng Date: Thu, 11 Jun 2026 20:32:33 +0000 Subject: [PATCH 1/2] do not authenticate the request if it designed to be skipped --- proxy_agent/src/proxy/proxy_server.rs | 85 ++++++++++++++------------- 1 file changed, 44 insertions(+), 41 deletions(-) diff --git a/proxy_agent/src/proxy/proxy_server.rs b/proxy_agent/src/proxy/proxy_server.rs index 5d1dc5ea..ed8a6157 100644 --- a/proxy_agent/src/proxy/proxy_server.rs +++ b/proxy_agent/src/proxy/proxy_server.rs @@ -475,53 +475,56 @@ impl ProxyServer { }; http_connection_context.log(LoggerLevel::Trace, claim_details.to_string()); - // authenticate the connection - let access_control_rules = match proxy_authorizer::get_access_control_rules( - ip.to_string(), - port, - self.access_control_shared_state.clone(), - ) - .await - { - Ok(rules) => rules, - Err(e) => { - self.log_connection_summary( - &mut http_connection_context, - StatusCode::INTERNAL_SERVER_ERROR, - false, - format!("Failed to get access control rules: {e}"), - ) - .await; - return Ok(Self::closed_response(StatusCode::INTERNAL_SERVER_ERROR)); - } - }; - http_connection_context.log(LoggerLevel::Trace, "Authorizing the request.".to_string()); - let result = proxy_authorizer::authorize( - ip.to_string(), - port, - http_connection_context.get_logger_mut_ref(), - request.uri().clone(), - claims.clone(), - access_control_rules, - ); - if result != AuthorizeResult::Ok { - // log to authorize failed connection summary - self.log_connection_summary( - &mut http_connection_context, - StatusCode::FORBIDDEN, - true, - "Authorize failed".to_string(), + // do not authenticate if the request is to be skipped + if !http_connection_context.should_skip_sig() { + // authenticate the connection + let access_control_rules = match proxy_authorizer::get_access_control_rules( + ip.to_string(), + port, + self.access_control_shared_state.clone(), ) - .await; - if result == AuthorizeResult::Forbidden { + .await + { + Ok(rules) => rules, + Err(e) => { + self.log_connection_summary( + &mut http_connection_context, + StatusCode::INTERNAL_SERVER_ERROR, + false, + format!("Failed to get access control rules: {e}"), + ) + .await; + return Ok(Self::closed_response(StatusCode::INTERNAL_SERVER_ERROR)); + } + }; + http_connection_context.log(LoggerLevel::Trace, "Authorizing the request.".to_string()); + let result = proxy_authorizer::authorize( + ip.to_string(), + port, + http_connection_context.get_logger_mut_ref(), + request.uri().clone(), + claims.clone(), + access_control_rules, + ); + if result != AuthorizeResult::Ok { + // log to authorize failed connection summary self.log_connection_summary( &mut http_connection_context, StatusCode::FORBIDDEN, - false, - format!("Block unauthorized request: {claim_details}"), + true, + "Authorize failed".to_string(), ) .await; - return Ok(Self::closed_response(StatusCode::FORBIDDEN)); + if result == AuthorizeResult::Forbidden { + self.log_connection_summary( + &mut http_connection_context, + StatusCode::FORBIDDEN, + false, + format!("Block unauthorized request: {claim_details}"), + ) + .await; + return Ok(Self::closed_response(StatusCode::FORBIDDEN)); + } } } From d6ad6ae471208c4435ac332df49c838fdab19644 Mon Sep 17 00:00:00 2001 From: Zhidong Peng Date: Fri, 12 Jun 2026 16:31:57 +0000 Subject: [PATCH 2/2] move should_skip_sig check away from the main workflow --- proxy_agent/src/proxy/proxy_authorizer.rs | 140 ++++++++++++++++++++++ proxy_agent/src/proxy/proxy_server.rs | 86 +++++++------ 2 files changed, 182 insertions(+), 44 deletions(-) diff --git a/proxy_agent/src/proxy/proxy_authorizer.rs b/proxy_agent/src/proxy/proxy_authorizer.rs index 23d54dc5..f4f8a28d 100644 --- a/proxy_agent/src/proxy/proxy_authorizer.rs +++ b/proxy_agent/src/proxy/proxy_authorizer.rs @@ -23,6 +23,7 @@ use super::authorization_rules::{AuthorizationMode, ComputedAuthorizationItem}; use super::proxy_connection::ConnectionLogger; use crate::shared_state::access_control_wrapper::AccessControlSharedState; use crate::{common::constants, common::result::Result, proxy::Claims}; +use proxy_agent_shared::hyper_client; use proxy_agent_shared::logger::LoggerLevel; #[derive(PartialEq)] @@ -232,9 +233,16 @@ pub fn authorize( port: u16, logger: &mut ConnectionLogger, request_uri: hyper::Uri, + request_method: hyper::Method, claims: Claims, access_control_rules: Option, ) -> AuthorizeResult { + // If the request should skip signature and the claims indicate elevated privileges, allow the request. + // This is a security measure to allow certain endpoints are exempt from enforcement regardless of the VM's configuration. + if hyper_client::should_skip_sig(&request_method, &request_uri) && claims.runAsElevated { + return AuthorizeResult::Ok; + } + let auth = get_authorizer(ip, port, claims); logger.write( LoggerLevel::Trace, @@ -685,4 +693,136 @@ mod tests { "HostGA authentication must be Forbidden with enforce deny rules" ); } + + #[tokio::test] + async fn authorize_skip_sig_test() { + // Build claims for an elevated and a non-elevated process. + let elevated_claims = crate::proxy::Claims { + userId: 0, + userName: "test".to_string(), + userGroups: vec!["test".to_string()], + processId: std::process::id(), + processName: OsString::from("test"), + processFullPath: PathBuf::from("test"), + processCmdLine: "test".to_string(), + runAsElevated: true, + clientIp: "127.0.0.1".to_string(), + clientPort: 0, + }; + let mut non_elevated_claims = elevated_claims.clone(); + non_elevated_claims.runAsElevated = false; + + let mut test_logger = ConnectionLogger::new(2, 2); + + // Set up an enforce-deny rule for WireServer so the normal authorize path + // would return Forbidden if the skip-sig early return did not take effect. + let access_control_shared_state = AccessControlSharedState::start_new(); + let enforce_deny_rules = AuthorizationItem { + defaultAccess: "deny".to_string(), + mode: "enforce".to_string(), + id: "id".to_string(), + rules: None, + }; + access_control_shared_state + .set_wireserver_rules(Some(enforce_deny_rules)) + .await + .unwrap(); + let wireserver_deny_rules = access_control_shared_state + .get_wireserver_rules() + .await + .unwrap(); + + // Skip-sig URIs (per hyper_client::should_skip_sig): + // o PUT /vmAgentLog + // o POST /machine/?comp=telemetrydata + let vm_agent_log_uri = hyper::Uri::from_str("/vmAgentLog").unwrap(); + let telemetry_uri = hyper::Uri::from_str("/machine/?comp=telemetrydata").unwrap(); + let non_skip_uri = hyper::Uri::from_str("http://localhost/test?").unwrap(); + + // 1. Skip-sig URI + elevated claims => Ok even though rule is enforce-deny. + assert!( + super::authorize( + crate::common::constants::WIRE_SERVER_IP.to_string(), + crate::common::constants::WIRE_SERVER_PORT, + &mut test_logger, + vm_agent_log_uri.clone(), + hyper::Method::PUT, + elevated_claims.clone(), + wireserver_deny_rules.clone(), + ) == AuthorizeResult::Ok, + "PUT /vmAgentLog with elevated claims must be Ok regardless of deny rules" + ); + assert!( + super::authorize( + crate::common::constants::WIRE_SERVER_IP.to_string(), + crate::common::constants::WIRE_SERVER_PORT, + &mut test_logger, + telemetry_uri.clone(), + hyper::Method::POST, + elevated_claims.clone(), + wireserver_deny_rules.clone(), + ) == AuthorizeResult::Ok, + "POST /machine/?comp=telemetrydata with elevated claims must be Ok regardless of deny rules" + ); + + // 2. Skip-sig URI but the claims are NOT elevated => must fall through to the + // normal authorizer, which forbids WireServer access for non-elevated callers. + assert!( + super::authorize( + crate::common::constants::WIRE_SERVER_IP.to_string(), + crate::common::constants::WIRE_SERVER_PORT, + &mut test_logger, + vm_agent_log_uri.clone(), + hyper::Method::PUT, + non_elevated_claims.clone(), + wireserver_deny_rules.clone(), + ) == AuthorizeResult::Forbidden, + "Skip-sig URI without elevated claims must fall through and be Forbidden" + ); + + // 3. Skip-sig URI methods that do not match the exempt method => still enforced. + // GET /vmAgentLog is not exempt, so the deny rule applies. + assert!( + super::authorize( + crate::common::constants::WIRE_SERVER_IP.to_string(), + crate::common::constants::WIRE_SERVER_PORT, + &mut test_logger, + vm_agent_log_uri.clone(), + hyper::Method::GET, + elevated_claims.clone(), + wireserver_deny_rules.clone(), + ) == AuthorizeResult::Forbidden, + "GET /vmAgentLog is not skip-sig and must be Forbidden under enforce-deny" + ); + + // 4. Non-skip-sig URI with elevated claims => normal authorizer path is used. + // Under enforce-deny the request must be Forbidden. + assert!( + super::authorize( + crate::common::constants::WIRE_SERVER_IP.to_string(), + crate::common::constants::WIRE_SERVER_PORT, + &mut test_logger, + non_skip_uri.clone(), + hyper::Method::GET, + elevated_claims.clone(), + wireserver_deny_rules.clone(), + ) == AuthorizeResult::Forbidden, + "Non skip-sig request must follow access control rules (Forbidden under enforce-deny)" + ); + + // 5. Skip-sig URI + elevated claims must also short-circuit for endpoints whose + // authorizer would otherwise reject everything (e.g. the ProxyAgent listener). + assert!( + super::authorize( + crate::common::constants::PROXY_AGENT_IP.to_string(), + crate::common::constants::PROXY_AGENT_PORT, + &mut test_logger, + telemetry_uri.clone(), + hyper::Method::POST, + elevated_claims.clone(), + None, + ) == AuthorizeResult::Ok, + "Skip-sig + elevated claims must short-circuit even on the ProxyAgent listener" + ); + } } diff --git a/proxy_agent/src/proxy/proxy_server.rs b/proxy_agent/src/proxy/proxy_server.rs index ed8a6157..9cd2258d 100644 --- a/proxy_agent/src/proxy/proxy_server.rs +++ b/proxy_agent/src/proxy/proxy_server.rs @@ -475,56 +475,54 @@ impl ProxyServer { }; http_connection_context.log(LoggerLevel::Trace, claim_details.to_string()); - // do not authenticate if the request is to be skipped - if !http_connection_context.should_skip_sig() { - // authenticate the connection - let access_control_rules = match proxy_authorizer::get_access_control_rules( - ip.to_string(), - port, - self.access_control_shared_state.clone(), + // authenticate the connection + let access_control_rules = match proxy_authorizer::get_access_control_rules( + ip.to_string(), + port, + self.access_control_shared_state.clone(), + ) + .await + { + Ok(rules) => rules, + Err(e) => { + self.log_connection_summary( + &mut http_connection_context, + StatusCode::INTERNAL_SERVER_ERROR, + false, + format!("Failed to get access control rules: {e}"), + ) + .await; + return Ok(Self::closed_response(StatusCode::INTERNAL_SERVER_ERROR)); + } + }; + http_connection_context.log(LoggerLevel::Trace, "Authorizing the request.".to_string()); + let result = proxy_authorizer::authorize( + ip.to_string(), + port, + http_connection_context.get_logger_mut_ref(), + request.uri().clone(), + request.method().clone(), + claims.clone(), + access_control_rules, + ); + if result != AuthorizeResult::Ok { + // log to authorize failed connection summary + self.log_connection_summary( + &mut http_connection_context, + StatusCode::FORBIDDEN, + true, + "Authorize failed".to_string(), ) - .await - { - Ok(rules) => rules, - Err(e) => { - self.log_connection_summary( - &mut http_connection_context, - StatusCode::INTERNAL_SERVER_ERROR, - false, - format!("Failed to get access control rules: {e}"), - ) - .await; - return Ok(Self::closed_response(StatusCode::INTERNAL_SERVER_ERROR)); - } - }; - http_connection_context.log(LoggerLevel::Trace, "Authorizing the request.".to_string()); - let result = proxy_authorizer::authorize( - ip.to_string(), - port, - http_connection_context.get_logger_mut_ref(), - request.uri().clone(), - claims.clone(), - access_control_rules, - ); - if result != AuthorizeResult::Ok { - // log to authorize failed connection summary + .await; + if result == AuthorizeResult::Forbidden { self.log_connection_summary( &mut http_connection_context, StatusCode::FORBIDDEN, - true, - "Authorize failed".to_string(), + false, + format!("Block unauthorized request: {claim_details}"), ) .await; - if result == AuthorizeResult::Forbidden { - self.log_connection_summary( - &mut http_connection_context, - StatusCode::FORBIDDEN, - false, - format!("Block unauthorized request: {claim_details}"), - ) - .await; - return Ok(Self::closed_response(StatusCode::FORBIDDEN)); - } + return Ok(Self::closed_response(StatusCode::FORBIDDEN)); } }