feat: add --with-ssl-cert-file option to build command #6046

aeijdenberg · 2025-03-09T05:48:43Z

This makes the root CAs from the host available to containers during a build.

What type of PR is this?

/kind feature

What this PR does / why we need it:

This makes it easier to use buildah to build an image in environments that require custom certificate authorities to make outbound internet connections (for example when using a TLS intercepting HTTPS proxy).

Currently buildah will automatically pass https_proxy (and related) environment variables as ephemeral environment variables to the container for RUN commands.

This change adds an option --with-ssl-cert-file (defaults to false) that when set will:

Resolve the root CA file currently in use by the host (uses SSL_CERT_FILE if set, else falls back to searching common paths).
Mount this file within the container during the RUN command at a different location (/host-ssl-cert-file).
Sets SSL_CERT_FILE=/host-ssl-cert-file ephemeral environment variable for RUN commands.

A similar outcome can be achieved today by editing buildah command to bind mount the current CA store and then edit each RUN instruction in the Containerfile to use, however it felt like it would be nice to make a short-cut for this which seems to augment well the existing https_proxy variable propagation.

Current workaround:

./bin/buildah build --secret=id=ca,src=/etc/ssl/certs/ca-certificates.crt -f - <<'EOF'
FROM alpine:latest
ENV SSL_CERT_FILE=/host-ca-file
RUN --mount=type=secret,id=ca,target=/host-ca-file echo "SSL_CERT_FILE: ${SSL_CERT_FILE} - Contents:" && head -n10 "${SSL_CERT_FILE}"
EOF

How to verify it

Behaviour without flag:

./bin/buildah build -f - <<'EOF'
FROM alpine:latest
RUN echo "SSL_CERT_FILE: ${SSL_CERT_FILE} - Contents:" && head -n10 "${SSL_CERT_FILE}"
EOF

Output:

STEP 1/2: FROM alpine:latest
STEP 2/2: RUN echo "SSL_CERT_FILE: ${SSL_CERT_FILE} - Contents:" && head -n10 "${SSL_CERT_FILE}"
SSL_CERT_FILE:  - Contents:
head: : No such file or directory
Error: building at STEP "RUN echo "SSL_CERT_FILE: ${SSL_CERT_FILE} - Contents:" && head -n10 "${SSL_CERT_FILE}"": while running runtime: exit status 1

Behaviour with flag:

./bin/buildah build --with-ssl-cert-file -f - <<'EOF'
FROM alpine:latest
RUN echo "SSL_CERT_FILE: ${SSL_CERT_FILE} - Contents:" && head -n10 "${SSL_CERT_FILE}"
EOF

Output:

STEP 1/2: FROM alpine:latest
STEP 2/2: RUN echo "SSL_CERT_FILE: ${SSL_CERT_FILE} - Contents:" && head -n10 "${SSL_CERT_FILE}"
SSL_CERT_FILE: /host-ssl-cert-file - Contents:
-----BEGIN CERTIFICATE-----
MIIH0zCCBbugAwIBAgIIXsO3pkN/pOAwDQYJKoZIhvcNAQEFBQAwQjESMBAGA1UE
(lots more output)

Tested with a local HTTPS proxy server with a self-signed CA and verified that basic apk add --update --no-cache python3 and similar commands worked without error.

(Tested similar on ubuntu image, although apt I think runs over HTTP so test wasn't as meaningful)

Includes unit and integration tests to verify behaviour.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Some issues to consider:

Search path for host CAs

To get the search path of commonly used host CA files I used the list found here:
https://cs.opensource.google/go/go/+/refs/tags/go1.24.1:src/crypto/x509/root_linux.go;l=9-17

Unfortunately that variable is private, and I'm not able to find anything within the standard x509 module that exports it or otherwise makes it available, which is why I put that list in a separate file and copy/pasted the Go LICENSE within there.

It feels less than ideal and open to other ideas.

Path to bind to within the container

This PR currently binds it to /host-ssl-cert-file so that it doesn't clash with commonly used paths within a container. This seemed to work fine in my limited testing when using SSL_CERT_FILE to point to it, however it's not clear whether SSL_CERT_FILE will work for all applications and whether it may be best to instead bind this on top of well-known locations for where this file normally is.

Happy to take feedback on alternate suggestions.

Does this PR introduce a user-facing change?

Adds --with-ssl-cert-file option to build command. If set this propagates the host CAs into the container during the build process.

docs/buildah-build.1.md

docs/buildah-from.1.md

TomSweeneyRedHat · 2025-03-20T20:32:16Z

util/x509_unix.go

+	"/etc/pki/tls/cacert.pem",                           // OpenELEC
+	"/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7
+	"/etc/ssl/cert.pem",                                 // Alpine Linux
+}


Is x509_unix.go an appropriate name? With the first line, I thought maybe x509_windows.go? Similar comment next file.

I think so? I was copying the style used for other files in this repo such as 30c2e31

TomSweeneyRedHat · 2025-03-20T20:35:54Z

/approve

openshift-ci · 2025-03-20T20:36:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aeijdenberg, TomSweeneyRedHat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [TomSweeneyRedHat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TomSweeneyRedHat · 2025-03-20T20:36:25Z

Generally looks good to me, a couple of nits and it looks like your branch needs updating.

packit-as-a-service · 2025-03-21T06:17:48Z

Ephemeral COPR build failed. @containers/packit-build please check.

aeijdenberg · 2025-03-21T11:07:45Z

Accepted changes, rebased, squashed and fixed the issue in the Fedora tests, by using the same pattern as for similar files (such as /etc/hostname). Thanks for the review.

TomSweeneyRedHat · 2025-03-21T19:09:48Z

@nalind or @Luap99 PTAL

Luap99

I don't have a strong opinion on this one but this seems like a slippery slope.

I can certainly see why this argument might be handy but allowing this might open the door for a lot of other similar things. Personally I think this is better done by callers to set up the env and mount in any way they like. Hard coding /host-ssl-cert-file just feels super awkward, and no it is not about the specific path name but just in general.

I would like to know what @nalind thinks about this as I am not really a buildah maintainer.

Luap99 · 2025-03-24T15:36:54Z

run_common.go

+	hostCertBytes, err := os.ReadFile(resolvedSSLCertPath)
+	if err != nil {
+		return "", fmt.Errorf("error reading cert file on host: %w", err)
+	}
+
+	cfile := filepath.Join(rdir, filepath.Base(containerPath))
+	if err = ioutils.AtomicWriteFile(cfile, hostCertBytes, 0o644); err != nil {
+		return "", fmt.Errorf("writing %s into the container: %w", containerPath, err)
+	}


I don't know how big the certs files can be but generally it seems best to copy it without reading the full thing into memory.

Changed to use io.Copy()

Luap99 · 2025-03-24T15:39:29Z

util/x509.go

+		}
+		return potFile, nil
+	}
+	return "", errors.New("please set SSL_CERT_FILE to use --ssl-cert-file option")


Nothing in this function relates to the cli option. Buildah might be used via the API as well so the cli options should generally not be used in the backend as these errors would make no sense to callers.

Thanks that makes sense. Have reworded the message.

aeijdenberg · 2025-03-25T09:19:38Z

I can certainly see why this argument might be handy but allowing this might open the door for a lot of other similar things. Personally I think this is better done by callers to set up the env and mount in any way they like. Hard coding /host-ssl-cert-file just feels super awkward, and no it is not about the specific path name but just in general.

If it wasn't set to a hard-coded path, and rather something like /tmp-3727282 instead (different random on each run) would that remove part of the objection?

ie this patch also sets env variable SSL_CERT_FILE to point to that path so it ougn't matter where it is mounted and that would avoid a hard-coded path name.

aeijdenberg · 2025-03-25T09:57:06Z

If it wasn't set to a hard-coded path, and rather something like /tmp-3727282 instead (different random on each run) would that remove part of the objection?

(I was making some other changes to address other feedback, and made a change like the above at the same time and pushed that onto this branch for discussion)

This makes the root CAs from the host available to containers during a build. Co-authored-by: Tom Sweeney <tsweeney@redhat.com> Signed-off-by: Adam Eijdenberg <adam@continusec.com>

openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 9, 2025

aeijdenberg force-pushed the addsslcertfileoption branch from ded206e to e5f136d Compare March 10, 2025 09:04

aeijdenberg changed the title ~~feat: add --ssl-cert-file option to build command~~ feat: add --with-ssl-cert-file option to build command Mar 10, 2025

TomSweeneyRedHat reviewed Mar 20, 2025

View reviewed changes

docs/buildah-build.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Mar 20, 2025

View reviewed changes

docs/buildah-from.1.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Mar 20, 2025

View reviewed changes

openshift-ci bot added the approved label Mar 20, 2025

aeijdenberg force-pushed the addsslcertfileoption branch 2 times, most recently from 8e4b52a to fd18d72 Compare March 21, 2025 10:34

Luap99 reviewed Mar 24, 2025

View reviewed changes

feat: add --with-ssl-cert-file option to build command

b81b823

This makes the root CAs from the host available to containers during a build. Co-authored-by: Tom Sweeney <tsweeney@redhat.com> Signed-off-by: Adam Eijdenberg <adam@continusec.com>

aeijdenberg force-pushed the addsslcertfileoption branch from 28070e3 to b81b823 Compare March 26, 2025 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add --with-ssl-cert-file option to build command #6046

feat: add --with-ssl-cert-file option to build command #6046

aeijdenberg commented Mar 9, 2025 •

edited

Loading

TomSweeneyRedHat Mar 20, 2025

aeijdenberg Mar 21, 2025

TomSweeneyRedHat commented Mar 20, 2025

openshift-ci bot commented Mar 20, 2025

TomSweeneyRedHat commented Mar 20, 2025

packit-as-a-service bot commented Mar 21, 2025

aeijdenberg commented Mar 21, 2025

TomSweeneyRedHat commented Mar 21, 2025

Luap99 left a comment

Luap99 Mar 24, 2025

aeijdenberg Mar 25, 2025

Luap99 Mar 24, 2025

aeijdenberg Mar 25, 2025

aeijdenberg commented Mar 25, 2025

aeijdenberg commented Mar 25, 2025

feat: add --with-ssl-cert-file option to build command #6046

Are you sure you want to change the base?

feat: add --with-ssl-cert-file option to build command #6046

Conversation

aeijdenberg commented Mar 9, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Search path for host CAs

Path to bind to within the container

Does this PR introduce a user-facing change?

TomSweeneyRedHat Mar 20, 2025

Choose a reason for hiding this comment

aeijdenberg Mar 21, 2025

Choose a reason for hiding this comment

TomSweeneyRedHat commented Mar 20, 2025

openshift-ci bot commented Mar 20, 2025

TomSweeneyRedHat commented Mar 20, 2025

packit-as-a-service bot commented Mar 21, 2025

aeijdenberg commented Mar 21, 2025

TomSweeneyRedHat commented Mar 21, 2025

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Mar 24, 2025

Choose a reason for hiding this comment

aeijdenberg Mar 25, 2025

Choose a reason for hiding this comment

Luap99 Mar 24, 2025

Choose a reason for hiding this comment

aeijdenberg Mar 25, 2025

Choose a reason for hiding this comment

aeijdenberg commented Mar 25, 2025

aeijdenberg commented Mar 25, 2025

aeijdenberg commented Mar 9, 2025 •

edited

Loading