mirror of
https://github.com/openai/codex.git
synced 2026-10-31 19:28:49 +00:00
Reload remote executor credentials safely across token rotation (#58251)
## Why ChatGPT credentials can rotate while a remote executor is idle. Registry requests need to follow stored rotations without continuing after logout or switching the executor's identity. Concurrent refreshes must also avoid overwriting newer credentials. ## What changed - Reload and validate ChatGPT credentials before registry requests, pinning the credential mode, account, user, and workspace policy. Reuse cached metadata for unchanged personal access tokens. - Retry registration when temporary credential resolution failures prevent a request from being sent. Bound credential resolution and block reuse of a token generation whose refresh outcome is unknown. - Restrict supported authentication modes before executor startup loads credentials or cloud configuration. - Serialize persistent credential writes across cooperating processes and compare stored credentials before committing refreshed tokens. - Replace `auth.json` atomically so overlapping readers see a complete document, preserving symlink targets and protecting file permissions, including Windows access controls. ## Testing Add coverage for credential rotation, logout, identity changes, cancelled refresh callers, temporary metadata failures, registration retries, concurrent credential writes, and atomic file replacement. GitOrigin-RevId: 6b210944bf8eba2f45bc27113f901d2c3cb06b48
This commit is contained in:
@@ -2051,7 +2051,6 @@ mod tests {
|
||||
use crate::FileSystemSandboxContext;
|
||||
use crate::ProcessId;
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT;
|
||||
#[cfg(not(windows))]
|
||||
use crate::client_api::DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT;
|
||||
use crate::client_api::ExecServerTransportParams;
|
||||
use crate::client_api::RemoteExecServerConnectArgs;
|
||||
@@ -2393,7 +2392,7 @@ mod tests {
|
||||
cwd: None,
|
||||
},
|
||||
client_name: "stdio-test-client".to_string(),
|
||||
initialize_timeout: Duration::from_secs(1),
|
||||
initialize_timeout: DEFAULT_REMOTE_EXEC_SERVER_INITIALIZE_TIMEOUT,
|
||||
resume_session_id: None,
|
||||
})
|
||||
.await
|
||||
|
||||
@@ -307,11 +307,7 @@ impl EnvironmentRegistryClient {
|
||||
.auth_provider
|
||||
.apply_auth_headers(HeaderMap::new())
|
||||
.await
|
||||
.map_err(|error| {
|
||||
ExecServerError::EnvironmentRegistryAuth(format!(
|
||||
"failed to resolve environment registry authentication: {error}"
|
||||
))
|
||||
})?;
|
||||
.map_err(auth::registry_auth_error)?;
|
||||
let executor_credential = if self.auth_mode == RegistryAuthMode::ExecutorCredential {
|
||||
// Scoped mode sends exactly one bearer and never accompanying account credentials.
|
||||
let token = (headers.len() == 1)
|
||||
|
||||
@@ -11,6 +11,15 @@ use super::RegistryAuthMode;
|
||||
use super::current_trace_context_headers;
|
||||
use crate::ExecServerError;
|
||||
|
||||
pub(super) fn registry_auth_error(error: codex_api::AuthError) -> ExecServerError {
|
||||
match error {
|
||||
codex_api::AuthError::Transient(_) => ExecServerError::RequestNotSent(
|
||||
"environment registry credential resolution is temporarily unavailable".to_string(),
|
||||
),
|
||||
error => super::direct::direct_auth_error(error),
|
||||
}
|
||||
}
|
||||
|
||||
impl EnvironmentRegistryClient {
|
||||
pub(super) async fn send_authenticated(
|
||||
&self,
|
||||
@@ -32,7 +41,7 @@ impl EnvironmentRegistryClient {
|
||||
self.auth_provider
|
||||
.apply_auth(request.clone())
|
||||
.await
|
||||
.map_err(super::direct::direct_auth_error)?,
|
||||
.map_err(registry_auth_error)?,
|
||||
None,
|
||||
)
|
||||
};
|
||||
|
||||
@@ -27,6 +27,20 @@ impl EnvironmentRegistryClient {
|
||||
.await
|
||||
{
|
||||
Ok(response) => return Ok(response),
|
||||
// Credential resolution failed before any registration was sent.
|
||||
// Keep the executor alive without replaying an ambiguous write.
|
||||
Err(ExecServerError::RequestNotSent(_)) => {
|
||||
let delay = registry_recovery_retry_delay(&retry_key, attempt);
|
||||
attempt = attempt.saturating_add(1);
|
||||
warn!(
|
||||
noise_event = "registration",
|
||||
noise_outcome = "retry",
|
||||
retry_attempt = attempt,
|
||||
retry_delay_ms = delay.as_millis() as u64,
|
||||
"Noise executor retrying unavailable credential resolution"
|
||||
);
|
||||
sleep(delay).await;
|
||||
}
|
||||
Err(ExecServerError::EnvironmentRegistryHttp { status, code, .. })
|
||||
if matches!(
|
||||
(status, code.as_deref()),
|
||||
|
||||
@@ -40,6 +40,7 @@ const SUCCESS_BODY: &str = r#"{"environment_id":"registration-retry-test","url":
|
||||
#[derive(Debug, Default)]
|
||||
struct RegistrationAuthProvider {
|
||||
calls: AtomicUsize,
|
||||
fail_first: bool,
|
||||
}
|
||||
|
||||
impl AuthProvider for RegistrationAuthProvider {
|
||||
@@ -48,11 +49,46 @@ impl AuthProvider for RegistrationAuthProvider {
|
||||
}
|
||||
|
||||
fn apply_auth_headers(&self, headers: HeaderMap) -> codex_api::AuthHeadersFuture<'_> {
|
||||
self.calls.fetch_add(1, Ordering::Relaxed);
|
||||
if self.calls.fetch_add(1, Ordering::Relaxed) == 0 && self.fail_first {
|
||||
return Box::pin(async {
|
||||
Err(codex_api::AuthError::Transient(
|
||||
"metadata unavailable".to_string(),
|
||||
))
|
||||
});
|
||||
}
|
||||
Box::pin(async move { Ok(headers) })
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn transient_credential_failure_retries_before_sending_registration() -> anyhow::Result<()> {
|
||||
let server = wiremock::MockServer::start().await;
|
||||
wiremock::Mock::given(wiremock::matchers::any())
|
||||
.respond_with(
|
||||
wiremock::ResponseTemplate::new(200).set_body_raw(SUCCESS_BODY, "application/json"),
|
||||
)
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
let provider = Arc::new(RegistrationAuthProvider {
|
||||
fail_first: true,
|
||||
..Default::default()
|
||||
});
|
||||
let client = EnvironmentRegistryClient::new(server.uri(), provider.clone())?;
|
||||
let response = timeout(
|
||||
Duration::from_secs(10),
|
||||
client.register_environment_with_retry(
|
||||
ENVIRONMENT_ID,
|
||||
&NoiseChannelIdentity::generate()?.public_key(),
|
||||
),
|
||||
)
|
||||
.await??;
|
||||
assert_eq!(response.environment_id, ENVIRONMENT_ID);
|
||||
assert_eq!(provider.calls.load(Ordering::Relaxed), 2);
|
||||
server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
struct RetryObserved(Arc<Notify>);
|
||||
|
||||
impl<S: tracing::Subscriber> tracing_subscriber::Layer<S> for RetryObserved {
|
||||
@@ -198,7 +234,7 @@ async fn registration_requires_a_confirmed_pre_write_failure_before_replay(
|
||||
assert_eq!(message, "registration unavailable");
|
||||
}
|
||||
}
|
||||
_ => anyhow::bail!("unexpected registration failure kind"),
|
||||
error => anyhow::bail!("unexpected registration failure kind: {error:?}"),
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user