feat: add Docker API TLS client-certificate support#1265
Merged
Conversation
buchdag
requested changes
Jun 17, 2026
There was a problem hiding this comment.
Pull request overview
Adds support for connecting acme-companion to a remote TLS-protected Docker daemon over tcp:// using Docker-style client certificates (DOCKER_TLS_VERIFY + DOCKER_CERT_PATH). This extends the existing docker_api transport handling (unix-socket vs TCP) and documents/tests the new behavior so users can run without mounting the Docker socket.
Changes:
- Add a TLS branch to
docker_apithat switches tohttps://and passes--cert/--key/--cacertwhenDOCKER_TLS_VERIFYis true. - Fail fast at startup when TLS is enabled but required cert files are missing/unreadable.
- Add docs + a new test suite (and CI registration) to validate the curl invocation and optionally perform a real TLS handshake via a socat sidecar.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/functions.sh |
Adds TLS client-certificate support to docker_api for TCP Docker hosts. |
app/entrypoint.sh |
Validates DOCKER_CERT_PATH and required PEM files when TLS is enabled. |
docs/Container-configuration.md |
Documents DOCKER_HOST, DOCKER_TLS_VERIFY, and DOCKER_CERT_PATH usage and mounting expectations. |
test/tests/docker_api_tls/run.sh |
New test verifying docker_api curl arguments and optional end-to-end TLS handshake. |
test/tests/docker_api_tls/expected-std-out.txt |
Expected output for the deterministic curl-stub portion of the new test. |
test/config.sh |
Registers the new docker_api_tls test in the global test list. |
.github/workflows/test.yml |
Adds docker_api_tls to the CI test matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1fa0c30 to
1459b5f
Compare
JamBalaya56562
added a commit
to JamBalaya56562/acme-companion
that referenced
this pull request
Jun 17, 2026
Single-commit tracking document of the open-issue triage. Reflects merged state as of 2026-06-18: PR nginx-proxy#1266 merged (closes nginx-proxy#918, acme.sh --log routed to /dev/stderr under DEBUG=1); PR nginx-proxy#1265 rebased onto main and mergeable (awaiting review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
|
@JamBalaya56562 looks like there is an issue with the test 🤔
|
1459b5f to
bf50913
Compare
Allow acme-companion to connect to a TLS-protected Docker daemon over tcp:// using client-certificate authentication, mirroring the Docker CLI and docker-gen convention (DOCKER_TLS_VERIFY + DOCKER_CERT_PATH with ca.pem / cert.pem / key.pem). - functions.sh: docker_api gains a mutually-exclusive TLS branch that adds --cert/--key/--cacert and switches the scheme to https://, gated by parse_true so only true/True/TRUE/1 enable it. - entrypoint.sh: check_docker_socket validates that DOCKER_CERT_PATH is set and the three certificate files are readable when TLS is enabled, failing fast with an actionable error instead of a silent curl failure. - docs: document DOCKER_HOST/DOCKER_TLS_VERIFY/DOCKER_CERT_PATH, stressing that the cert path is in-container and must be volume-mounted. - tests: new docker_api_tls test verifying the curl invocation per transport (curl stub, CI) plus an optional real-TLS handshake via a socat sidecar (RUN_TLS_INTEGRATION=1); registered in config.sh and the CI matrix. Revives and completes PR nginx-proxy#673 with the maintainer's requested changes (parse_true validation, documentation clarity, tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- test/docker_api_tls: remove the hidden RUN_TLS_INTEGRATION-gated optional integration test (maintainer request to avoid hidden optional tests); the deterministic curl-stub test already fully covers the docker_api change. - test/docker_api_tls: build the stub command string with `cat` instead of `read -d ''`, which returns success and stays robust if `set -e` is added. - docs: clarify that the Docker TLS port 2376 is conventional, not required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bf50913 to
c0a8b29
Compare
Contributor
Author
|
Thanks for the heads-up! Two things were going on:
Both should be sorted now. 🙏 |
buchdag
approved these changes
Jun 18, 2026
JamBalaya56562
added a commit
to JamBalaya56562/acme-companion
that referenced
this pull request
Jun 18, 2026
Single-commit tracking document of the open-issue triage. Reflects merged state as of 2026-06-18: PR nginx-proxy#1266 merged (closes nginx-proxy#918, acme.sh --log routed to /dev/stderr under DEBUG=1); PR nginx-proxy#1265 rebased onto main and mergeable (awaiting review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JamBalaya56562
added a commit
to JamBalaya56562/acme-companion
that referenced
this pull request
Jun 18, 2026
Single-commit tracking document of the open-issue triage. Reflects merged state as of 2026-06-18: PR nginx-proxy#1266 merged (closes nginx-proxy#918, acme.sh --log routed to /dev/stderr under DEBUG=1); PR nginx-proxy#1265 rebased onto main and mergeable (awaiting review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JamBalaya56562
added a commit
to JamBalaya56562/acme-companion
that referenced
this pull request
Jun 18, 2026
Single-commit tracking document of the open-issue triage. Reflects merged state as of 2026-06-18: PR nginx-proxy#1266 merged (closes nginx-proxy#918, acme.sh --log routed to /dev/stderr under DEBUG=1); PR nginx-proxy#1265 rebased onto main and mergeable (awaiting review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JamBalaya56562
added a commit
to JamBalaya56562/acme-companion
that referenced
this pull request
Jun 19, 2026
Single-commit tracking document of the open-issue triage. Reflects merged state as of 2026-06-18: PR nginx-proxy#1266 merged (closes nginx-proxy#918, acme.sh --log routed to /dev/stderr under DEBUG=1); PR nginx-proxy#1265 rebased onto main and mergeable (awaiting review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What
Add support for connecting to a TLS-protected Docker daemon over
tcp://using client-certificate authentication.This revives and completes #673, which has been open since 2020, addressing the maintainer's requested changes.
Why
When the Docker daemon is only reachable over a
tcp://endpoint secured with--tlsverify,docker_apipreviously always used plainhttp://, so every API call failed. acme-companion could not be used against a remote/secured Docker host.How
Mirrors the Docker CLI and docker-gen convention so a single certificate directory can be shared across containers:
DOCKER_TLS_VERIFY— set totrue(orTrue/TRUE/1) to enable TLS client auth.DOCKER_CERT_PATH— in-container directory containingca.pem,cert.pem,key.pem.Changes:
app/functions.sh—docker_apigains a mutually-exclusive TLS branch (after the unix-socket branch) that appends--cert/--key/--cacertand switches the scheme tohttps://. It is gated by the existingparse_truehelper, so onlytrue/True/TRUE/1enable it (addresses the review request to not accept any non-empty value).app/entrypoint.sh—check_docker_socketnow validates, when TLS is enabled, thatDOCKER_CERT_PATHis set and the three certificate files are readable, failing fast with an actionable error (with a-vmount example) instead of a silent curl failure.docs/Container-configuration.md— documents the variables and emphasises thatDOCKER_CERT_PATHis an in-container path that must be volume-mounted.test/tests/docker_api_tls/— new test that stubscurland asserts the exact invocationdocker_apibuilds for each transport (TLS GET/POST, TLS disabled, unix socket), verifying both the cert flags and theparse_truegating. It needs no external TLS daemon, so it is deterministic on CI runners. Registered intest/config.shand added to the CI matrix (.github/workflows/test.yml).Maintainer requests from #673 addressed
DOCKER_TLS_VERIFYvalidated via the project'strue/True/TRUE/1convention (parse_true).status/pr-needs-tests).Testing
docker_api_tlspasses under both2containersand3containers(the test is transport-focused and produces identical output regardless of the setup).docker_apitest passes under both setups (no regression).shellcheckclean on all modified scripts.Closes #754
Supersedes #673 (revives that proposal with the maintainer's requested changes).
🤖 Generated with Claude Code