Expose trust key path in config #32587

dmcgowan · 2017-04-13T00:15:22Z

Allows storing key under any directory. In the case where the "/etc/docker" directory is not preserved, this file can be specified to a location where it will be preserved to ensure the ID does not change across restarts.

Note this key is currently only used today to generate the ID used in Docker info and for manifest schema v1 pushes. The key signature and finger on these manifests are not checked or used any longer for security, deprecated by notary.

Removes old key migration from a pre-release of Docker which put the key under the home directory and was used to preserve ID used for swarm v1 after the file moved.

closes #32135

ping @rogaha

rogaha · 2017-04-13T00:27:40Z

In the future as we talked before we could get rid of the trust key and replace it with an ID file, but for now it fixes the issue. Thanks @dmcgowan!

dmcgowan · 2017-04-13T00:34:54Z

@rogaha I did look at that, I forgot we need to still do something when pushing old v1 manifests. Although the key we use for pushing those manifests can be completely ephemeral, it would require generating some sort of key.

rogaha

LGTM

thaJeztah

wasn't sure if the path should now be configurable through the daemon.json, but if so, it may need some more changes

thaJeztah · 2017-04-16T22:25:37Z

daemon/config/config.go

@@ -101,7 +101,7 @@ type CommonConfig struct {
 	RootDeprecated       string                    `json:"graph,omitempty"`
 	Root                 string                    `json:"data-root,omitempty"`
 	SocketGroup          string                    `json:"group,omitempty"`
-	TrustKeyPath         string                    `json:"-"`
+	TrustKeyPath         string                    `json:"key-path,omitempty"`


Does this actually work?

echo '{"debug":true,"key-path":"/hello-world.json"}' > /etc/docker/daemon.json dockerd unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives don't match any configuration option: key-path

dmcgowan · 2017-04-26T22:59:39Z

@thaJeztah ugh, updated to add a flag as the config code requires. It is unfortunate adding a flag for this.

thaJeztah

Some questions 😊

thaJeztah · 2017-05-02T18:58:32Z

cli/flags/common.go

@@ -69,6 +69,8 @@ func (commonOpts *CommonOptions) InstallFlags(flags *pflag.FlagSet) {
 	flags.Var(opts.NewQuotedString(&tlsOptions.CertFile), "tlscert", "Path to TLS certificate file")
 	flags.Var(opts.NewQuotedString(&tlsOptions.KeyFile), "tlskey", "Path to TLS key file")

+	flags.Var(opts.NewQuotedString(&commonOpts.TrustKey), "key-path", "Path to key file for ID and image signing")


should we mention (default: /etc/docker/key.json)? Not sure what the default is on Windows

Also; this is in common flags, so this allows me to use;

docker --key-path=/foobar info

But doing so doesn't make a difference for the ID printed in docker info

@thaJeztah that's very unusual use-case. I think we are going to use the key-path as a dockerd parameter only.

Yes discussed with @dmcgowan earlier this week; and it needs to be moved to a different location so that it doesn't show up in the client

👍 thanks for the update @thaJeztah!

dmcgowan · 2017-05-05T00:02:53Z

Updated, option and flag are now daemon only

rogaha · 2017-05-05T08:02:35Z

👍

thaJeztah · 2017-05-05T21:59:02Z

moving to code review

rogaha · 2017-05-08T13:07:11Z

@thaJeztah PTAL

thaJeztah

LGTM

ping @dnephin PTAL

dnephin

cli/flags/common.go is in docker/cli as well. It looks like we set cli.keyFile but never use it. Can we delete this code from docker/cli as well?

dnephin · 2017-05-08T15:30:22Z

cmd/dockerd/config.go

@@ -53,6 +55,8 @@ func installCommonConfigFlags(conf *config.Config, flags *pflag.FlagSet) {

 	flags.StringVar(&conf.MetricsAddress, "metrics-addr", "", "Set default address and port to serve the metrics api on")

+	flags.Var(opts.NewQuotedString(&conf.TrustKeyPath), "key-path", "Path to key file for ID and image signing")


I think the flag key-path might not provide enough context to the user. Could this be trust-key-path, or something like that?

Problem is that it is not really used for trust anymore since the signatures are not checked and the ID is not in Swarm mode for cryptographic identity. It is basically just used for deprecated features and not something users will want to set themselves. I only added a flag because the config parser required having the flag as well.

In that case could we make the name deprecated-key-path, and Also add

flags.MarkHidden("deprecated-key-path")

So we hide it from help.

If we expect it to be removed at some point we could use MarkDeprecated() instead of hidden

ping @dmcgowan.

Much prefer it to be MarkDeprecated(). That would have probably saved me two days of head banging and hair pulling trying to find a way around a read-only /etc/docker before I discovered this 😒

@paddy-hack I am currently banging my head - could you clue me in on what you ended up doing to support read-only /etc?

It seems like the only solution would be to generate a JWK key manually and point to it with this config option, which seems subpar since dockerd clearly has the ability to generate a key on its own - it just refuses to generate it in any other directory...

The "trustkey" has been removed in Docker 23.0 and up; support for pushing the legacy images was removed already (the only remaining purpose for libtrust), and Docker 23.0 now stores an "engine-id" file in /var/lib/docker/engine-id (when using the default data location /var/lib/docker/), and the daemon will no longer write a file to /etc/docker/.

ls -l /var/lib/docker total 88 drwx------ 2 root root 4096 May 25 2019 builder drwx------ 5 root root 4096 Mar 18 2021 buildkit drwx--x--- 3 root root 4096 Apr 4 20:45 containers -rw------- 1 root root 59 Mar 7 12:15 engine-id drwx------ 3 root root 4096 Jun 27 2022 image drwxr-x--- 3 root root 4096 May 25 2019 network drwx--x--- 81 root root 36864 Apr 4 20:49 overlay2 drwx------ 4 root root 4096 May 25 2019 plugins drwx------ 2 root root 4096 Mar 28 12:19 runtimes drwx------ 5 root root 4096 Mar 28 12:19 swarm drwx------ 2 root root 4096 Apr 4 20:50 tmp drwx------ 2 root root 4096 May 25 2019 trust drwx-----x 5 root root 4096 Mar 28 12:19 volumes

For existing installations that have a /etc/docker/key.json and are upgraded to 23.0, the key ID is copied to the engine-id file (to keep the same "ID" in docker info output). For fresh installs, or situations where no key.json is present, a random ID (GUID) is generated, and used as ID.

For details, see these PRs;

[23.0 backport] use ad-hoc libtrust trustkey #44566

Remove trustkey id migration and config.TrustKeyPath #44329

daemon: separate daemon ID from trust-key, and disable generating #43555

@harrisonmg In my case, I was trying to mount a single docker-daemon.json "file" from a Kubernetes ConfigMap on /etc/docker/ in any container our GitLab Runner would spin up for CI/CD jobs. Doing so resulted in /etc/docker/ becoming read-only so the daemon could no longer create a key.json file at its default location. I worked around the issue by setting the deprecated-key-path in my docker-daemon.json file 😸

Like so

{ "deprecated-key-path": "/var/lib/docker/key.json", "registry-mirrors": [ "https://docker-registry.mirror.internal" ] }

@thaJeztah Unfortunately for me, I don't control the Docker versions that many of our CI/CD jobs want to use 😿

Allows storing key under any directory. In the case where the "/etc/docker" directory is not preserved, this file can be specified to a location where it will be preserved to ensure the ID does not change across restarts. Note this key is currently only used today to generate the ID used in Docker info and for manifest schema v1 pushes. The key signature and finger on these manifests are not checked or used any longer for security, deprecated by notary. Removes old key migration from a pre-release of Docker which put the key under the home directory and was used to preserve ID used for swarm v1 after the file moved. closes moby#32135 Signed-off-by: Derek McGowan <derek@mcgstyle.net>

dmcgowan · 2017-05-10T21:19:21Z

Renamed key and marked as hidden. Since it was hidden it will not show up in help, which was desired from the beginning. Thanks @dnephin for pointing out that feature. Further added comments explaining the state of this key and the intention to fully deprecate it.

dnephin · 2017-05-10T21:23:02Z

Looks like the docker/cli reference needs to be updated? ci is failing

dnephin

LGTM when CI is green

dmcgowan · 2017-05-10T21:46:37Z

CI is failing but looks like a Jenkins problem, I restarted and failed with same error

rogaha · 2017-05-11T19:00:49Z

@thaJeztah thanks for re-triggering the tests. It's all green now! :)

thaJeztah · 2017-05-11T19:03:42Z

Yay!

GordonTheTurtle added the status/0-triage label Apr 13, 2017

dmcgowan force-pushed the trust-key-config branch from 2ef65ea to 3021157 Compare April 13, 2017 00:22

rogaha approved these changes Apr 13, 2017

View reviewed changes

vdemeester added status/1-design-review and removed status/0-triage labels Apr 13, 2017

vdemeester requested a review from thaJeztah April 13, 2017 08:39

thaJeztah requested changes Apr 16, 2017

View reviewed changes

dmcgowan force-pushed the trust-key-config branch from 3021157 to e0757be Compare April 26, 2017 22:58

GordonTheTurtle assigned yongtang Apr 28, 2017

thaJeztah reviewed May 2, 2017

View reviewed changes

thaJeztah added this to the 17.06.0 milestone May 2, 2017

rogaha mentioned this pull request May 3, 2017

Add extra prometheus metrics #32792

Merged

dmcgowan force-pushed the trust-key-config branch 2 times, most recently from 74f1450 to 7030c90 Compare May 5, 2017 00:01

thaJeztah added status/2-code-review and removed status/1-design-review labels May 5, 2017

dmcgowan force-pushed the trust-key-config branch from 7030c90 to 95d42a2 Compare May 5, 2017 22:45

dmcgowan mentioned this pull request May 5, 2017

Remove unused trust key docker/cli#28

Merged

dmcgowan force-pushed the trust-key-config branch from 95d42a2 to 0856d1f Compare May 5, 2017 22:57

thaJeztah approved these changes May 8, 2017

View reviewed changes

dnephin reviewed May 8, 2017

View reviewed changes

dmcgowan force-pushed the trust-key-config branch from 0856d1f to e428c82 Compare May 10, 2017 21:16

dnephin reviewed May 10, 2017

View reviewed changes

thaJeztah merged commit 5771687 into moby:master May 11, 2017

thaJeztah added the impact/cli label Jun 27, 2017

thaJeztah mentioned this pull request Mar 3, 2018

Constant ID for Docker Demon #36463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose trust key path in config #32587

Expose trust key path in config #32587

dmcgowan commented Apr 13, 2017

rogaha commented Apr 13, 2017 •

edited

dmcgowan commented Apr 13, 2017

rogaha left a comment

thaJeztah left a comment

thaJeztah Apr 16, 2017

dmcgowan commented Apr 26, 2017

thaJeztah left a comment

thaJeztah May 2, 2017

thaJeztah May 2, 2017

rogaha May 3, 2017

thaJeztah May 4, 2017

rogaha May 4, 2017

dmcgowan commented May 5, 2017

rogaha commented May 5, 2017

thaJeztah commented May 5, 2017

rogaha commented May 8, 2017

thaJeztah left a comment

dnephin left a comment

dnephin May 8, 2017 •

edited

dmcgowan May 8, 2017

dnephin May 8, 2017

rogaha May 10, 2017

paddy-hack Aug 29, 2022

harrisonmg Apr 5, 2023

thaJeztah Apr 5, 2023

paddy-hack Apr 6, 2023

dmcgowan commented May 10, 2017

dnephin commented May 10, 2017

dnephin left a comment

dmcgowan commented May 10, 2017

rogaha commented May 11, 2017

thaJeztah commented May 11, 2017

		@@ -53,6 +55,8 @@ func installCommonConfigFlags(conf config.Config, flags pflag.FlagSet) {

		flags.StringVar(&conf.MetricsAddress, "metrics-addr", "", "Set default address and port to serve the metrics api on")

		flags.Var(opts.NewQuotedString(&conf.TrustKeyPath), "key-path", "Path to key file for ID and image signing")

Expose trust key path in config #32587

Expose trust key path in config #32587

Conversation

dmcgowan commented Apr 13, 2017

rogaha commented Apr 13, 2017 • edited

dmcgowan commented Apr 13, 2017

rogaha left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented Apr 26, 2017

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

dmcgowan commented May 5, 2017

rogaha commented May 5, 2017

thaJeztah commented May 5, 2017

rogaha commented May 8, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dnephin May 8, 2017 • edited

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

dmcgowan commented May 10, 2017

dnephin commented May 10, 2017

dnephin left a comment

Choose a reason for hiding this comment

dmcgowan commented May 10, 2017

rogaha commented May 11, 2017

thaJeztah commented May 11, 2017

rogaha commented Apr 13, 2017 •

edited

dnephin May 8, 2017 •

edited