interfaces/{default,account-control}: Use username/group instead of uid/gid

[jdstrand]

This is branched off #3804.

[zyga]

Have a look at my comment, I didn't review more than this.

[zyga]

Interestingly this will start failing unless user lookup is done inside the mount namespace with /etc/group from the core snap. On my suse box I see this:

cat /etc/group | grep shadow
shadow:x:15:

But compare with:

cat /snap/core/current/etc/group | grep shadow
shadow:x:42:

So -1 unless we ensure this is done correctly.

[jdstrand]

But on classic /etc is the /etc from the host, not the core snap, so we should always have the same uid/gid (in fact, this change is precisely to address the issue with suse and us hardcoding 42).

That said, I'll need to look at the testsuite to fix TestCompile on suse.

[zyga]

At runtime, a strictly confined snap will fail to chown shdow ... because shadow will resolve to 42 but the generated seccomp profile, compiled from the "outside" will allow 15. What am I missing in my reasoning?

[jdstrand]

We discussed this on IRC (my previous comment stands).

[codecov-io]

Codecov Report

Merging #3805 into master will decrease coverage by 0.12%.
The diff coverage is 73.91%.

@@            Coverage Diff             @@
##           master    #3805      +/-   ##
==========================================
- Coverage   75.81%   75.69%   -0.13%     
==========================================
  Files         402      407       +5     
  Lines       34793    35185     +392     
==========================================
+ Hits        26380    26634     +254     
- Misses       6540     6668     +128     
- Partials     1873     1883      +10

Impacted Files	Coverage Δ
interfaces/builtin/account_control.go	100% <ø> (ø)	⬆️
cmd/snap-seccomp/main.go	56.49% <73.91%> (+6.7%)	⬆️
cmd/snap/cmd_aliases.go	93.33% <0%> (-1.67%)	⬇️
interfaces/builtin/i2c.go	65.95% <0%> (-1.39%)	⬇️
interfaces/builtin/network_manager.go	76.92% <0%> (-1.13%)	⬇️
interfaces/builtin/iio.go	69.64% <0%> (-0.54%)	⬇️
daemon/api.go	72.45% <0%> (-0.18%)	⬇️
cmd/snap/cmd_snap_op.go	63.63% <0%> (-0.11%)	⬇️
interfaces/builtin/fuse_support.go	100% <0%> (ø)	⬆️
interfaces/builtin/framebuffer.go	100% <0%> (ø)	⬆️
... and 32 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a96aa5...7846e7c. Read the comment docs.

[mvo5]

Nice work! One question inline but only because I'm curious, no blocker in any sense.

[mvo5]

Curious, why do we need this wrapper? Doesn't cgo find getgrnam_r without it? From a quick test (on amd64/zesty only) it seems there is no harm when I remove it.

[jdstrand]

@mvo5 - again, I literally copied this from upstream's golang code. I did not analyze it extensively or try to improve it cause I felt it important to have it operate exactly like upstream when we switch over. If people want me to reimplement this, I can, but I'm not sure I see the benefit because we'll lose all that when we switch over. user.Lookup already exists, user.LookupGroup does not yet, but it will.

[mvo5]

Thanks for this explanation. Sorry, I was not aware that you answered this already :)

[jdstrand]

@mvo5 - well, the 'again' wasn't so much for this portion, but for other parts-- but the rationale was the same in my head. Plus that happened in 3804 (which this is based off of), so easy to miss. :)

[chipaca]

generally LGTM; a couple of nits and a wrongly-named (and placed) testing function.

[chipaca]

nitpick: %q, not "%s"

[jdstrand]

Done

[chipaca]

nitpick: %q, not "%s"

[jdstrand]

Done

[chipaca]

why is this called MockFindGid, if ti calls the actual findGid?

[chipaca]

I'd expect this to be called just FindGid, and to be in export_test.go.

[jdstrand]

Discussed on IRC. I was having trouble with accessing findGid from the tests bug I've fixed that. Thanks for the review! :)

[jdstrand]

Done

[jdstrand]

Thanks for the reviews! :)