feat(auth): add caller-attested X.509 transport capability - #523
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 48 mixed files remain; 4 existing customizations changed. Compared
44 existing customizations unchanged
4 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 32869867675 --repo openai/openai-ruby \
--name castiron-custom-code-32869867675-1 --dir /tmp/castiron-custom-code-32869867675-1
git apply --stat /tmp/castiron-custom-code-32869867675-1/custom-code.patch
cat /tmp/castiron-custom-code-32869867675-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 8057a053dca400ab0796f8da6ae814a57689e396 a6b28ca0dad1344a0a3eadd49a18f874a6162dd6
python3 scripts/castiron/custom_code_report.py report \
--base 8057a053dca400ab0796f8da6ae814a57689e396 \
--head a6b28ca0dad1344a0a3eadd49a18f874a6162dd6 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-a6b28ca0dad1
cat /tmp/castiron-custom-code-a6b28ca0dad1/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2471744f67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
21858c6 to
55a5103
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55a51038b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
55a5103 to
6fe267a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fe267ab93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6fe267a to
fc9061b
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc9061b342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fc9061b to
f67b8f2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f67b8f2683
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f67b8f2 to
a5ab8f7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ab8f70db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a5ab8f7 to
7065e61
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7065e619a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7065e61 to
4faf6fc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4faf6fc712
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
## Summary Second PR in the stacked X.509 workload-identity series; targets the transport-capability branch rather than `main`. - Add immutable `OpenAI::Auth::X509WorkloadIdentity` configuration for the mapped identity-provider and service-account IDs. - Implement the exact pinned X.509 token-exchange request against `https://mtls.auth.openai.com/oauth/token` using only the attested transport. - Require complete bounded OAuth JSON, strict bearer grammar, access-token issuance/type markers, and finite positive expiry of at most one hour. - Close response resources, honor authentication deadlines, and redact unknown/nested OAuth errors and sensitive network diagnostics. - Include focused malformed-response and credential-isolation tests plus matching Sorbet/RBS declarations. The 64 KiB bound applies only to the small OAuth token-exchange response; ordinary API JSON responses and SSE events remain unlimited by this change. ## Verification - `bundle exec ruby test/openai/auth/x509_token_exchange_test.rb` - `bundle exec ruby test/openai/auth/x509_transport_test.rb` - `bundle exec rake lint` ## Stack 1. Attested transport and real-wire security boundary. 2. **This PR:** X.509 identity and pinned token-exchange protocol. 3. Public-client integration and two-leg mTLS end-to-end test. 4. Refresh/retry/concurrency and final-credential hardening. 5. Customer documentation, enrolled-credential live smoke, and installed-gem end-to-end coverage. Review requested from `@openai/sdks-team` because this change introduces an authentication protocol.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f203acc968
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa736d40b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
One substantive HTTP request-framing security issue remains in the X.509 transport capability.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
apcha-oai
left a comment
apcha-oai
left a comment
There was a problem hiding this comment.
[sdk codex review] Approved at 5eb9eae682b495613823debb99dbc2fade62b5a9 after two independent full reviews of all 18 changed files, supporting runtime/contracts, tests and existing feedback. No substantiated merge-blocking defect found.
This reviewed range includes the attested transport and the token-exchange code merged from #524, despite the older transport-only PR description. Checked destination/authority and proxy restrictions, TLS verification callbacks and pool cleanup, immutable header validation, the fixed four-field exchange protocol, token-response validation, and lazy response/cleanup error sanitization. The prior framing-header and nested OAuth-code findings are fixed in this snapshot; no duplicate comments added.
Primary logs for CI 32812201617 confirm 1,291 tests per Ruby 3.3.12, 3.4.10 and 4.0.6, with zero failures/errors/skips. OkTest passed 237 tests across 42 suites. Both tested merge 11d772677cc7d5d545c938a9ca0c2a7efdcb255e, this head plus current main e7dd78b8c21a994edb2b9e94a3f2938cc5a2b17c; actual diff merge-base is c3fe427c6e9d9fad78acd23118b897ab0d6c5611.
Head/base, author, open/non-draft state, mergeability and feedback were rechecked before submission. No candidate code, tests, live API calls or security scans were run. This code review does not replace any separately required human/owner sign-off.
) ## Summary Final open integration PR in the stacked X.509 workload-identity series. The lifecycle-hardening PR (#526) and documentation/live-smoke PR (#527) have already been merged into this branch, so this PR now contains their complete reviewed changes as well. - Accept `OpenAI::Auth::X509WorkloadIdentity` through the existing `workload_identity:` client option and require an attested `X509Transport` as `http_client:`. - Default only certificate-authenticated clients to their approved mTLS API origin and reject provider-owned, mismatched, or unsupported destinations before credentials are acquired. - Reuse the exact caller-owned transport for both the pinned issuer exchange and the API request; keep existing API-key, JWT/ID workload-identity, admin, and ordinary transport behavior. - Fail closed for unsupported Realtime WebSocket connections. - Add a public-client, real-wire end-to-end test with independently terminating issuer/API TLS servers, a real client certificate and chain, authenticated HTTP CONNECT, and observed bearer/header separation. - Include bounded refresh/retry/concurrency handling, one-time credential replay, custom mTLS base-path preservation, and final credential/header hardening from #526. - Include customer documentation, installed-gem verification, production-enrolled certificate examples, and the optional protected live-smoke workflow from #527. - Inherit request-smuggling prevention and sanitized nested issuer OAuth error codes from the transport/token-exchange base PR (#523). ## Verification - Full Ruby 4 suite: **1,340 tests, 12,115 assertions, zero failures/errors**. - Full Ruby 3.3 suite: **1,340 tests, 12,114 assertions, zero failures/errors**. - `bundle exec rake lint`: **2,765 files inspected, no offenses**. - Real production X.509 issuer exchange and mTLS API request succeeded with the enrolled restricted-project service account, including with ambient debug logging enabled. - Focused real-wire issuer/API, framing-security, OAuth-redaction, lifecycle, installed-gem, protected-workflow, and sanitized-example regression coverage all pass. ## Stack 1. **#523 (open):** attested transport, real-wire security boundary, and pinned token-exchange protocol; #524 has already merged into this base. 2. **#525 (this PR, open):** public-client integration, lifecycle/retry hardening, customer documentation, protected optional live smoke, and installed-gem end-to-end coverage; #526 and #527 have already merged into this branch. Review requested from `@openai/sdks-team` because this change affects client authentication, endpoints, TLS, and a protected CI workflow.
| rescue OpenAI::Errors::AuthenticationError | ||
| raise unless retry_count.zero? && request_replayable?(request) | ||
| @workload_identity_auth.invalidate_token | ||
| @workload_identity_auth.invalidate_token(context.fetch(:token)) if x509_request |
| @workload_identity_auth.invalidate_token | ||
| @workload_identity_auth.invalidate_token(context.fetch(:token)) if x509_request | ||
| replay_allowed = request_replayable?(request) | ||
| replay_allowed &&= x509_request ? replay_state.empty? : retry_count.zero? |
| raise unless replay_allowed | ||
|
|
||
| if x509_request | ||
| replay_state << true |
|
|
||
| if x509_request | ||
| replay_state << true | ||
| replay_state.freeze |
| if x509_request | ||
| replay_state << true | ||
| replay_state.freeze | ||
| issuer_retries = context.fetch(:issuer_retries) |
| replay_state << true | ||
| replay_state.freeze | ||
| issuer_retries = context.fetch(:issuer_retries) | ||
| auth_max_retries = context.fetch(:auth_max_retries) |
| replay_state.freeze | ||
| issuer_retries = context.fetch(:issuer_retries) | ||
| auth_max_retries = context.fetch(:auth_max_retries) | ||
| api_max_retries = context.fetch(:api_max_retries) |
| issuer_retries = context.fetch(:issuer_retries) | ||
| auth_max_retries = context.fetch(:auth_max_retries) | ||
| api_max_retries = context.fetch(:api_max_retries) | ||
| log_context = context.fetch(:log_context) |
| send_retry_header: send_retry_header | ||
| ) | ||
| rescue OpenAI::Errors::AuthenticationError | ||
| @workload_identity_auth.invalidate_token(context.fetch(:token)) if x509_request |
|
|
||
| if (context = request[:x509_request_context]) | ||
| context[:issuer_retries] = previous_issuer_retries + attempts | ||
| context[:token] = token.dup.freeze |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b28ca0da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -197,16 +291,49 @@ class Client < OpenAI::Internal::Transport::BaseClient | |||
| send_retry_header: send_retry_header | |||
| ) | |||
| rescue OpenAI::Errors::AuthenticationError | |||
There was a problem hiding this comment.
When an X.509 API request containing a signed or otherwise sensitive query receives a terminal HTTP error, BaseClient constructs the resulting APIStatusError with the complete prepared URL, while this handler only intercepts authentication failures for replay. Consequently, errors such as 400, 403, 404, 429, 5xx, and the final 401 expose the query and fragment through error.url; reconstruct or sanitize these errors before they leave this boundary.
AGENTS.md reference: AGENTS.md:L35-L38
Useful? React with 👍 / 👎.
| rescue Timeout::Error => error | ||
| raise unless x509_request | ||
|
|
||
| raise OpenAI::Errors::APITimeoutError.new(url: request.fetch(:url), message: error.message), cause: nil |
There was a problem hiding this comment.
When the X.509 token exchange or its retry delay exhausts the request deadline for an API URL containing a sensitive query or fragment, this newly constructed APITimeoutError retains the complete API URL through its public url accessor. This bypasses the transport's URL sanitization because the timeout is raised in authentication preparation rather than network dispatch; strip the query and fragment before constructing the error.
AGENTS.md reference: AGENTS.md:L35-L38
Useful? React with 👍 / 👎.
| "RUBYOPT" => nil, | ||
| "RUBYLIB" => nil | ||
| } | ||
| output, status = Open3.capture2e(environment, RbConfig.ruby, smoke_script, chdir: directory) |
There was a problem hiding this comment.
If the installed-gem child stalls in its real issuer/API handshakes, response streaming, proxy shutdown, or connection cleanup, Open3.capture2e waits indefinitely and can hang the entire test job. Fresh evidence beyond the already-fixed wire-test subprocess is that this separate real-wire child still has no bounded wait or TERM/KILL cleanup; run it through the same timeout-safe subprocess helper.
Useful? React with 👍 / 👎.
3 participants
Summary
First PR in a five-part X.509 workload-identity stack. This change adds only the explicit, caller-owned mTLS transport capability; it does not enable OAuth exchange or change
OpenAI::Clientauthentication.OpenAI::Auth::X509Transportfor one concreteOpenAI::NetHTTPClient, an explicitly attested static certificate identity, the pinned token issuer, and approved global/US/EU mTLS API origins.Net::HTTPconnection, including pooled connections.Verification
bundle exec ruby test/openai/auth/x509_transport_test.rbbundle exec ruby test/openai/net_http_client_mtls_test.rbbundle exec rake lintStack
Review requested from
@openai/sdks-teambecause this change touches authentication and transport security.