Sterner warnings and deprecation notice for unauthenticated tcp access #41285

cpuguy83 · 2020-07-28T23:42:15Z

People keep doing this and getting pwned because they accidentally left
it exposed to the internet.

The warning about doing this has been there forever.
This introduces a sleep after warning.
To disable the extra sleep users must explicitly specify --tls=false
or --tlsverify=false

Warning also specifies this sleep will be removed in the next release
where the flag will be required if running unauthenticated.

AkihiroSuda · 2020-07-29T04:21:59Z

Needs deprecation period?

AkihiroSuda · 2020-07-29T05:01:21Z

Past discussion: #37299 #36357

thaJeztah

I agree this is bad, but we need to look at how to deal with this (see my comment)

thaJeztah · 2020-07-29T07:39:16Z

integration/container/daemon_test.go

+ err := d.StartWithError("--host", "tcp://127.0.0.1:0")
+ assert.Assert(t, err != nil)


I've especially seen the "localhost" / 127.0.0.1 case being in use quite frequently (on Desktop). Should we follow the same rules as for "insecure registries", and

have an exception for 127.0.0.0/8

add a flag to allow the range to be customised?

I know all are bad, and insecure (containers can access, depending on their network settings, so can compromise the host) but also concerned that there's many "valid" (between big brackets) use-cases

@justincormack @tonistiigi @tiborvass any suggestions?

Should we follow the same rules as for "insecure registries",

Exposing the API socket to other processes is more critical than "insecure registries".
I don't think the same rule should apply here.

on Desktop

Do you mean Docker for Mac/Win? Or Linux desktop?

Exposing the API socket to other processes is more critical than "insecure registries". I don't think the same rule should apply here.

Yeah, so perhaps some explicit "I know what I'm doing" flag (as proposed in #37299)

Do you mean Docker for Mac/Win? Or Linux desktop?

At least Docker Desktop for Windows (some of that may be because (IIRC) in the past it didn't support named pipes, and defaulted to enabling the API on tcp as well)

Maybe we can keep localhost (what about ipv6?) for now and phase it out later?

Another approach, we could generate TLS certs at a particular location for localhost and have the docker client automatically look for these certs.

On Linux, allowing localhost doesn't seem meaningful. If we are not sure about Windows, we can keep localhost on Windows for now.

Another approach, we could generate TLS certs at a particular location for localhost and have the docker client automatically look for these certs.

Is it just like docker:dind image does? https://github.com/docker-library/docker/blob/master/19.03/dind/dockerd-entrypoint.sh#L21-L90

tao12345666333 · 2020-07-30T02:04:00Z

cmd/dockerd/daemon.go

@@ -599,7 +599,7 @@ func loadListeners(cli *DaemonCli, serverConfig *apiserver.Config) ([]string, er

 // It's a bad idea to bind to TCP without tlsverify.
 if proto == "tcp" && (serverConfig.TLSConfig == nil || serverConfig.TLSConfig.ClientAuth != tls.RequireAndVerifyClientCert) {
- logrus.Warn("[!] DON'T BIND ON ANY IP ADDRESS WITHOUT setting --tlsverify IF YOU DON'T KNOW WHAT YOU'RE DOING [!]")
+ return nil, errors.New("Binding to IP address without --tlsverify is no longer supported")


I think we should have a deprecation process.

Disabling directly will cause some users to be unable to use it normally after the upgrade. (maybe they are not ready yet. of course, it is possible that most of these users use the default configuration unix domain socket)

And we have not notified when it will be officially disabled IIRC.

tonistiigi · 2020-07-30T03:18:20Z

Plenty of valid reasons to do run raw tcp, even on 0.0.0.0. Eg. if you are on a VM, container, local isolated compute cluster etc. I'm in favor of adding --allow-insecure-connections (or less verbose). Not setting that flag will show the deprecation warning and after deprecation period is over start failing on startup.

cpuguy83 · 2020-07-31T20:17:34Z

I don't see the point of deprecation for this.
People are already ignoring the warning plus we have a stable release in 19.03 that people can fall back to.

I'm fine with adding a flag to allow this case through. The more verbose the better 😄 --yes-i-really-want-to-give-root-access-to-anyone-on-the-network 😂

tianon · 2020-07-31T20:51:15Z

I agree with the sentiments already shared -- I use this in quite a few environments where it's not the best solution, but the security tradeoff is acceptable, so I'd definitely be sad if it were completely gone, but I'm perfectly happy with an explicit flag to enable it (even if said flag is really verbose like the proposed --yes-i-really-want-to-give-root-access-to-anyone-on-the-network 😄).

(I definitely don't want to see something like --privileged where the "let me be insecure" flag is very short and doesn't really tell the user much about what they're opening up.)

jglick · 2020-08-01T13:40:01Z

IIUC this would block image updates for idioms like jenkinsci/kubernetes-plugin#581. Could probably be adapted to use a Unix-domain socket via volume mount.

olljanat · 2020-08-30T22:54:53Z

IMO, best option would be automatically generated certificates and enabled TLS verify for TCP connections. Most people are too lazy to study how to generate certificates but most probably they are able copy certificate files to client if there instructions about it.

And for those who really don't care about security there can be that super long and desribe switch available.

cpuguy83 · 2020-09-15T22:40:04Z

I have updated this as follows:

Add flag --allow-unauthenticated-machine-root-access-to-network which needs to be specified for each -H that is unauthenticated tcp.

Without this flag the daemon will sleep 30s for each unauthenticated -H.

I have special cased localhost (both v4 and v6 should be handled) to not sleep in this case... not sure if that's the best decision but 🤷‍♂️.

Warnings are a bit more stern, including claiming that the flag will be required in the next release instead of having a sleep.

cpuguy83 · 2020-09-16T21:37:11Z

Looks like 2 tests need an update.
Will push this soon.

cpuguy83 · 2020-09-17T00:04:02Z

Updated to fix tests.
Had to add a special case for host addresses specified as a hostname (e.g. "localhost") since -H supports this and it is trying to skip the sleeps for loopback addresses.

thaJeztah · 2020-09-17T18:36:18Z

Discussing with @tonistiigi @tiborvass @cpuguy83

for this PR:
- don't print warning if user has set --tls=false and/or --tlsverify=false
- update messages to mention that TLS will be enable by default in a future release
future release; enable both --tls and --tlsverify by default: if a user didn't specify options, the daemon will fail to start as no certs are provided
add options to explicitly disable tls / tlfverify: --disable-tls, --disable-tls-verify (name can be discussed)

We'll need to update https://github.com/docker/cli/blob/master/docs/deprecated.md to mention "deprecation" of unprotected TCP access.

cpuguy83 · 2020-09-18T20:25:11Z

Updated to have this work with --tls=false or --tlsverify=false

thaJeztah

just some quick comments/questions (didn't do a full review on my computer)

thaJeztah · 2020-09-19T10:55:27Z

cmd/dockerd/daemon.go

+ }
+
+ if conf.TLSVerify == nil && conf.TLS != nil {
+ conf.TLSVerify = conf.TLS


This is a change in behavior, correct? (automatically enable verify)

If so, I'm ok with that change, but we'll probably need to mention that explicitly in changelogs

Yes, this is what we discussed on the PR review call.

cmd/dockerd/options.go

cmd/dockerd/daemon.go

thaJeztah · 2020-09-24T11:35:10Z

@cpuguy83 two tests are failing;

=== Failed
=== FAIL: cmd/dockerd TestLoadDaemonCliConfigWithTLSVerify (0.00s)
    daemon_test.go:115: assertion failed: 0x40009cd540 (loadedConfig.TLS *bool) != true (true bool)

=== FAIL: cmd/dockerd TestLoadDaemonCliConfigWithExplicitTLSVerifyFalse (0.00s)
    daemon_test.go:128: invalid Comparison: 0x4000a02c80 (*bool)

thaJeztah

LGTM if green 👍

thaJeztah · 2020-09-24T19:56:54Z

Arf.. one more test failure @cpuguy83

=== RUN   TestAuthZPluginTLS
--- FAIL: TestAuthZPluginTLS (1.67s)
    authz_plugin_test.go:132: assertion failed: error is not nil: error during connect: Get https://localhost:4271/v1.41/version: http: server gave HTTP response to HTTPS client

cpuguy83 · 2020-09-24T23:37:19Z

~~🤔 Doesn't fail locally.~~

Wrong branch.

People keep doing this and getting pwned because they accidentally left it exposed to the internet. The warning about doing this has been there forever. This introduces a sleep after warning. To disable the extra sleep users must explicitly specify `--tls=false` or `--tlsverify=false` Warning also specifies this sleep will be removed in the next release where the flag will be required if running unauthenticated. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

thaJeztah

LGTM

cpuguy83 requested review from AkihiroSuda, thaJeztah, tiborvass and tonistiigi July 28, 2020 23:42

thaJeztah requested changes Jul 29, 2020

View reviewed changes

thaJeztah added area/api area/daemon area/security impact/changelog impact/deprecation status/1-design-review labels Jul 29, 2020

tao12345666333 reviewed Jul 30, 2020

View reviewed changes

thaJeztah added this to the 20.03.0 milestone Aug 13, 2020

olljanat mentioned this pull request Aug 30, 2020

Adding a DinD example jenkinsci/kubernetes-plugin#581

Merged

cpuguy83 force-pushed the no_more_pwns branch from f2f7ac4 to 4e7bd20 Compare September 15, 2020 22:34

cpuguy83 force-pushed the no_more_pwns branch from 4e7bd20 to 9f30f25 Compare September 15, 2020 23:17

cpuguy83 force-pushed the no_more_pwns branch 2 times, most recently from 78dfdad to 688181d Compare September 17, 2020 00:03

cpuguy83 force-pushed the no_more_pwns branch from 688181d to 10f4843 Compare September 17, 2020 00:04

cpuguy83 force-pushed the no_more_pwns branch 2 times, most recently from 7cafe7d to 0790a51 Compare September 18, 2020 20:15

cpuguy83 requested a review from thaJeztah September 18, 2020 20:25

cpuguy83 force-pushed the no_more_pwns branch from 0790a51 to e532ae8 Compare September 18, 2020 20:26

cpuguy83 changed the title ~~Do not listen on TCP address if w/o TLS verify~~ Sterner warnings and deprecation notice for unauthenticated tcp access Sep 18, 2020

cpuguy83 force-pushed the no_more_pwns branch from e532ae8 to 5a6e731 Compare September 18, 2020 23:26

thaJeztah requested changes Sep 19, 2020

View reviewed changes

cpuguy83 force-pushed the no_more_pwns branch 2 times, most recently from 60b0aa2 to 9d9dc1c Compare September 24, 2020 18:33

thaJeztah approved these changes Sep 24, 2020

View reviewed changes

tiborvass approved these changes Sep 24, 2020

View reviewed changes

thaJeztah added status/4-merge and removed status/1-design-review labels Sep 24, 2020

cpuguy83 force-pushed the no_more_pwns branch from 9d9dc1c to 5f5285a Compare September 25, 2020 00:22

thaJeztah approved these changes Sep 25, 2020

View reviewed changes

thaJeztah merged commit ccc1233 into moby:master Sep 25, 2020

cpuguy83 deleted the no_more_pwns branch September 25, 2020 14:39

cpuguy83 mentioned this pull request Sep 25, 2020

dockerd: make -H tcp://... even harder without --tlsverify #37299

Closed

thaJeztah mentioned this pull request Sep 30, 2020

Add daemon or engine subcommand docker/cli#2718

Open

corhere mentioned this pull request Jul 20, 2023

Deprecate Docker API over TCP without TLS #36357

Open

thaJeztah mentioned this pull request Mar 8, 2024

docs: deprecate TCP connection without TLS docker/cli#4928

Merged

vvoland mentioned this pull request Mar 13, 2024

Be more explicit about non-TLS TCP access deprecation #47556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sterner warnings and deprecation notice for unauthenticated tcp access #41285

Sterner warnings and deprecation notice for unauthenticated tcp access #41285

cpuguy83 commented Jul 28, 2020 •

edited

Loading

AkihiroSuda commented Jul 29, 2020

AkihiroSuda commented Jul 29, 2020

thaJeztah left a comment

thaJeztah Jul 29, 2020

AkihiroSuda Jul 29, 2020

AkihiroSuda Jul 29, 2020

thaJeztah Jul 29, 2020

cpuguy83 Jul 29, 2020

AkihiroSuda Jul 29, 2020

tao12345666333 Jul 30, 2020

tao12345666333 Jul 30, 2020

tonistiigi commented Jul 30, 2020

cpuguy83 commented Jul 31, 2020

tianon commented Jul 31, 2020

jglick commented Aug 1, 2020

olljanat commented Aug 30, 2020

cpuguy83 commented Sep 15, 2020

cpuguy83 commented Sep 16, 2020

cpuguy83 commented Sep 17, 2020

thaJeztah commented Sep 17, 2020

cpuguy83 commented Sep 18, 2020

thaJeztah left a comment

thaJeztah Sep 19, 2020

cpuguy83 Sep 20, 2020

thaJeztah commented Sep 24, 2020

thaJeztah left a comment

thaJeztah commented Sep 24, 2020

cpuguy83 commented Sep 24, 2020 •

edited

Loading

thaJeztah left a comment

		err := d.StartWithError("--host", "tcp://127.0.0.1:0")
		assert.Assert(t, err != nil)

Sterner warnings and deprecation notice for unauthenticated tcp access #41285

Sterner warnings and deprecation notice for unauthenticated tcp access #41285

Conversation

cpuguy83 commented Jul 28, 2020 • edited Loading

AkihiroSuda commented Jul 29, 2020

AkihiroSuda commented Jul 29, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Jul 30, 2020

cpuguy83 commented Jul 31, 2020

tianon commented Jul 31, 2020

jglick commented Aug 1, 2020

olljanat commented Aug 30, 2020

cpuguy83 commented Sep 15, 2020

cpuguy83 commented Sep 16, 2020

cpuguy83 commented Sep 17, 2020

thaJeztah commented Sep 17, 2020

cpuguy83 commented Sep 18, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Sep 24, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Sep 24, 2020

cpuguy83 commented Sep 24, 2020 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

cpuguy83 commented Jul 28, 2020 •

edited

Loading

cpuguy83 commented Sep 24, 2020 •

edited

Loading