Skip to content
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

Loosen permissions on /etc/docker directory #37847

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 14, 2018

fixes #37840
fixes docker/cli#1358
closes #37619

The /etc/docker directory is used both by the dockerd daemon and the docker cli (if installed on the same host as the daemon).

In situations where the /etc/docker directory does not exist, and an initial key.json (legacy trust key) is generated (at the default location), the /etc/docker/ directory was created with 0700 permissions, making the directory only accessible by root.

Given that the 0600 permissions on the key itself already protect it from being used by other users, the permissions of /etc/docker can be less restrictive.

This patch changes the permissions for the directory to 0755, so that the CLI (if executed as non-root) can also access this directory.

NOTE: "strictly", this patch is only needed for situations where no custom
location for the trustkey is specified (not overridden with --deprecated-key-path),
but setting the permissions only for the "default" case would make
this more complicated.

To verify this patch;

make binary shell

# inside the container;
make install

# verify the directory doesn't exist
ls -la /etc/ | grep docker

# start, and terminate the `dockerd` daemon
dockerd
^C

# check that the directory was created with the correct permissions
ls -la /etc/ | grep docker
drwxr-xr-x 2 root root    4096 Sep 14 12:11 docker

The `/etc/docker` directory is used both by the dockerd daemon
and the docker cli (if installed on the saem host as the daemon).

In situations where the `/etc/docker` directory does not exist,
and an initial `key.json` (legacy trust key) is generated (at the
default location), the `/etc/docker/` directory was created with
0700 permissions, making the directory only accessible by `root`.

Given that the `0600` permissions on the key itself already protect
it from being used by other users, the permissions of `/etc/docker`
can be less restrictive.

This patch changes the permissions for the directory to `0755`, so
that the CLI (if executed as non-root) can also access this directory.

> **NOTE**: "strictly", this patch is only needed for situations where no _custom_
> location for the trustkey is specified (not overridden with `--deprecated-key-path`),
> but setting the permissions only for the "default" case would make
> this more complicated.

```bash
make binary shell

make install

ls -la /etc/ | grep docker

dockerd
^C

ls -la /etc/ | grep docker
drwxr-xr-x 2 root root    4096 Sep 14 12:11 docker
```

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

ping @ijc @clnperez @dmcgowan PTAL

@ijc
Copy link
Contributor

ijc commented Sep 14, 2018

Thanks, I don't think this strictly deals with docker/cli#1358 since the CLI should probably be robust to permissions failures anyway.

Is the case in daemon/cluster/cluster.go:New which I found not relevant to fix as well, or did I misinterpret what config.Root was at this point (I had concluded it was /etc/docker).

@thaJeztah
Copy link
Member Author

thaJeztah commented Sep 14, 2018

Yeah, it's a bit confusing; config.Root is --data-root on the daemon (default /var/lib/docker). The key.json is stored inside the daemon "config dir":

moby/cmd/dockerd/daemon.go

Lines 400 to 404 in 9d276b8

if conf.TrustKeyPath == "" {
conf.TrustKeyPath = filepath.Join(
getDaemonConfDir(conf.Root),
defaultTrustKeyFile)
}

which on Windows is a subdirectory of data-root;

func getDaemonConfDir(root string) string {
return filepath.Join(root, `\config`)
}

Whereas on Linux, this is hard-coded to /etc/docker (for the default);

func getDaemonConfDir(_ string) string {
return "/etc/docker"
}

@ijc
Copy link
Contributor

ijc commented Sep 14, 2018

Got it, thanks! LGTM

@codecov
Copy link

codecov bot commented Sep 14, 2018

Codecov Report

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

@@            Coverage Diff            @@
##             master   #37847   +/-   ##
=========================================
  Coverage          ?   36.13%           
=========================================
  Files             ?      610           
  Lines             ?    45069           
  Branches          ?        0           
=========================================
  Hits              ?    16287           
  Misses            ?    26542           
  Partials          ?     2240

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM

@justincormack PTAL?

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Engine can sometimes create /etc/docker with perms 0700? "docker manifest inspect --insecure" doesn't work
5 participants