feat(agentstack-server): add ECR registry compatibility#2376
feat(agentstack-server): add ECR registry compatibility#2376
Conversation
ECR (and other registries using Basic auth) were incompatible because the code assumed all registries use the OAuth2 Bearer token exchange flow. Changes: - Detect Basic vs Bearer auth scheme from www-authenticate header - For Basic auth registries (e.g. ECR), send credentials directly instead of attempting token exchange - Handle missing Docker-Content-Digest header by computing digest from response body as fallback - Refactor auth types to use RegistryAuthInfo named tuple tracking both scheme and token URL Signed-off-by: Radek Ježek <radek.jezek@ibm.com>
dosubot
bot
added
the
size:L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds compatibility for ECR and other registries using Basic authentication by detecting the auth scheme from the www-authenticate header. While the changes are well-structured, the implementation introduces several Server-Side Request Forgery (SSRF) vulnerabilities. The application trusts the realm parameter from the www-authenticate header without validation and follows redirects on user-controlled registry URLs, which could allow an attacker to make the server connect to internal services or leak registry credentials. It is recommended to implement strict validation for the realm URL and disable or restrict redirect following for registry requests. Additionally, consider improving the robustness of parsing Bearer authentication parameters to avoid potential KeyError exceptions.
| raise Exception(f"Failed to authenticate: {auth_resp.status_code}, {auth_resp.text}") | ||
| return auth_resp.json()["token"] | ||
| auth_resp = await client.get( | ||
| token_url, |
There was a problem hiding this comment.
The token_url used here is constructed from the realm parameter of the www-authenticate header received from a registry (see line 194). Since the realm URL is not validated, an attacker can provide a malicious registry that points the realm to an internal service or an arbitrary external URL. This leads to a Server-Side Request Forgery (SSRF) vulnerability. Additionally, any configured credentials for the registry are sent to this unvalidated URL in the Authorization header.
Remediation: Validate that the realm URL's host matches the registry's host or belongs to a trusted list of authentication providers before making the request.
| async def get_registry_auth_info(self) -> RegistryAuthInfo | None: | ||
| if self.registry not in AUTH_URL_PER_REGISTRY: | ||
| async with httpx.AsyncClient() as client: | ||
| registry_resp = await client.get(self.get_manifest_url, follow_redirects=True) |
There was a problem hiding this comment.
Making a request to a user-supplied registry URL with follow_redirects=True can lead to Server-Side Request Forgery (SSRF). An attacker can provide a registry URL they control that redirects the request to internal services, potentially bypassing network restrictions.
Remediation: Consider setting follow_redirects=False or implementing a custom redirect handler that validates the redirect target URL to ensure it does not point to internal or sensitive addresses.
| headers["Authorization"] = auth_header | ||
|
|
||
| async with httpx.AsyncClient() as client: | ||
| manifest_resp = await client.get(self.get_manifest_url, headers=headers, follow_redirects=True) |
There was a problem hiding this comment.
Making a request to a user-supplied registry URL with follow_redirects=True can lead to Server-Side Request Forgery (SSRF). An attacker can provide a registry URL they control that redirects the request to internal services, potentially bypassing network restrictions.
Remediation: Consider setting follow_redirects=False or implementing a custom redirect handler that validates the redirect target URL to ensure it does not point to internal or sensitive addresses.
| auth_url = f"{params['realm']}?service={params['service']}&scope=repository:{{repository}}:{{permissions}}" | ||
| AUTH_URL_PER_REGISTRY[self.registry] = RegistryAuthInfo(RegistryAuthScheme.BEARER, auth_url) |
There was a problem hiding this comment.
Directly accessing params['realm'] and params['service'] can cause a KeyError if these are not present in the www-authenticate header. It's safer to use .get() and validate that the parameters exist before constructing the URL.
| auth_url = f"{params['realm']}?service={params['service']}&scope=repository:{{repository}}:{{permissions}}" | |
| AUTH_URL_PER_REGISTRY[self.registry] = RegistryAuthInfo(RegistryAuthScheme.BEARER, auth_url) | |
| realm = params.get("realm") | |
| service = params.get("service") | |
| if not realm or not service: | |
| raise ValueError(f"Incomplete Bearer auth params in www-authenticate header: {header}") | |
| auth_url = f"{realm}?service={service}&scope=repository:{{repository}}:{{permissions}}" | |
| AUTH_URL_PER_REGISTRY[self.registry] = RegistryAuthInfo(RegistryAuthScheme.BEARER, auth_url) |
1 participant
TODO: autogenerated
Ref: #2375
Summary
BasicvsBearerauth scheme from thewww-authenticateresponse headerDocker-Content-Digestheader (ECR doesn't always return it) by computing the digest from the response bodyTest plan
mise run agentstack-server:test:unit— 100 passed)