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: support parsing 'u:' and 'g:' for username and groups #3804
Conversation
jdstrand
added some commits
Aug 24, 2017
Some checks were not successful
jdstrand
referenced this pull request
Aug 24, 2017
Merged
interfaces/{default,account-control}: Use username/group instead of uid/gid #3805
zyga
reviewed
Aug 25, 2017
Looks okay in general but I have questions about memory management and about possibly-blocking user and group lookups when using networked systems.
| @@ -135,6 +136,11 @@ package main | ||
| // return htobe64(val); | ||
| //} | ||
| // | ||
| +//static int mygetgrnam_r(const char *name, struct group *grp,char *buf, | ||
| +// size_t buflen, struct group **result) { | ||
| +// return getgrnam_r(name, grp, buf, buflen, result); |
zyga
Aug 25, 2017
Contributor
I wonder if we should take any special considerations for any networked systems. This can block for any amount of time, correct?
jdstrand
Aug 25, 2017
Contributor
IMO, no because everything we will be using it for will be in the local databases. Systems with network NSS modules always include the local database module first so that the system can boot up or function when the system is down. Consider debugging a broken system with no networking-- you need to be able to sudo/su. Not to mention it would be a terribly non-performant system if it had to reach out over the network for 'root' or 'adm'.
Keeping that in mind, here are the only places we are going to use this:
- 'root'
- 'daemon'
- 'shadow'
- snapd managed users
For 1 and 2, both are defined in the LSB as being required on any Linux system (http://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/usernames.html)
For 3, the shadow database is in place everywhere in practice. If snapd is ported to a system that uses a different name, it is easy to adjust account-control to do if isNonStandardDistro then use nonstandard-shadow-group. We can cross that bridge when we get there.
For 4, snapd will be adding users to the local system via local system commands (eg, useradd and groupadd)
jdstrand
Aug 25, 2017
Contributor
"Keeping that in mind, here are the only places we are going to use this"
Note, it is technically possible we might introduce an interface to allow modifying something in /etc, like we do for shadow, but that would be exactly the same as for shadow.
| +func alloc(kind bufferKind) *memBuffer { | ||
| + sz := kind.initialSize() | ||
| + return &memBuffer{ | ||
| + ptr: C.malloc(sz), |
| +} | ||
| + | ||
| +func (mb *memBuffer) resize(newSize C.size_t) { | ||
| + mb.ptr = C.realloc(mb.ptr, newSize) |
zyga
Aug 25, 2017
Contributor
Do we need to handle realloc failing here? (In which case the original pointer is not free'd). Also when newSize is zero (which is a valid size) NULL MAY be returned.
| + } | ||
| + newSize := buf.size * 2 | ||
| + if !isSizeReasonable(int64(newSize)) { | ||
| + return fmt.Errorf("internal buffer exceeds %d bytes", maxBufferSize) |
|
Please |
|
@zyga - note that I literally copied upstream's user.LookupGroup that is in master with only the slightest changes (eg, I didn't pull back user.Group). In other words, this implementation is going to behave exactly like user.Lookup(). The reason being when user.LookupGroup is available, we'd immediately go to that. |
zyga
dismissed
their
stale review
Aug 25, 2017
I'm re-considering my review after discussion on IRC
|
"Marking as -1 until we discuss and address the issue also found in #3805" Please see #3805 (comment) |
mvo5
and others
added some commits
Aug 25, 2017
Some checks were not successful
Some checks were not successful
codecov-io
commented
Aug 25, 2017
Codecov Report
@@ Coverage Diff @@
## master #3804 +/- ##
==========================================
- Coverage 75.81% 75.81% -0.01%
==========================================
Files 402 402
Lines 34793 34885 +92
==========================================
+ Hits 26380 26449 +69
- Misses 6540 6559 +19
- Partials 1873 1877 +4
Continue to review full report at Codecov.
|
mvo5
merged commit 7954c38
into
snapcore:master
Aug 30, 2017
6 of 7 checks passed
|
Thanks for the reviews! :) |
jdstrand commentedAug 24, 2017
This adds support in snap-seccomp to parse 'u:' and 'g:' in the policy so we can do things like:
This is a required building block for security policy support for https://forum.snapcraft.io/t/multiple-users-and-groups-in-snaps/1461. It will also be the basis for allowing use of the 'daemon' user and group by default (see https://bugs.launchpad.net/snappy/+bug/1619888/comments/6) for privilege dropping, etc.