oci: add OciCredentials model and harden credential resolution#654
Open
bennyz wants to merge 1 commit into
Open
oci: add OciCredentials model and harden credential resolution#654bennyz wants to merge 1 commit into
bennyz wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactor OCI credential resolution to return a frozen Pydantic OciCredentials model; improve registry parsing and auth-file lookup; normalize env-vars; update flasher/qemu/ridesx to consume OciCredentials; and update tests to assert credential object fields. ChangesOCI Credentials Model Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/common/oci.py (1)
202-233: 💤 Low valueMinor: stale phrasing in docstring.
The "Returns" section still reads
OciCredentials — ``(None, None)`` if no credentials are found., which is leftover tuple wording. Consider tightening to e.g. "Returns anOciCredentials()(with both fieldsNone) when no match is found." Same applies toresolve_oci_credentials's docstring on Line 246.🤖 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 `@python/packages/jumpstarter/jumpstarter/common/oci.py` around lines 202 - 233, Update the docstring "Returns" sections for read_auth_file_credentials (and similarly for resolve_oci_credentials) to remove the stale tuple phrasing and instead state that the function returns an OciCredentials instance with both fields set to None when no credentials are found (e.g., "Returns an OciCredentials() with both fields None when no match is found."). Ensure the wording matches style of other docstrings and clearly references OciCredentials rather than a tuple.
🤖 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.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/oci.py`:
- Around line 202-233: Update the docstring "Returns" sections for
read_auth_file_credentials (and similarly for resolve_oci_credentials) to remove
the stale tuple phrasing and instead state that the function returns an
OciCredentials instance with both fields set to None when no credentials are
found (e.g., "Returns an OciCredentials() with both fields None when no match is
found."). Ensure the wording matches style of other docstrings and clearly
references OciCredentials rather than a tuple.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb07caaf-ed55-43ef-a2ab-dd973a46295d
📒 Files selected for processing (8)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.pypython/packages/jumpstarter/jumpstarter/common/oci.pypython/packages/jumpstarter/jumpstarter/common/oci_test.py
raballew
reviewed
May 7, 2026
Contributor
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)
python/packages/jumpstarter/jumpstarter/common/oci.py (1)
228-236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse strict base64 validation for
authentries.
base64.b64decode()is permissive by default, allowingauthvalues with trailing garbage to decode successfully. Usevalidate=Trueto reject malformed entries and fall back to explicitusername/passwordfields.Suggested fix
+import binascii import json import logging import os import tomllib @@ - decoded = base64.b64decode(auth_b64).decode("utf-8") + decoded = base64.b64decode(auth_b64, validate=True).decode("utf-8") username, password = decoded.split(":", 1) if username and password: return username, password - except (ValueError, UnicodeDecodeError) as e: + except (binascii.Error, ValueError, UnicodeDecodeError) as e: logger.warning(f"Failed to decode auth entry for {key}: {e}")🤖 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 `@python/packages/jumpstarter/jumpstarter/common/oci.py` around lines 228 - 236, The base64 decode for auth entries is too permissive; update the decoding to use strict validation by calling base64.b64decode(auth_b64, validate=True) and catch binascii.Error in addition to ValueError and UnicodeDecodeError; if validation fails or the decoded value does not split into username and password, fall back to reading explicit "username" and "password" fields instead of silently accepting malformed data and log the failure via logger.warning referencing the key and exception (symbols to edit: auth_b64, decoded, username, password, logger.warning).
🤖 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
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1295-1306: The _resolve_oci_credentials helper currently annotates
its return type with OciCredentials before it’s imported and swallows the
original validation error; update the file to import OciCredentials (and any
typing-only imports) under a TYPE_CHECKING guard so the return annotation is
valid for the type checker, import Pydantic’s ValidationError and replace the
broad except (ValueError, Exception) with except ValidationError as err, then
re-raise the click.ClickException using from err to preserve exception chaining;
keep the function name _resolve_oci_credentials and the existing
click.ClickException message and use the already-imported
resolve_oci_credentials symbol as needed.
In `@python/packages/jumpstarter/jumpstarter/common/oci.py`:
- Around line 115-127: The code treats the first segment before "/" as a
registry even for namespace/image refs (e.g., library/ubuntu), breaking
credential lookup; update the logic in the block that computes registry (using
the variables parts and registry) to also consider a single-segment host-looking
piece without a dot or "localhost" (and without a numeric port) as an
unqualified image when the original input contains a single "/" (i.e.,
namespace/image form). Concretely, when parts = url.split("/",1) and registry
has no "." and registry != "localhost" and is not a numeric port specifier, call
_get_unqualified_search_registries() for namespace/image refs instead of
returning [registry]; keep the existing port-detection behavior for true
host:port cases and ensure other returns (e.g., [registry]) remain unchanged.
---
Outside diff comments:
In `@python/packages/jumpstarter/jumpstarter/common/oci.py`:
- Around line 228-236: The base64 decode for auth entries is too permissive;
update the decoding to use strict validation by calling
base64.b64decode(auth_b64, validate=True) and catch binascii.Error in addition
to ValueError and UnicodeDecodeError; if validation fails or the decoded value
does not split into username and password, fall back to reading explicit
"username" and "password" fields instead of silently accepting malformed data
and log the failure via logger.warning referencing the key and exception
(symbols to edit: auth_b64, decoded, username, password, logger.warning).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f66e0188-25da-4377-ba49-df1b26e11b26
📒 Files selected for processing (8)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.pypython/packages/jumpstarter/jumpstarter/common/oci.pypython/packages/jumpstarter/jumpstarter/common/oci_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
raballew
requested changes
May 11, 2026
Comment on lines
108
to
110
| if oci_username or oci_password: | ||
| if bool(oci_username) != bool(oci_password): | ||
| raise ValueError("OCI authentication requires both username and password") |
Member
There was a problem hiding this comment.
This code path still uses the old pattern
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`:
- Around line 110-113: The current truthiness check treats "" and " "
differently; change the branch to detect non-blank credential inputs by trimming
whitespace: compute trimmed_username = oci_username.strip() if oci_username is
not None else "" and trimmed_password = oci_password.strip() if oci_password is
not None else ""; if either trimmed value is non-empty, construct creds =
OciCredentials(username=trimmed_username or None, password=trimmed_password or
None) so blank strings become None, otherwise call creds =
resolve_oci_credentials(oci_url). This ensures OciCredentials and
resolve_oci_credentials behave consistently for blank inputs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 869bb98e-832a-4e3e-93e8-b753a2cc9626
📒 Files selected for processing (8)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.pypython/packages/jumpstarter/jumpstarter/common/oci.pypython/packages/jumpstarter/jumpstarter/common/oci_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (5)
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.py
- python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- python/packages/jumpstarter/jumpstarter/common/oci.py
- python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py
- python/packages/jumpstarter/jumpstarter/common/oci_test.py
bennyz
force-pushed
the
oci-follow-up
branch
3 times, most recently
from
25324bf to
f4d6f53
Compare
raballew
requested changes
May 12, 2026
Comment on lines
302
to
320
| oci_url: OCI image reference (e.g. ``oci://quay.io/org/image:tag``). | ||
|
|
||
| Returns: | ||
| Tuple of (username, password), or (None, None) if no credentials | ||
| are found from any source. | ||
| OciCredentials with both fields set, or both None. | ||
| """ | ||
| username = os.environ.get("OCI_USERNAME") | ||
| password = os.environ.get("OCI_PASSWORD") | ||
|
|
||
| if username is not None: | ||
| username = username.strip() or None | ||
| if password is not None: | ||
| password = password.strip() or None | ||
|
|
||
| if username and password: | ||
| logger.info("Using OCI registry credentials from environment variables") | ||
| return username, password | ||
| return OciCredentials(username=username, password=password) | ||
|
|
||
| if username or password: | ||
| logger.warning( |
Member
There was a problem hiding this comment.
[MEDIUM] The credential resolution precedence is split across layers. resolve_oci_credentials only implements levels 2-3 (env vars, auth files), while level 1 (explicit args) is independently reimplemented by each driver with different logic. Adding optional username/password parameters to resolve_oci_credentials would consolidate the full three-level precedence in one place.
AI-generated, human reviewed
bennyz
force-pushed
the
oci-follow-up
branch
2 times, most recently
from
e0128a0 to
6979cc1
Compare
raballew
requested changes
May 12, 2026
| oci_username, | ||
| oci_password, | ||
| creds.username, | ||
| creds.plain_password, |
Member
There was a problem hiding this comment.
[MEDIUM] creds.plain_password exercises the OciCredentials.plain_password property, but both test_flash_oci_auto_success and test_flash_oci_auto_error_cases mock resolve_oci_credentials to return OciCredentials() (empty/unauthenticated), so the password is always None in tests. The branch where plain_password actually calls get_secret_value() is never exercised.
Consider adding a test that mocks resolve_oci_credentials to return OciCredentials(username="user", password=SecretStr("pass")) and verifies that flash_oci_image is called with the plain-text password value.
AI-generated, human reviewed
Comment on lines
238
to
+258
| @@ -140,82 +242,109 @@ def _lookup_credentials_in_auth_data(auth_data: dict, registry: str) -> tuple[st | |||
| auth_b64 = value.get("auth") | |||
| if auth_b64: | |||
| try: | |||
| decoded = base64.b64decode(auth_b64).decode("utf-8") | |||
| decoded = base64.b64decode(auth_b64, validate=True).decode("utf-8") | |||
| username, password = decoded.split(":", 1) | |||
| return username, password | |||
| except (ValueError, UnicodeDecodeError) as e: | |||
| logger.warning(f"Failed to decode auth entry for {key}: {e}") | |||
| continue | |||
| if username and password: | |||
| return OciCredentials(username=username, password=password) | |||
| except (binascii.Error, ValueError, UnicodeDecodeError) as e: | |||
| logger.warning("Failed to decode auth entry for %s: %s", key, e) | |||
|
|
|||
| # Some auth files use separate username/password fields | |||
| username = value.get("username") | |||
| password = value.get("password") | |||
| if username and password: | |||
| return username, password | |||
| return OciCredentials(username=username, password=password) | |||
|
|
|||
| return None, None | |||
| return OciCredentials() | |||
Member
There was a problem hiding this comment.
[LOW] _lookup_credentials_in_auth_data calls value.get("auth") assuming each value in auths.items() is a dict. If an auth file contains a non-dict value (e.g. {"auths": {"quay.io": "string"}}), an AttributeError is raised and not caught, halting credential resolution.
Consider adding if not isinstance(value, dict): continue at the start of the loop body.
AI-generated, human reviewed
bennyz
force-pushed
the
oci-follow-up
branch
2 times, most recently
from
ba3cd74 to
283279a
Compare
raballew
approved these changes
May 12, 2026
Introduce a Pydantic OciCredentials model that normalizes empty/whitespace strings and enforces both-or-neither validation. Update all callers to use the new model. Add tests for digest parsing, Docker Hub bare images, and whitespace env var handling. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
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
2 participants
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.
Introduce a Pydantic OciCredentials model that normalizes empty/whitespace strings and enforces both-or-neither validation. Update all callers to use the new model. Add tests for digest parsing, Docker Hub bare images, and whitespace env var handling.
follow up on #649