Merge lp:~thumper/juju-core/juju-ssh-dir-as-user into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Fix the creation of ~/.juju/ssh to be user owned

If the user's first interaction is with bootstrapping the
local provider with sudo, we want to make sure that the
directory and content is owned by the user.

Drive by fix for the cmd/juju MainSuite. I had a root owned
~/.juju/ssh directory, and the test was trying to read it and
failing, so I isolated the test by having it use the FakeHomeSuite
which overrides $HOME (and other things).

Many other utilities written to help with dealing with
creating directories and changing ownership of files to the
user if running under sudo.

https://codereview.appspot.com/52950043/

Fix the creation of ~/.juju/ssh to be user owned

If the user's first interaction is with bootstrapping the
local provider with sudo, we want to make sure that the
directory and content is owned by the user.

Drive by fix for the cmd/juju MainSuite. I had a root owned
~/.juju/ssh directory, and the test was trying to read it and
failing, so I isolated the test by having it use the FakeHomeSuite
which overrides $HOME (and other things).

Many other utilities written to help with dealing with
creating directories and changing ownership of files to the
user if running under sudo.

https://codereview.appspot.com/52950043/

[Tim Penhey (thumper)]

Reviewers: mp+201876_code.launchpad.net,

Message:
Please take a look.

Description:
Fix the creation of ~/.juju/ssh to be user owned

If the user's first interaction is with bootstrapping the
local provider with sudo, we want to make sure that the
directory and content is owned by the user.

Drive by fix for the cmd/juju MainSuite. I had a root owned
~/.juju/ssh directory, and the test was trying to read it and
failing, so I isolated the test by having it use the FakeHomeSuite
which overrides $HOME (and other things).

Many other utilities written to help with dealing with
creating directories and changing ownership of files to the
user if running under sudo.

https://code.launchpad.net/~thumper/juju-core/juju-ssh-dir-as-user/+merge/201876

(do not edit description out of merge proposal)

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

Affected files (+194, -8 lines):
   A [revision details]
   M cmd/juju/main_test.go
   M utils/file.go
   M utils/file_test.go
   M utils/ssh/clientkeys.go
   M utils/sudo.go
   M utils/sudo_test.go

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/52950043/diff/1/utils/file.go
File utils/file.go (right):

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode104
utils/file.go:104: func IsDirectory(path string) bool {
In light of my comments on MkDirAllForUser, I think this should just be
ditched. Some callers will care about symlinks, some won't.

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode106
utils/file.go:106: if os.IsNotExist(err) {
This is redundant, given the below check.

https://codereview.appspot.com/52950043/diff/1/utils/sudo.go
File utils/sudo.go (right):

https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode63
utils/sudo.go:63: if IsDirectory(dir) {
How about symlinks?

I think, rather than IsDirectory, use os.Lstat and os.IsNotExist. If
there's a file in the path, then the function can just fail when we
attempt to mkdir.

IOW, something like:

_, err := os.Lstat(dir)
if os.IsNotExist(err) {
     if err := MkdirAllForUser(filepath.Dir(dir), perm); err != nil {
         return err
     }
     err = MkdirForUser(dir, perm)
}
if err != nil {
     return err
}
return ChownToUser(dir)

https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode79
utils/sudo.go:79: os.RemoveAll(topMostDir)
This scares me. Can we please not remove things on failure? What if
chown $HOME is a symlink, and chmod fails? Then we try to RemoveAll
$HOME.

https://codereview.appspot.com/52950043/

[Andrew Wilkins (axwalk)]

On 2014/01/16 03:09:31, axw wrote:
> https://codereview.appspot.com/52950043/diff/1/utils/file.go
> File utils/file.go (right):

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode104
> utils/file.go:104: func IsDirectory(path string) bool {
> In light of my comments on MkDirAllForUser, I think this should just
be ditched.
> Some callers will care about symlinks, some won't.

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode106
> utils/file.go:106: if os.IsNotExist(err) {
> This is redundant, given the below check.

> https://codereview.appspot.com/52950043/diff/1/utils/sudo.go
> File utils/sudo.go (right):

> https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode63
> utils/sudo.go:63: if IsDirectory(dir) {
> How about symlinks?

> I think, rather than IsDirectory, use os.Lstat and os.IsNotExist. If
there's a
> file in the path, then the function can just fail when we attempt to
mkdir.

> IOW, something like:

> _, err := os.Lstat(dir)
> if os.IsNotExist(err) {
> if err := MkdirAllForUser(filepath.Dir(dir), perm); err != nil {
> return err
> }
> err = MkdirForUser(dir, perm)
> }
> if err != nil {
> return err
> }
> return ChownToUser(dir)

> https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode79
> utils/sudo.go:79: os.RemoveAll(topMostDir)
> This scares me. Can we please not remove things on failure? What if
chown $HOME
> is a symlink, and chmod fails? Then we try to RemoveAll $HOME.

LGTM pending changes

https://codereview.appspot.com/52950043/

[Andrew Wilkins (axwalk)]

On 2014/01/16 03:09:31, axw wrote:
> https://codereview.appspot.com/52950043/diff/1/utils/file.go
> File utils/file.go (right):

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode104
> utils/file.go:104: func IsDirectory(path string) bool {
> In light of my comments on MkDirAllForUser, I think this should just
be ditched.
> Some callers will care about symlinks, some won't.

https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode106
> utils/file.go:106: if os.IsNotExist(err) {
> This is redundant, given the below check.

> https://codereview.appspot.com/52950043/diff/1/utils/sudo.go
> File utils/sudo.go (right):

> https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode63
> utils/sudo.go:63: if IsDirectory(dir) {
> How about symlinks?

> I think, rather than IsDirectory, use os.Lstat and os.IsNotExist. If
there's a
> file in the path, then the function can just fail when we attempt to
mkdir.

> IOW, something like:

> _, err := os.Lstat(dir)
> if os.IsNotExist(err) {
> if err := MkdirAllForUser(filepath.Dir(dir), perm); err != nil {
> return err
> }
> err = MkdirForUser(dir, perm)
> }
> if err != nil {
> return err
> }
> return ChownToUser(dir)

> https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode79
> utils/sudo.go:79: os.RemoveAll(topMostDir)
> This scares me. Can we please not remove things on failure? What if
chown $HOME
> is a symlink, and chmod fails? Then we try to RemoveAll $HOME.

LGTM pending changes

https://codereview.appspot.com/52950043/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/52950043/

=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2013-12-16 20:01:53 +0000
+++ cmd/juju/main_test.go	2014-01-16 03:48:53 +0000
@@ -16,14 +16,12 @@
 
 	"launchpad.net/gnuflag"
 	gc "launchpad.net/gocheck"
-	"launchpad.net/loggo"
 
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/juju/osenv"
 	_ "launchpad.net/juju-core/provider/dummy"
 	"launchpad.net/juju-core/testing"
-	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/version"
 )
 
@@ -32,7 +30,7 @@
 }
 
 type MainSuite struct {
-	testbase.LoggingSuite
+	testing.FakeHomeSuite
 }
 
 var _ = gc.Suite(&MainSuite{})
@@ -79,10 +77,6 @@
 	return helpText(&SyncToolsCommand{}, "juju sync-tools")
 }
 
-func (s *MainSuite) TestTearDown(c *gc.C) {
-	loggo.ResetLoggers()
-}
-
 func (s *MainSuite) TestRunMain(c *gc.C) {
 	defer testing.MakeSampleHome(c).Restore()
 	// The test array structure needs to be inline here as some of the

=== modified file 'utils/ssh/clientkeys.go'
--- utils/ssh/clientkeys.go	2014-01-14 03:35:21 +0000
+++ utils/ssh/clientkeys.go	2014-01-16 03:48:53 +0000
@@ -61,7 +61,7 @@
 		// Directory exists but contains no keys;
 		// fall through and create one.
 	}
-	if err := os.MkdirAll(dir, 0700); err != nil {
+	if err := utils.MkdirAllForUser(dir, 0700); err != nil {
 		return err
 	}
 	keyfile, key, err := generateClientKey(dir)
@@ -97,6 +97,11 @@
 		os.Remove(privkeyFilename)
 		return "", nil, err
 	}
+	if err := utils.ChownToUser(privkeyFilename, privkeyFilename+PublicKeySuffix); err != nil {
+		os.Remove(privkeyFilename)
+		os.Remove(privkeyFilename + PublicKeySuffix)
+		return "", nil, err
+	}
 	return privkeyFilename, clientPrivateKey, nil
 }
 

=== modified file 'utils/sudo.go'
--- utils/sudo.go	2013-10-02 04:08:00 +0000
+++ utils/sudo.go	2014-01-16 03:48:53 +0000
@@ -6,9 +6,16 @@
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strconv"
 )
 
+// CheckIfRoot is a simple function that we can use to determine if
+// the ownership of files and directories we create.
+var CheckIfRoot = func() bool {
+	return os.Getuid() == 0
+}
+
 // 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
@@ -30,3 +37,53 @@
 	}
 	return
 }
+
+// MkdirForUser will call down to os.Mkdir and if the user is running as root,
+// the ownership will be changed to the sudo user.
+func MkdirForUser(dir string, perm os.FileMode) error {
+	if err := os.Mkdir(dir, perm); err != nil {
+		return err
+	}
+	return ChownToUser(dir)
+}
+
+// MkdirAllForUser will call down to os.MkdirAll and if the user is running as
+// root, the ownership will be changed to the sudo user for each directory
+// that was created.
+func MkdirAllForUser(dir string, perm os.FileMode) error {
+	toCreate := []string{}
+	path := dir
+	for {
+		_, err := os.Lstat(path)
+		if os.IsNotExist(err) {
+			toCreate = append(toCreate, path)
+		} else {
+			break
+		}
+		path = filepath.Dir(path)
+	}
+
+	if err := os.MkdirAll(dir, perm); err != nil {
+		return err
+	}
+	return ChownToUser(toCreate...)
+}
+
+// ChownToUser will attempt to change the ownership of all the paths
+// to the user returned by the SudoCallerIds method.  Ownership change
+// will only be attempted if we are running as root.
+func ChownToUser(paths ...string) error {
+	if !CheckIfRoot() {
+		return nil
+	}
+	uid, gid, err := SudoCallerIds()
+	if err != nil {
+		return err
+	}
+	for _, path := range paths {
+		if err := os.Chown(path, uid, gid); err != nil {
+			return err
+		}
+	}
+	return nil
+}

=== modified file 'utils/sudo_test.go'
--- utils/sudo_test.go	2013-10-02 01:27:45 +0000
+++ utils/sudo_test.go	2014-01-16 03:48:53 +0000
@@ -5,9 +5,12 @@
 
 import (
 	"os"
+	"os/user"
+	"path/filepath"
 
 	gc "launchpad.net/gocheck"
 
+	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/utils"
 )
@@ -16,6 +19,8 @@
 	testbase.LoggingSuite
 }
 
+var _ = gc.Suite(&sudoSuite{})
+
 func (s *sudoSuite) TestSudoCallerIds(c *gc.C) {
 	s.PatchEnvironment("SUDO_UID", "0")
 	s.PatchEnvironment("SUDO_GID", "0")
@@ -56,3 +61,89 @@
 		}
 	}
 }
+
+func (s *sudoSuite) TestMkDirForUserAsUser(c *gc.C) {
+	dir := filepath.Join(c.MkDir(), "new-dir")
+	err := utils.MkdirForUser(dir, 0755)
+	c.Assert(err, gc.IsNil)
+	c.Assert(dir, jc.IsDirectory)
+}
+
+func (s *sudoSuite) setupBadSudoUid() {
+	s.PatchEnvironment("SUDO_UID", "omg")
+	s.PatchEnvironment("SUDO_GID", "omg")
+	s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
+}
+
+func (s *sudoSuite) setupGoodSudoUid(c *gc.C) {
+	user, err := user.Current()
+	c.Assert(err, gc.IsNil)
+	s.PatchEnvironment("SUDO_UID", user.Uid)
+	s.PatchEnvironment("SUDO_GID", user.Gid)
+	s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
+}
+
+func (s *sudoSuite) TestMkDirForUserRoot(c *gc.C) {
+	s.setupGoodSudoUid(c)
+	dir := filepath.Join(c.MkDir(), "new-dir")
+	err := utils.MkdirForUser(dir, 0755)
+	c.Assert(err, gc.IsNil)
+	c.Assert(dir, jc.IsDirectory)
+}
+
+func (s *sudoSuite) TestMkDirForUserWithError(c *gc.C) {
+	s.setupBadSudoUid()
+	dir := filepath.Join(c.MkDir(), "new-dir")
+	err := utils.MkdirForUser(dir, 0755)
+	c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
+	// The directory is still there.
+	c.Assert(dir, jc.IsDirectory)
+}
+
+func (s *sudoSuite) TestMkDirAllForUserAsUser(c *gc.C) {
+	dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
+	err := utils.MkdirAllForUser(dir, 0755)
+	c.Assert(err, gc.IsNil)
+	c.Assert(dir, jc.IsDirectory)
+}
+
+func (s *sudoSuite) TestMkDirAllForUserRoot(c *gc.C) {
+	s.setupGoodSudoUid(c)
+
+	dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
+	err := utils.MkdirAllForUser(dir, 0755)
+	c.Assert(err, gc.IsNil)
+	c.Assert(dir, jc.IsDirectory)
+}
+
+func (s *sudoSuite) TestMkDirAllForUserWithError(c *gc.C) {
+	s.setupBadSudoUid()
+
+	base := c.MkDir()
+	dir := filepath.Join(base, "new-dir", "and-another")
+	err := utils.MkdirAllForUser(dir, 0755)
+	c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
+	// The directory is still there.
+	c.Assert(dir, jc.IsDirectory)
+}
+
+func (s *sudoSuite) TestChownToUserNotRoot(c *gc.C) {
+	// Just in case someone runs the suite as root, we don't want to fail.
+	s.PatchValue(&utils.CheckIfRoot, func() bool { return false })
+
+	nonExistant := filepath.Join(c.MkDir(), "non-existant")
+	err := utils.ChownToUser(nonExistant)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *sudoSuite) TestChownToUserBadUid(c *gc.C) {
+	s.setupBadSudoUid()
+	err := utils.ChownToUser(c.MkDir())
+	c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
+}
+
+func (s *sudoSuite) TestChownToUser(c *gc.C) {
+	s.setupGoodSudoUid(c)
+	err := utils.ChownToUser(c.MkDir(), c.MkDir())
+	c.Assert(err, gc.IsNil)
+}