feat(ssl-certs): add --ssl-certs flag to bundle CA certificates into images#81
feat(ssl-certs): add --ssl-certs flag to bundle CA certificates into images#81feloy wants to merge 3 commits into
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds a new ChangesAdd system CA certificates support
Sequence Diagram(s)sequenceDiagram
participant user as User
participant cli as Cli Parser
participant main as main()
participant run as run()
participant certs as certs module
participant gen as containerfile::generate()
participant docker as Docker Build
user->>cli: --ssl-certs=/path/to/certs
cli->>main: ssl_certs = Some(Some(path))
main->>run: ssl_certs parameter
run->>certs: copy_from_file(context_dir, path)
certs-->>run: Ok(()) or IO error
run->>gen: with_ca_certs = true
gen-->>run: Containerfile with COPY + trust-update
run->>docker: Containerfile with CA bundle
docker-->>run: Built image with trusted CA
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/containerfile.rs (1)
190-220:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove Ubuntu CA trust setup before the first
apt-getnetwork call.The Ubuntu path currently installs the custom CA only after
apt-get update && apt-get install, so it cannot help the package manager trust a corporate MITM/proxy during the build. That misses the core--ssl-certsobjective for Ubuntu images, and the test at Lines 1005-1014 now hard-codes that wrong order.Also applies to: 1005-1014
🤖 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 `@src/containerfile.rs` around lines 190 - 220, The CA install block is placed after the apt-get install so custom CA can't be used by the package manager during build; in the function ubuntu_system_stage move the ca_cert_section (the COPY certs/... and update-ca-certificates) to appear before the RUN apt-get update && apt-get install... line in the formatted Dockerfile string so the CA is installed prior to any network package operations (also update the corresponding test that asserts order around the same block referenced at lines ~1005-1014 to reflect the new order).
🤖 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 `@src/certs.rs`:
- Around line 67-69: copy_from_file currently treats a zero-byte file as success
and calls write_cert_to_context, which makes callers (e.g., src/main.rs) think
CA certs were staged; change copy_from_file to read the file into content, check
if content.is_empty(), and if so return an Err (e.g.,
io::Error::new(io::ErrorKind::InvalidData, "empty certificate bundle")) instead
of calling write_cert_to_context; keep the existing success path unchanged so
non-empty files still call write_cert_to_context.
---
Outside diff comments:
In `@src/containerfile.rs`:
- Around line 190-220: The CA install block is placed after the apt-get install
so custom CA can't be used by the package manager during build; in the function
ubuntu_system_stage move the ca_cert_section (the COPY certs/... and
update-ca-certificates) to appear before the RUN apt-get update && apt-get
install... line in the formatted Dockerfile string so the CA is installed prior
to any network package operations (also update the corresponding test that
asserts order around the same block referenced at lines ~1005-1014 to reflect
the new order).
🪄 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: 9ba0a052-9564-4889-b4d5-08095327b9a5
📒 Files selected for processing (7)
.agents/skills/add-cli-flag/SKILL.mdREADME.mdsrc/certs.rssrc/containerfile.rssrc/main.rstests/fixtures/test-ca.crttests/integration_test.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@wherka-ama this is a port of the PR openkaiden/kdn#556 , this repo being the new way to build images for kaiden |
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 `@README.md`:
- Line 25: The heading "### workspace.json fields" jumps from H1 to H3; change
that heading to H2 (replace "### workspace.json fields" with "## workspace.json
fields") or alternatively insert the missing parent H2 above it so the document
follows a proper H1→H2→H3 hierarchy; update any adjacent table-of-contents or
anchors if present to reflect the new heading level.
🪄 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: c0e74af7-7097-4b95-8def-ecbd29fbcc23
📒 Files selected for processing (7)
.agents/skills/add-cli-flag/SKILL.mdREADME.mdsrc/certs.rssrc/containerfile.rssrc/main.rstests/fixtures/test-ca.crttests/integration_test.rs
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/test-ca.crt
- .agents/skills/add-cli-flag/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration_test.rs
- src/certs.rs
- src/containerfile.rs
- src/main.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@README.md`:
- Line 25: The heading "### workspace.json fields" jumps from H1 to H3; change
that heading to H2 (replace "### workspace.json fields" with "## workspace.json
fields") or alternatively insert the missing parent H2 above it so the document
follows a proper H1→H2→H3 hierarchy; update any adjacent table-of-contents or
anchors if present to reflect the new heading level.
🪄 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: c0e74af7-7097-4b95-8def-ecbd29fbcc23
📒 Files selected for processing (7)
.agents/skills/add-cli-flag/SKILL.mdREADME.mdsrc/certs.rssrc/containerfile.rssrc/main.rstests/fixtures/test-ca.crttests/integration_test.rs
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/test-ca.crt
- .agents/skills/add-cli-flag/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration_test.rs
- src/certs.rs
- src/containerfile.rs
- src/main.rs
🛑 Comments failed to post (1)
README.md (1)
25-25:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix heading level jump at Line 25.
### workspace.json fieldsskips from H1 to H3, which violates MD001 and can break doc structure/navigation in some renderers. Use##here (or insert the missing parent H2).🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 25-25: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
🤖 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 `@README.md` at line 25, The heading "### workspace.json fields" jumps from H1 to H3; change that heading to H2 (replace "### workspace.json fields" with "## workspace.json fields") or alternatively insert the missing parent H2 above it so the document follows a proper H1→H2→H3 hierarchy; update any adjacent table-of-contents or anchors if present to reflect the new heading level.Source: Linters/SAST tools
I would prefer to keep this PR focus on Linux, and create follow-up PRs for other systems |
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
I have a question
should it use the ssl cert automatically (without the empty --ssl-certs=)
I would assume that by default it reads them, that I can specify one or that I can disable it with another flag like --disable-ssl-certs but IMHO I'm finding the pattern --ssl-certs= not a classic pattern ?
I've updated to include auto-detected certs by default. |
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 `@src/main.rs`:
- Around line 411-423: In the ContainerfileCapture struct's run method, the file
read operation using std::fs::read_to_string(path).unwrap_or_default() silently
falls back to an empty string when reading fails, which masks errors and allows
tests to pass incorrectly. Replace the unwrap_or_default() call with proper
error handling that propagates the error up the call chain using the ? operator,
so that file read failures are caught and fail the operation immediately instead
of continuing with empty content.
🪄 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: c1da17f3-731e-4d94-acdf-cfdd99f50929
📒 Files selected for processing (7)
.agents/skills/add-cli-flag/SKILL.mdREADME.mdsrc/certs.rssrc/containerfile.rssrc/main.rstests/fixtures/test-ca.crttests/integration_test.rs
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/add-cli-flag/SKILL.md
- tests/fixtures/test-ca.crt
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration_test.rs
- src/certs.rs
- src/containerfile.rs
…images
Enterprise environments that intercept HTTPS traffic with corporate
proxies need a custom CA certificate trusted both during the build
(so dnf/apt can reach repos) and at runtime (so the agent API calls
work). The flag accepts an optional FILE; omitting the value triggers
auto-discovery across 7 common Linux CA bundle paths. Certs are
installed in the final image via update-ca-trust (DNF) or
update-ca-certificates (Ubuntu), and persist into the final sandbox
image via the FROM system AS final inheritance.
To test auto-discover mode (uses the host's CA bundle if present):
openshell-image-builder --ssl-certs= myimage:latest
To test explicit file mode:
openshell-image-builder \
--ssl-certs /etc/ssl/certs/ca-certificates.crt \
myimage:latest
To verify the cert landed in the final image — Ubuntu base (default):
podman run --rm myimage:latest -c \
'test -f /usr/local/share/ca-certificates/system-ca.crt && echo found'
To test with other base images, create a config.toml first. DNF-based
images (Fedora, UBI, Hummingbird) store the cert under the pki trust
path instead. Example configs and the verification command:
# Fedora
echo '[openshell_image_builder.base_image]
image = "fedora"
tag = "latest"' > /tmp/myconfig/config.toml
# UBI
echo '[openshell_image_builder.base_image]
image = "ubi"
tag = "latest"' > /tmp/myconfig/config.toml
# Hummingbird
echo '[openshell_image_builder.base_image]
image = "hummingbird"
tag = "latest-builder"' > /tmp/myconfig/config.toml
Then build and verify:
openshell-image-builder \
--config /tmp/myconfig \
--ssl-certs /etc/ssl/certs/ca-certificates.crt \
myimage:latest
podman run --rm myimage:latest -c \
'test -f /etc/pki/ca-trust/source/anchors/system-ca.crt && echo found'
To verify error on a missing file:
openshell-image-builder --ssl-certs /nonexistent/bundle.crt myimage:latest
# exits non-zero with an OS error
Closes openkaiden#62
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
…ssl-certs CA certificates from the host are now bundled into every image by default. Use --ssl-certs <FILE> to specify an explicit bundle instead, or --disable-ssl-certs to opt out entirely. The bare --ssl-certs (no value) is removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
…lencing it unwrap_or_default() swallowed file-read failures in the test helper, letting tests pass with empty content when the Containerfile was unreadable. Replaced with ? so the error surfaces immediately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
3 participants
Enterprise environments that intercept HTTPS traffic with corporate proxies need a custom CA certificate trusted both during the build (so dnf/apt can reach repos) and at runtime (so the agent API calls work).
CA certificates from the host are now bundled into every image by default: the tool auto-discovers across 7 common Linux CA bundle paths and uses the first one it finds. Use
--ssl-certs <FILE>to point to a specific bundle instead, or--disable-ssl-certsto opt out entirely.Certs are installed in the final image via
update-ca-trust(DNF) orupdate-ca-certificates(Ubuntu), and persist into the final sandbox image via theFROM system AS finalinheritance.To test default auto-discover mode (uses the host's CA bundle if present):
To test explicit file mode:
To build without any certificates:
To verify the cert landed in the final image — Ubuntu base (default):
podman run --rm myimage:latest -c \ 'test -f /usr/local/share/ca-certificates/system-ca.crt && echo found'To test with other base images, create a config.toml first. DNF-based images (Fedora, UBI, Hummingbird) store the cert under the pki trust path instead. Example configs and the verification command:
Then build and verify:
openshell-image-builder \ --config /tmp/myconfig \ --ssl-certs /etc/ssl/certs/ca-certificates.crt \ myimage:latest podman run --rm myimage:latest -c \ 'test -f /etc/pki/ca-trust/source/anchors/system-ca.crt && echo found'To verify error on a missing file:
openshell-image-builder --ssl-certs /nonexistent/bundle.crt myimage:latest # exits non-zero with an OS errorCloses #62