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-seccomp,snap-confine-ns},osutil,interfaces/account_control: workaround unit test failures on Arch #4135
Conversation
zyga
reviewed
Nov 2, 2017
Thank you for attacking this problem.
Some low and high level comments inline.
| + // /etc/shadow and use the gid that owns it | ||
| + var shadowGid uint64 | ||
| + if gid, err := osutil.FindGid("shadow"); err != nil { | ||
| + var stat syscall.Stat_t |
| + var shadowGid uint64 | ||
| + if gid, err := osutil.FindGid("shadow"); err != nil { | ||
| + var stat syscall.Stat_t | ||
| + err := syscall.Stat("/etc/shadow", &stat) |
zyga
Nov 2, 2017
Contributor
I think we could twist this test a little by just stating /etc/shadow unconditionally and using whatever the group there for the rest of the code below. Alternatively use something that works on Arch as well and just carry on.
The real problem is in the account_control interface which grants fchown to u:root and g:shadow. This will fail at runtime and needs to be fixed to grant the actual group owner of /etc/shadow
bboozzoo
Nov 2, 2017
Contributor
Agreed, skipping the fallback and doing stat() unconditionally will make the code easier. I'll amend the PR.
codecov-io
commented
Nov 2, 2017
•
Edited 5 times
-
codecov-io
Nov 8, 2017
-
codecov-io
Nov 8, 2017
-
codecov-io
Nov 7, 2017
-
codecov-io
Nov 7, 2017
-
codecov-io
Nov 2, 2017
-
Nov 8, 2017 -
Nov 8, 2017 -
Nov 7, 2017 -
Nov 7, 2017 -
Nov 2, 2017
Codecov Report
@@ Coverage Diff @@
## master #4135 +/- ##
==========================================
+ Coverage 75.52% 75.67% +0.14%
==========================================
Files 436 437 +1
Lines 37811 37870 +59
==========================================
+ Hits 28558 28657 +99
+ Misses 7256 7209 -47
- Partials 1997 2004 +7
Continue to review full report at Codecov.
|
bboozzoo
added some commits
Nov 2, 2017
Some checks were not successful
|
Added one last patch to workaround missing 'nogroup' and rebased on top of c6d0682 |
bboozzoo
changed the title from
Workaround unit test failures on Arch
to
cmd/{snap-seccomp,snap-confine-ns},osutil,interfaces/account_control: workaround unit test failures on Arch
Nov 3, 2017
| @@ -63,6 +63,34 @@ func lookupGroup(groupname string) (string, error) { | ||
| return strconv.Itoa(int(grp.gr_gid)), nil | ||
| } | ||
| +// Use implementation from: | ||
| +// https://golang.org/src/os/user/cgo_lookup_unix.go |
bboozzoo
Nov 3, 2017
Contributor
you mean this one?
// from https://golang.org/src/os/user/user.go
type Group struct {
Gid string // group ID
Name string // group name
}
bboozzoo
Nov 3, 2017
Contributor
I pulled in os/user.Group as suggested. FindGroupOwning() was updated to return *Group, error, other calls were returning either gid or name, so I left those intact.
| + buf := alloc(groupBuffer) | ||
| + defer buf.free() | ||
| + err := retryWithBuffer(buf, func() syscall.Errno { | ||
| + // mygetgrgid_r is a wrapper around getgrgid_r to |
| + | ||
| + }) | ||
| + if err != nil { | ||
| + return "", fmt.Errorf("group: lookup groupid %d: %v", gid, err) |
chipaca
Nov 3, 2017
Member
"cannot lookup group %d: %v" or somesuch (key being the starting with "cannot")
| + return "", fmt.Errorf("group: lookup groupid %d: %v", gid, err) | ||
| + } | ||
| + if result == nil { | ||
| + return "", fmt.Errorf("group: unknown group %d", gid) |
chipaca
Nov 3, 2017
Member
errors should usually start with a cannot <do this thing you asked me to do>
| @@ -156,3 +184,28 @@ func FindGid(group string) (uint64, error) { | ||
| //return strconv.ParseUint(group.Gid, 10, 64) | ||
| return strconv.ParseUint(group, 10, 64) | ||
| } | ||
| + | ||
| +// FindGid returns the identifier of the given UNIX group name. |
mvo5
reviewed
Nov 3, 2017
Thanks for working on this. Two suggestions for missing tests otherwise very nice.
| - shadowGid, err := osutil.FindGid("shadow") | ||
| + // The 'shadow' group is different in different distributions, instead | ||
| + // of second guessing group name, look at the owner of /etc/shadow | ||
| + shadowGid, shadowGroup, err := osutil.FindGroupOwning("/etc/shadow") |
| @@ -156,3 +184,28 @@ func FindGid(group string) (uint64, error) { | ||
| //return strconv.ParseUint(group.Gid, 10, 64) | ||
| return strconv.ParseUint(group, 10, 64) | ||
| } | ||
| + | ||
| +// FindGid returns the identifier of the given UNIX group name. | ||
| +func FindGroup(gid uint64) (string, error) { |
mvo5
Nov 3, 2017
Collaborator
Can we write tests for this? A simple smoke test based on the local user or something?
zyga
Nov 3, 2017
Contributor
Ha, the reason this started was that there's just nothing sane about groups across systems.
Still +1 on a test.
bboozzoo
Nov 3, 2017
Contributor
Added some tests to cover find group ID/name and group owning a particular file.
| +} | ||
| + | ||
| +// FindGroupOwning obtains UNIX group ID and name owning file `path`. | ||
| +func FindGroupOwning(path string) (uint64, string, error) { |
mvo5
Nov 3, 2017
Collaborator
And this as well? Again, something simple is enough for me. E.g. create a file, ensure its the group of the user running the test and call this helper or something like this?
bboozzoo
added some commits
Nov 3, 2017
Some checks were not successful
Some checks were not successful
|
I will update the test to use Thoughts? |
|
@bboozzoo to expand on the which binaries need to be statically linked. This is a bit more subtle and complex. The Ubuntu 16.04 build is special as it is used to construct the core snap which is participating in re-execution in many places. On Arch we don't really need to link anything statically until we are ready to use base snaps where the base snap transition in the execution environment will pose a problem. At that time we should be able to use snapd from core so again, we don't really strongly need that. |
bboozzoo
added some commits
Nov 7, 2017
Some checks were not successful
chipaca
reviewed
Nov 7, 2017
Loos good! Just minor nits.
If I make the same point twice below you'll know github is weird(er).
| - | ||
| - c.Check(group.Gid, Equals, self.Gid) | ||
| + if c.Check(err, IsNil) { | ||
| + c.Check(group.Gid, Equals, self.Gid) |
| + | ||
| +func (s *groupSuite) TestSelfOwnedFile(c *C) { | ||
| + self, err := RealUser() | ||
| + c.Check(err, IsNil) |
chipaca
Nov 7, 2017
Member
in general you'd use Assert on errors, unless the error doesn't affect the rest of the test; here I think it very much does
| + defer os.Remove(name) | ||
| + | ||
| + group, err := FindGroupOwning(name) | ||
| + if c.Check(err, IsNil) { |
Some checks were not successful
|
Do we need another round of reviews or is this good to be merged now? |
|
I'll do second review shortly (just woke up) |
mvo5
approved these changes
Nov 8, 2017
This looks very nice, thanks for doing these fixes. I have some nitpick comments inside about conventions/style etc. Please check them out but I'm fine merging this and doing the style fixes in a followup.
| - e = &mount.Entry{Options: []string{"x-snapd.gid=nogroup"}} | ||
| + var nogroup string | ||
| + var nogroupGid uint64 | ||
| + // try to find a suitable 'nogroup' like group |
mvo5
Nov 8, 2017
Collaborator
(nitpick) Maybe expand the comment a tiny bit? Something like: try to find a suitable 'nogroup' like group, e.g. arch uses "nobody"
| +func (iface *accountControlInterface) SecCompConnectedPlug(spec *seccomp.Specification, plug *interfaces.Plug, Attrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error { | ||
| + if snippet, err := makeAccountControlSecCompSnippet(); err != nil { | ||
| + return err | ||
| + } else { |
mvo5
Nov 8, 2017
Collaborator
(nitpick) we don't need the } else { here our convention is like: foo, err := thing()\n if err != nil {return err}\nuse(thing). So something like the lines below will be more idiomatic for our code base:
snippet, err := makeAccountControlSecCompSnippet()
if err != nil {
return err
}
spec.AddSnippet(snippet)
return nil
is fine.
| + * | ||
| + */ | ||
| + | ||
| +package osutil |
mvo5
Nov 8, 2017
Collaborator
(nitpick) we use a slightly different pattern when writing (most) unit tests. The package is called foo_test instead of foo which forces to use only the public interfaces of the thing that gets tested. This is similar to what the go upstream source is using. If internal access is needed we use a export_test.go file that is part of package "foo" to only export things for tests. For this particular test changes should be minimal as it is only testing public functions. So just renaming the package from osutil to osutil_test should be enough (plus import of osutil and prefixing the methods).
| + . "gopkg.in/check.v1" | ||
| +) | ||
| + | ||
| +type groupSuite struct { |
mvo5
Nov 8, 2017
Collaborator
(nitpick^2) empty structs are mostly written type groupSuite struct {}, i.e. no extra newline. We don't always follow that rule but mostly :)
| +type groupSuite struct { | ||
| +} | ||
| + | ||
| +var _ = Suite(&groupSuite{}) |
bboozzoo
added some commits
Nov 8, 2017
Some checks were not successful
Some checks were not successful
|
Merge? (once the tests are green) |
|
once green, go for it |
This was referenced Nov 8, 2017
|
Interesting, travis failed with: linode:fedora-25-64:tests/main/classic-custom-device-reg error is: linode:ubuntu-core-16-64:tests/main/snap-auto-mount error: |
|
Closing this for now. As discussed in the meetng, we'll try to kill most of |
bboozzoo commentedNov 2, 2017
There is no 'shadow' group in ArchLinux, what causes a failure in
cmd/snap-seccomp.TestCompile. Try to workaround the problem by looking at owning gid of/etc/shadowand figuring out the corresponding group name based on this.As a side effect, a helper for looking up group name by gid was added. The implementation is based on similar code in https://tip.golang.org/src/os/user/cgo_lookup_unix.go
@zyga can you take a look?