feat: add TLS certificate support for Docker contexts#3728
Conversation
|
Hi @Itx-Psycho0. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
knative-prow
Bot
added
size/L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3728 +/- ##
==========================================
+ Coverage 56.95% 57.34% +0.39%
==========================================
Files 181 181
Lines 21116 21225 +109
==========================================
+ Hits 12026 12172 +146
+ Misses 7866 7823 -43
- Partials 1224 1230 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Extends Docker context detection (from knative#3684) to support TLS certificates stored in Docker contexts. This enables secure connections to remote Docker daemons configured via Docker contexts. Changes: - Extended getDockerContextHost() to getDockerContextConfig() which returns both host and TLS configuration - Added DockerContextConfig struct to hold host and TLS settings - Modified newHttpClient() to check Docker context first, then fall back to environment variables - Added newHttpClientFromContext() to create HTTP client from context config - Load TLS certificates directly from context (no temp files or env vars) - Added comprehensive test with mock TLS daemon Benefits: - Remote Docker setups work automatically with TLS - Consistent with Docker CLI behavior - No manual environment variables needed - Proper TLS support for secure connections - Clean implementation without temp files Fixes knative#3719
Itx-Psycho0
force-pushed
the
feat/docker-context-tls
branch
from
5ecc50a to
1af6e75
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/ok-to-test |
knative-prow
Bot
added
ok-to-test
Review: feat: add TLS certificate support for Docker contextsBranch: OverallThe approach of building the TLS config in-memory via Significant1. Wrong fallback TLS path
// Docker stores context TLS files in contexts/meta/<sha256-hash>/
hash := sha256.Sum256([]byte(contexts[0].Name))
tlsPath = filepath.Join(dockerConfigDir, "contexts", "meta", fmt.Sprintf("%x", hash))Docker stores TLS files under The test masks this because it always sets 2. Precedence inversion: context TLS overrides explicit env vars
func newHttpClient() *http.Client {
// First, try to get TLS config from Docker context
if contextConfig := getDockerContextConfig(); contextConfig != nil && len(contextConfig.TLSCert) > 0 && len(contextConfig.TLSKey) > 0 {
return newHttpClientFromContext(contextConfig)
}
// Fall back to environment variables
tlsVerifyStr, tlsVerifyChanged := os.LookupEnv("DOCKER_TLS_VERIFY")
...Context is checked before 3. Context TLS applied even when
|
|
Thanks for the detailed review @matejvasek! I see the issues, let me fix them:
Working on the fixes now! |
Fixes based on @matejvasek's review: Significant fixes: 1. Fixed TLS path: use contexts/tls/<hash>/ not contexts/meta/<hash>/ 2. Fixed precedence: env vars (DOCKER_TLS_VERIFY) now override context 3. Context TLS only applies when host came from context detection 4. Cache context config to avoid calling 'docker context inspect' twice Minor improvements: 5. Unexported dockerContextConfig (internal-only struct) 6. Removed redundant DOCKER_CONFIG passthrough (auto-inherited) 7. Added error logging for malformed certificates The implementation now correctly: - Checks DOCKER_TLS_VERIFY env var first - Only uses context TLS when env vars are not set - Only applies context TLS when host came from context - Calls docker CLI once instead of twice - Logs warnings for cert loading failures
Review: feat: add TLS certificate support for Docker contextsBranch: OverallGood iteration. The core approach — building TLS config in-memory via What was fixed
Remaining issues1.
|
|
Also please add test for the previous TLS functionality using the envvars too. |
Address remaining code review feedback from matejvasek:
1. Remove dead code:
- Removed GetDockerContextHostFunc variable
- Removed getDockerContextHost() wrapper function
These are no longer called anywhere since getDockerContextConfig()
is now called directly.
2. Add test for fallback TLS path:
- Added TestNewClient_DockerContextTLS_FallbackPath
- Tests the scenario where storage.TLSPath is "<IN MEMORY>" or empty
- Verifies that TLS certificates are still found via the calculated
path based on context name hash (contexts/tls/<sha256-hash>/)
- This exercises the fallback logic that was previously untested
All tests pass including the new fallback path test.
knative-prow
Bot
added
size/XL
|
Please add also test for the "old" functionality: TLS without context via the envvars. |
Add TestNewClient_TLS_EnvVars to test the original TLS functionality using environment variables (DOCKER_TLS_VERIFY, DOCKER_CERT_PATH) without Docker context. This ensures backward compatibility with the pre-context TLS configuration method and addresses Matej's feedback to test the 'old' functionality. The test: - Creates a TLS-enabled mock Docker daemon - Sets up TLS certificates in a directory - Configures TLS via environment variables (not context) - Verifies successful TLS connection and client cert authentication All tests pass including the new env vars test.
matejvasek
left a comment
matejvasek
left a comment
There was a problem hiding this comment.
Review
Issues
1. Bug: TLS cert path missing the endpoint subdirectory
Docker stores context TLS files at contexts/tls/<hash>/docker/{ca,cert,key}.pem (where docker is the endpoint name), not directly at contexts/tls/<hash>/. The Docker CLI source (store.go ResetEndpointTLSMaterial) writes to:
filepath.Join(s.tls, contextDirOf(name), endpointName, fileName)
And docker context inspect's Storage.TLSPath returns the context-level directory (contexts/tls/<hash>/), not the endpoint-level one.
The PR reads from:
os.ReadFile(filepath.Join(tlsPath, "ca.pem")) // contexts/tls/<hash>/ca.pemBut should read from:
os.ReadFile(filepath.Join(tlsPath, "docker", "ca.pem")) // contexts/tls/<hash>/docker/ca.pemThis applies to both the Storage.TLSPath path and the fallback calculated path. The tests pass because they create certs at the wrong level to match the code, not where Docker actually stores them. On a real Docker installation with TLS-enabled contexts, the certs would not be found.
2. Mock daemon doesn't serve the Info endpoint
startMockDaemon returns "OK" (text/plain) for all requests. The TLS tests call dockerClient.Info() and assert nfo.Info.ID == "mock-daemon":
nfo, err := dockerClient.Info(ctx, client.InfoOptions{})
if nfo.Info.ID != "mock-daemon" {
t.Errorf(...)
}The Docker client should fail to JSON-decode "OK" into a system.Info struct, causing Fatalf on the error check. If it somehow succeeds, the ID would be "", not "mock-daemon", so the Errorf would fire. Either the mock needs to serve proper JSON for /info, or the Info() assertions should be removed.
3. Context TLS tests don't isolate from DOCKER_TLS_VERIFY
TestNewClient_DockerContextTLS and the fallback variant set DOCKER_HOST="" and DOCKER_CONFIG but don't unset DOCKER_TLS_VERIFY. If the test environment has DOCKER_TLS_VERIFY set, newHttpClient() takes the env-var code path instead of the context path, and the test doesn't exercise what it claims. These tests should explicitly unset DOCKER_TLS_VERIFY and DOCKER_CERT_PATH.
4. Test meta.json includes Storage field that Docker doesn't persist
createDockerContextConfigWithTLS writes a Storage block into meta.json. Docker CLI doesn't store Storage in meta.json -- it computes it at runtime from the store directory structure. The real docker context inspect will ignore the Storage in meta.json and compute its own value. This means the test's TLSPath value may or may not match what docker context inspect actually returns, making the test fragile.
Minor / Style
- Code duplication --
newHttpClient(env var path) andnewHttpClientFromContextduplicate the dialer + transport +http.Clientconstruction. A small shared helper for the bottom half would reduce this. - Silent cert read failures --
getDockerContextConfigsilently returns empty cert slices whenos.ReadFilefails for individual cert files. A debug/trace log would help users troubleshoot TLS issues (the PR does logX509KeyPairfailures, which is good, but not file-read failures).
Positive
- Caching
contextConfigto avoid callingdocker context inspecttwice is a good improvement. - Proper precedence: env vars override context config.
- Clean separation of
newHttpClientFromContextfrom the env-var path. - Good test structure with TLS mock daemon (CA, server cert, client cert).
- Removing the exported
GetDockerContextHostFuncmock variable in favor ofDOCKER_CONFIG-based test isolation is the right approach. - The fallback path calculation for
<IN MEMORY>TLS paths is a nice touch.
There was a problem hiding this comment.
Review: PR #3728 — feat: add TLS certificate support for Docker contexts
Summary
This PR extends the Docker context detection (from #3684) to also load TLS certificates stored in Docker contexts. It renames getDockerContextHost() to getDockerContextConfig(), returning a dockerContextConfig struct that carries both the host and TLS cert/key/CA bytes. The newHttpClient() function is updated to accept this config and fall back to context-based TLS when env vars aren't set.
Issues
1. TLS cert path lookup assumes flat directory — Docker uses a subdirectory per endpoint
At docker_client.go:450-453, the fallback path is computed as:
hash := sha256.Sum256([]byte(contexts[0].Name))
tlsPath = filepath.Join(dockerConfigDir, "contexts", "tls", fmt.Sprintf("%x", hash))But Docker actually stores TLS files under contexts/tls/<hash>/docker/ (one subdirectory per endpoint name). The certs won't be found at the computed path on a real Docker installation. The tests pass only because they write certs directly into contexts/tls/<hash>/ — mimicking the bug rather than real Docker layout.
2. contextConfig is passed to newHttpClient even when DOCKER_HOST is set via env
At line 184, newHttpClient(contextConfig) is called whenever the scheme is TCP. But contextConfig is only populated when the default socket doesn't exist (the os.IsNotExist branch). If DOCKER_HOST is set as an env var (line 94), contextConfig will be nil, so this works correctly — but only by coincidence. The code would be clearer if the call was newHttpClient(nil) when DOCKER_HOST came from the environment, or if the logic was restructured.
3. Missing DOCKER_TLS_VERIFY env var cleanup in context TLS tests
TestNewClient_DockerContextTLS and TestNewClient_DockerContextTLS_FallbackPath set DOCKER_HOST and DOCKER_CONFIG via t.Setenv, but they don't explicitly clear DOCKER_TLS_VERIFY. If DOCKER_TLS_VERIFY happens to be set in the test runner's environment, the env-var path in newHttpClient would take precedence over context TLS, and the test would pass for the wrong reason. Adding t.Setenv("DOCKER_TLS_VERIFY", "") would make the tests more robust.
4. os.Getenv("HOME") won't work on all platforms
At docker_client.go:446:
dockerConfigDir = filepath.Join(os.Getenv("HOME"), ".docker")On Windows, HOME is typically not set (USERPROFILE is used instead). The test already skips Windows, but the production code doesn't. Consider using os.UserHomeDir() instead, which is cross-platform.
5. CodeQL warning about InsecureSkipVerify
The two CodeQL alerts on InsecureSkipVerify are false positives in the context of this PR — the InsecureSkipVerify in the env-var path was already present before this PR, and in the context path it's only set when SkipTLSVerify is explicitly configured. However, a //nolint or #nosec annotation with a justification comment would silence the scanner and document the intent.
Minor Nits
- The
"crypto/sha256"and"fmt"imports were added to the production code solely for the TLS path fallback logic. If issue #1 above is fixed to read theTLSPathfromdocker context inspectoutput correctly, thesha256import may become unnecessary. - The comment at line 399 (
// Note: DOCKER_CONFIG is automatically inherited from parent environment) is obvious and could be removed. newHttpClientFromContextlogs to stderr on cert parse failure (fmt.Fprintf(os.Stderr, ...)). The rest of the codebase doesn't use this pattern — consider using a proper logger or just returning nil.
What's Good
- Clean precedence model: env vars override context config.
- The
dockerContextConfigstruct is a reasonable abstraction for carrying context state. - The test coverage is thorough: context TLS, fallback path, and env-var backward compatibility are all covered with real mTLS connections to mock daemons.
- The caching of
contextConfigto avoid callingdocker context inspecttwice is a good optimization.
Verdict
The core idea is sound, but the TLS path lookup (issue #1) appears to be incorrect for real Docker installations, which would make this feature silently not work outside of the tests. That should be verified and fixed before merging.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Itx-Psycho0 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
Description
Extends Docker context detection (from #3684) to support TLS certificates stored in Docker contexts. This enables secure connections to remote Docker daemons configured via Docker contexts.
Fixes #3719
Changes
Extended
getDockerContextHost()togetDockerContextConfig()which returns both host and TLS configurationAdded
DockerContextConfigstruct to hold host and TLS settingsLoad TLS certificates from context's
tls/directoryWrite certificates to temp directory and configure via environment variables
Reuse existing TLS functionality via
newHttpClient()Added comprehensive test with mock TLS daemon
Benefits
Remote Docker setups work automatically with TLS
Consistent with Docker CLI behavior - if
dockercommands work,funccommands workNo manual environment variables needed
Proper TLS support for secure connections
Testing
Added
TestNewClient_DockerContextTLSwith mock TLS-enabled daemonAll existing Docker tests pass
make checkpassesRelated
PR fix: detect Docker host from context when DOCKER_HOST is not set #3684 - Initial Docker context detection implementation (merged)