Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolving missing ca-certificates dependency that causes client ssl verify to fail #216

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

Darlelet
Copy link
Contributor

@Darlelet Darlelet commented Jun 28, 2023

Without this package, /etc/ssl/certs is either empty or incomplete, and so does the @system-ca variable within haproxy.

This results in some haproxy client ssl features not working out of the box. For instance, using httpclient with https endpoints will not work with default config since ssl verify is on by default.

For the debian image:

  • the ca-certificates package was already installed as a build dependency, but all build dependencies are automatically removed during the image cleanup, so I moved it out of the build dependencies.
  • As the openssl binary is a hard dependency for ca-certificates package (probably to regen/reconfigure the certs), we manually remove it during the image cleanup using rm.

How to test it:

  • show ssl ca-file @system-ca haproxy cli should not return errors and should display a large list of CA certificates (more than 1)

@Darlelet
Copy link
Contributor Author

Sorry for the bump, making sure this wasn't overlooked @tianon, I can understand if this is not a priority 😄

Thanks!

@tianon
Copy link
Member

tianon commented Dec 14, 2023

Somehow I missed this completely and appreciate the bump! 🙇 ❤️

I think we should move the installation up to a separate layer (before ENV HAPROXY_VERSION) that just installs ca-certificates.

Also, I'm not thrilled about the removal of the openssl binary -- I agree it's not ideal that it gets installed too, but I don't think I understand what's solved by just removing the binary (except that now dpkg is going to be annoyed, as are any users trying to do update-ca-certificates after they install their own certs 😅).

@Darlelet
Copy link
Contributor Author

Darlelet commented Dec 15, 2023

No problem! Ah yes indeed I missed that, thanks for the pointers!
I moved the ca-certificates installation in the first RUN section (before ENV declarations), but maybe you meant a completely separate layer?

@tianon
Copy link
Member

tianon commented Dec 18, 2023

maybe you meant a completely separate layer?

Yeah, sorry, I meant exactly that 😅

Maybe even a comment to remind us why so we don't remove it in the future 👀

diff --git a/Dockerfile.template b/Dockerfile.template
index f4154c8..0f5afbb 100644
--- a/Dockerfile.template
+++ b/Dockerfile.template
@@ -1,6 +1,13 @@
 {{ if env.variant == "alpine" then ( -}}
 FROM alpine:{{ .alpine }}
 
+# runtime dependencies
+RUN set -eux; \
+	apk add --no-cache \
+# @system-ca: https://github.com/docker-library/haproxy/pull/216
+		ca-certificates \
+	;
+
 # roughly, https://git.alpinelinux.org/aports/tree/main/haproxy/haproxy.pre-install?h=3.12-stable
 RUN set -eux; \
 	addgroup --gid 99 --system haproxy; \
@@ -18,6 +25,15 @@ RUN set -eux; \
 {{ ) else ( -}}
 FROM debian:{{ .debian }}
 
+# runtime dependencies
+RUN set -eux; \
+	apt-get update; \
+	apt-get install -y --no-install-recommends \
+# @system-ca: https://github.com/docker-library/haproxy/pull/216
+		ca-certificates \
+	; \
+	rm -rf /var/lib/apt/lists/*
+
 # roughly, https://salsa.debian.org/haproxy-team/haproxy/-/blob/732b97ae286906dea19ab5744cf9cf97c364ac1d/debian/haproxy.postinst#L5-6
 RUN set -eux; \
 	groupadd --gid 99 --system haproxy; \

@tianon
Copy link
Member

tianon commented Dec 18, 2023

Hmm, thinking about it more there's a pretty compelling argument for not installing it on Alpine, but I don't feel super strongly about it one way or another -- @yosifkit?

@yosifkit
Copy link
Member

Hmm, thinking about it more there's a pretty compelling argument for not installing it on Alpine, but I don't feel super strongly about it one way or another -- @yosifkit?

It seems fine to install it. ca-certificates is installed in other webserver/proxy servers: nginx:alpine, php:alpine, and httpd:alpine. It is also relatively pretty small:

$ docker images haproxy
REPOSITORY   TAG       IMAGE ID       CREATED      SIZE
haproxy      alpine    aa6c154774d2   2 days ago   25.3MB

$ docker run -it --user root haproxy:alpine sh
# apk add --no-cache ca-certificates
...
(1/1) Installing ca-certificates (20230506-r0)
...
# apk info ca-certificates
...
ca-certificates-20230506-r0 installed size:
688 KiB

…erify to fail

Without this package, '/etc/ssl/certs' is either empty or incomplete, and
so does the @system-ca variable within haproxy.

This results in some haproxy client ssl features not working out of the
box. For instance, using httpclient with https endpoints will not work
with default config since ssl verify is on by default.

See PR docker-library#216.

Co-authored-by: Tianon Gravi <[email protected]>
@Darlelet
Copy link
Contributor Author

Thank you very much for your help guys!
I updated the PR to include @tianon modifications 😄

@tianon
Copy link
Member

tianon commented Dec 21, 2023

Your patience and long-suffering is appreciated! ❤️

@tianon tianon merged commit 79a32f7 into docker-library:master Dec 21, 2023
34 checks passed
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Dec 21, 2023
Changes:

- docker-library/haproxy@79a32f7: Merge pull request docker-library/haproxy#216 from Darlelet/ca-certs
- docker-library/haproxy@71cd019: Resolving missing ca-certificates dependency that causes client ssl verify to fail
martin-g pushed a commit to martin-g/docker-official-images that referenced this pull request Apr 3, 2024
Changes:

- docker-library/haproxy@79a32f7: Merge pull request docker-library/haproxy#216 from Darlelet/ca-certs
- docker-library/haproxy@71cd019: Resolving missing ca-certificates dependency that causes client ssl verify to fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants