Don't use TLS for socket connections#1581
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1581 +/- ##
==========================================
- Coverage 55.26% 55.24% -0.03%
==========================================
Files 289 289
Lines 19385 19394 +9
==========================================
+ Hits 10713 10714 +1
- Misses 7977 7986 +9
+ Partials 695 694 -1 |
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1581 +/- ##
==========================================
- Coverage 55.26% 55.24% -0.03%
==========================================
Files 289 289
Lines 19385 19394 +9
==========================================
+ Hits 10713 10714 +1
- Misses 7977 7986 +9
+ Partials 695 694 -1 |
|
|
||
| // IsSocket checks if the given address is a Unix-socket (linux) or named pipe (Windows) | ||
| func IsSocket(addr string) bool { | ||
| addrParts := strings.SplitN(addr, "://", 2) |
There was a problem hiding this comment.
Is this not a poor man's reimplementation of net.Parse()?
It also reimplements the head of parseDockerDaemonHost (immediately above) which also then switches on the scheme. Perhaps parseDockerDaemonHost function should return some sort of tls indicator, which is plumbed up through the getServerHost→ParseHost→parseDockerDaemonHost call chain (which seems fairly linear, so not too much collateral change required) for use in NewAPIClientFromFlags?
Perhaps that plumbing could take the form of returning the appropriate []clientOpts to be used?
There was a problem hiding this comment.
Is this not a poor man's reimplementation of
net.Parse()?
Yes, it's similar to what url.Parse() provides; not sure if we need all the functionality of url.Parse() for this though.
It also reimplements the head of
parseDockerDaemonHost(immediately above)
Yes, I was in doubt if I would extract it to a function (e.g. getScheme(host)), but that felt a bit over the top for two lines of code; happy to change though.
Perhaps that plumbing could take the form of returning the appropriate []clientOpts to be used?
Looking if we should integrate this into client.WithHost(host). i.e., if I set a unix, npipe, or fd host, then disable TLS. wdyt?
There was a problem hiding this comment.
I agree with client.WithHost(host) doing the right thing, whether that means doing it there directly or further down the callchain I have no string opinion on.
| } | ||
|
|
||
| // IsSocket checks if the given address is a Unix-socket (linux) or named pipe (Windows) | ||
| func IsSocket(addr string) bool { |
There was a problem hiding this comment.
Can we add a test on this function?
thaJeztah
force-pushed
the
dont_use_tls_for_sockets
branch
from
40de743 to
cd2ad94
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1581 +/- ##
==========================================
- Coverage 55.10% 55.09% -0.02%
==========================================
Files 359 359
Lines 30046 30056 +10
==========================================
Hits 16558 16558
- Misses 12542 12552 +10
Partials 946 946 🚀 New features to boost your workflow:
|
thaJeztah
force-pushed
the
dont_use_tls_for_sockets
branch
from
cd2ad94 to
417909d
Compare
thaJeztah
force-pushed
the
dont_use_tls_for_sockets
branch
2 times, most recently
from
4fc279a to
1f4131e
Compare
Before this patch:
mkdir -p ./tempconfig && touch ./tempconfig/ca.pem ./tempconfig/cert.pem ./tempconfig/key.pem
DOCKER_TLS_VERIFY=1 DOCKER_CONFIG=./tempconfig DOCKER_HOST=unix:///var/run/docker.sock docker info
Failed to initialize: failed to retrieve context tls info: ca.pem seems invalid
With this patch:
DOCKER_TLS_VERIFY=1 DOCKER_CONFIG=./tempconfig DOCKER_HOST=unix:///var/run/docker.sock docker info
Client:
Version: 28.1.1-25-g2dfe7b558.m
Context: default
...
Note that the above is just to illustrate; there's still parts in context-
related code that will check for, and load TLS-related files ahead of time.
We should make some of that code lazy-loading (i.e., don't load these until
we're actually gonna make an API connection). For example, if the TLS files
are missing;
rm ./tempconfig/*.pem
DOCKER_TLS_VERIFY=1 DOCKER_CONFIG=./tempconfig DOCKER_HOST=unix:///var/run/docker.sock docker info
Failed to initialize: unable to resolve docker endpoint: open tempconfig/ca.pem: no such file or directory
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
dont_use_tls_for_sockets
branch
from
1f4131e to
67f029e
Compare
7 participants
fixes moby/moby#36535
Before this patch:
With this patch:
Note that the above is just to illustrate; there's still parts in context-
related code that will check for, and load TLS-related files ahead of time.
We should make some of that code lazy-loading (i.e., don't load these until
we're actually gonna make an API connection). For example, if the TLS files
are missing;