Windows: create daemon root with ACL by lowenna · Pull Request #28274 · moby/moby

lowenna · 2016-11-11T00:35:23Z

Signed-off-by: John Howard [email protected]

This PR ensures the Windows daemon creates the root with an ACL for Local System and Built-In Administrators. @NathanMcCauley @thaJeztah

vieux · 2016-11-11T00:47:36Z

SGTM

johnstep · 2016-11-11T00:55:41Z

cmd/dockerd/daemon.go

This code does not run on Windows, due to the early exit above. Therefore, I think this can be changed back to MkdirAll, and maybe MkdirAllWithACL can be Windows only.

Fair. Updated in next push (imminent)

johnstep · 2016-11-11T01:00:42Z

pkg/system/filesys_windows.go

Update the comment to reflect that it grants full access to administrators and system.

Updated in next push (imminent)

Signed-off-by: John Howard <[email protected]>

lowenna · 2016-11-11T01:54:45Z

@johnstep Comments addressed. Rebased following minor merge conflict after the merge of the 'plug-ins coming out of experimental' PR just now.

tonistiigi · 2016-11-11T02:20:57Z

cmd/dockerd/daemon.go

-		opts.common.TrustKey = filepath.Join(
-			getDaemonConfDir(),
-			cliflags.DefaultTrustKeyFile)
-	}


@jhowardmsft Could you explain the moved line here?

It's to ensure the root is created first, before anything else. The current ordering creates other files prior to the daemon root, indirectly creating the root in the process.

I understand that. Can you point me where in configuration loading we create it so the order of initializing trust key path becomes significant.

I'd have to debug it again, can't immediately see it. But I know it was the case when creating the commit back at the weekend.

tonistiigi · 2016-11-11T02:27:25Z

cmd/dockerd/daemon.go

+	if err := daemon.CreateDaemonRoot(cli.Config); err != nil {
+		return err
+	}
+


Same question: can we keep this inside NewDaemon. From go api standpoint it doesn't make sense that this preparation is needed before calling NewDaemon.

Is it possible to move pidfile/migrateKey instead?

Same answer as above.

Actually, not in the current code flow. The startup path for the daemon is kind of strange and all over the place. I have to guarantee that the very first thing done is to create the root before any other file is created.

I think we can do better but let's leave that to some other PR.

Yes, agreed. I wanted to be as minimally invasive as possible and just fix the immediate issue.

johnstep

LGTM

lowenna · 2016-11-11T03:05:48Z

@crosbymichael @tiborvass @cpuguy83 In the hope that this might get in ahead of the code freeze cutoff. Appreciate you're all busy though. Or asleep ;)

tonistiigi · 2016-11-11T07:35:07Z

LGTM

thaJeztah · 2016-11-11T08:09:16Z

ping @riyazdf PTAL

riyazdf

code LGTM with nits.

However, I think we should add windows-specific tests around these permissions tests to ensure there are correct and there are no future regressions. This can happen in a follow-up PR.

riyazdf · 2016-11-11T16:06:18Z

cmd/dockerd/daemon.go

 	}
+
 	// FIXME: why is this down here instead of with the other TrustKey logic above?
 	cli.TrustKeyPath = opts.common.TrustKey


leaving a breadcrumb since we touched relevant code to this FIXME: especially from the changes in this PR it seems that we can move this to the TrustKey logic above. We can do this in a followup PR.

riyazdf · 2016-11-11T16:06:49Z

cmd/dockerd/daemon_windows.go

-func getDaemonConfDir() string {
-	return os.Getenv("PROGRAMDATA") + `\docker\config`
+func getDaemonConfDir(root string) string {
+	return filepath.Join(root, `\config`)


nit: the \ might be redundant for filepath.Join in windows

riyazdf · 2016-11-11T16:10:35Z

daemon/daemon_windows.go

 	config.Root = rootDir
 	// Create the root directory if it doesn't exists
-	if err := system.MkdirAll(config.Root, 0700); err != nil && !os.IsExist(err) {
+	if err := system.MkdirAllWithACL(config.Root, 0); err != nil && !os.IsExist(err) {


nit: it might be worth leaving a comment about why 0 is acceptable for Windows since that argument isn't used as permissions

riyazdf · 2016-11-11T16:11:43Z

pkg/system/filesys_windows.go

+// and Local System.
+func mkdirWithACL(name string) error {
+	sa := syscall.SecurityAttributes{Length: 0}
+	sddl := "D:P(A;OICI;GA;;;BA)(A;OICI;GA;;;SY)"


nit: might be worth leaving a comment explaining what this expression semantically means

thaJeztah · 2016-11-11T16:17:21Z

@jhowardmsft can you do a follow up to address those nits? I'll move this to "merge"

lowenna · 2016-11-11T16:44:35Z

Yes.

GordonTheTurtle added the status/0-triage label Nov 11, 2016

icecrime added this to the 1.13.0 milestone Nov 11, 2016

icecrime added status/1-design-review and removed status/0-triage labels Nov 11, 2016

johnstep reviewed Nov 11, 2016

View reviewed changes

thaJeztah assigned NathanMcCauley Nov 11, 2016

johnstep reviewed Nov 11, 2016

View reviewed changes

tonistiigi added status/2-code-review and removed status/1-design-review labels Nov 11, 2016

lowenna force-pushed the jjh/acl branch from a5f16fb to 68ba3d8 Compare November 11, 2016 01:47

Windows: create daemon root with ACL

46ec4c1

Signed-off-by: John Howard <[email protected]>

lowenna force-pushed the jjh/acl branch from 68ba3d8 to 46ec4c1 Compare November 11, 2016 01:51

tonistiigi reviewed Nov 11, 2016

View reviewed changes

johnstep approved these changes Nov 11, 2016

View reviewed changes

lowenna unassigned NathanMcCauley Nov 11, 2016

riyazdf approved these changes Nov 11, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 11, 2016

vieux merged commit 2712bb2 into moby:master Nov 11, 2016

lowenna deleted the jjh/acl branch November 11, 2016 17:35

thaJeztah mentioned this pull request Dec 21, 2024

pkg/system: deprecate MkdirAll and remove custom volume GUID handling #49162

Merged

Comments

Conversation

lowenna commented Nov 11, 2016

Uh oh!

vieux commented Nov 11, 2016

Uh oh!

johnstep Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Nov 11, 2016

Uh oh!

tonistiigi commented Nov 11, 2016

Uh oh!

thaJeztah commented Nov 11, 2016

Uh oh!

riyazdf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 11, 2016

Uh oh!

lowenna commented Nov 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

johnstep Nov 11, 2016 •

edited

Loading

tonistiigi Nov 11, 2016 •

edited

Loading