flashers: chunk long credentials over serial

[bennyz]

serial access cannot handle very long strings we need to pass, for example, jwt token from openshift. instead chunk into smaller pieces and write incrementally

also remove the cmdline --oci-username and --oci-password, and leave only env vars

Summary by CodeRabbit

• New Features

  • OCI registry credentials can be sourced from environment variables for OCI image flows.

• Documentation

  • CLI docs updated to remove OCI credential command-line options.

• Improvements

  • Credential files are now written as base64-encoded chunks, decoded on target, and created with stricter permissions; redaction limited to passwords.

• Tests

  • Added tests for env-sourced credentials, chunked encoding/decoding, long-token handling, and HTTP vs OCI behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Removed CLI OCI credential options; added environment-based OCI credential resolution for oci:// image paths; changed FLS credential-file creation to write base64-encoded chunks and decode them on target. Tests added/updated to cover env resolution, chunking, and long-token handling.

Changes

Cohort / File(s)	Summary
Documentation python/packages/jumpstarter-driver-flashers/README.md	Removed --oci-username and --oci-password from CLI documentation.
Core Implementation python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py	Added base64 import; added _resolve_oci_credentials() to read OCI_USERNAME/OCI_PASSWORD from environment for oci:// paths; removed oci_username/oci_password from flash() CLI signature and its callers; narrowed redaction to only oci_password; updated _setup_fls_oci_credential_file() to emit base64 .b64 chunks, decode on target to final creds file, and apply restrictive perms.
Tests python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py	Added tests for env-based OCI credential resolution (success/failure), updated credential-file tests to validate chunked base64 write/decode and chmod, added long-token chunking tests, and verified non-OCI HTTP paths ignore OCI env vars.

Sequence Diagram

sequenceDiagram
    participant CLI as User/CLI
    participant Client as BaseFlasherClient
    participant Env as Environment
    participant FS as FileSystem
    participant FLS as FlasherService

    CLI->>Client: flash(image="oci://...", oci_username=None, oci_password=None)
    Client->>Client: detect "oci://" scheme
    Client->>Env: read OCI_USERNAME & OCI_PASSWORD
    Env-->>Client: return username/password or None
    Client->>Client: _resolve_oci_credentials() -> (user, pass)
    Client->>FS: write /tmp/fls_creds.b64 (base64 chunks)
    FS-->>Client: chunks written
    Client->>FS: decode .b64 -> /tmp/fls_creds
    Client->>FS: chmod 600 /tmp/fls_creds
    Client->>FLS: call FLS with credential file path
    FLS->>FS: read /tmp/fls_creds
    FLS-->>Client: flash completed

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• build(deps): bump the go-operator-dependencies group across 1 directory with 9 updates #610: Modifies the same flash() CLI/driver surface and signatures in client.py.
• flashers: add support for passing OCI credentials to fls #211: Earlier changes to OCI credential handling and FLS credential-file flow in the same client module.

Suggested reviewers

• raballew
• mangelajo

Poem

🐇 I nibbled env vars in the night,

tucked secrets into base64 light,
split tokens into tidy rolls,
decoded softly on target knolls,
then watched the flasher gleam and bright.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'flashers: chunk long credentials over serial' directly and clearly summarizes the main change: implementing chunking of long credential strings for serial transmission.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)

1356-1384: Unnecessary initial truncation of creds_file on Line 1366.

The true > creds_file on Line 1366 is redundant — Line 1380 (base64 -d ... > creds_file) overwrites it with a redirect. This costs one extra serial roundtrip for no benefit. Consider removing it, or if the intent is to ensure the file exists early, a comment would help clarify.

Proposed fix: remove the unnecessary truncation

         with self._temporarily_disable_console_debug_stream(console):
-            console.sendline(f"true > {shlex.quote(creds_file)}")
-            console.expect(prompt, timeout=EXPECT_TIMEOUT_DEFAULT)
-
             # Write base64 data in chunks to a temp file
             b64_file = f"{creds_file}.b64"
             console.sendline(f"true > {shlex.quote(b64_file)}")

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)

136-223: Consider extracting the duplicated MockConsole class.

MockConsole is defined identically in both test_setup_fls_oci_credential_file (Line 140) and test_setup_fls_oci_credential_file_chunks_long_tokens (Line 187). Extract it to module scope or a pytest fixture to reduce duplication.

Also, import base64 on Lines 169 and 215 could be moved to the file-level imports for clarity.

Proposed refactor: extract MockConsole and top-level import

Add at the top of the file:

import base64

And define MockConsole at module level (e.g., near MockFlasherClient):

class MockConsole:
    def __init__(self):
        self.logfile_read = object()
        self.sent_lines = []
        self.expect_calls = []

    def sendline(self, line):
        self.sent_lines.append(line)

    def expect(self, prompt, timeout=None):
        self.expect_calls.append((prompt, timeout))

Then remove the inline definitions and import base64 from within the test bodies.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bennyz]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

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-driver-flashers/jumpstarter_driver_flashers/client.py (1)

1318-1321: ⚠️ Potential issue | 🟠 Major

The .b64 temp file containing encoded credentials is never cleaned up.

_cleanup_fls_oci_credential_file removes only creds_file but leaves {creds_file}.b64 on disk. While base64 is not encryption, the file still contains recoverable credentials.

🔒 Proposed fix: also remove the .b64 file

     def _cleanup_fls_oci_credential_file(self, console, prompt: str, creds_file: str):
         with self._temporarily_disable_console_debug_stream(console):
             console.sendline(f"rm -f {shlex.quote(creds_file)}")
             console.expect(prompt, timeout=EXPECT_TIMEOUT_DEFAULT)
+            console.sendline(f"rm -f {shlex.quote(creds_file + '.b64')}")
+            console.expect(prompt, timeout=EXPECT_TIMEOUT_DEFAULT)

🤖 Fix all issues with AI agents

In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 138-139: The docstring string literal for the flash method was
misplaced below _resolve_oci_credentials and is now a no-op; move the
triple-quoted string """Flash image to DUT""" directly beneath the def
flash(...): signature so it becomes the actual docstring for the flash method
(update the def flash in jumpstarter_driver_flashers.client to place the
docstring immediately after the def line and remove the stray string below
_resolve_oci_credentials).

🧹 Nitpick comments (2)

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)

583-583: Redaction scope narrowed to only oci_password — intentional?

Previously both oci_username and oci_password were redacted. Now only the password is redacted. If the username is a service account name this is fine, but for some OCI registries the username can also be sensitive (e.g., API key IDs). Consider whether oci_username should remain redacted.

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)

181-221: Good long-token chunking test, but the cleanup test doesn't cover the .b64 file.

The chunking and roundtrip assertions are solid. However, none of the tests verify that .b64 temp file is cleaned up — which aligns with the gap flagged in client.py. Once the cleanup fix is applied, consider adding an assertion for it here.

Also, MockConsole is duplicated between this test and test_setup_and_cleanup_fls_oci_credential_file (line 135). Consider extracting it to module level.

[bennyz]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1279-1311: The temp base64 file (b64_file) created in
_setup_fls_oci_credential_file is never removed; after decoding and setting
perms on creds_file, send a console command to delete the intermediate file
(e.g., console.sendline(f"rm -f {shlex.quote(b64_file)}") followed by
console.expect(prompt,...)) so /tmp/fls_creds.b64 is cleaned up; ideally place
this rm call after the chmod step inside the with
self._temporarily_disable_console_debug_stream(console) block (or ensure it runs
in a finally/cleanup path if you want guaranteed removal on errors).

🧹 Nitpick comments (2)

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (2)

164-164: Move import base64 to the top of the test file.

base64 is imported inline in two test functions. Hoisting it to the module level keeps the style consistent and avoids redundant imports.

Proposed fix

+import base64
 import shlex
 from concurrent.futures import CancelledError

Then remove the inline import base64 on lines 164 and 210.

Also applies to: 210-210

---

182-192: MockConsole is duplicated across two tests — consider extracting it.

The identical MockConsole class appears in both test_setup_fls_oci_credential_file (line 135) and test_setup_fls_oci_credential_file_chunks_long_tokens (line 182). Extracting it to module scope would reduce duplication.

[coderabbitai]

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-driver-flashers/jumpstarter_driver_flashers/client.py (1)

1296-1305: ⚠️ Potential issue | 🟡 Minor

Stale error message references removed CLI options.

_validate_oci_credentials still mentions --oci-username and --oci-password, but those CLI flags were removed in this PR. CLI users who hit this error will see instructions that don't apply.

Proposed fix

         if bool(username) != bool(password):
             raise click.ClickException(
-                "OCI authentication requires both --oci-username and --oci-password "
-                "(or OCI_USERNAME and OCI_PASSWORD environment variables)"
+                "OCI authentication requires both OCI_USERNAME and OCI_PASSWORD "
+                "environment variables (or both oci_username and oci_password arguments)"
             )

🧹 Nitpick comments (2)

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)

1353-1385: Chunked base64 credential writing — solid approach for serial buffer limits.

The implementation correctly:

• Encodes credentials as base64 to avoid shell-special characters in raw content.
• Splits into 512-byte chunks to stay well within typical serial line buffer limits.
• Uses shlex.quote on all dynamic values for injection safety.
• Decodes on-target with base64 -d.

One minor note: the umask 077 on line 1367 persists for the rest of the shell session, which could restrict permissions on files created by subsequent commands (e.g., fls writing temp files). In a single-user initramfs context this is likely harmless, but wrapping it in a subshell would be more defensive:

Optional: scope umask to a subshell

-            console.sendline(f"umask 077 && true > {shlex.quote(creds_file)}")
+            console.sendline(f"(umask 077 && true > {shlex.quote(creds_file)})")

python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)

183-223: Good test for the long-token chunking behavior.

Validates that a ~1400-char JWT-like token produces multiple chunks, each within serial buffer limits, and that the full roundtrip decode is correct.

One small observation: the assertion on line 222 relies on shlex.quote not adding quotes around a purely alphanumeric string (which is correct per CPython's shlex._find_unsafe regex). If the test token ever includes characters like +, /, or = (common in real JWTs), shlex.quote would wrap the token, and the assertion would need adjustment. Consider adding a brief comment or a follow-up test with a more realistic JWT character set.