Skip to content

Commit

Permalink
move uid/gid checks to osutil.EnsureUserGroup(). Thanks to pedronis
Browse files Browse the repository at this point in the history
  • Loading branch information
Jamie Strandboge committed Aug 15, 2019
1 parent 1daac64 commit 1527e50
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 138 deletions.
48 changes: 43 additions & 5 deletions osutil/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,58 @@ type AddUserOptions struct {
// allows as valid usernames
var IsValidUsername = regexp.MustCompile(`^[a-z0-9][-a-z0-9+._]*$`).MatchString

// UserGroupAdd uses the standard shadow utilities' 'useradd' and 'groupadd'
// EnsureUserGroup uses the standard shadow utilities' 'useradd' and 'groupadd'
// commands for creating non-login system users and groups that is portable
// cross-distro. It will create the group with groupname 'name' and gid 'id' as
// well as the user with username 'name' and uid 'id'. Importantly, 'useradd'
// and 'groupadd' will use NSS to determine if a uid/gid is already assigned
// (so LDAP, etc are consulted), but will themselves only add to local files,
// which is exactly what we want since we don't want snaps to be blocked on
// LDAP, etc lookups.
func UserGroupAdd(name string, id uint32, extraUsers bool) error {
func EnsureUserGroup(name string, id uint32, extraUsers bool) error {
if !IsValidUsername(name) {
return fmt.Errorf("cannot add user/group %q: name contains invalid characters", name)
return fmt.Errorf(`cannot add user/group %q: name contains invalid characters`, name)
}

// useradd --user-group will choose a gid from the range defined in
// login.defs, so first call groupadd and use --gid with useradd.
uid, uidErr := FindUid(name)
if uidErr != nil {
if _, ok := uidErr.(user.UnknownUserError); !ok {
return uidErr
}
}

gid, gidErr := FindGid(name)
if gidErr != nil {
if _, ok := gidErr.(user.UnknownGroupError); !ok {
return gidErr
}
}

if uidErr == nil && gidErr == nil {
if uid != uint64(id) {
return fmt.Errorf(`found unexpected uid for user %q: %d`, name, uid)
} else if gid != uint64(id) {
return fmt.Errorf(`found unexpected gid for group %q: %d`, name, gid)
}
// found the user and group with expected values
return nil
}

// If the user and group do not exist, snapd will create both, so if
// the admin removed one of them, error and don't assume we can just
// add the missing one
if uidErr != nil && gidErr == nil {
return fmt.Errorf(`cannot add user/group %q: group exists and user does not`, name)
} else if uidErr == nil && gidErr != nil {
return fmt.Errorf(`cannot add user/group %q: user exists and group does not`, name)
}

// At this point, we know that the user and group don't exist, so
// create them.

// First create the group. useradd --user-group will choose a gid from
// the range defined in login.defs, so first call groupadd and use
// --gid with useradd.
groupCmdStr := []string{
"groupadd",
"--system",
Expand Down Expand Up @@ -109,6 +146,7 @@ func UserGroupAdd(name string, id uint32, extraUsers bool) error {

cmd = exec.Command(userCmdStr[0], userCmdStr[1:]...)
if output, err := cmd.CombinedOutput(); err != nil {
// TODO: call delgroup
return fmt.Errorf("useradd failed with: %s", OutputErr(output, err))
}

Expand Down
20 changes: 10 additions & 10 deletions osutil/user_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,8 @@ func (s *createUserSuite) TestIsValidUsername(c *check.C) {
}
}

func (s *createUserSuite) TestUserGroupAddExtraUsersFalse(c *check.C) {
err := osutil.UserGroupAdd("lakatos", 123456, false)
func (s *createUserSuite) TestEnsureUserGroupExtraUsersFalse(c *check.C) {
err := osutil.EnsureUserGroup("lakatos", 123456, false)
c.Assert(err, check.IsNil)

c.Check(s.mockGroupAdd.Calls(), check.DeepEquals, [][]string{
Expand All @@ -296,8 +296,8 @@ func (s *createUserSuite) TestUserGroupAddExtraUsersFalse(c *check.C) {
})
}

func (s *createUserSuite) TestUserGroupAddExtraUsersTrue(c *check.C) {
err := osutil.UserGroupAdd("lakatos", 123456, true)
func (s *createUserSuite) TestEnsureUserGroupExtraUsersTrue(c *check.C) {
err := osutil.EnsureUserGroup("lakatos", 123456, true)
c.Assert(err, check.IsNil)

c.Check(s.mockGroupAdd.Calls(), check.DeepEquals, [][]string{
Expand All @@ -308,23 +308,23 @@ func (s *createUserSuite) TestUserGroupAddExtraUsersTrue(c *check.C) {
})
}

func (s *createUserSuite) TestUserGroupAddBadUser(c *check.C) {
err := osutil.UserGroupAdd("k!", 123456, false)
func (s *createUserSuite) TestEnsureUserGroupBadUser(c *check.C) {
err := osutil.EnsureUserGroup("k!", 123456, false)
c.Assert(err, check.ErrorMatches, `cannot add user/group "k!": name contains invalid characters`)
}

func (s *createUserSuite) TestUserGroupAddFailedGroupadd(c *check.C) {
func (s *createUserSuite) TestEnsureUserGroupFailedGroupadd(c *check.C) {
mockGroupAdd := testutil.MockCommand(c, "groupadd", "echo some error; exit 1")
defer mockGroupAdd.Restore()

err := osutil.UserGroupAdd("lakatos", 123456, false)
err := osutil.EnsureUserGroup("lakatos", 123456, false)
c.Assert(err, check.ErrorMatches, "groupadd failed with: some error")
}

func (s *createUserSuite) TestUserGroupAddFailedUseradd(c *check.C) {
func (s *createUserSuite) TestEnsureUserGroupFailedUseradd(c *check.C) {
mockUserAdd := testutil.MockCommand(c, "useradd", "echo some error; exit 1")
defer mockUserAdd.Restore()

err := osutil.UserGroupAdd("lakatos", 123456, false)
err := osutil.EnsureUserGroup("lakatos", 123456, false)
c.Assert(err, check.ErrorMatches, "useradd failed with: some error")
}
51 changes: 15 additions & 36 deletions overlord/snapstate/check_snap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -571,10 +571,7 @@ func earlyEpochCheck(info *snap.Info, snapst *SnapState) error {
}

// check that the listed system users are valid
var (
findUid = osutil.FindUid
findGid = osutil.FindGid
)
var ensureUserGroup = osutil.EnsureUserGroup

func checkSystemUsernames(si *snap.Info) error {
// No need to check support if no system-usernames
Expand Down Expand Up @@ -604,6 +601,7 @@ func checkSystemUsernames(si *snap.Info) error {
return err
}

extrausers := !release.OnClassic
for _, user := range si.SystemUsernames {
var id uint32
id, ok := supportedSystemUsernames[user.Name]
Expand All @@ -613,44 +611,25 @@ func checkSystemUsernames(si *snap.Info) error {

switch user.Scope {
case "shared":
_, uidErr := findUid(user.Name)
_, gidErr := findGid(user.Name)
if uidErr != nil && gidErr != nil {
// If neither exist, create the user and group
extrausers := !release.OnClassic
err := osutil.UserGroupAdd(user.Name, id, extrausers)
if err != nil {
return err
}
} else if uidErr != nil || gidErr != nil {
return fmt.Errorf(`snap %q requires that both the "%s" system user and group are present on the system.`, si.InstanceName(), user.Name)
// Create the snap-range-<base>-root user and group so
// systemd-nspawn can avoid our range. Our ranges will always
// be in 65536 chunks, so mask off the lower bits to obtain our
// base (see above)
rangeStart := id & 0xFFFF0000
rangeName := fmt.Sprintf("snap-range-%d-root", rangeStart)
if err := ensureUserGroup(rangeName, rangeStart, extrausers); err != nil {
return fmt.Errorf(`snap %q requires system username "%s": %v`, si.InstanceName(), user.Name, err)
}

// Create the requested user and group
if err := ensureUserGroup(user.Name, id, extrausers); err != nil {
return fmt.Errorf(`snap %q requires system username "%s": %v`, si.InstanceName(), user.Name, err)
}
case "private", "external":
return fmt.Errorf(`snap %q requires unsupported user scope "%s" for this version of snapd`, si.InstanceName(), user.Scope)
default:
return fmt.Errorf(`snap %q requires unsupported user scope "%s"`, si.InstanceName(), user.Scope)
}

// Create the snap-range-<base>-root user and group so
// systemd-nspawn can avoid our range. Our ranges will always
// be in 65536 chunks, so mask off the lower bits to obtain our
// base (see above)
rangeStart := id & 0xFFFF0000
rangeName := fmt.Sprintf("snap-range-%d-root", rangeStart)
_, uidErr := findUid(rangeName)
_, gidErr := findGid(rangeName)
if uidErr != nil && gidErr != nil {
// If neither exist, create the user and group
extrausers := !release.OnClassic
err := osutil.UserGroupAdd(rangeName, rangeStart, extrausers)
if err != nil {
return err
}
} else if uidErr != nil {
return fmt.Errorf(`snap %q requires id-collision-detector "%s" system user`, si.InstanceName(), rangeName)
} else if gidErr != nil {
return fmt.Errorf(`snap %q requires id-collision-detector "%s" system group`, si.InstanceName(), rangeName)
}
}
return nil
}
Expand Down
97 changes: 20 additions & 77 deletions overlord/snapstate/check_snap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -926,14 +926,12 @@ func (s *checkSnapSuite) TestCheckSnapdHappy(c *C) {

// Note, invalid usernames checked in snap/info_snap_yaml.go
var systemUsernamesTests = []struct {
sysIDs string
classic bool
noGroup bool
noUser bool
noRangeUser bool
noRangeGroup bool
scVer string
error string
sysIDs string
classic bool
noRangeUser bool
noUser bool
scVer string
error string
}{{
sysIDs: "snap_daemon: shared",
scVer: "dead 2.4.1 deadbeef bpf-actlog",
Expand Down Expand Up @@ -984,39 +982,17 @@ var systemUsernamesTests = []struct {
scVer: "dead 2.4.1 deadbeef bpf-actlog",
classic: true,
error: `requires unsupported system username "allowed-not"`,
}, {
sysIDs: "snap_daemon: shared",
noGroup: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires that both the \"snap_daemon\" system user and group are present on the system.`,
}, {
sysIDs: "snap_daemon: shared",
classic: true,
noGroup: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires that both the \"snap_daemon\" system user and group are present on the system.`,
}, {
sysIDs: "snap_daemon: shared",
noUser: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires that both the \"snap_daemon\" system user and group are present on the system.`,
error: `requires system username "snap_daemon": cannot add user/group "snap_daemon", group exists and user does not`,
}, {
sysIDs: "snap_daemon: shared",
classic: true,
noUser: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires that both the \"snap_daemon\" system user and group are present on the system.`,
}, { // this triggers osutil.UserGroupAdd() where we'll mock useradd/groupadd
sysIDs: "snap_daemon: shared",
noUser: true,
noGroup: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
}, { // this triggers osutil.UserGroupAdd() where we'll mock useradd/groupadd
sysIDs: "snap_daemon: shared",
classic: true,
noUser: true,
noGroup: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires system username "snap_daemon": cannot add user/group "snap_daemon", group exists and user does not`,
}, {
sysIDs: "snap_daemon: shared",
scVer: "dead 2.3.3 deadbeef bpf-actlog",
Expand All @@ -1042,78 +1018,45 @@ var systemUsernamesTests = []struct {
classic: true,
scVer: "dead 2.4.1 deadbeef -",
error: `system usernames require a snapd built against golang-seccomp >= 0.9.1`,
}, {
sysIDs: "snap_daemon: shared",
noRangeGroup: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires id-collision-detector "snap-range-524288-root" system group`,
}, {
sysIDs: "snap_daemon: shared",
classic: true,
noRangeGroup: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires id-collision-detector "snap-range-524288-root" system group`,
}, {
sysIDs: "snap_daemon: shared",
noRangeUser: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires id-collision-detector "snap-range-524288-root" system user`,
error: `requires system username "snap_daemon": cannot add user/group "snap-range-524288-root", group exists and user does not`,
}, {
sysIDs: "snap_daemon: shared",
classic: true,
noRangeUser: true,
scVer: "dead 2.4.1 deadbeef bpf-actlog",
error: `requires id-collision-detector "snap-range-524288-root" system user`,
error: `requires system username "snap_daemon": cannot add user/group "snap-range-524288-root", group exists and user does not`,
}}

func (s *checkSnapSuite) TestCheckSnapSystemUsernames(c *C) {
restore := release.MockOnClassic(false)
defer restore()

mockGroupAdd := testutil.MockCommand(c, "groupadd", "exit 0")
defer mockGroupAdd.Restore()
mockUserAdd := testutil.MockCommand(c, "useradd", "exit 0")
defer mockUserAdd.Restore()

for _, test := range systemUsernamesTests {
restore = interfaces.MockSeccompCompilerVersionInfo(func(_ string) (string, error) {
return test.scVer, nil
})
defer restore()

release.OnClassic = test.classic
if test.noGroup {
restore = snapstate.MockFindGid(func(name string) (uint64, error) {
return 0, fmt.Errorf("user: unknown group %s", name)
})
} else if test.noRangeGroup {
restore = snapstate.MockFindGid(func(name string) (uint64, error) {
if name == "snap_daemon" {
return 123, nil
}
return 0, fmt.Errorf("user: unknown group %s", name)
})
} else {
restore = snapstate.MockFindGid(func(name string) (uint64, error) {
return 123, nil
})
}
defer restore()

if test.noUser {
restore = snapstate.MockFindUid(func(name string) (uint64, error) {
return 0, fmt.Errorf("user: unknown user %s", name)
if test.noRangeUser {
restore = snapstate.MockEnsureUserGroup(func(name string, id uint32, extraUsers bool) error {
return fmt.Errorf(`cannot add user/group "%s", group exists and user does not`, name)
})
} else if test.noRangeUser {
restore = snapstate.MockFindUid(func(name string) (uint64, error) {
if name == "snap_daemon" {
return 123, nil
} else if test.noUser {
restore = snapstate.MockEnsureUserGroup(func(name string, id uint32, extraUsers bool) error {
if name == "snap-range-524288-root" {
return nil
}
return 0, fmt.Errorf("user: unknown user %s", name)
return fmt.Errorf(`cannot add user/group "%s", group exists and user does not`, name)
})
} else {
restore = snapstate.MockFindUid(func(name string) (uint64, error) {
return 124, nil
restore = snapstate.MockEnsureUserGroup(func(name string, id uint32, extraUsers bool) error {
return nil
})
}
defer restore()
Expand Down
14 changes: 4 additions & 10 deletions overlord/snapstate/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,10 @@ func MockPrerequisitesRetryTimeout(d time.Duration) (restore func()) {
return func() { prerequisitesRetryTimeout = old }
}

func MockFindUid(mock func(name string) (uint64, error)) (restore func()) {
old := findUid
findUid = mock
return func() { findUid = old }
}

func MockFindGid(mock func(name string) (uint64, error)) (restore func()) {
old := findGid
findGid = mock
return func() { findGid = old }
func MockEnsureUserGroup(mock func(name string, id uint32, extraUsers bool) error) (restore func()) {
old := ensureUserGroup
ensureUserGroup = mock
return func() { ensureUserGroup = old }
}

var (
Expand Down

0 comments on commit 1527e50

Please sign in to comment.