/snapd

Bugfix/classic sudo #266

Merged
merged 10 commits into from Jan 7, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@mvo5 @elopio @niemeyer @chipaca @pedronis
@mvo5
Collaborator

mvo5 commented Dec 18, 2015

This branch fixes two issues:

  • the user in the classic environment is not in the "sudo" group so the sudo command won't work
  • when enabling classic fails for whatever reason the broken/incomplete dir is now removed.
  • pass the environment to the child when re-execing with sudo

mvo5 added some commits Dec 18, 2015

@mvo5
Add "$SUDO_USER" to the "sudo" group in the classic environment 
The $SUDO_USER in classic is not in the sudo group by default.
We enable it at classic creation time to ensure that sudo works
as expected.
dd52a13
@mvo5
Remove classic dir if "create" fails 
If creating of the classic environment fails for whatever reason
we need to remove it instead of leaving a broken classic directory
around.
522102e
@mvo5
Pass the parent environemnt when re-execing for "sudo" 
The "snappy shell classic" command checks if it is running as
a normal user and if that is the case it will re-exec itself
with the "sudo" command. Its using syscall.Exec() for this.

In order to have a working environment (so that e.g. TERM is
correct) we now pass the parent environment.

All checks have passed

3 successful checks
@snappy-m-o
Integration tests — Success 54 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.03%) to 66.987%
Details
b65cdc1
classic/create.go
+ return fmt.Errorf("failed to add %s to the sudo users: %s", sudoUser, err)
+ }
+ }
+
@pedronis

pedronis Dec 18, 2015

Contributor

maybe I'm not understanding but shouldn't we copy the whole sudo group from outside to inside? it feels a bit strange to copy exactly the user that created the classic env

@mvo5

mvo5 Dec 21, 2015

Collaborator

Yes we should. I fixed that now, the lack of getgrnam() put me off a bit when I did the initial implementation. But you are correct, we should consider all users.

@niemeyer

niemeyer Jan 5, 2016

Contributor

Not sure about what's the vanilla state of an installed system these days, but FWIW my main local user has local access because I'm in the admin group, rather than sudo. Both are in sudoers by default, IIRC. Again, not a very reliable data point, but makes me wonder about what the plain image in 16.04 is likely to have.

@mvo5

mvo5 Jan 5, 2016

Collaborator

@niemeyer On older installs there is indeed the "admin" group instead of the (newer) "sudo" group. This is not be a problem for us because we control how the snappy image is created. Currently this is the default user permission: http://bazaar.launchpad.net/~ubuntu-core-dev/livecd-rootfs/trunk/view/head:/live-build/ubuntu-core/hooks/01-setup_user.chroot (adm, sudo). If people add users manually we will honor the groups of those users too this way on install of classic.

mvo5 added some commits Dec 21, 2015

@mvo5
Implement new "group" package that wraps getgrnam_r() 
The classic mode will need support for reading all group members
of the "sudo" group. There is no native getgrnam() in golang, so
this package implements that.
76f3536
@mvo5
Add all users from the snappy "sudo" group in classic 
Lookup all members of the sudo group in the snappy system
and add them to the classic environment.

Some checks were not successful

2 failing and 1 successful checks
@snappy-m-o
Integration tests — 20 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.03%) to 67.045%
Details
534512a
@mvo5
Do not build group_other.go on linux,darwin,freebsd

Some checks were not successful

2 failing and 1 successful checks
@snappy-m-o
Integration tests — 20 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.001%) to 67.021%
Details
9648e68
@elopio
Member

elopio commented Jan 4, 2016

This is a bug fix, so can you please report the bugs in launchpad with steps to reproduce them?
@mvo5
Collaborator

mvo5 commented Jan 5, 2016

@elopio thanks! https://bugs.launchpad.net/snappy/+bug/1530827 has steps to reproduce it now
@mvo5
Collaborator

mvo5 commented Jan 5, 2016

retest this please
@mvo5
Collaborator

mvo5 commented Jan 5, 2016

Fwiw, the travis failure is a panic in "go vet" from go tip - I tried to reproduce this here but with the version of go vet from 5. Jan I do not get the error. Travis is using the go vet version 19. Dec for some reason.
group/group_linux.go
+
+ p := unsafe.Pointer(result.gr_mem)
+ for {
+ if (*(**C.char)(p)) == nil {
@niemeyer

niemeyer Jan 5, 2016

Contributor

The conditional can go into the for statement above instead. I'd also check that p itself isn't nil, just for extra safety. Shouldn't happen, but good practice before dereferencing a pointer from an external API.

group/group_linux_test.go
+ cmd := exec.Command("getent", "group", name)
+ output, err := cmd.Output()
+ if err != nil {
+ return grp, fmt.Errorf("getenv failed for %s: %s", name, err)
@niemeyer

niemeyer Jan 5, 2016

Contributor

s/env/ent/

@mvo5
Simplify getgrnam(), thanks to Gustavo

All checks have failed

2 failing checks
@snappy-m-o
Integration tests — 20 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
d62f0bd
group/group_linux_test.go
+ c.Assert(err, IsNil)
+ groups, err := Getgrnam("adm")
+ c.Assert(err, IsNil)
+ c.Assert(expected, DeepEquals, groups)
@niemeyer

niemeyer Jan 5, 2016

Contributor

FWIW, the order of arguments is reversed here: Assert(obtained, DeepEquals, expected)

group/group.go
+ *
+ */
+
+package group
@niemeyer

niemeyer Jan 5, 2016

Contributor

Feels slightly too fine grained to have a package just for this. Do we have some sort of "osutil" package? Should we have one?

@mvo5

mvo5 Jan 5, 2016

Collaborator

I have no strong opinion either way, happy to rename to "osutil".

@mvo5
Fix review comments (thanks Gustavo!)

Some checks were not successful

2 failing and 1 successful checks
@snappy-m-o
Integration tests — 20 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.4%) to 67.402%
Details
6bb2197
@niemeyer
Contributor

niemeyer commented Jan 5, 2016

A few comments, but overall LGTM.

mvo5 added some commits Jan 6, 2016

@mvo5
Rename the "group" package to "osutil" (thanks Gustavo)

Some checks were not successful

2 failing and 1 successful checks
@snappy-m-o
Integration tests — 20 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.4%) to 67.402%
Details
a9b0e63
@mvo5
Merge remote-tracking branch 'upstream/master' into bugfix/classic-sudo

Some checks were not successful

1 failing and 2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@snappy-m-o
Integration tests — Success 54 tests run, 0 skipped, 0 failed.
Details
coverage/coveralls — Coverage decreased (-0.003%) to 67.386%
Details
c60fcdb
@mvo5
Collaborator

mvo5 commented Jan 6, 2016

Thanks for the excellent feedback! I addressed the comments. Please let me know if there is anything else I can do.
@niemeyer
Contributor

niemeyer commented Jan 6, 2016

Thanks Michael! LGTM
@chipaca
Member

chipaca commented Jan 6, 2016

👍

mvo5 added a commit that referenced this pull request Jan 7, 2016

@mvo5
Merge pull request #266 from mvo5/bugfix/classic-sudo 
Bugfix/classic sudo
1282a18

@mvo5 mvo5 merged commit 1282a18 into snapcore:master Jan 7, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Integration tests Success 54 tests run, 0 skipped, 0 failed.
Details
coverage/coveralls Coverage decreased (-0.003%) to 67.386%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment