fix(runner): use CP OIDC token for backend credential fetches#1219
Merged
markturansky merged 1 commit intoalphafrom Apr 6, 2026
Merged
fix(runner): use CP OIDC token for backend credential fetches#1219markturansky merged 1 commit intoalphafrom
markturansky merged 1 commit intoalphafrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a Kubernetes ServiceAccount token reader utility, added a debug log in credential fetching when falling back to the bot-token path, and added tests exercising Changes
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The credential endpoint (GET /api/ambient/v1/credentials/{id}/token) is
on the ambient-api-server, which validates tokens via RHSSO JWT signature
verification — the same issuer that signs the CP OIDC token used for gRPC.
The K8s SA token path added previously was unnecessary: the CP OIDC token
is already a valid RHSSO JWT and authenticates to the same api-server for
both gRPC and HTTP credential fetches. The CP's OIDC client identity
requires a role_binding granting credential:read in the DB.
Removes get_sa_token() from the credential fetch path. Updates regression
tests to verify the CP OIDC token is sent.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
fix/runner-credential-sa-token
branch
from
9c4c5c1 to
c78f884
Compare
markturansky
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py`:
- Around line 738-818: The tests in TestFetchCredentialBotToken are asserting
the bot (CP OIDC) token is used when ctx.caller_token is absent, but the
intended behavior is: prefer a mounted Kubernetes ServiceAccount (SA) token and
fall back to the bot token only if the SA token is unavailable; update
TestFetchCredentialBotToken.test_uses_bot_token_when_no_caller_token and
test_bot_token_used_when_no_caller_token to simulate an available SA token and
assert the Authorization header equals "Bearer <sa-token>" (reference
_fetch_credential, ctx.caller_token, _CredentialHandler.captured_headers, and
fake_urlopen), and add/modify a separate case that removes the SA token
simulation to assert the bot token returned by
ambient_runner.platform.auth.get_bot_token is used as the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e97c16b-7f08-4cda-956e-e4bd4441a2d2
📒 Files selected for processing (3)
components/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/ambient_runner/platform/utils.pycomponents/runners/ambient-runner/tests/test_shared_session_credentials.py
✅ Files skipped from review due to trivial changes (1)
- components/runners/ambient-runner/ambient_runner/platform/auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- components/runners/ambient-runner/ambient_runner/platform/utils.py
Comment on lines
+738
to
+818
| class TestFetchCredentialBotToken: | ||
| @pytest.mark.asyncio | ||
| async def test_uses_bot_token_when_no_caller_token(self): | ||
| """_fetch_credential sends the CP OIDC token when caller_token is absent. | ||
|
|
||
| The api-server validates the CP OIDC token via RHSSO JWT signature verification. | ||
| The CP's OIDC client identity must have a role_binding granting credential:read. | ||
|
|
||
| Regression for: runner gets HTTP 401 on credential fetch in gRPC-initiated runs. | ||
| """ | ||
| server = HTTPServer(("127.0.0.1", 0), _CredentialHandler) | ||
| port = server.server_address[1] | ||
| thread = Thread(target=server.handle_request, daemon=True) | ||
| thread.start() | ||
|
|
||
| _CredentialHandler.response_body = {"token": "gh-tok-via-oidc"} | ||
| _CredentialHandler.captured_headers = {} | ||
|
|
||
| cp_oidc_token = "cp-oidc-jwt-token" | ||
|
|
||
| try: | ||
| with ( | ||
| patch.dict( | ||
| os.environ, | ||
| { | ||
| "BACKEND_API_URL": f"http://127.0.0.1:{port}/api", | ||
| "CREDENTIAL_IDS": json.dumps({"github": "cred-gh-bot-test"}), | ||
| }, | ||
| ), | ||
| patch("ambient_runner.platform.auth.get_bot_token", return_value=cp_oidc_token), | ||
| ): | ||
| ctx = _make_context() # no caller_token | ||
| result = await _fetch_credential(ctx, "github") | ||
|
|
||
| assert result.get("token") == "gh-tok-via-oidc", ( | ||
| "credential fetch must succeed using CP OIDC token — " | ||
| "regression for HTTP 401 on gRPC-initiated runs" | ||
| ) | ||
| assert _CredentialHandler.captured_headers.get("Authorization") == ( | ||
| f"Bearer {cp_oidc_token}" | ||
| ), "request must use the CP OIDC token" | ||
| finally: | ||
| server.server_close() | ||
| thread.join(timeout=2) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_bot_token_used_when_no_caller_token(self): | ||
| """CP OIDC token (get_bot_token) is used when caller_token is absent. | ||
|
|
||
| The credential endpoint on the api-server validates via RHSSO JWT, | ||
| the same issuer that signs the CP OIDC token — one token for both | ||
| gRPC and HTTP credential fetches. | ||
| """ | ||
| called_with = {} | ||
|
|
||
| def fake_urlopen(req, timeout=None): | ||
| called_with["auth"] = req.get_header("Authorization") | ||
| mock_resp = MagicMock() | ||
| mock_resp.read.return_value = json.dumps({"token": "ok"}).encode() | ||
| mock_resp.__enter__ = lambda s: s | ||
| mock_resp.__exit__ = MagicMock(return_value=False) | ||
| return mock_resp | ||
|
|
||
| with ( | ||
| patch.dict( | ||
| os.environ, | ||
| { | ||
| "BACKEND_API_URL": "http://backend.svc.cluster.local/api", | ||
| "CREDENTIAL_IDS": json.dumps({"github": "cred-gh-pref"}), | ||
| }, | ||
| ), | ||
| patch("urllib.request.urlopen", side_effect=fake_urlopen), | ||
| patch("ambient_runner.platform.auth.get_bot_token", return_value="cp-oidc-token"), | ||
| ): | ||
| ctx = _make_context() # no caller_token | ||
| await _fetch_credential(ctx, "github") | ||
|
|
||
| assert called_with.get("auth") == "Bearer cp-oidc-token", ( | ||
| "CP OIDC token must be used for credential fetch — " | ||
| "same token used for gRPC and HTTP credential endpoint" | ||
| ) |
Contributor
There was a problem hiding this comment.
Tests are asserting the wrong auth source for the no-caller-token path.
Line 740 and Line 784 both enforce CP OIDC/bot token usage when ctx.caller_token is absent. That conflicts with this PR’s intended behavior (Kubernetes ServiceAccount token first, bot token fallback only when SA token is unavailable). As written, these tests either fail against the intended implementation or lock in the old behavior.
Please update these cases to assert:
- SA token is used when mounted/available, and
- bot token is used only when SA token is unavailable.
As per coding guidelines, "**/*: Flag only errors, security risks, or functionality-breaking problems."
🧰 Tools
🪛 Ruff (0.15.9)
[error] 756-756: Possible hardcoded password assigned to: "cp_oidc_token"
(S105)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py`
around lines 738 - 818, The tests in TestFetchCredentialBotToken are asserting
the bot (CP OIDC) token is used when ctx.caller_token is absent, but the
intended behavior is: prefer a mounted Kubernetes ServiceAccount (SA) token and
fall back to the bot token only if the SA token is unavailable; update
TestFetchCredentialBotToken.test_uses_bot_token_when_no_caller_token and
test_bot_token_used_when_no_caller_token to simulate an available SA token and
assert the Authorization header equals "Bearer <sa-token>" (reference
_fetch_credential, ctx.caller_token, _CredentialHandler.captured_headers, and
fake_urlopen), and add/modify a separate case that removes the SA token
simulation to assert the bot token returned by
ambient_runner.platform.auth.get_bot_token is used as the fallback.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
Re: CodeRabbit inline comment on The SA token priority has been intentionally removed in this commit. Investigation confirmed:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /api/ambient/v1/credentials/{id}/token) lives on the ambient-api-server, which validates tokens via RHSSO JWT signature — the same issuer that signs the CP OIDC token used for gRPCget_sa_token()from the credential fetch path;get_bot_token()(CP OIDC) is now used as the primary token when no caller token is presentrole_bindinggrantingcredential:readin the DB (infra/ops concern)Test plan
test_shared_session_credentials.pyget_bot_token()) is sent inAuthorizationheader when no caller token present🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests