feat(cli): add --proxy-cert flag for proxy SSL inspection support #511
Conversation
Summary of ChangesHello @doringeman, 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 addresses the need for Docker Model Runner to operate seamlessly within corporate environments that utilize SSL inspection proxies. By introducing a Highlights
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of appending the mounted proxy cert directly to
/etc/ssl/certs/ca-certificates.crt, consider using the distro-native mechanism (e.g., keeping it in/usr/local/share/ca-certificatesand invokingupdate-ca-certificates) to avoid brittle assumptions about the trust store path and format. - The
execInContainerhelper now always runs commands asroot; if only the CA-bundle update needs elevated privileges, consider making the user configurable or adding a dedicatedexecAsRootInContainerto limit the blast radius of future uses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of appending the mounted proxy cert directly to `/etc/ssl/certs/ca-certificates.crt`, consider using the distro-native mechanism (e.g., keeping it in `/usr/local/share/ca-certificates` and invoking `update-ca-certificates`) to avoid brittle assumptions about the trust store path and format.
- The `execInContainer` helper now always runs commands as `root`; if only the CA-bundle update needs elevated privileges, consider making the user configurable or adding a dedicated `execAsRootInContainer` to limit the blast radius of future uses.
## Individual Comments
### Comment 1
<location> `cmd/cli/pkg/standalone/containers.go:95` </location>
<code_context>
execConfig := container.ExecOptions{
- Cmd: []string{"sh", "-c", cmd},
+ Cmd: []string{"sh", "-c", cmd},
+ User: "root",
}
execResp, err := dockerClient.ContainerExecCreate(ctx, containerID, execConfig)
</code_context>
<issue_to_address>
**🚨 issue (security):** Running all execs as root may be unnecessarily privileged and surprising for containers that define a non-root default user.
This change alters behavior for images that deliberately run as non-root and may violate security expectations by always elevating to root. Please consider limiting root usage to the specific privileged operations that need it (e.g., via an `asRoot` flag or a separate helper), and keep default execs running as the container’s configured user.
</issue_to_address>
### Comment 2
<location> `cmd/cli/pkg/standalone/containers.go:456-470` </location>
<code_context>
}
}
+
+ // Add proxy certificate to the system CA bundle
+ if created && proxyCert != "" {
+ printer.Printf("Adding proxy certificate to CA bundle...\n")
+ // Append the proxy cert to the system CA bundle
+ appendCmd := "cat " + proxyCertContainerPath + " >> /etc/ssl/certs/ca-certificates.crt"
+ if err := execInContainer(ctx, dockerClient, resp.ID, appendCmd); err != nil {
+ printer.Printf("Warning: failed to add proxy certificate to CA bundle: %v\n", err)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Appending directly to ca-certificates.crt is brittle and bypasses the distro’s certificate management tooling.
The current `cat ... >> /etc/ssl/certs/ca-certificates.crt` approach:
- Assumes that specific bundle path/format, which can differ across images.
- Skips `update-ca-certificates` (or equivalent), so a future CA regeneration may discard this change.
Given the cert is already mounted under `/usr/local/share/ca-certificates`, consider calling the image’s CA update command (e.g., `update-ca-certificates`) instead of editing the bundle directly for better portability and durability.
```suggestion
// Add proxy certificate to the system CA bundle
if created && proxyCert != "" {
printer.Printf("Updating CA certificates inside container...\n")
// Ask the image's CA management tooling to pick up the mounted cert from /usr/local/share/ca-certificates
updateCmd := "if command -v update-ca-certificates >/dev/null 2>&1; then " +
"update-ca-certificates; " +
"elif command -v update-ca-trust >/dev/null 2>&1; then " +
"update-ca-trust extract; " +
"else " +
"echo 'No known CA update tool found (tried update-ca-certificates, update-ca-trust)' >&2; " +
"exit 1; " +
"fi"
if err := execInContainer(ctx, dockerClient, resp.ID, updateCmd); err != nil {
printer.Printf("Warning: failed to update CA certificates inside container: %v\n", err)
} else {
// Restart the container so the model-runner process picks up the refreshed CA bundle
printer.Printf("Restarting container to apply CA certificate...\n")
if err := dockerClient.ContainerRestart(ctx, resp.ID, container.StopOptions{}); err != nil {
printer.Printf("Warning: failed to restart container after adding CA certificate: %v\n", err)
}
}
}
```
</issue_to_address>
### Comment 3
<location> `cmd/cli/pkg/standalone/containers.go:325-331` </location>
<code_context>
hostConfig.Mounts = append(hostConfig.Mounts, ascendMounts...)
}
+ if proxyCert != "" {
+ hostConfig.Mounts = append(hostConfig.Mounts, mount.Mount{
+ Type: mount.TypeBind,
+ Source: proxyCert,
+ Target: proxyCertContainerPath,
+ ReadOnly: true,
+ })
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Binding the proxy cert path without validation may produce a non-obvious Docker error if the file is missing or invalid.
If `--proxy-cert` points to a missing or unreadable file, the container will fail to start due to the bind mount, but the CLI will only show a generic startup error. Consider validating the path (existence/readability) before adding the mount so you can return a clearer, targeted error when the certificate path is invalid.
Suggested implementation:
```golang
import (
"os"
```
```golang
if proxyCert != "" {
info, err := os.Stat(proxyCert)
if err != nil {
// Fail fast with a clear, targeted error instead of a generic Docker bind error
return nil, nil, fmt.Errorf("proxy certificate path %q is not accessible: %w", proxyCert, err)
}
if !info.Mode().IsRegular() {
return nil, nil, fmt.Errorf("proxy certificate path %q is not a regular file", proxyCert)
}
hostConfig.Mounts = append(hostConfig.Mounts, mount.Mount{
Type: mount.TypeBind,
Source: proxyCert,
Target: proxyCertContainerPath,
ReadOnly: true,
})
}
```
Because I cannot see the full function, you will need to:
1. Adjust the `return nil, nil, fmt.Errorf(...)` lines to match the actual return types of the function where this `proxyCert` logic lives.
- For example, if the function returns `(config *container.Config, hostConfig *container.HostConfig, err error)`, the `nil, nil, err` form is correct; otherwise, replace the `nil` placeholders appropriately.
2. Ensure `fmt` is imported in this file (if it is not already):
- Add `fmt` to the import block: `import ("fmt" ... )`.
3. If `os` is already imported, remove the duplicate `os` line from the suggested import change.
4. Keep error messages consistent with the rest of the CLI (e.g. wrapping with your own error type or prefixing with command context if that is the existing pattern).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| execConfig := container.ExecOptions{ | ||
| Cmd: []string{"sh", "-c", cmd}, | ||
| Cmd: []string{"sh", "-c", cmd}, | ||
| User: "root", |
There was a problem hiding this comment.
🚨 issue (security): Running all execs as root may be unnecessarily privileged and surprising for containers that define a non-root default user.
This change alters behavior for images that deliberately run as non-root and may violate security expectations by always elevating to root. Please consider limiting root usage to the specific privileged operations that need it (e.g., via an asRoot flag or a separate helper), and keep default execs running as the container’s configured user.
| if proxyCert != "" { | ||
| hostConfig.Mounts = append(hostConfig.Mounts, mount.Mount{ | ||
| Type: mount.TypeBind, | ||
| Source: proxyCert, | ||
| Target: proxyCertContainerPath, | ||
| ReadOnly: true, | ||
| }) |
There was a problem hiding this comment.
suggestion (bug_risk): Binding the proxy cert path without validation may produce a non-obvious Docker error if the file is missing or invalid.
If --proxy-cert points to a missing or unreadable file, the container will fail to start due to the bind mount, but the CLI will only show a generic startup error. Consider validating the path (existence/readability) before adding the mount so you can return a clearer, targeted error when the certificate path is invalid.
Suggested implementation:
import (
"os" if proxyCert != "" {
info, err := os.Stat(proxyCert)
if err != nil {
// Fail fast with a clear, targeted error instead of a generic Docker bind error
return nil, nil, fmt.Errorf("proxy certificate path %q is not accessible: %w", proxyCert, err)
}
if !info.Mode().IsRegular() {
return nil, nil, fmt.Errorf("proxy certificate path %q is not a regular file", proxyCert)
}
hostConfig.Mounts = append(hostConfig.Mounts, mount.Mount{
Type: mount.TypeBind,
Source: proxyCert,
Target: proxyCertContainerPath,
ReadOnly: true,
})
}Because I cannot see the full function, you will need to:
- Adjust the
return nil, nil, fmt.Errorf(...)lines to match the actual return types of the function where thisproxyCertlogic lives.- For example, if the function returns
(config *container.Config, hostConfig *container.HostConfig, err error), thenil, nil, errform is correct; otherwise, replace thenilplaceholders appropriately.
- For example, if the function returns
- Ensure
fmtis imported in this file (if it is not already):- Add
fmtto the import block:import ("fmt" ... ).
- Add
- If
osis already imported, remove the duplicateosline from the suggested import change. - Keep error messages consistent with the rest of the CLI (e.g. wrapping with your own error type or prefixing with command context if that is the existing pattern).
![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 introduces a --proxy-cert flag to support SSL inspection by corporate proxies. The changes are well-integrated into the install-runner, reinstall-runner, start-runner, and restart-runner commands. The implementation correctly mounts the user-provided certificate into the container and triggers a restart. My review includes one suggestion to improve the robustness of how the system's CA certificate bundle is updated inside the container, to ensure broader compatibility with different applications.
Signed-off-by: Dorin Geman <[email protected]>
doringeman
force-pushed
the
proxy-cert
branch
from
8c97e43 to
e712f68
Compare
1 participant
Fixes #500.
Add support for corporate proxies that perform SSL interception allowing users to provide a CA certificate file.
The flag is available on install-runner, reinstall-runner, start-runner and restart-runner commands.
When provided, the certificate is:
Test
HTTP_PROXYandHTTPS_PROXY:E.g.,