OSAC-850: Replace static OC tokens with auto-refreshing Keycloak JWTs…#69
OSAC-850: Replace static OC tokens with auto-refreshing Keycloak JWTs…#69SiddarthR56 wants to merge 1 commit into
Conversation
|
@SiddarthR56: This pull request references OSAC-850 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SiddarthR56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Walkthrough
ChangesKeycloak JWT Migration and Token Auto-Refresh
Sequence Diagram(s)sequenceDiagram
rect rgba(255, 100, 100, 0.5)
Note over TestFixture,GRPCClient: Token Initialization
end
participant TestFixture as conftest.py Fixture
participant GRPCClient as GRPCClient
participant Keycloak as Keycloak /token endpoint
participant gRPC as gRPC Service
TestFixture->>GRPCClient: GRPCClient(address=..., token_factory=lambda: get_jwt(...))
GRPCClient-->>TestFixture: instance (no token fetched yet)
rect rgba(100, 149, 237, 0.5)
Note over GRPCClient,gRPC: Per-call refresh flow
end
TestFixture->>GRPCClient: call(method, request)
GRPCClient->>GRPCClient: access self._token
alt cached token near expiry or missing
GRPCClient->>Keycloak: token_factory() → get_jwt(keycloak_url, user, password)
Keycloak-->>GRPCClient: new JWT (with exp claim)
GRPCClient->>GRPCClient: cache token + _jwt_exp(token)
end
GRPCClient->>gRPC: grpcurl -H "Authorization: Bearer <token>"
gRPC-->>GRPCClient: response
GRPCClient-->>TestFixture: parsed result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security Considerations🔒 Risk: Token Expiry and Replay Attacks (Severity: Medium) The PR mitigates token staleness by introducing a refresh-before-expiry mechanism. However, reviewers should verify:
🔑 Risk: Keycloak Credential Storage (Severity: Medium) Test fixtures now require
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
85-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand injection vulnerability in shell script construction - Risk: High
_make_jwt_token_script()directly interpolateskeycloak_url,username, andpasswordinto a shell command string without any escaping or sanitization. If any of these values contain shell metacharacters (e.g.,$(cmd),;,|, backticks), arbitrary command execution is possible.Impact: While the current callers use hardcoded usernames and environment-sourced passwords, this function is exposed for reuse. A password containing
; rm -rf /or$(malicious_cmd)would execute arbitrary commands. Per coding guidelines: "no shell=True, os.system, or backtick exec with user input."🔐 Proposed fix using shell escaping
+import shlex + + def _make_jwt_token_script(keycloak_url: str, username: str, password: str) -> str: + # Escape all values to prevent shell injection + safe_url = shlex.quote(keycloak_url) + safe_username = shlex.quote(username) + safe_password = shlex.quote(password) return ( - f"curl -sk -X POST {keycloak_url}/realms/osac/protocol/openid-connect/token" - f" -d grant_type=password -d client_id=osac-cli" - f" -d username={username} -d password={password} -d scope=openid" + f"curl -sk -X POST {safe_url}/realms/osac/protocol/openid-connect/token" + f" -d grant_type=password -d client_id=osac-cli" + f" -d username={safe_username} -d password={safe_password} -d scope=openid" " | python3 -c \"import sys,json;print(json.load(sys.stdin)['access_token'])\"" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 85 - 91, The _make_jwt_token_script() function constructs a shell command by directly interpolating keycloak_url, username, and password parameters without escaping, creating a command injection vulnerability. Fix this by using shlex.quote() to properly escape all three parameters (keycloak_url, username, and password) before interpolating them into the f-string command. This ensures that any shell metacharacters in these values are safely treated as literal characters rather than executable commands.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/grpc_client.py`:
- Around line 35-40: The _token property in the token refresh logic performs a
check-then-act pattern without synchronization. Multiple threads could
simultaneously evaluate the token expiration condition and all trigger a refresh
via _token_factory(), causing unnecessary concurrent requests. Add thread
synchronization (such as a lock) around the token refresh logic between the
expiration check and the _cached_token assignment to ensure only one thread
refreshes the token at a time when the expiration condition is detected.
- Around line 19-25: The `_jwt_exp()` function lacks error handling for
malformed JWT tokens. Add try-except blocks to catch and handle potential
exceptions that could occur when the token does not have exactly 3 parts
(IndexError from split/indexing), when base64 decoding fails (binascii.Error),
when JSON parsing fails (json.JSONDecodeError), or when the exp claim is missing
(KeyError). For each error condition, raise a descriptive exception that clearly
indicates the JWT token is invalid rather than allowing the raw exceptions to
propagate, so test failures provide helpful authentication error messages
instead of unclear stack traces.
---
Outside diff comments:
In `@tests/conftest.py`:
- Around line 85-91: The _make_jwt_token_script() function constructs a shell
command by directly interpolating keycloak_url, username, and password
parameters without escaping, creating a command injection vulnerability. Fix
this by using shlex.quote() to properly escape all three parameters
(keycloak_url, username, and password) before interpolating them into the
f-string command. This ensures that any shell metacharacters in these values are
safely treated as literal characters rather than executable commands.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ae82e9f6-c554-4bee-8c95-b2b6639712a5
📒 Files selected for processing (3)
tests/conftest.pytests/core/grpc_client.pytests/vmaas/public_ip/helpers.py
| @property | ||
| def _token(self) -> str: | ||
| if self._cached_token is None or time.time() >= self._token_exp - _TOKEN_REFRESH_MARGIN_SECONDS: | ||
| self._cached_token = self._token_factory() | ||
| self._token_exp = _jwt_exp(self._cached_token) | ||
| return self._cached_token |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Thread-safety concern in token refresh logic - Risk: Low
The _token property performs a check-then-act pattern without synchronization. In a multi-threaded test scenario, concurrent calls could trigger multiple simultaneous token refreshes.
Impact: Not critical for test code, but could cause unnecessary Keycloak requests if tests run with threading. Since this is test infrastructure with session-scoped fixtures, the practical risk is minimal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/core/grpc_client.py` around lines 35 - 40, The _token property in the
token refresh logic performs a check-then-act pattern without synchronization.
Multiple threads could simultaneously evaluate the token expiration condition
and all trigger a refresh via _token_factory(), causing unnecessary concurrent
requests. Add thread synchronization (such as a lock) around the token refresh
logic between the expiration check and the _cached_token assignment to ensure
only one thread refreshes the token at a time when the expiration condition is
detected.
There was a problem hiding this comment.
If you are already using the osac tool you can run osac get token. That will use an existing valid token, if it isn't expired, or else it will request a new one using the refresh token, if available. For example:
# Login once:
osac login \
--ca-file bundle.pem \
--client-id osac-admin \
--client-secret ... \
https://fulfillment-api.osac.svc.cluster.local:8000
# Get tokens, many times, the tool takes care of refreshing it as needed:
osac get token
eyJhbGciOiJSUzI1NiIsInR5cCIgOiA...The tool can also help if you need to get a particular claim from the token. For example, if you want the exp claim:
# The tool can generate JSON output:
osac get token --payload
{
"acr": "1",
"azp": "osac-admin",
"email_verified": false,
"exp": 1781513381,
"iat": 1781513081,
"iss": "https://keycloak.keycloak.svc.cluster.local:8000/realms/osac",
"jti": "trrtcc:de9e5645-c7bb-a7e9-384b-da4bbd3b1f7d",
"scope": "openid email profile",
"sub": "b9d3a317-1664-422e-859c-6e616aea64b0",
"typ": "Bearer",
"username": "service-account-osac-admin"
}
# So you can pipe it to JQ to get claims:
osac get token --payload | jq -r '.exp'
1781513381
# The tool also knows to translate Unix time to RFC 3339 format, which is more readable:
osac get token --payload --rfc-3339 | jq -r '.exp'
2026-06-15T10:49:41+02:00
… in GRPCClient and test fixtures Assisted-by: Cursor/Claude
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/conftest.py (1)
92-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRisk: High — Shell injection via unsanitized password in command string
The
_make_jwt_token_scriptfunction directly interpolatesusernameandpasswordinto a shell command string without escaping. If the password (or username) contains shell metacharacters (e.g.,$(whoami),;, backticks,|), arbitrary command execution is possible when this script is invoked.Attack vector: An attacker who controls
OSAC_JWT_PASSWORDenvironment variable (e.g., via CI config manipulation or env file poisoning) can inject shell commands.Impact: Remote code execution in the test runner context. While this is test infrastructure, it could compromise CI secrets, exfiltrate credentials, or pivot to production systems if the test environment has access.
As per coding guidelines: "Command: no shell=True, os.system, or backtick exec with user input" and "Validate at trust boundaries with allow-lists, not deny-lists."
🛡️ Proposed fix with proper shell escaping
+import shlex + + def _make_jwt_token_script(keycloak_url: str, username: str, password: str) -> str: + # Escape values for safe shell interpolation + safe_url = shlex.quote(keycloak_url) + safe_user = shlex.quote(username) + safe_pass = shlex.quote(password) return ( - f"curl -sk -X POST {keycloak_url}/realms/osac/protocol/openid-connect/token" - f" -d grant_type=password -d client_id=osac-cli" - f" -d username={username} -d password={password} -d scope=openid" + f"curl -sk -X POST {safe_url}/realms/osac/protocol/openid-connect/token" + f" -d grant_type=password -d client_id=osac-cli" + f" -d username={safe_user} -d password={safe_pass} -d scope=openid" " | python3 -c \"import sys,json;print(json.load(sys.stdin)['access_token'])\"" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 92 - 98, The _make_jwt_token_script function has a shell injection vulnerability where username and password parameters are directly interpolated into the shell command without proper escaping. To fix this, import shlex module and use shlex.quote() to escape both the username and password parameters before inserting them into the f-string command. This ensures that shell metacharacters in these user-controlled inputs are treated as literal strings rather than command syntax, preventing arbitrary command execution when the script is invoked.Source: Coding guidelines
tests/core/grpc_client.py (1)
42-47: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRisk: Low — Bearer token visible in process listing
The token is passed as a command-line argument to
grpcurl(Line 43). On multi-tenant CI systems, other users can view command-line arguments viaps auxor/proc/<pid>/cmdline, potentially exposing the JWT.Impact: Limited for test infrastructure—tokens are short-lived and test environments are typically isolated. However, if these tests run on shared CI runners, the token could be captured during its validity window.
Mitigation (optional): Pass the token via stdin or environment variable if
grpcurlsupports it, or accept this as a known trade-off for test code simplicity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/grpc_client.py` around lines 42 - 47, The call method passes the JWT token as a command-line argument to grpcurl in the Authorization header, which exposes it to process listing attacks on shared CI runners. Refactor the call method to pass the token via an environment variable (e.g., GRPCURL_AUTH) or stdin instead of as a command-line argument, removing it from the args list and setting it as an environment variable when invoking the run function. This prevents the token from being visible in process listings while maintaining the same authentication behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/conftest.py`:
- Around line 92-98: The _make_jwt_token_script function has a shell injection
vulnerability where username and password parameters are directly interpolated
into the shell command without proper escaping. To fix this, import shlex module
and use shlex.quote() to escape both the username and password parameters before
inserting them into the f-string command. This ensures that shell metacharacters
in these user-controlled inputs are treated as literal strings rather than
command syntax, preventing arbitrary command execution when the script is
invoked.
In `@tests/core/grpc_client.py`:
- Around line 42-47: The call method passes the JWT token as a command-line
argument to grpcurl in the Authorization header, which exposes it to process
listing attacks on shared CI runners. Refactor the call method to pass the token
via an environment variable (e.g., GRPCURL_AUTH) or stdin instead of as a
command-line argument, removing it from the args list and setting it as an
environment variable when invoking the run function. This prevents the token
from being visible in process listings while maintaining the same authentication
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d66da76a-0586-4f7e-9215-22126d5e9bf0
📒 Files selected for processing (3)
tests/conftest.pytests/core/grpc_client.pytests/vmaas/public_ip/helpers.py
|
/test e2e-vmaas |
|
@SiddarthR56: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary by CodeRabbit