mirror of
https://github.com/openai/codex.git
synced 2026-10-23 02:08:39 +00:00
fix(core): constrain plaintext credential injection
Co-authored-by: Codex noreply@openai.com
This commit is contained in:
committed by
Winston Howes
parent
ed623147c1
commit
259af7522f
@@ -359,6 +359,7 @@ pub struct NetworkRequirementsToml {
|
||||
pub allow_upstream_proxy: Option<bool>,
|
||||
pub dangerously_allow_non_loopback_proxy: Option<bool>,
|
||||
pub dangerously_allow_all_unix_sockets: Option<bool>,
|
||||
pub dangerously_allow_plaintext_credential_injection: Option<bool>,
|
||||
pub domains: Option<NetworkDomainPermissionsToml>,
|
||||
/// When true, only managed `allowed_domains` are respected while managed
|
||||
/// network enforcement is active. User allowlist entries are ignored.
|
||||
@@ -375,6 +376,7 @@ struct RawNetworkRequirementsToml {
|
||||
allow_upstream_proxy: Option<bool>,
|
||||
dangerously_allow_non_loopback_proxy: Option<bool>,
|
||||
dangerously_allow_all_unix_sockets: Option<bool>,
|
||||
dangerously_allow_plaintext_credential_injection: Option<bool>,
|
||||
domains: Option<NetworkDomainPermissionsToml>,
|
||||
#[serde(default)]
|
||||
allowed_domains: Option<Vec<String>>,
|
||||
@@ -402,6 +404,7 @@ impl<'de> Deserialize<'de> for NetworkRequirementsToml {
|
||||
allow_upstream_proxy,
|
||||
dangerously_allow_non_loopback_proxy,
|
||||
dangerously_allow_all_unix_sockets,
|
||||
dangerously_allow_plaintext_credential_injection,
|
||||
domains,
|
||||
allowed_domains,
|
||||
managed_allowed_domains_only,
|
||||
@@ -430,6 +433,7 @@ impl<'de> Deserialize<'de> for NetworkRequirementsToml {
|
||||
allow_upstream_proxy,
|
||||
dangerously_allow_non_loopback_proxy,
|
||||
dangerously_allow_all_unix_sockets,
|
||||
dangerously_allow_plaintext_credential_injection,
|
||||
domains: domains
|
||||
.or_else(|| legacy_domain_permissions_from_lists(allowed_domains, denied_domains)),
|
||||
managed_allowed_domains_only,
|
||||
@@ -481,6 +485,7 @@ pub struct NetworkConstraints {
|
||||
pub allow_upstream_proxy: Option<bool>,
|
||||
pub dangerously_allow_non_loopback_proxy: Option<bool>,
|
||||
pub dangerously_allow_all_unix_sockets: Option<bool>,
|
||||
pub dangerously_allow_plaintext_credential_injection: Option<bool>,
|
||||
pub domains: Option<NetworkDomainPermissionsToml>,
|
||||
/// When true, only managed `allowed_domains` are respected while managed
|
||||
/// network enforcement is active. User allowlist entries are ignored.
|
||||
@@ -508,6 +513,7 @@ impl From<NetworkRequirementsToml> for NetworkConstraints {
|
||||
allow_upstream_proxy,
|
||||
dangerously_allow_non_loopback_proxy,
|
||||
dangerously_allow_all_unix_sockets,
|
||||
dangerously_allow_plaintext_credential_injection,
|
||||
domains,
|
||||
managed_allowed_domains_only,
|
||||
unix_sockets,
|
||||
@@ -520,6 +526,7 @@ impl From<NetworkRequirementsToml> for NetworkConstraints {
|
||||
allow_upstream_proxy,
|
||||
dangerously_allow_non_loopback_proxy,
|
||||
dangerously_allow_all_unix_sockets,
|
||||
dangerously_allow_plaintext_credential_injection,
|
||||
domains,
|
||||
managed_allowed_domains_only,
|
||||
unix_sockets,
|
||||
@@ -3324,6 +3331,7 @@ command = "python3 /enterprise/hooks/pre.py"
|
||||
enabled = true
|
||||
allow_upstream_proxy = false
|
||||
dangerously_allow_all_unix_sockets = true
|
||||
dangerously_allow_plaintext_credential_injection = false
|
||||
managed_allowed_domains_only = true
|
||||
allow_local_binding = false
|
||||
|
||||
@@ -3353,6 +3361,12 @@ command = "python3 /enterprise/hooks/pre.py"
|
||||
sourced_network.value.dangerously_allow_all_unix_sockets,
|
||||
Some(true)
|
||||
);
|
||||
assert_eq!(
|
||||
sourced_network
|
||||
.value
|
||||
.dangerously_allow_plaintext_credential_injection,
|
||||
Some(false)
|
||||
);
|
||||
assert_eq!(
|
||||
sourced_network.value.domains.as_ref(),
|
||||
Some(&NetworkDomainPermissionsToml {
|
||||
|
||||
@@ -249,6 +249,16 @@ impl NetworkProxySpec {
|
||||
constraints.dangerously_allow_all_unix_sockets =
|
||||
Some(dangerously_allow_all_unix_sockets);
|
||||
}
|
||||
if let Some(dangerously_allow_plaintext_credential_injection) =
|
||||
requirements.dangerously_allow_plaintext_credential_injection
|
||||
{
|
||||
config
|
||||
.network
|
||||
.dangerously_allow_plaintext_credential_injection =
|
||||
dangerously_allow_plaintext_credential_injection;
|
||||
constraints.dangerously_allow_plaintext_credential_injection =
|
||||
Some(dangerously_allow_plaintext_credential_injection);
|
||||
}
|
||||
let managed_allowed_domains = if hard_deny_allowlist_misses {
|
||||
Some(
|
||||
requirements
|
||||
|
||||
@@ -40,6 +40,48 @@ fn build_state_with_audit_metadata_threads_metadata_to_state() {
|
||||
assert_eq!(state.audit_metadata(), &metadata);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn managed_requirement_disables_plaintext_credential_injection() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
config
|
||||
.network
|
||||
.dangerously_allow_plaintext_credential_injection = true;
|
||||
let requirements = NetworkConstraints {
|
||||
dangerously_allow_plaintext_credential_injection: Some(false),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let spec = NetworkProxySpec::from_config_and_constraints(
|
||||
config,
|
||||
Some(requirements),
|
||||
&PermissionProfile::workspace_write(),
|
||||
)
|
||||
.expect("managed requirement should override the user setting");
|
||||
|
||||
assert!(
|
||||
!spec
|
||||
.config
|
||||
.network
|
||||
.dangerously_allow_plaintext_credential_injection
|
||||
);
|
||||
assert_eq!(
|
||||
spec.constraints
|
||||
.dangerously_allow_plaintext_credential_injection,
|
||||
Some(false)
|
||||
);
|
||||
|
||||
let mut candidate = spec.config.clone();
|
||||
candidate
|
||||
.network
|
||||
.dangerously_allow_plaintext_credential_injection = true;
|
||||
let err = validate_policy_against_constraints(&candidate, &spec.constraints)
|
||||
.expect_err("managed constraint should reject re-enabling plaintext injection");
|
||||
assert_eq!(
|
||||
err.to_string(),
|
||||
"invalid value for network.dangerously_allow_plaintext_credential_injection: true (allowed false (disabled by managed config))"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn requirements_allowed_domains_are_a_baseline_for_user_allowlist() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
|
||||
@@ -159,6 +159,14 @@ fn apply_network_constraints(network: NetworkToml, constraints: &mut NetworkProx
|
||||
if let Some(dangerously_allow_all_unix_sockets) = network.dangerously_allow_all_unix_sockets {
|
||||
constraints.dangerously_allow_all_unix_sockets = Some(dangerously_allow_all_unix_sockets);
|
||||
}
|
||||
if let Some(dangerously_allow_plaintext_credential_injection) = network
|
||||
.mitm
|
||||
.as_ref()
|
||||
.and_then(|mitm| mitm.dangerously_allow_plaintext_credential_injection)
|
||||
{
|
||||
constraints.dangerously_allow_plaintext_credential_injection =
|
||||
Some(dangerously_allow_plaintext_credential_injection);
|
||||
}
|
||||
if let Some(domains) = network.domains.as_ref() {
|
||||
let mut config = NetworkProxyConfig::default();
|
||||
if let Some(allowed_domains) = constraints.allowed_domains.take() {
|
||||
|
||||
@@ -325,6 +325,32 @@ dangerously_allow_all_unix_sockets = true
|
||||
assert_eq!(constraints.dangerously_allow_all_unix_sockets, Some(true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_network_constraints_includes_plaintext_credential_injection_flag() {
|
||||
let config: toml::Value = toml::from_str(
|
||||
r#"
|
||||
default_permissions = "dev"
|
||||
|
||||
[permissions.dev.network.mitm]
|
||||
dangerously_allow_plaintext_credential_injection = false
|
||||
"#,
|
||||
)
|
||||
.expect("permissions profile should parse");
|
||||
let network = selected_network_from_tables(
|
||||
network_tables_from_toml(&config).expect("permissions profile should deserialize"),
|
||||
)
|
||||
.expect("permissions profile should select a network table")
|
||||
.expect("network table should be present");
|
||||
|
||||
let mut constraints = NetworkProxyConstraints::default();
|
||||
apply_network_constraints(network, &mut constraints);
|
||||
|
||||
assert_eq!(
|
||||
constraints.dangerously_allow_plaintext_credential_injection,
|
||||
Some(false)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn selected_network_from_tables_ignores_builtin_profile_without_permissions_table() {
|
||||
let config: toml::Value = toml::from_str(
|
||||
|
||||
@@ -0,0 +1,145 @@
|
||||
use anyhow::Result;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::mount_sse_sequence;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::skip_if_sandbox;
|
||||
use core_test_support::skip_if_target_windows;
|
||||
use core_test_support::test_codex::TestCodexHarness;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::test_codex::turn_permission_fields;
|
||||
use core_test_support::wait_for_event;
|
||||
use core_test_support::wait_for_event_match;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::collections::HashMap;
|
||||
use std::fs;
|
||||
use std::sync::Arc;
|
||||
use tempfile::TempDir;
|
||||
|
||||
const REAL_OPENAI_API_KEY: &str =
|
||||
"sk-proj-abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_";
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn configured_credential_broker_virtualizes_child_environment() -> Result<()> {
|
||||
skip_if_target_windows!(Ok(()), "uses a POSIX shell command");
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let home = Arc::new(TempDir::new()?);
|
||||
fs::write(
|
||||
home.path().join("config.toml"),
|
||||
r#"
|
||||
default_permissions = "workspace"
|
||||
|
||||
[features.network_proxy]
|
||||
credential_broker = true
|
||||
|
||||
[permissions.workspace.filesystem]
|
||||
":minimal" = "read"
|
||||
|
||||
[permissions.workspace.network]
|
||||
enabled = true
|
||||
mode = "full"
|
||||
"#,
|
||||
)?;
|
||||
let builder = test_codex().with_home(home).with_config(|config| {
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
config.permissions.shell_environment_policy.r#set = HashMap::from([(
|
||||
"OPENAI_API_KEY".to_string(),
|
||||
REAL_OPENAI_API_KEY.to_string(),
|
||||
)]);
|
||||
});
|
||||
let harness = TestCodexHarness::with_auto_env_builder(builder).await?;
|
||||
let test = harness.test();
|
||||
|
||||
assert!(test.config.features.enabled(Feature::NetworkProxy));
|
||||
assert!(
|
||||
test.config.permissions.network.is_some(),
|
||||
"credential broker config should start the managed network proxy"
|
||||
);
|
||||
|
||||
let call_id = "credential-broker-child-env";
|
||||
let args = json!({
|
||||
"cmd": "printf '%s\\n%s' \"$OPENAI_API_KEY\" \"$CODEX_NETWORK_PROXY_CREDENTIAL_BROKER_ACTIVE\"",
|
||||
"yield_time_ms": 1_000,
|
||||
});
|
||||
let responses = vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
];
|
||||
mount_sse_sequence(harness.server(), responses).await;
|
||||
|
||||
let session_model = test.session_configured.model.clone();
|
||||
let cwd = test.config.cwd.clone();
|
||||
let (sandbox_policy, permission_profile) = turn_permission_fields(
|
||||
test.config.permissions.permission_profile().clone(),
|
||||
cwd.as_path(),
|
||||
);
|
||||
test.codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "show the brokered child credential".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: codex_protocol::protocol::ThreadSettingsOverrides {
|
||||
environments: Some(local_selections(cwd)),
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
permission_profile,
|
||||
collaboration_mode: Some(codex_protocol::config_types::CollaborationMode {
|
||||
mode: codex_protocol::config_types::ModeKind::Default,
|
||||
settings: codex_protocol::config_types::Settings {
|
||||
model: session_model,
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let end = wait_for_event_match(&test.codex, |event| match event {
|
||||
EventMsg::ExecCommandEnd(event) if event.call_id == call_id => Some(event.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(end.exit_code, 0);
|
||||
let output = end.aggregated_output.trim().lines().collect::<Vec<_>>();
|
||||
assert_eq!(output.len(), 2, "unexpected command output: {output:?}");
|
||||
let brokered_openai_api_key = output[0];
|
||||
assert_ne!(brokered_openai_api_key, REAL_OPENAI_API_KEY);
|
||||
assert!(brokered_openai_api_key.starts_with("sk-proj-"));
|
||||
assert_eq!(brokered_openai_api_key.len(), REAL_OPENAI_API_KEY.len());
|
||||
assert_eq!(output[1], "1");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -48,6 +48,7 @@ mod compact;
|
||||
mod compact_remote;
|
||||
mod compact_remote_parity;
|
||||
mod compact_resume_fork;
|
||||
mod credential_broker;
|
||||
mod current_time_reminder;
|
||||
mod deprecation_notice;
|
||||
mod exec;
|
||||
|
||||
@@ -1883,6 +1883,44 @@ mod tests {
|
||||
assert!(validate_policy_against_constraints(&config, &constraints).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_policy_against_constraints_disallows_plaintext_credential_injection_without_managed_opt_in()
|
||||
{
|
||||
let constraints = NetworkProxyConstraints {
|
||||
dangerously_allow_plaintext_credential_injection: Some(false),
|
||||
..NetworkProxyConstraints::default()
|
||||
};
|
||||
|
||||
let config = NetworkProxyConfig {
|
||||
network: NetworkProxySettings {
|
||||
enabled: true,
|
||||
dangerously_allow_plaintext_credential_injection: true,
|
||||
..NetworkProxySettings::default()
|
||||
},
|
||||
};
|
||||
|
||||
assert!(validate_policy_against_constraints(&config, &constraints).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_policy_against_constraints_allows_plaintext_credential_injection_with_managed_opt_in()
|
||||
{
|
||||
let constraints = NetworkProxyConstraints {
|
||||
dangerously_allow_plaintext_credential_injection: Some(true),
|
||||
..NetworkProxyConstraints::default()
|
||||
};
|
||||
|
||||
let config = NetworkProxyConfig {
|
||||
network: NetworkProxySettings {
|
||||
enabled: true,
|
||||
dangerously_allow_plaintext_credential_injection: true,
|
||||
..NetworkProxySettings::default()
|
||||
},
|
||||
};
|
||||
|
||||
assert!(validate_policy_against_constraints(&config, &constraints).is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_policy_against_constraints_disallows_allow_all_unix_sockets_when_allowlist_is_managed()
|
||||
{
|
||||
|
||||
@@ -30,6 +30,7 @@ pub struct NetworkProxyConstraints {
|
||||
pub allow_upstream_proxy: Option<bool>,
|
||||
pub dangerously_allow_non_loopback_proxy: Option<bool>,
|
||||
pub dangerously_allow_all_unix_sockets: Option<bool>,
|
||||
pub dangerously_allow_plaintext_credential_injection: Option<bool>,
|
||||
pub allowed_domains: Option<Vec<String>>,
|
||||
pub allowlist_expansion_enabled: Option<bool>,
|
||||
pub denied_domains: Option<Vec<String>>,
|
||||
@@ -216,6 +217,28 @@ pub fn validate_policy_against_constraints(
|
||||
},
|
||||
)?;
|
||||
|
||||
let allow_plaintext_credential_injection =
|
||||
constraints.dangerously_allow_plaintext_credential_injection;
|
||||
validate(
|
||||
config
|
||||
.network
|
||||
.dangerously_allow_plaintext_credential_injection,
|
||||
move |candidate| match allow_plaintext_credential_injection {
|
||||
Some(true) | None => Ok(()),
|
||||
Some(false) => {
|
||||
if *candidate {
|
||||
Err(invalid_value(
|
||||
"network.dangerously_allow_plaintext_credential_injection",
|
||||
"true",
|
||||
"false (disabled by managed config)",
|
||||
))
|
||||
} else {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
},
|
||||
)?;
|
||||
|
||||
if let Some(allow_local_binding) = constraints.allow_local_binding {
|
||||
validate(config.network.allow_local_binding, move |candidate| {
|
||||
if *candidate && !allow_local_binding {
|
||||
|
||||
@@ -468,6 +468,7 @@ fn format_network_constraints(network: &NetworkConstraints) -> String {
|
||||
allow_upstream_proxy,
|
||||
dangerously_allow_non_loopback_proxy,
|
||||
dangerously_allow_all_unix_sockets,
|
||||
dangerously_allow_plaintext_credential_injection,
|
||||
domains,
|
||||
managed_allowed_domains_only,
|
||||
unix_sockets,
|
||||
@@ -496,6 +497,13 @@ fn format_network_constraints(network: &NetworkConstraints) -> String {
|
||||
"dangerously_allow_all_unix_sockets={dangerously_allow_all_unix_sockets}"
|
||||
));
|
||||
}
|
||||
if let Some(dangerously_allow_plaintext_credential_injection) =
|
||||
dangerously_allow_plaintext_credential_injection
|
||||
{
|
||||
parts.push(format!(
|
||||
"dangerously_allow_plaintext_credential_injection={dangerously_allow_plaintext_credential_injection}"
|
||||
));
|
||||
}
|
||||
if let Some(domains) = domains {
|
||||
parts.push(format!(
|
||||
"domains={}",
|
||||
|
||||
Reference in New Issue
Block a user