snap/naming: add validator for snap security tag #8408
Conversation
Security tags were only created by snapd, never read from external sources. With the upcoming refresh app awareness patches we will have to parse and validate security tags that are embedded in cgroup names. This patch provides the validator. It was cherry-picked from the main feature branch. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
When we are validating security tags we don't need to split the tag over each dot, only over the first four we expect to see. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
When checking security tags we can look at the fixes bits ("snap" and
"hook") before we even attempt to look at the variable parts.
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
snap/naming/validate_test.go
Outdated
| // invalid number of components are rejected. | ||
| c.Check(naming.ValidateSecurityTag("snap.pkg.hook.surprise."), ErrorMatches, "invalid security tag") | ||
| c.Check(naming.ValidateSecurityTag("snap.pkg.hook."), ErrorMatches, "invalid security tag") | ||
| c.Check(naming.ValidateSecurityTag("snap.pkg.hook"), IsNil) // surprise, it is a valid app name! |
There was a problem hiding this comment.
Nitpick (not a blocker) perhaps you don't want to use 'surprise' here since you use 'surprise' up above to indicate an error? Perhaps just // actually a valid app name! :) or similar?
snap/naming/validate.go
Outdated
| // | ||
| // TODO: handle the weird udev variant. | ||
| func ValidateSecurityTag(tag string) error { | ||
| parts := strings.SplitN(tag, ".", 4) |
There was a problem hiding this comment.
The SplitN() change is fine but I actually preferred Split() since SplitN(..., 4) with something that would've had more parts forces the additional check in the ValidateApp()/ValidateHook() rather than erroring early off the simple length check. Not a blocker, just an observation.
There was a problem hiding this comment.
Indeed, I'll change it to SplitN(..., 5) so that the length check remains an early warning and add a comment as to why.
snap/naming/validate.go
Outdated
| } | ||
| if err := ValidateInstance(snapName); err != nil { | ||
| return errInvalidSecurityTag | ||
| } |
There was a problem hiding this comment.
Again, not a blocker, but you could slightly refactor and put the snapLiteral and ValidateInstance() checks before the switch since they are identical for both case 3 and 4 (you'd then obviously have to check the length is at least 2 though).
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Thanks to Jamie for the idea :) Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Security tags were only created by snapd, never read from external
sources. With the upcoming refresh app awareness patches we will have to
parse and validate security tags that are embedded in cgroup names.
This patch provides the validator. It was cherry-picked from the main
feature branch.
Signed-off-by: Zygmunt Krynicki me@zygoon.pl