From 1981706196164984ea6b75f0c8b7318c7fd6ca5a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 19 Oct 2022 14:34:45 +0200 Subject: [PATCH] daemon: remove migrateTrustKeyID() The migration code is in the 22.06 branch, and if we don't migrate the only side-effect is the daemon's ID being regenerated (as a UUID). Signed-off-by: Sebastiaan van Stijn --- daemon/daemon.go | 10 +--- daemon/id.go | 29 --------- integration-cli/docker_cli_daemon_test.go | 73 ----------------------- integration/daemon/daemon_test.go | 51 +++------------- 4 files changed, 9 insertions(+), 154 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 8c7a4b51b8..5aeb8ac2f2 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -931,14 +931,6 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, err } - // Try to preserve the daemon ID (which is the trust-key's ID) when upgrading - // an existing installation; this is a "best-effort". - idPath := filepath.Join(config.Root, "engine-id") - err = migrateTrustKeyID(config.TrustKeyPath, idPath) - if err != nil { - logrus.WithError(err).Warnf("unable to migrate engine ID; a new engine ID will be generated") - } - // Check if Devices cgroup is mounted, it is hard requirement for container security, // on Linux. // @@ -951,7 +943,7 @@ func NewDaemon(ctx context.Context, config *config.Config, pluginStore *plugin.S return nil, errors.New("Devices cgroup isn't mounted") } - d.id, err = loadOrCreateID(idPath) + d.id, err = loadOrCreateID(filepath.Join(config.Root, "engine-id")) if err != nil { return nil, err } diff --git a/daemon/id.go b/daemon/id.go index 9eb73d2292..cf520ca156 100644 --- a/daemon/id.go +++ b/daemon/id.go @@ -4,10 +4,8 @@ import ( "os" "github.com/docker/docker/pkg/ioutils" - "github.com/docker/libtrust" "github.com/google/uuid" "github.com/pkg/errors" - "github.com/sirupsen/logrus" ) // loadOrCreateID loads the engine's ID from idPath, or generates a new ID @@ -32,30 +30,3 @@ func loadOrCreateID(idPath string) (string, error) { } return id, nil } - -// migrateTrustKeyID migrates the daemon ID of existing installations. It returns -// an error when a trust-key was found, but we failed to read it, or failed to -// complete the migration. -// -// We migrate the ID so that engines don't get a new ID generated on upgrades, -// which may be unexpected (and users may be using the ID for various purposes). -func migrateTrustKeyID(deprecatedTrustKeyPath, idPath string) error { - if _, err := os.Stat(idPath); err == nil { - // engine ID file already exists; no migration needed - return nil - } - trustKey, err := libtrust.LoadKeyFile(deprecatedTrustKeyPath) - if err != nil { - if err == libtrust.ErrKeyFileDoesNotExist { - // no existing trust-key found; no migration needed - return nil - } - return err - } - id := trustKey.PublicKey().KeyID() - if err := ioutils.AtomicWriteFile(idPath, []byte(id), os.FileMode(0600)); err != nil { - return errors.Wrap(err, "error saving ID file") - } - logrus.Info("successfully migrated engine ID") - return nil -} diff --git a/integration-cli/docker_cli_daemon_test.go b/integration-cli/docker_cli_daemon_test.go index 804e77a37d..f3f8030dee 100644 --- a/integration-cli/docker_cli_daemon_test.go +++ b/integration-cli/docker_cli_daemon_test.go @@ -35,7 +35,6 @@ import ( "github.com/docker/docker/opts" testdaemon "github.com/docker/docker/testutil/daemon" units "github.com/docker/go-units" - "github.com/docker/libtrust" "github.com/moby/sys/mount" "golang.org/x/sys/unix" "gotest.tools/v3/assert" @@ -556,24 +555,6 @@ func (s *DockerDaemonSuite) TestDaemonAllocatesListeningPort(c *testing.T) { } } -func (s *DockerDaemonSuite) TestDaemonKeyGeneration(c *testing.T) { - // TODO: skip or update for Windows daemon - os.Remove("/etc/docker/key.json") - c.Setenv("DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE", "1") - s.d.Start(c) - s.d.Stop(c) - - k, err := libtrust.LoadKeyFile("/etc/docker/key.json") - if err != nil { - c.Fatalf("Error opening key file") - } - kid := k.KeyID() - // Test Key ID is a valid fingerprint (e.g. QQXN:JY5W:TBXI:MK3X:GX6P:PD5D:F56N:NHCS:LVRZ:JA46:R24J:XEFF) - if len(kid) != 59 { - c.Fatalf("Bad key ID: %s", kid) - } -} - // GH#11320 - verify that the daemon exits on failure properly // Note that this explicitly tests the conflict of {-b,--bridge} and {--bip} options as the means // to get a daemon init failure; no other tests for -b/--bip conflict are therefore required @@ -1201,60 +1182,6 @@ func (s *DockerDaemonSuite) TestDaemonUnixSockCleanedUp(c *testing.T) { } } -func (s *DockerDaemonSuite) TestDaemonWithWrongkey(c *testing.T) { - type Config struct { - Crv string `json:"crv"` - D string `json:"d"` - Kid string `json:"kid"` - Kty string `json:"kty"` - X string `json:"x"` - Y string `json:"y"` - } - - os.Remove("/etc/docker/key.json") - c.Setenv("DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE", "1") - s.d.Start(c) - s.d.Stop(c) - - config := &Config{} - bytes, err := os.ReadFile("/etc/docker/key.json") - if err != nil { - c.Fatalf("Error reading key.json file: %s", err) - } - - // byte[] to Data-Struct - if err := json.Unmarshal(bytes, &config); err != nil { - c.Fatalf("Error Unmarshal: %s", err) - } - - // replace config.Kid with the fake value - config.Kid = "VSAJ:FUYR:X3H2:B2VZ:KZ6U:CJD5:K7BX:ZXHY:UZXT:P4FT:MJWG:HRJ4" - - // NEW Data-Struct to byte[] - newBytes, err := json.Marshal(&config) - if err != nil { - c.Fatalf("Error Marshal: %s", err) - } - - // write back - if err := os.WriteFile("/etc/docker/key.json", newBytes, 0400); err != nil { - c.Fatalf("Error os.WriteFile: %s", err) - } - - defer os.Remove("/etc/docker/key.json") - - if err := s.d.StartWithError(); err == nil { - c.Fatalf("It should not be successful to start daemon with wrong key: %v", err) - } - - content, err := s.d.ReadLogFile() - assert.Assert(c, err == nil) - - if !strings.Contains(string(content), "Public Key ID does not match") { - c.Fatalf("Missing KeyID message from daemon logs: %s", string(content)) - } -} - func (s *DockerDaemonSuite) TestDaemonRestartKillWait(c *testing.T) { s.d.StartWithBusybox(c) diff --git a/integration/daemon/daemon_test.go b/integration/daemon/daemon_test.go index 42e4155269..2242d3d4f1 100644 --- a/integration/daemon/daemon_test.go +++ b/integration/daemon/daemon_test.go @@ -24,62 +24,27 @@ import ( "gotest.tools/v3/skip" ) -const ( - libtrustKey = `{"crv":"P-256","d":"dm28PH4Z4EbyUN8L0bPonAciAQa1QJmmyYd876mnypY","kid":"WTJ3:YSIP:CE2E:G6KJ:PSBD:YX2Y:WEYD:M64G:NU2V:XPZV:H2CR:VLUB","kty":"EC","x":"Mh5-JINSjaa_EZdXDttri255Z5fbCEOTQIZjAcScFTk","y":"eUyuAjfxevb07hCCpvi4Zi334Dy4GDWQvEToGEX4exQ"}` - libtrustKeyID = "WTJ3:YSIP:CE2E:G6KJ:PSBD:YX2Y:WEYD:M64G:NU2V:XPZV:H2CR:VLUB" -) - -func TestConfigDaemonLibtrustID(t *testing.T) { - skip.If(t, runtime.GOOS == "windows") - - d := daemon.New(t) - defer d.Stop(t) - - trustKey := filepath.Join(d.RootDir(), "key.json") - err := os.WriteFile(trustKey, []byte(libtrustKey), 0644) - assert.NilError(t, err) - - cfg := filepath.Join(d.RootDir(), "daemon.json") - err = os.WriteFile(cfg, []byte(`{"deprecated-key-path": "`+trustKey+`"}`), 0644) - assert.NilError(t, err) - - d.Start(t, "--config-file", cfg) - info := d.Info(t) - assert.Equal(t, info.ID, libtrustKeyID) -} - func TestConfigDaemonID(t *testing.T) { skip.If(t, runtime.GOOS == "windows") d := daemon.New(t) defer d.Stop(t) - trustKey := filepath.Join(d.RootDir(), "key.json") - err := os.WriteFile(trustKey, []byte(libtrustKey), 0644) - assert.NilError(t, err) - - cfg := filepath.Join(d.RootDir(), "daemon.json") - err = os.WriteFile(cfg, []byte(`{"deprecated-key-path": "`+trustKey+`"}`), 0644) - assert.NilError(t, err) - - // Verify that on an installation with a trust-key present, the ID matches - // the trust-key ID, and that the ID has been migrated to the engine-id file. - d.Start(t, "--config-file", cfg, "--iptables=false") + d.Start(t, "--iptables=false") info := d.Info(t) - assert.Equal(t, info.ID, libtrustKeyID) - - idFile := filepath.Join(d.RootDir(), "engine-id") - id, err := os.ReadFile(idFile) - assert.NilError(t, err) - assert.Equal(t, string(id), libtrustKeyID) + assert.Check(t, info.ID != "") d.Stop(t) // Verify that (if present) the engine-id file takes precedence const engineID = "this-is-the-engine-id" - err = os.WriteFile(idFile, []byte(engineID), 0600) + idFile := filepath.Join(d.RootDir(), "engine-id") + assert.Check(t, os.Remove(idFile)) + // Using 0644 to allow rootless daemons to read the file (ideally + // we'd chown the file to have the remapped user as owner). + err := os.WriteFile(idFile, []byte(engineID), 0o644) assert.NilError(t, err) - d.Start(t, "--config-file", cfg, "--iptables=false") + d.Start(t, "--iptables=false") info = d.Info(t) assert.Equal(t, info.ID, engineID) d.Stop(t)