-
Notifications
You must be signed in to change notification settings - Fork 575
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
snap/validate.go: disallow snap layouts with new top-level directories #8985
snap/validate.go: disallow snap layouts with new top-level directories #8985
Conversation
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
9ff2d2c
to
ef9ba38
Compare
We previously wouldn't fail on verification for snaps that used new top-level directories, and would fail at runtime, which produces a very unhelpful message. Also adjust some test yamls in interfaces/mount tests, which are not valid as they are for new top-level directories in /. Finally, add an additional unit test for the test case with LP #1831010 that is already covered by spread tests, but not yet by unit tests. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
ef9ba38
to
7ae57af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
@anonymouse64 my review was hasty, the validator needs to expand variables or it will lead to failures in supported cases like this:
|
snap/validate.go
Outdated
@@ -400,6 +400,38 @@ func ValidateLayoutAll(info *Info) error { | |||
} | |||
sort.Strings(paths) | |||
|
|||
// Validate that each source path is not a new top-level directory | |||
for pathSrc := range info.Layout { | |||
cleanPathSrc := info.ExpandSnapVariables(filepath.Clean(pathSrc)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is puzzling. Having expanded the variables it still is confused. Looking at the rest of the code we seem to use Layout.Path
J
In actual usages outside of the unit tests, we will use the layout.Path element so use that instead. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// this list was produced by taking all of the top level | ||
// directories in the core snap and removing the explicitly | ||
// denied top-level directories | ||
case "bin", "etc", "lib", "lib64", "meta", "mnt", "opt", "root", "sbin", "snap", "srv", "usr", "var", "writable": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orthogonal to the PR, but shouldn't /writable
be denied too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm not sure, since the directory is there and since 2.4X we don't mount /writable inside the snap's mount namespace anyways I don't see why we should disallow /writable, @zyga what do you think ?
We previously wouldn't fail on verification for snaps that used new top-level directories, and would fail at runtime, which produces a very unhelpful message.
Also adjust some test yamls in interfaces/mount tests, which are not valid as they are for new top-level directories in /.
Finally, add an additional unit test for the test case with LP #1831010 that is already covered by spread tests, but not yet by unit tests.
See https://forum.snapcraft.io/t/snap-application-works-in-classic-mode-fails-in-dev-mode-and-strict/18562/7 for an example of such a confusion.