Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
skills/types: add bool-file #462
Conversation
zyga
added some commits
Feb 8, 2016
stevenwilkin
reviewed
Feb 8, 2016
| +// BoolFileType is the type of all the bool-file skills. | ||
| +type BoolFileType struct{} | ||
| + | ||
| +// String() returns the same value as Name(). |
stevenwilkin
reviewed
Feb 8, 2016
| + return t.Name() | ||
| +} | ||
| + | ||
| +// Name returns the name of the bool-file type (always "bool-file"). |
stevenwilkin
reviewed
Feb 8, 2016
| + // To provide GPIOs we need extra permissions to export/unexport and to | ||
| + // set the direction of each pin. | ||
| + if t.isGPIO(skill) { | ||
| + snippet := []byte(` |
stevenwilkin
Feb 8, 2016
Member
Maybe have this []byte defined at the top of the function instead of inside the switch statement?
stevenwilkin
reviewed
Feb 8, 2016
| + return nil, nil | ||
| + case skills.SecuritySeccomp: | ||
| + return nil, nil | ||
| + case skills.SecurityDBus: |
stevenwilkin
Feb 8, 2016
Member
Could perhaps have:
case skills.SecuritySeccomp, skills.SecurityDBus:
return nil, nil
stevenwilkin
reviewed
Feb 8, 2016
stevenwilkin
reviewed
Feb 8, 2016
| + `skill slot is not of type "bool-file"`) | ||
| +} | ||
| + | ||
| +func (s *BoolFileTypeSuite) TestSlotSecuritySnippet(c *C) { |
stevenwilkin
Feb 8, 2016
Member
There's a lot happening in this test, maybe break it into 2: 1 testing error conditions and 1 testing the happy path?
stevenwilkin
reviewed
Feb 8, 2016
| +var evalSymlinks = filepath.EvalSymlinks | ||
| + | ||
| +// MockEvalSymlinks replaces the path/filepath.EvalSymlinks function used inside the caps package. | ||
| +func MockEvalSymlinks(test *testutil.BaseTest, fn func(string) (string, error)) { |
stevenwilkin
Feb 8, 2016
Member
This test helper will end up in the build, can it go inside a *_test.go file?
|
retest this please |
jdstrand
commented on skills/types/bool_file.go in 4098a64
Feb 8, 2016
|
I worry that these paths are hardcoded instead of declarative via a file that the snappy command would parse. Rules need to change for various reasons and hardcoding them this way means that maintenance of policy is considerably more difficult. Furthermore, having rules hard-coded in the snappy codebase makes it extremely difficult to audit the policy. Much better IMO would be to treat these skill types that need security policy like the old school caps-- each type gets a corresponding security policy file that can be maintained separately from the snappy source. Is /sys/class/gpio/import valid? I had not seen this. I had seen /sys/class/gpio/unexport. I suspect that these rules are too lenient. The skill is called 'bool-file' but these rules will allow changing the 'direction' on all gpio pins (files) rather than the file that is specified. I thought the way this was supposed to work was that these would be per pin as opposed to all pins. I also suspect that these won't be enough permissions and other accesses in /sys/class/gpio or elsewhere will be needed. Eg, I think I've seen others needing things like /sys/devices/gpio1/..., but I'm not a gpio expert. |
|
We've selected hardcoding to explore how capabilities/skills will work out in practice. We can consider moving to external files later but for now this is the agreed direction. I've corrected import (it should be unexport), thanks for spotting that! The "bool-file" skill has two sides. The provider of the skill can indeed change the As for needing more permissions, let's try and see. It is always easier to add more :) |
jdstrand
replied
Feb 9, 2016
|
Re hardcoding> I guess if that is the decision, that is 'ok' for now, but I strongly feel this is going to be a maintenance burden and something we'll want to address before 16.04 is released. I also worry if we continue hardcoding we will be entrenched. Thank you for explaining the two sides for the skill-- I forgot this point in reviewing the merge since it didn't jump out at me that this would happen. Since this is the case and since this is implied but not completely clear unless reviewing more of the code, can you add a test case for the consumer side to make sure it has the specific pin in security policy instead of the glob? That way we can futureproof ourselves against side-effects on this security-sensitive code. Re more permissions: sure :) |
jdstrand
commented on skills/types/bool_file.go in 4098a64
Feb 8, 2016
|
This only allows write. I suspect 'r' is also desired and perhaps 'k' (locking). I'm not sure why 'l' (hard linking) is needed, but it is probably ok. |
|
Ah, I didn't know that As for reading, yeah, I'll add that back. For a moment I was treating bool files as "sinks" but I don't think we need to get there yet. |
jdstrand
commented on skills/types/bool_file.go in 4098a64
Feb 8, 2016
|
This allows '..' (though not '../'). |
|
Thanks for spotting this, would |
jdstrand
replied
Feb 9, 2016
|
[^/.]+ is not quite right I don't think unless we can be 100% sure that '.' is not a valid character for these. You could add a second check to see if '../' is present if that would be easier. |
zyga
added some commits
Feb 9, 2016
|
I've addressed every single comment made. Please have a 2nd look. NOTE: please don't merge this unless @jdstrand gives it a +1. |
jdstrand
reviewed
Feb 9, 2016
| + "^/sys/class/gpio/gpio[0-9]+/value$") | ||
| +var boolFileAllowedPathPatterns = []*regexp.Regexp{ | ||
| + // The brightness of standard LED class device | ||
| + regexp.MustCompile("^/sys/class/leds/[^/]+/brightness$"), |
jdstrand
reviewed
Feb 9, 2016
| + gpioSnippet := []byte(` | ||
| +/sys/class/gpio/export rw, | ||
| +/sys/class/gpio/unexport rw, | ||
| +/sys/class/gpio/gpio[0-9]+/direction rw, |
jdstrand
Feb 9, 2016
Contributor
Ok, but I reiterate my concerns regarding maintainability, auditability and getting entrenched with hard-coding.
jdstrand
reviewed
Feb 9, 2016
| + if err != nil { | ||
| + return nil, fmt.Errorf("cannot compute skill slot security snippet: %v", err) | ||
| + } | ||
| + return []byte(fmt.Sprintf("%s rwk,\n", path)), nil |
jdstrand
reviewed
Feb 9, 2016
| + }, PanicMatches, "skill is not sanitized") | ||
| +} | ||
| + | ||
| +func (s *BoolFileTypeSuite) TestSkillSecuritySnippetUnusedSecurtySystems(c *C) { |
jdstrand
reviewed
Feb 9, 2016
| + snippet, err = s.t.SlotSecuritySnippet(s.ledSkill, skills.SecurityApparmor) | ||
| + c.Assert(err, IsNil) | ||
| + c.Assert(snippet, DeepEquals, []byte( | ||
| + "(dereferenced)/sys/class/leds/input27::capslock/brightness rwk,\n")) |
jdstrand
Feb 9, 2016
Contributor
Can we add an additional check here to make sure that SkillSecuritySnippet doesn't end up in SlotSecuritySnippet's output to make sure we futureproof ourselves from accidentally giving too many permissions to the slot?
zyga
Feb 9, 2016
Contributor
Done, can you check if that is what you had on your mind? Look at TestSlotSecurityDoesNotContainSkillSecurity
zyga
added some commits
Feb 9, 2016
|
Please have another look. |
jdstrand
reviewed
Feb 10, 2016
| + } | ||
| + if strings.Contains(path, "..") { | ||
| + return fmt.Errorf("bool-file path cannot contain %q", "..") | ||
| + } |
jdstrand
reviewed
Feb 10, 2016
| + skillSnippet, err = s.t.SkillSecuritySnippet(s.gpioSkill, skills.SecurityApparmor) | ||
| + c.Assert(err, IsNil) | ||
| + // Ensure that we don't accidentally give skill-side permissions to slot-side. | ||
| + c.Assert(bytes.Contains(slotSnippet, skillSnippet), Equals, false) |
jdstrand
Feb 10, 2016
Contributor
Based on https://golang.org/pkg/bytes/#Contains, this looks great. Thanks!
|
Just so it doesn't get lost in comments, I'd like to reiterate my concerns regarding maintainability, auditability and getting entrenched with hard-coding the rules, but understand that is what was decided for the initial direction. +1 |
chipaca
reviewed
Feb 11, 2016
| +} | ||
| + | ||
| +func (t *BoolFileType) dereferencedPath(skill *skills.Skill) (string, error) { | ||
| + if path, ok := skill.Attrs["path"].(string); ok { |
chipaca
Feb 11, 2016
Member
As far as I can tell here skill.Attrs["path"] can come from "outside" (as skill is passed in unchanged from an exported function), so you need filepath.Clean here again, don't you?
zyga
Feb 11, 2016
Contributor
Fixed
Edit: though in practice those are sysfs symlinks but fixed anyway.
chipaca
reviewed
Feb 11, 2016
| +} | ||
| + | ||
| +// isGPIO checks if a given bool-file skill refers to a GPIO pin. | ||
| +func (t *BoolFileType) isGPIO(skill *skills.Skill) bool { |
zyga
added some commits
Feb 11, 2016
zyga commentedFeb 8, 2016
This patch adds the "bool-file" skill type. Boolean files can be used to
represent LEDs and GPIOs as files that react to writes of "0" or "1".
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com