mirror of
https://github.com/openai/codex.git
synced 2026-11-14 14:49:31 +00:00
Protect shell snapshots when credential brokerage is enabled (#43909)
## Why Shell startup files can introduce credentials or overwrite brokered dummy values. Snapshot capture and replay need to respect the command's sandbox and environment policy without persisting real credentials. ## What changed - Capture and validate brokered snapshots per command using its requested shell, working directory, login mode, sandbox, and environment policy. - Discover shell-initialized credentials and prepare snapshot exports for brokered replay. Preserve shell functions and aliases while preventing startup files from overwriting dummy credentials during replay. - Rebuild snapshots as credential broker state changes and retain each command's snapshot for the lifetime of its process. - Reject brokered POSIX shell execution when a protected snapshot cannot be created, and reject brokerage with `shell_zsh_fork`. Restore real credentials and remove brokerage markers for unsupported PowerShell and Cmd execution; escalated commands bypass protected snapshots. ## Testing Add regression coverage for credential discovery and redaction, environment filtering, readonly exports, shell function and heredoc replay, command working directories and login modes, sandbox enforcement, and missing or disabled snapshots. GitOrigin-RevId: 685356cb9e80f11cf69d053e4d239ee6ccf43a9e
This commit is contained in:
1 parent
1530f828cb
commit
2e220af1f6
25 files changed
+2080
-133
No files matched your search
@@ -46,6 +46,7 @@ use crate::session::step_settings::ResolvedStepSettings;
|
||||
use crate::session::step_settings::StepSettings;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::session_prefix::format_inter_agent_completion_message;
|
||||
use crate::shell_snapshot::SnapshotCredentialBrokerState;
|
||||
use crate::skills_load_input_from_config;
|
||||
use crate::stream_events_utils::mark_thread_memory_mode_polluted_if_external_context;
|
||||
use crate::turn_metadata::TurnMetadataState;
|
||||
@@ -1165,6 +1166,9 @@ impl Session {
|
||||
.cloned()
|
||||
else {
|
||||
self.services.network_proxy.store(None);
|
||||
self.services
|
||||
.turn_environments
|
||||
.set_snapshot_credential_broker(SnapshotCredentialBrokerState::Inactive);
|
||||
return;
|
||||
};
|
||||
|
||||
@@ -1191,11 +1195,22 @@ impl Session {
|
||||
// listeners and must not be exposed as active managed proxy runtimes.
|
||||
if !spec.enabled() {
|
||||
self.services.network_proxy.store(None);
|
||||
self.services
|
||||
.turn_environments
|
||||
.set_snapshot_credential_broker(SnapshotCredentialBrokerState::Inactive);
|
||||
return;
|
||||
}
|
||||
if let Some(started_proxy) = self.services.network_proxy.load_full() {
|
||||
if let Err(err) = spec.apply_to_started_proxy(started_proxy.as_ref()).await {
|
||||
warn!("failed to refresh managed network proxy for sandbox change: {err}");
|
||||
} else {
|
||||
self.services
|
||||
.turn_environments
|
||||
.set_snapshot_credential_broker(if spec.credential_broker_enabled() {
|
||||
SnapshotCredentialBrokerState::Ready(started_proxy.proxy())
|
||||
} else {
|
||||
SnapshotCredentialBrokerState::Inactive
|
||||
});
|
||||
}
|
||||
return;
|
||||
}
|
||||
@@ -1214,11 +1229,25 @@ impl Session {
|
||||
.await
|
||||
{
|
||||
Ok((started_proxy, _session_network_proxy)) => {
|
||||
if spec.credential_broker_enabled() {
|
||||
self.services
|
||||
.turn_environments
|
||||
.set_snapshot_credential_broker(SnapshotCredentialBrokerState::Ready(
|
||||
started_proxy.proxy(),
|
||||
));
|
||||
}
|
||||
self.services
|
||||
.network_proxy
|
||||
.store(Some(Arc::new(started_proxy)));
|
||||
}
|
||||
Err(err) => {
|
||||
self.services
|
||||
.turn_environments
|
||||
.set_snapshot_credential_broker(if spec.credential_broker_enabled() {
|
||||
SnapshotCredentialBrokerState::Unavailable
|
||||
} else {
|
||||
SnapshotCredentialBrokerState::Inactive
|
||||
});
|
||||
warn!("failed to start managed network proxy for sandbox change: {err}");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,6 +14,7 @@ use crate::hook_mcp_executor::CoreHookMcpExecutor;
|
||||
use crate::responses_metadata::CodexResponsesMetadata;
|
||||
use crate::responses_metadata::CodexResponsesRequestKind;
|
||||
use crate::shell_snapshot::ShellSnapshot;
|
||||
use crate::shell_snapshot::SnapshotCredentialBrokerState;
|
||||
use crate::state::ActiveTurn;
|
||||
use codex_extension_api::ExtensionDataInit;
|
||||
use codex_http_client::ClientRouteClass;
|
||||
@@ -1091,13 +1092,10 @@ impl Session {
|
||||
}),
|
||||
});
|
||||
}
|
||||
let effective_config = config.config_layer_stack.effective_config();
|
||||
let config_path = config.codex_home.join(CONFIG_TOML_FILE);
|
||||
if let Some(event) = unstable_features_warning_event(
|
||||
config
|
||||
.config_layer_stack
|
||||
.effective_config()
|
||||
.get("features")
|
||||
.and_then(TomlValue::as_table),
|
||||
effective_config.get("features").and_then(TomlValue::as_table),
|
||||
config.suppress_unstable_features_warning,
|
||||
&config.features,
|
||||
&config_path.display().to_string(),
|
||||
@@ -1217,7 +1215,27 @@ impl Session {
|
||||
} else {
|
||||
shell::default_user_shell()
|
||||
};
|
||||
let use_executor_shell_snapshots = config.features.enabled(Feature::ShellSnapshotV2)
|
||||
let credential_broker_available = config.features.enabled(Feature::NetworkProxy)
|
||||
&& config
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.network
|
||||
.as_ref()
|
||||
.is_none_or(|network| network.value.enabled != Some(false));
|
||||
let credential_broker_configured = credential_broker_available
|
||||
&& effective_config
|
||||
.get("features")
|
||||
.and_then(|features| features.get("network_proxy"))
|
||||
.and_then(|network_proxy| network_proxy.get("credential_broker"))
|
||||
.and_then(TomlValue::as_bool)
|
||||
.unwrap_or(false);
|
||||
let credential_broker_active = credential_broker_configured
|
||||
&& config
|
||||
.permissions
|
||||
.network
|
||||
.as_ref()
|
||||
.is_some_and(crate::config::NetworkProxySpec::credential_broker_enabled);
|
||||
let prefer_executor_shell_snapshots = config.features.enabled(Feature::ShellSnapshotV2)
|
||||
&& config.features.enabled(Feature::ShellTool)
|
||||
&& config.features.enabled(Feature::UnifiedExec)
|
||||
&& matches!(
|
||||
@@ -1229,14 +1247,26 @@ impl Session {
|
||||
),
|
||||
codex_tools::UnifiedExecShellMode::Direct
|
||||
);
|
||||
let use_executor_shell_snapshots =
|
||||
prefer_executor_shell_snapshots && !credential_broker_active;
|
||||
let shell_snapshot = if config.features.enabled(Feature::ShellSnapshot)
|
||||
&& !use_executor_shell_snapshots
|
||||
&& (!use_executor_shell_snapshots || credential_broker_available)
|
||||
{
|
||||
let snapshot_credential_broker = credential_broker_available.then(|| {
|
||||
let state = if credential_broker_active {
|
||||
SnapshotCredentialBrokerState::Starting
|
||||
} else {
|
||||
SnapshotCredentialBrokerState::Inactive
|
||||
};
|
||||
watch::channel(state).0
|
||||
});
|
||||
ShellSnapshot::new(
|
||||
config.codex_home.clone(),
|
||||
thread_id,
|
||||
session_telemetry.clone(),
|
||||
state_db_ctx.clone(),
|
||||
snapshot_credential_broker,
|
||||
prefer_executor_shell_snapshots,
|
||||
)
|
||||
} else {
|
||||
ShellSnapshot::disabled()
|
||||
@@ -1355,6 +1385,17 @@ impl Session {
|
||||
} else {
|
||||
(None, None)
|
||||
};
|
||||
if let Some(network_proxy) = network_proxy.as_ref()
|
||||
&& config
|
||||
.permissions
|
||||
.network
|
||||
.as_ref()
|
||||
.is_some_and(crate::config::NetworkProxySpec::credential_broker_enabled)
|
||||
{
|
||||
turn_environments.set_snapshot_credential_broker(
|
||||
SnapshotCredentialBrokerState::Ready(network_proxy.proxy()),
|
||||
);
|
||||
}
|
||||
|
||||
// Hooks and extensions share one stable thread-owned MCP runtime handle.
|
||||
let mcp_runtime = Arc::new(McpRuntime::empty(
|
||||
|
||||
@@ -5644,8 +5644,12 @@ async fn session_configuration_apply_permission_profile_accepts_direct_write_roo
|
||||
);
|
||||
}
|
||||
|
||||
#[test_case::test_case(false; "ordinary_proxy")]
|
||||
#[test_case::test_case(true; "credential_broker")]
|
||||
#[tokio::test]
|
||||
async fn active_profile_update_rebuilds_network_proxy_config() -> std::io::Result<()> {
|
||||
async fn active_profile_update_rebuilds_network_proxy_config(
|
||||
credential_broker: bool,
|
||||
) -> std::io::Result<()> {
|
||||
let codex_home = tempfile::tempdir().expect("create codex home");
|
||||
let cwd = tempfile::tempdir().expect("create cwd");
|
||||
let permissions = PermissionsToml {
|
||||
@@ -5690,7 +5694,14 @@ async fn active_profile_update_rebuilds_network_proxy_config() -> std::io::Resul
|
||||
]),
|
||||
};
|
||||
let base_config = ConfigToml {
|
||||
features: Some(toml::from_str("network_proxy = true").expect("valid features")),
|
||||
features: Some(
|
||||
toml::from_str(if credential_broker {
|
||||
"network_proxy = { enabled = true, credential_broker = true }"
|
||||
} else {
|
||||
"network_proxy = true"
|
||||
})
|
||||
.expect("valid features"),
|
||||
),
|
||||
default_permissions: Some("locked-down".to_string()),
|
||||
permissions: Some(permissions),
|
||||
..Default::default()
|
||||
@@ -5752,6 +5763,7 @@ async fn active_profile_update_rebuilds_network_proxy_config() -> std::io::Resul
|
||||
.expect("selected profile proxy should become the session proxy config");
|
||||
assert_eq!(network.proxy_host_and_port(), "127.0.0.1:43128");
|
||||
assert!(!network.socks_enabled());
|
||||
assert_eq!(network.credential_broker_enabled(), credential_broker);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -6,7 +6,9 @@ use crate::config::TokenBudgetConfig;
|
||||
use crate::environment_selection::EnvironmentConfigOrigin;
|
||||
use crate::environment_selection::TurnEnvironmentSnapshot;
|
||||
use crate::exec_policy::AllowPrefixRules;
|
||||
use crate::shell_snapshot::ShellSnapshot;
|
||||
use crate::shell_snapshot::ShellSnapshotFile;
|
||||
use crate::shell_snapshot::ShellSnapshotSandbox;
|
||||
use crate::tools::sandboxing::executor_windows_sandbox_level;
|
||||
use arc_swap::ArcSwap;
|
||||
use codex_core_plugins::PluginCommandAttribution;
|
||||
@@ -56,6 +58,7 @@ pub(crate) struct TurnEnvironment {
|
||||
/// OS reported by the selected executor; `None` for legacy executors.
|
||||
pub(crate) executor_platform_os: Option<String>,
|
||||
pub(crate) shell_snapshot: ShellSnapshotTask,
|
||||
pub(crate) shell_snapshot_builder: Option<Box<ShellSnapshot>>,
|
||||
pub(crate) shell_snapshot_v2_supported: bool,
|
||||
}
|
||||
|
||||
@@ -76,6 +79,7 @@ impl TurnEnvironment {
|
||||
shell,
|
||||
executor_platform_os: None,
|
||||
shell_snapshot: futures::future::ready(None).boxed().shared(),
|
||||
shell_snapshot_builder: None,
|
||||
shell_snapshot_v2_supported: false,
|
||||
}
|
||||
}
|
||||
@@ -99,14 +103,47 @@ impl TurnEnvironment {
|
||||
config
|
||||
}
|
||||
|
||||
pub(crate) fn shell_snapshot(&self, cwd: &AbsolutePathBuf) -> Option<AbsolutePathBuf> {
|
||||
if self.selection.cwd != PathUri::from_abs_path(cwd) {
|
||||
pub(crate) async fn shell_snapshot(
|
||||
&self,
|
||||
cwd: &AbsolutePathBuf,
|
||||
command: &[String],
|
||||
shell: &shell::Shell,
|
||||
config: &Config,
|
||||
sandbox: Option<ShellSnapshotSandbox>,
|
||||
) -> Option<Arc<ShellSnapshotFile>> {
|
||||
let credential_broker_enabled = config
|
||||
.permissions
|
||||
.network
|
||||
.as_ref()
|
||||
.is_some_and(|spec| spec.enabled() && spec.credential_broker_enabled());
|
||||
let cwd_matches_selection = self.selection.cwd == PathUri::from_abs_path(cwd);
|
||||
if !config.features.enabled(Feature::ShellSnapshot)
|
||||
|| (!cwd_matches_selection && !credential_broker_enabled)
|
||||
|| command.len() < 3
|
||||
|| !matches!(command[1].as_str(), "-lc" | "-c")
|
||||
|| (command[1] == "-c" && !credential_broker_enabled)
|
||||
{
|
||||
return None;
|
||||
}
|
||||
self.shell_snapshot
|
||||
.peek()?
|
||||
.as_deref()
|
||||
.map(ShellSnapshotFile::path)
|
||||
if credential_broker_enabled {
|
||||
let sandbox = sandbox?;
|
||||
self.shell_snapshot_builder
|
||||
.as_ref()?
|
||||
.as_ref()
|
||||
.clone()
|
||||
.build(
|
||||
Arc::clone(&self.environment),
|
||||
PathUri::from_abs_path(cwd),
|
||||
Some(shell.clone()),
|
||||
command[1] == "-lc",
|
||||
self.shell_environment_policy().clone(),
|
||||
Some(sandbox),
|
||||
)
|
||||
.await
|
||||
.filter(|snapshot| snapshot.is_brokered_for(self.shell_environment_policy()))
|
||||
} else {
|
||||
self.shell_snapshot.peek()?.clone()
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn cwd(&self) -> &PathUri {
|
||||
|
||||
Reference in new issue
Block a user