-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add warning if REST API is accessible through an insecure connection #37684
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
daemon/info.go
Outdated
func (daemon *Daemon) fillAPIInfo(v *types.Info) { | ||
cnf := daemon.configStore | ||
|
||
warn := ` |
There was a problem hiding this comment.
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.
Actually; may not even need the Lines 593 to 595 in cf72051
|
8d25be5
to
ab05c66
Compare
Codecov Report
@@ Coverage Diff @@
## master #37684 +/- ##
=========================================
Coverage ? 36.01%
=========================================
Files ? 609
Lines ? 45060
Branches ? 0
=========================================
Hits ? 16229
Misses ? 26605
Partials ? 2226 |
@cpuguy83 updated; PTAL |
ab05c66
to
34441c2
Compare
daemon/info.go
Outdated
continue | ||
} | ||
if !cfg.TLS { | ||
v.Warnings = append(v.Warnings, fmt.Sprintf("WARNING: API is accessible on http://%s without TLS protection.%s", addr, warn)) |
There was a problem hiding this comment.
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/
daemon/info.go
Outdated
continue | ||
} | ||
if !cfg.TLSVerify { | ||
v.Warnings = append(v.Warnings, fmt.Sprintf("WARNING: API is accessible on https://%s without TLS verification.%s", addr, warn)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLS client verification
LGTM, after @cpuguy83's comments are addressed. |
cfg := daemon.configStore | ||
for _, host := range cfg.Hosts { | ||
// cnf.Hosts is normalized during startup, so should always have a scheme/proto | ||
h := strings.SplitN(host, "://", 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably fine for this PR, but it would be nice to eliminate this by being able to do something like host.Protocol()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
34441c2
to
547b993
Compare
Thanks for reviewing @cyphar 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please ignore powerpc build results. The IBM Power infrastructure is experiencing outage at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
- A picture of a cute animal (not mandatory but encouraged)