Add warning if REST API is accessible through an insecure connection by thaJeztah · Pull Request #37684 · moby/moby

thaJeztah · 2018-08-21T13:54:19Z

The remote API allows full privilege escalation and is equivalent to having root access on the host. Because of this, the API should never be accessible through an insecure connection (TCP without TLS, or TCP without TLS verification).

Although a warning is already logged on startup if the daemon uses an insecure configuration, this warning is not very visible (unless someone decides to read the logs).

This patch attempts to make insecure configuration more visible by sending back warnings through the API (which will be printed when using docker info).

- Description for the changelog

+ Add warning if REST API is accessible through an insecure connection [moby#37684](https://github.com/moby/moby/pull/37684)

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2018-08-21T13:56:22Z

ping @vdemeester @cpuguy83 @justincormack

related to #37299 (/cc @cyphar)

vdemeester

LGTM 🐸

cpuguy83

Seems ok, but I don't like that it's parsing hosts for the info endpoint, especially since these don't change while the daemon is running.

Since we already have to parse the hosts can we store this info somewhere?

cpuguy83 · 2018-08-21T16:50:28Z

daemon/info.go

Nit, this should be a constant.

thaJeztah · 2018-08-21T17:54:21Z

Actually; may not even need the ParseHost(), because the startup code parses this, and writes it back to Config;

moby/cmd/dockerd/daemon.go

Lines 593 to 595 in cf72051

    
           if cli.Config.Hosts[i], err = dopts.ParseHost(cli.Config.TLS, cli.Config.Hosts[i]); err != nil { 
        
           	return nil, fmt.Errorf("error parsing -H %s : %v", cli.Config.Hosts[i], err) 
        
           }

codecov · 2018-08-21T18:07:38Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2629fe9). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37684   +/-   ##
=========================================
  Coverage          ?   36.01%           
=========================================
  Files             ?      609           
  Lines             ?    45060           
  Branches          ?        0           
=========================================
  Hits              ?    16229           
  Misses            ?    26605           
  Partials          ?     2226

thaJeztah · 2018-08-21T18:08:13Z

@cpuguy83 updated; PTAL

cpuguy83 · 2018-08-21T19:27:37Z

daemon/info.go

s/without TLS protection/unencrypted/

cpuguy83 · 2018-08-21T19:28:25Z

daemon/info.go

TLS client verification

cyphar · 2018-08-21T19:50:10Z

LGTM, after @cpuguy83's comments are addressed.

cpuguy83 · 2018-08-21T19:51:52Z

daemon/info.go

Probably fine for this PR, but it would be nice to eliminate this by being able to do something like host.Protocol()

Yes; tried to keep this PR minimal, but I'm thinking of exposing some of this information (e.g., the configured hosts) in docker info as well. I can do some refactoring as part of that 👍

The remote API allows full privilege escalation and is equivalent to having root access on the host. Because of this, the API should never be accessible through an insecure connection (TCP without TLS, or TCP without TLS verification). Although a warning is already logged on startup if the daemon uses an insecure configuration, this warning is not very visible (unless someone decides to read the logs). This patch attempts to make insecure configuration more visible by sending back warnings through the API (which will be printed when using `docker info`). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2018-08-21T20:10:43Z

Thanks for reviewing @cyphar 🤗

cpuguy83

LGTM

andrewhsu · 2018-08-21T23:52:10Z

Please ignore powerpc build results. The IBM Power infrastructure is experiencing outage at the moment.

tiborvass

LGTM

thaJeztah requested a review from vdemeester as a code owner August 21, 2018 13:54

GordonTheTurtle added the status/0-triage label Aug 21, 2018

thaJeztah added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/security and removed status/0-triage labels Aug 21, 2018

vdemeester approved these changes Aug 21, 2018

View reviewed changes

cpuguy83 requested changes Aug 21, 2018

View reviewed changes

daemon/info.go Outdated

Copy link

Member

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, this should be a constant.

vdemeester reacted with thumbs up emoji

thaJeztah force-pushed the add_remote_api_warning branch from 8d25be5 to ab05c66 Compare August 21, 2018 18:07

thaJeztah force-pushed the add_remote_api_warning branch from ab05c66 to 34441c2 Compare August 21, 2018 18:10

cpuguy83 reviewed Aug 21, 2018

View reviewed changes

daemon/info.go Outdated

Copy link

Member

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/without TLS protection/unencrypted/

cpuguy83 reviewed Aug 21, 2018

View reviewed changes

daemon/info.go Outdated

Copy link

Member

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLS client verification

cpuguy83 reviewed Aug 21, 2018

View reviewed changes

thaJeztah force-pushed the add_remote_api_warning branch from 34441c2 to 547b993 Compare August 21, 2018 20:03

cpuguy83 approved these changes Aug 21, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Aug 21, 2018

tiborvass approved these changes Aug 21, 2018

View reviewed changes

tiborvass merged commit 1cc3dee into moby:master Aug 21, 2018

thaJeztah deleted the add_remote_api_warning branch August 21, 2018 23:52

AkihiroSuda mentioned this pull request Sep 18, 2018

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

Closed

thaJeztah mentioned this pull request Oct 31, 2018

Fix double "unix://" scheme in TestInfoAPIWarnings #38115

Merged

thaJeztah mentioned this pull request Oct 31, 2018

[18.09 backport] Fix double "unix://" scheme in TestInfoAPIWarnings docker-archive/engine#103

Merged

olljanat mentioned this pull request Jan 19, 2019

edit deamon.json added "hosts": ["tcp://0.0.0.0:2375","unix:///var/run/docker.sock"],then start failed #38495

Closed

Conversation

thaJeztah commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 21, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 21, 2018

Uh oh!

codecov bot commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Aug 21, 2018

Uh oh!

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

cyphar commented Aug 21, 2018

Uh oh!

cpuguy83 Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 21, 2018

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

andrewhsu commented Aug 21, 2018

Uh oh!

tiborvass left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thaJeztah commented Aug 21, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading