Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-update-ns: re-factor secureMkdirAll into secureMk{Prefix,Dir} #4163
Conversation
Some checks were not successful
This was referenced Nov 7, 2017
codecov-io
commented
Nov 8, 2017
•
Edited 3 times
-
codecov-io
Nov 8, 2017
-
codecov-io
Nov 8, 2017
-
codecov-io
Nov 8, 2017
-
Nov 8, 2017 -
Nov 8, 2017 -
Nov 8, 2017
Codecov Report
@@ Coverage Diff @@
## master #4163 +/- ##
==========================================
+ Coverage 75.52% 75.59% +0.07%
==========================================
Files 436 436
Lines 37766 37853 +87
==========================================
+ Hits 28521 28615 +94
+ Misses 7250 7245 -5
+ Partials 1995 1993 -2
Continue to review full report at Codecov.
|
bboozzoo
reviewed
Nov 8, 2017
Looks reasonable. There's this one thing with name = "/" that I indicated, might be worth double checking.
| + | ||
| +// secureMkdir creates a directory at i-th entry of absolute path represented | ||
| +// by segments. This function can be used to construct subsequent elements of | ||
| +// the constructed path. |
bboozzoo
Nov 8, 2017
Contributor
Maybe add a note that secureMkDir returns a file descriptor to newly created segment and the it's -1 on errors.
| - // Open the root directory and start there. | ||
| - fd, err = sysOpen("/", openFlags, 0) | ||
| + segments := strings.FieldsFunc(filepath.Clean(name), func(c rune) bool { return c == '/' }) | ||
| + |
bboozzoo
Nov 8, 2017
Contributor
You should probably check if len(segments) > 0. Haven't tracked through the code but I think you'll get a panic with name = "/".
zyga
added some commits
Nov 8, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
| +} | ||
| + | ||
| +// Ensure that we can create a directory in the top-level directory. | ||
| +func (s *utilsSuite) TestSecureMkdirAllLevel1(c *C) { |
pedronis
Nov 13, 2017
Contributor
don't we need a test for for update.SecureMkDirAll("/") ? should that fail if perm or gid/uid aren't the only possible ones?
zyga
Nov 13, 2017
Contributor
I don't think this needs to be this paranoid. First of all, we don't chown things we don't create and we don't create / ever :-) (we just open it, there's no code to try to create it). Lastly we are still confined and we cannot open / in writable mode.
zyga
Nov 13, 2017
•
Edited 1 time
-
zyga
Nov 13, 2017
Contributor
-
Nov 13, 2017
I added a Level0 test here actually, let me see https://github.com/snapcore/snapd/pull/4163/files#diff-0c4753259207bf20c552a1115ebb8759R65
stolowski
approved these changes
Nov 13, 2017
+1 with two tiny nitpicks from me, and assuming Maciej's and Samuele's feedback is adressed.
| + // Declare var and don't assign-declare below to ensure we don't swallow | ||
| + // any errors by mistake. | ||
| + var err error | ||
| + var fd int |
zyga
Nov 13, 2017
Contributor
Commented in the other PR that we cannot do that as it triggers ineffectual assignment error.
| + segment := segments[i] | ||
| + made := true | ||
| + var err error | ||
| + var newFd int |
stolowski
merged commit f6f387f
into
snapcore:master
Nov 13, 2017
3 of 7 checks passed
jdstrand
reviewed
Nov 13, 2017
The directory creation itself still looks ok (ie, going from '/' to '/path/to/something' via mkdirat/openat.
However I found the decomposition and resulting API a bit awkward. Can you describe why it needs to be this way?
| @@ -45,6 +47,87 @@ var ( | ||
| sysFchown = syscall.Fchown | ||
| ) | ||
| +// Create directories for all but the last segments and return the file |
| @@ -45,6 +47,87 @@ var ( | ||
| sysFchown = syscall.Fchown | ||
| ) | ||
| +// Create directories for all but the last segments and return the file | ||
| +// descriptor to the leaf directory. This function is a base for secure |
jdstrand
Nov 13, 2017
•
Edited 1 time
-
jdstrand
Nov 13, 2017
Contributor
-
Nov 13, 2017
Rather than 'leaf directory' I think you mean 'parent'. Honestly, I found the naming here a bit confusing. secureMkDirParent() might be more clear...
I still find the code hard to read and the decomposition feels awkward where secureMkDirPrefix is passed all the segments (elements or tokens might be a better name for the variable...) and it creates everything but the last one. Why not just pass secureMkDirAll the parent and be done with it?
Put another way, why 3 functions: secureMkDirAll which creates everything, secureMkDirPrefix which creates all but the last and secureMkDir which creates an arbitrary internal element? Why isn't it sufficient to have a single function that just goes from '/' to the path you want? I guess I don't understand the point of this PR-- can you describe where you are going and why this needs to be decomposed in this way?
pedronis
Nov 13, 2017
Contributor
afaict the description says that the plan is to reuse and compose *Prefix together with functions that create symlinks or files instead of dirs
jdstrand
Nov 13, 2017
Contributor
That doesn't explain why the API is decomposed and implemented in the manner it is-- maybe its just me, but, while it works, I found it somewhat convoluted.
jdstrand
Nov 13, 2017
•
Edited 1 time
-
jdstrand
Nov 13, 2017
Contributor
-
Nov 13, 2017
For example, having one function to create a directory, one to create a symlink and one to create a file would be clear. The symlink and the file functions would simply call the directory function with the dirname of the symlink/file to make sure the directory was created before doing the symlink or the file operation.
| + "/"+strings.Join(segments[:i], "/"), err) | ||
| + } | ||
| + if made { | ||
| + // Chown each segment that we made. |
jdstrand
Nov 13, 2017
Contributor
This comment is misleading. You only created one segment. s/each/the/
| - fd, err = sysOpenat(fd, segment, openFlags, 0) | ||
| + if len(segments) > 0 { | ||
| + // Create the final segment as a directory. | ||
| + fd, err = secureMkDir(fd, segments, len(segments)-1, perm, uid, gid) |
jdstrand
Nov 13, 2017
•
Contributor
Again, this is awkward without knowing the reason why you broke it up in this way. We do:
segments := ...
...
fd, err := secureMkPrefix(segments, perm, uid, gid)
...
fd, err = secureMkDir(fd, segments, len(segments)-1, perm, uid, gid)
To me, an API something along the lines of this seems more clear:
// create the dir and any needed parent directories
createParents := true
fd, err := secureMkDir(segments, perm, uid, gid, createParents)
...
// create only the specified directory and no parents
createParents = false
fd, err := secureMkDir(segments, perm, uid, gid, createParents)
Then if you just want to create the parent directory (and its parents) for some reason, you just do:
fd, err := secureMkDir(segments[0:len(segments)-1], perm, uid, gid, true)
|
Unfortunately this was merged without my review (I said I would complete it by today but by the time I got to it, it was merged). |
zyga commentedNov 7, 2017
The act of "securely creating a directory" can be decomposed into
"securely create the prefix" and "securely create the leaf". Naturally
the leaf can now vary and we can easily create leaf directories, files
and symlinks.
This patch sets the stage for that to happen, by splitting the main
function into a pair of functions. On the outside nothing changes but
tests needed a slight tweak to accommodate how life-cycle of file
descriptors looks like.
There are also several new tests to improve code coverage in various
edge cases.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com