snap: understand directories in layout blacklist #4636
Conversation
The layout blacklist contains string prefixes that are forbidden in subsequent layout elements. The prefixes are just paths of each layout elements. To work correctly a layout element that describes a directory (vs a file) needs to contain the trailing slash. In other words, this layout is valid: - directory /etc/demo (blacklist contribution: /etc/demo/) - file /etc/demo.conf (blacklist contribution: /etc/demo.conf) Because /etc/demo.conf is not a prefix of /etc/demo/, this should be allowed by the validator. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #4636 +/- ##
==========================================
+ Coverage 78.32% 78.34% +0.01%
==========================================
Files 465 465
Lines 32984 32993 +9
==========================================
+ Hits 25836 25847 +11
+ Misses 5017 5016 -1
+ Partials 2131 2130 -1
Continue to review full report at Codecov.
|
snap/validate.go
Outdated
| for i := range blacklist { | ||
| if strings.HasPrefix(mountPoint, blacklist[i]) { | ||
| if (strings.HasSuffix(blacklist[i], "/") && strings.HasPrefix(effectivePath, blacklist[i])) || effectivePath == blacklist[i] { |
There was a problem hiding this comment.
Minor suggestion (maybe it'll be useful later on):
// Conflicts checks whether this layout is in conflict with other layout
func (layout *Layout) Conflicts(other *Layout) bool {
...
}
...
for _, other := range blacklist {
if layout.Conflicts(other) {
return fmt.Errorf("...")
}
}
There was a problem hiding this comment.
Yeah, I have something similar since my discussion with. @chipaca yesterday. I'll tweak and push.
snap/validate.go
Outdated
| for i := range blacklist { | ||
| if strings.HasPrefix(mountPoint, blacklist[i]) { | ||
| if (strings.HasSuffix(blacklist[i], "/") && strings.HasPrefix(effectivePath, blacklist[i])) || effectivePath == blacklist[i] { |
There was a problem hiding this comment.
Yeah, some sort of helper might make this easier to read
There was a problem hiding this comment.
I implemented a helper I discussed with @chipaca now.
Layout validation is somewhat tricky. To make it more scalable and easier to understand layout elements are now validated against a list of constraints that can be specialized to particular layout entries (mounted file-systems, symlinks, etc.) In addition some more tests show how this behaves in practice. Some of the ideas are thanks to Chipaca, thank you man! Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap/validate.go
Outdated
| IsOffLimits(path string) bool | ||
| } | ||
|
|
||
| // mountedDirectory represents a mounted file-system tree or a bind-mounted directory. |
No, because /foo will make the /foo/bar symlink off-limits. The whole layout gets rejected anyway.
Yes because in phase one of layouts we are overly conservative about what is allowed and we explicitly deny anything that may overlay with another layout element.
I thought about it but |
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The layout blacklist contains string prefixes that are forbidden in
subsequent layout elements. The prefixes are just paths of each layout
elements. To work correctly a layout element that describes a directory
(vs a file) needs to contain the trailing slash. In other words, this
layout is valid:
Because /etc/demo.conf is not a prefix of /etc/demo/, this should be
allowed by the validator.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com