MGMT-20908: Forward authorization#4
Conversation
This patch changes the MCP server so that if the `Authorization` header contains a bearer token it will forward it to the assisted service REST API. To existing behaviour to get the use the offline token from the environment variable `OFFLINE_TOKEN` is preserved when there is no `Authorization` header in the request. Note that in order to test this it is necessary to have a MCP client that is able to send the `Authorization` header. I did so making a small change to the `mcp` command line tool (from the https://github.com/f/mcptools project) so that it reads the token from a `TOKEN` environment variable: ```diff diff --git a/cmd/mcptools/commands/utils.go b/cmd/mcptools/commands/utils.go index f3a9be0..19918f2 100644 --- a/cmd/mcptools/commands/utils.go +++ b/cmd/mcptools/commands/utils.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "os" "strings" "time" @@ -47,7 +48,13 @@ var CreateClientFunc = func(args []string, _ ...client.ClientOption) (*client.Cl var err error if len(args) == 1 && IsHTTP(args[0]) { - c, err = client.NewSSEMCPClient(args[0]) + token := os.Getenv("TOKEN") + c, err = client.NewSSEMCPClient( + args[0], + client.WithHeaders(map[string]string{ + "Authorization": fmt.Sprintf("Bearer %s", token), + }), + ) if err != nil { return nil, err } ``` Then I can put the access token in that environment variable, and call a tool. For example, start the server without the offline token: ```shell $ unset OFFLINE_TOKEN $ uv run server.py ``` Then get the token and call some tool: ```shell $ ocm login --use-auth-code $ TOKEN=$(ocm token) mcp call list_clusters http://localhost:8000/sse | jq [ { "name": "sno", "id": "aef5bb4e-397d-4e21-a776-5f1405b2cf10", "openshift_version": "4.19.0", "status": "pending-for-input" } ] ``` Related: https://issues.redhat.com/browse/MGMT-20908 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@jhernand: This pull request references MGMT-20908 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 task to target the "4.20.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: jhernand 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 |
openshift-ci
Bot
added
the
size/M
WalkthroughThe changes implement a new token retrieval mechanism by introducing a Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@jhernand: This pull request references MGMT-20908 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 task to target the "4.20.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server.py (2)
10-10: Fix formatting: Add required blank lines before function definitions.Static analysis tools report missing blank lines before function definitions.
+ def get_offline_token() -> str:+ def get_access_token() -> str:Also applies to: 35-35
35-66: Review the token extraction and refresh logic for security and correctness.The implementation looks good overall but has a few considerations:
- Security: The bearer token extraction correctly validates the Authorization header format
- Fallback logic: Properly falls back to offline token refresh when no bearer token is present
- Error handling: Uses
raise_for_status()to handle HTTP errors appropriatelyHowever, there are some areas for improvement:
Consider these enhancements:
def get_access_token() -> str: """Retrieve the access token. This function tries to get the Red Hat OpenShift Cluster Manager (OCM) access token. First it tries to extract it from the authorization header, and if it isn't there then it tries to generate a new one using the offline token. Returns: str: The access token. Raises: RuntimeError: If it isn't possible to obtain or generate the access token. """ # First try to get the token from the authorization header: request = mcp.get_context().request_context.request if request is not None: header = request.headers.get("Authorization") if header is not None: parts = header.split() - if len(parts) == 2 and parts[0].lower() == "bearer": + if len(parts) == 2 and parts[0].lower() == "bearer" and parts[1].strip(): return parts[1] # Now try to get the offline token, and generate a new access token from it: params = { "client_id": "cloud-services", "grant_type": "refresh_token", "refresh_token": get_offline_token(), } sso_url = os.environ.get("SSO_URL", "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token") - response = requests.post(sso_url, data=params) + response = requests.post(sso_url, data=params, timeout=30) response.raise_for_status() return response.json()["access_token"]Suggested improvements:
- Add validation to ensure the bearer token is not empty
- Add timeout to the HTTP request to prevent hanging
service_client/assisted_service_api.py (1)
12-23: Consider adding token validation during initialization.While the current implementation is functional, consider adding basic token validation to fail fast if an invalid token is provided:
def __init__(self, access_token: str): + if not access_token or not access_token.strip(): + raise ValueError("Access token cannot be empty") self.access_token = access_token self.pull_secret = self._get_pull_secret() self.inventory_url = os.environ.get("INVENTORY_URL", "https://api.openshift.com/api/assisted-install/v2") self.client_debug = os.environ.get("CLIENT_DEBUG", "False").lower() == "true"This would help catch token-related issues early in the initialization process rather than during the first API call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server.py(14 hunks)service_client/assisted_service_api.py(1 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
server.py
[error] 10-10: expected 2 blank lines, found 1
(E302)
[error] 35-35: expected 2 blank lines, found 1
(E302)
🔇 Additional comments (4)
server.py (2)
4-4: LGTM: Required import for OAuth2 token refresh.The
requestslibrary import is correctly added to support the HTTP POST call for token generation in theget_access_token()function.
86-86: LGTM: Consistent update to use access tokens.All
InventoryClientinstantiations have been correctly updated to useget_access_token()instead of the previous offline token approach. This ensures consistent token handling across all API operations.Also applies to: 104-104, 123-123, 140-140, 161-161, 185-185, 209-209, 232-232, 246-246, 260-260, 279-279, 300-300
service_client/assisted_service_api.py (2)
12-14: LGTM: Clean refactoring to accept access tokens directly.The constructor change simplifies the token handling by accepting an access token directly instead of managing offline token exchange internally. The immediate call to
_get_pull_secret()ensures the pull secret is available when needed.
18-23: LGTM: Simplified pull secret retrieval using access token.The method correctly uses the instance's access token directly in the Authorization header. The error handling with
raise_for_status()is appropriate for detecting authentication failures.
|
/lgtm |
3 participants
This patch changes the MCP server so that if the
Authorizationheader contains a bearer token it will forward it to the assisted service REST API.To existing behaviour to get the use the offline token from the environment variable
OFFLINE_TOKENis preserved when there is noAuthorizationheader in the request.Note that in order to test this it is necessary to have a MCP client that is able to send the
Authorizationheader. I did so making a small change to themcpcommand line tool (from the https://github.com/f/mcptools project) so that it reads the token from aTOKENenvironment variable:Then I can put the access token in that environment variable, and call a tool. For example, start the server without the offline token:
$ unset OFFLINE_TOKEN $ uv run server.pyThen get the token and call some tool:
Related: https://issues.redhat.com/browse/MGMT-20908
Summary by CodeRabbit
New Features
Refactor