Merge lp:~thumper/juju-core/local-sudo-caller into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Add some extra tests around the sudo checks.

Encapsulate the sudo checks, and make a function to
get the uid and gid for the user.

https://codereview.appspot.com/11321043/

Add some extra tests around the sudo checks.

Encapsulate the sudo checks, and make a function to
get the uid and gid for the user.

https://codereview.appspot.com/11321043/

[Tim Penhey (thumper)]

Reviewers: mp+174904_code.launchpad.net,

Message:
Please take a look.

Description:
Add some extra tests around the sudo checks.

Encapsulate the sudo checks, and make a function to
get the uid and gid for the user.

https://code.launchpad.net/~thumper/juju-core/local-sudo-caller/+merge/174904

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11321043/

Affected files:
   A [revision details]
   M environs/local/config.go
   M environs/local/config_test.go
   M environs/local/export_test.go

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode16
environs/local/config.go:16: var rootCheckFunction = func() bool {
Perhaps checkRoot() is better?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode86
environs/local/config.go:86: // change ownership of the directories.
I don't think the above comment belongs here

https://codereview.appspot.com/11321043/

[John A Meinel (jameinel)]

LGTM

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode16
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 03:56:35, wallyworld wrote:
> Perhaps checkRoot() is better?

checkIfRoot ?

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go
File environs/local/config_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go#newcode87
environs/local/config_test.go:87: defer
local.SetRootCheckFunction(rootCheck)
I would probably label this origRootCheck or something to that effect,
so it is immediately clear that this was the value before setting it
without having to dig up the return value of SetRootCheckFunction.

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go
File environs/local/export_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go#newcode17
environs/local/export_test.go:17: }
This is fine as is, though for testing stuff I do still prefer the style
of

return func() {
  rootCheckFunction = old
}

But if you ever want to inspect what the real root check does, I suppose
this is fine, too.

https://codereview.appspot.com/11321043/

[Roger Peppe (rogpeppe)]

LGTM with a few minor suggestions and general support of john's remarks,
thought i don't mind much.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode16
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 06:55:50, jameinel wrote:
> On 2013/07/16 03:56:35, wallyworld wrote:
> > Perhaps checkRoot() is better?

> checkIfRoot ?

isRoot? (or even "amRoot")

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode81
environs/local/config.go:81: // if the values have been set. If the
values haven't been set, then
this reads slightly ambiguously to me.

perhaps:

// getSudoCallerIds returns the user id and group id of the SUDO caller.
// If either is unset, it returns zero for both values.
// An error is returned if the relevant environment variables
// are not valid integers.
?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode84
environs/local/config.go:84: func getSudoCallerIds() (uid, gid int, err
error) {
sudoCallerIds ?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode92
environs/local/config.go:92: logger.Errorf("expected %q for SUDO_UID to
be an int: %v", uidStr, err)
do we need to log this error here? wouldn't
it be better to return a better error which
will be surely be logged later?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode98
environs/local/config.go:98: uid = 0 // clear out any value we may have
i think that rather than messing around assigning to the return values,
i'd do:

if uidStr == "" || gidStr == "" {
     return 0, 0, nil
}
uid, err := strconv.Atoi(...)
if err != nil {
    return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
}
gid, err := stdconv.Atoi(...)
if err != nil {
    return 0, 0, fmt.Errorf("similar...")
}
return uid, gid, nil

this keeps things a bit more obvious, with better error messages
to boot (the error from Atoi is rarely useful)

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode126
environs/local/config.go:126: if info.IsDir() && err == nil {
if err == nil && info.IsDir() {

(if there's an error, the info may be nil)

although in fact, i think:

if info != nil && info.IsDir() {

is probably better still.
not that any of this is likely to happen running as root.

https://codereview.appspot.com/11321043/

[Frank Mueller (themue)]

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode17
environs/local/config.go:17: return os.Getuid() == 0
Any need why this is a variable?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode98
environs/local/config.go:98: uid = 0 // clear out any value we may have
On 2013/07/16 13:30:01, rog wrote:
> i think that rather than messing around assigning to the return
values,
> i'd do:

> if uidStr == "" || gidStr == "" {
> return 0, 0, nil
> }
> uid, err := strconv.Atoi(...)
> if err != nil {
> return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
> }
> gid, err := stdconv.Atoi(...)
> if err != nil {
> return 0, 0, fmt.Errorf("similar...")
> }
> return uid, gid, nil

> this keeps things a bit more obvious, with better error messages
> to boot (the error from Atoi is rarely useful)

+1

https://codereview.appspot.com/11321043/

[Tim Penhey (thumper)]

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode16
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 13:30:01, rog wrote:
> On 2013/07/16 06:55:50, jameinel wrote:
> > On 2013/07/16 03:56:35, wallyworld wrote:
> > > Perhaps checkRoot() is better?
> >
> >
> > checkIfRoot ?

> isRoot? (or even "amRoot")

Went with checkIfRoot

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode17
environs/local/config.go:17: return os.Getuid() == 0
On 2013/07/16 14:14:35, mue wrote:
> Any need why this is a variable?

Yes, we need to override it for tests.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode81
environs/local/config.go:81: // if the values have been set. If the
values haven't been set, then
On 2013/07/16 13:30:01, rog wrote:
> this reads slightly ambiguously to me.

> perhaps:

> // getSudoCallerIds returns the user id and group id of the SUDO
caller.
> // If either is unset, it returns zero for both values.
> // An error is returned if the relevant environment variables
> // are not valid integers.
> ?

Shamelessly copied.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode84
environs/local/config.go:84: func getSudoCallerIds() (uid, gid int, err
error) {
On 2013/07/16 13:30:01, rog wrote:
> sudoCallerIds ?

You really don't like "get" do you?

Renamed.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode86
environs/local/config.go:86: // change ownership of the directories.
On 2013/07/16 03:56:35, wallyworld wrote:
> I don't think the above comment belongs here

No it isn't. Deleted.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode92
environs/local/config.go:92: logger.Errorf("expected %q for SUDO_UID to
be an int: %v", uidStr, err)
On 2013/07/16 13:30:01, rog wrote:
> do we need to log this error here? wouldn't
> it be better to return a better error which
> will be surely be logged later?

Cleaned up like below.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode98
environs/local/config.go:98: uid = 0 // clear out any value we may have
On 2013/07/16 14:14:35, mue wrote:
> On 2013/07/16 13:30:01, rog wrote:
> > i think that rather than messing around assigning to the return
values,
> > i'd do:
> >
> > if uidStr == "" || gidStr == "" {
> > return 0, 0, nil
> > }
> > uid, err := strconv.Atoi(...)
> > if err != nil {
> > return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
> > }
> > gid, err := stdconv.Atoi(...)
> > if err != nil {
> > return 0, 0, fmt.Errorf("similar...")
> > }
> > return uid, gid, nil
> >
> > this keeps things a bit more obvious, with better error messages
> > to boot (the error from Atoi is rarely useful)

> +1

Much nicer.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode126
environs/local/config.go:126: if info.IsDir() && err == nil {
On 2013/07/16 13:30:01, rog wrote:
> if err == nil && info.IsDir() {

> (if there's an error,...

=== modified file 'environs/local/config.go'
--- environs/local/config.go	2013-07-07 22:34:06 +0000
+++ environs/local/config.go	2013-07-16 21:59:25 +0000
@@ -13,6 +13,10 @@
 	"launchpad.net/juju-core/schema"
 )
 
+var checkIfRoot = func() bool {
+	return os.Getuid() == 0
+}
+
 var configChecker = schema.StrictFieldMap(
 	schema.Fields{
 		"root-dir": schema.String(),
@@ -31,9 +35,8 @@
 
 func newEnvironConfig(config *config.Config, attrs map[string]interface{}) *environConfig {
 	user := os.Getenv("USER")
-	root := false
-	if user == "root" {
-		root = true
+	root := checkIfRoot()
+	if root {
 		sudo_user := os.Getenv("SUDO_USER")
 		if sudo_user != "" {
 			user = sudo_user
@@ -74,6 +77,28 @@
 	return filepath.Join(c.rootDir(), filename)
 }
 
+// sudoCallerIds returns the user id and group id of the SUDO caller.
+// If either is unset, it returns zero for both values.
+// An error is returned if the relevant environment variables
+// are not valid integers.
+func sudoCallerIds() (int, int, error) {
+	uidStr := os.Getenv("SUDO_UID")
+	gidStr := os.Getenv("SUDO_GID")
+
+	if uidStr == "" || gidStr == "" {
+		return 0, 0, nil
+	}
+	uid, err := strconv.Atoi(uidStr)
+	if err != nil {
+		return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", uidStr)
+	}
+	gid, err := strconv.Atoi(gidStr)
+	if err != nil {
+		return 0, 0, fmt.Errorf("invalid value %q for SUDO_GID", gidStr)
+	}
+	return uid, gid, nil
+}
+
 func (c *environConfig) createDirs() error {
 	for _, dirname := range []string{
 		c.sharedStorageDir(),
@@ -88,23 +113,14 @@
 	if c.runningAsRoot {
 		// If we have SUDO_UID and SUDO_GID, start with rootDir(), and
 		// change ownership of the directories.
-		uidStr := os.Getenv("SUDO_UID")
-		gidStr := os.Getenv("SUDO_GID")
-		if uidStr != "" && gidStr != "" {
-			uid, err := strconv.Atoi(uidStr)
-			if err != nil {
-				logger.Errorf("Expected %q for SUDO_UID to be an int: %v", uidStr, err)
-				return err
-			}
-			gid, err := strconv.Atoi(gidStr)
-			if err != nil {
-				logger.Errorf("Expected %q for SUDO_GID to be an int: %v", gidStr, err)
-				return err
-			}
-
+		uid, gid, err := sudoCallerIds()
+		if err != nil {
+			return err
+		}
+		if uid != 0 || gid != 0 {
 			filepath.Walk(c.rootDir(),
 				func(path string, info os.FileInfo, err error) error {
-					if info.IsDir() && err == nil {
+					if info != nil && info.IsDir() {
 						if err := os.Chown(path, uid, gid); err != nil {
 							return err
 						}

=== modified file 'environs/local/config_test.go'
--- environs/local/config_test.go	2013-07-15 02:58:47 +0000
+++ environs/local/config_test.go	2013-07-16 21:59:25 +0000
@@ -83,6 +83,8 @@
 }
 
 func (s *configSuite) TestNamespaceRootNoSudo(c *gc.C) {
+	restore := local.SetRootCheckFunction(func() bool { return true })
+	defer restore()
 	err := os.Setenv("USER", "root")
 	c.Assert(err, gc.IsNil)
 	testConfig := minimalConfig(c)
@@ -90,6 +92,8 @@
 }
 
 func (s *configSuite) TestNamespaceRootWithSudo(c *gc.C) {
+	restore := local.SetRootCheckFunction(func() bool { return true })
+	defer restore()
 	err := os.Setenv("USER", "root")
 	c.Assert(err, gc.IsNil)
 	err = os.Setenv("SUDO_USER", "tester")
@@ -99,6 +103,47 @@
 	c.Assert(local.ConfigNamespace(testConfig), gc.Equals, "tester-test")
 }
 
+func (s *configSuite) TestSudoCallerIds(c *gc.C) {
+	defer os.Setenv("SUDO_UID", os.Getenv("SUDO_UID"))
+	defer os.Setenv("SUDO_GID", os.Getenv("SUDO_GID"))
+	for _, test := range []struct {
+		uid         string
+		gid         string
+		errString   string
+		expectedUid int
+		expectedGid int
+	}{{
+		uid: "",
+		gid: "",
+	}, {
+		uid:         "1001",
+		gid:         "1002",
+		expectedUid: 1001,
+		expectedGid: 1002,
+	}, {
+		uid:       "1001",
+		gid:       "foo",
+		errString: `invalid value "foo" for SUDO_GID`,
+	}, {
+		uid:       "foo",
+		gid:       "bar",
+		errString: `invalid value "foo" for SUDO_UID`,
+	}} {
+		os.Setenv("SUDO_UID", test.uid)
+		os.Setenv("SUDO_GID", test.gid)
+		uid, gid, err := local.SudoCallerIds()
+		if test.errString == "" {
+			c.Assert(err, gc.IsNil)
+			c.Assert(uid, gc.Equals, test.expectedUid)
+			c.Assert(gid, gc.Equals, test.expectedGid)
+		} else {
+			c.Assert(err, gc.ErrorMatches, test.errString)
+			c.Assert(uid, gc.Equals, 0)
+			c.Assert(gid, gc.Equals, 0)
+		}
+	}
+}
+
 type configRootSuite struct {
 	baseProviderSuite
 }

=== modified file 'environs/local/export_test.go'
--- environs/local/export_test.go	2013-07-15 02:58:47 +0000
+++ environs/local/export_test.go	2013-07-16 21:59:25 +0000
@@ -10,6 +10,14 @@
 
 var Provider = provider
 
+// SetRootCheckFunction allows tests to override the check for a root user.
+// The return value is the function to restore the old value.
+func SetRootCheckFunction(f func() bool) func() {
+	old := checkIfRoot
+	checkIfRoot = f
+	return func() { checkIfRoot = old }
+}
+
 // ConfigNamespace returns the result of the namespace call on the
 // localConfig.
 func ConfigNamespace(cfg *config.Config) string {
@@ -35,3 +43,7 @@
 		localConfig.mongoDir(),
 	}
 }
+
+func SudoCallerIds() (uid, gid int, err error) {
+	return sudoCallerIds()
+}