juju-core

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

  1. juju-ssh-dir-as-user
  2. Merge into trunk
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 2212
Proposed branch: lp:~thumper/juju-core/juju-ssh-dir-as-user
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 254 lines (+155/-8)
4 files modified
cmd/juju/main_test.go (+1/-7)
utils/ssh/clientkeys.go (+6/-1)
utils/sudo.go (+57/-0)
utils/sudo_test.go (+91/-0)
To merge this branch: bzr merge lp:~thumper/juju-core/juju-ssh-dir-as-user
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+201876@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Andrew Wilkins (axwalk) 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.

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

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Revision history for this message
Tim Penhey (thumper) wrote :

Please take a look.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1616
17 "launchpad.net/gnuflag"17 "launchpad.net/gnuflag"
18 gc "launchpad.net/gocheck"18 gc "launchpad.net/gocheck"
19 "launchpad.net/loggo"
2019
21 "launchpad.net/juju-core/cmd"20 "launchpad.net/juju-core/cmd"
22 "launchpad.net/juju-core/environs/config"21 "launchpad.net/juju-core/environs/config"
23 "launchpad.net/juju-core/juju/osenv"22 "launchpad.net/juju-core/juju/osenv"
24 _ "launchpad.net/juju-core/provider/dummy"23 _ "launchpad.net/juju-core/provider/dummy"
25 "launchpad.net/juju-core/testing"24 "launchpad.net/juju-core/testing"
26 "launchpad.net/juju-core/testing/testbase"
27 "launchpad.net/juju-core/version"25 "launchpad.net/juju-core/version"
28)26)
2927
@@ -32,7 +30,7 @@
32}30}
3331
34type MainSuite struct {32type MainSuite struct {
35 testbase.LoggingSuite33 testing.FakeHomeSuite
36}34}
3735
38var _ = gc.Suite(&MainSuite{})36var _ = gc.Suite(&MainSuite{})
@@ -79,10 +77,6 @@
79 return helpText(&SyncToolsCommand{}, "juju sync-tools")77 return helpText(&SyncToolsCommand{}, "juju sync-tools")
80}78}
8179
82func (s *MainSuite) TestTearDown(c *gc.C) {
83 loggo.ResetLoggers()
84}
85
86func (s *MainSuite) TestRunMain(c *gc.C) {80func (s *MainSuite) TestRunMain(c *gc.C) {
87 defer testing.MakeSampleHome(c).Restore()81 defer testing.MakeSampleHome(c).Restore()
88 // The test array structure needs to be inline here as some of the82 // The test array structure needs to be inline here as some of the
8983
=== 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 @@
61 // Directory exists but contains no keys;61 // Directory exists but contains no keys;
62 // fall through and create one.62 // fall through and create one.
63 }63 }
64 if err := os.MkdirAll(dir, 0700); err != nil {64 if err := utils.MkdirAllForUser(dir, 0700); err != nil {
65 return err65 return err
66 }66 }
67 keyfile, key, err := generateClientKey(dir)67 keyfile, key, err := generateClientKey(dir)
@@ -97,6 +97,11 @@
97 os.Remove(privkeyFilename)97 os.Remove(privkeyFilename)
98 return "", nil, err98 return "", nil, err
99 }99 }
100 if err := utils.ChownToUser(privkeyFilename, privkeyFilename+PublicKeySuffix); err != nil {
101 os.Remove(privkeyFilename)
102 os.Remove(privkeyFilename + PublicKeySuffix)
103 return "", nil, err
104 }
100 return privkeyFilename, clientPrivateKey, nil105 return privkeyFilename, clientPrivateKey, nil
101}106}
102107
103108
=== 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 @@
6import (6import (
7 "fmt"7 "fmt"
8 "os"8 "os"
9 "path/filepath"
9 "strconv"10 "strconv"
10)11)
1112
13// CheckIfRoot is a simple function that we can use to determine if
14// the ownership of files and directories we create.
15var CheckIfRoot = func() bool {
16 return os.Getuid() == 0
17}
18
12// SudoCallerIds returns the user id and group id of the SUDO caller.19// SudoCallerIds returns the user id and group id of the SUDO caller.
13// If either is unset, it returns zero for both values.20// If either is unset, it returns zero for both values.
14// An error is returned if the relevant environment variables21// An error is returned if the relevant environment variables
@@ -30,3 +37,53 @@
30 }37 }
31 return38 return
32}39}
40
41// MkdirForUser will call down to os.Mkdir and if the user is running as root,
42// the ownership will be changed to the sudo user.
43func MkdirForUser(dir string, perm os.FileMode) error {
44 if err := os.Mkdir(dir, perm); err != nil {
45 return err
46 }
47 return ChownToUser(dir)
48}
49
50// MkdirAllForUser will call down to os.MkdirAll and if the user is running as
51// root, the ownership will be changed to the sudo user for each directory
52// that was created.
53func MkdirAllForUser(dir string, perm os.FileMode) error {
54 toCreate := []string{}
55 path := dir
56 for {
57 _, err := os.Lstat(path)
58 if os.IsNotExist(err) {
59 toCreate = append(toCreate, path)
60 } else {
61 break
62 }
63 path = filepath.Dir(path)
64 }
65
66 if err := os.MkdirAll(dir, perm); err != nil {
67 return err
68 }
69 return ChownToUser(toCreate...)
70}
71
72// ChownToUser will attempt to change the ownership of all the paths
73// to the user returned by the SudoCallerIds method. Ownership change
74// will only be attempted if we are running as root.
75func ChownToUser(paths ...string) error {
76 if !CheckIfRoot() {
77 return nil
78 }
79 uid, gid, err := SudoCallerIds()
80 if err != nil {
81 return err
82 }
83 for _, path := range paths {
84 if err := os.Chown(path, uid, gid); err != nil {
85 return err
86 }
87 }
88 return nil
89}
3390
=== 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 @@
55
6import (6import (
7 "os"7 "os"
8 "os/user"
9 "path/filepath"
810
9 gc "launchpad.net/gocheck"11 gc "launchpad.net/gocheck"
1012
13 jc "launchpad.net/juju-core/testing/checkers"
11 "launchpad.net/juju-core/testing/testbase"14 "launchpad.net/juju-core/testing/testbase"
12 "launchpad.net/juju-core/utils"15 "launchpad.net/juju-core/utils"
13)16)
@@ -16,6 +19,8 @@
16 testbase.LoggingSuite19 testbase.LoggingSuite
17}20}
1821
22var _ = gc.Suite(&sudoSuite{})
23
19func (s *sudoSuite) TestSudoCallerIds(c *gc.C) {24func (s *sudoSuite) TestSudoCallerIds(c *gc.C) {
20 s.PatchEnvironment("SUDO_UID", "0")25 s.PatchEnvironment("SUDO_UID", "0")
21 s.PatchEnvironment("SUDO_GID", "0")26 s.PatchEnvironment("SUDO_GID", "0")
@@ -56,3 +61,89 @@
56 }61 }
57 }62 }
58}63}
64
65func (s *sudoSuite) TestMkDirForUserAsUser(c *gc.C) {
66 dir := filepath.Join(c.MkDir(), "new-dir")
67 err := utils.MkdirForUser(dir, 0755)
68 c.Assert(err, gc.IsNil)
69 c.Assert(dir, jc.IsDirectory)
70}
71
72func (s *sudoSuite) setupBadSudoUid() {
73 s.PatchEnvironment("SUDO_UID", "omg")
74 s.PatchEnvironment("SUDO_GID", "omg")
75 s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
76}
77
78func (s *sudoSuite) setupGoodSudoUid(c *gc.C) {
79 user, err := user.Current()
80 c.Assert(err, gc.IsNil)
81 s.PatchEnvironment("SUDO_UID", user.Uid)
82 s.PatchEnvironment("SUDO_GID", user.Gid)
83 s.PatchValue(&utils.CheckIfRoot, func() bool { return true })
84}
85
86func (s *sudoSuite) TestMkDirForUserRoot(c *gc.C) {
87 s.setupGoodSudoUid(c)
88 dir := filepath.Join(c.MkDir(), "new-dir")
89 err := utils.MkdirForUser(dir, 0755)
90 c.Assert(err, gc.IsNil)
91 c.Assert(dir, jc.IsDirectory)
92}
93
94func (s *sudoSuite) TestMkDirForUserWithError(c *gc.C) {
95 s.setupBadSudoUid()
96 dir := filepath.Join(c.MkDir(), "new-dir")
97 err := utils.MkdirForUser(dir, 0755)
98 c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
99 // The directory is still there.
100 c.Assert(dir, jc.IsDirectory)
101}
102
103func (s *sudoSuite) TestMkDirAllForUserAsUser(c *gc.C) {
104 dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
105 err := utils.MkdirAllForUser(dir, 0755)
106 c.Assert(err, gc.IsNil)
107 c.Assert(dir, jc.IsDirectory)
108}
109
110func (s *sudoSuite) TestMkDirAllForUserRoot(c *gc.C) {
111 s.setupGoodSudoUid(c)
112
113 dir := filepath.Join(c.MkDir(), "new-dir", "and-another")
114 err := utils.MkdirAllForUser(dir, 0755)
115 c.Assert(err, gc.IsNil)
116 c.Assert(dir, jc.IsDirectory)
117}
118
119func (s *sudoSuite) TestMkDirAllForUserWithError(c *gc.C) {
120 s.setupBadSudoUid()
121
122 base := c.MkDir()
123 dir := filepath.Join(base, "new-dir", "and-another")
124 err := utils.MkdirAllForUser(dir, 0755)
125 c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
126 // The directory is still there.
127 c.Assert(dir, jc.IsDirectory)
128}
129
130func (s *sudoSuite) TestChownToUserNotRoot(c *gc.C) {
131 // Just in case someone runs the suite as root, we don't want to fail.
132 s.PatchValue(&utils.CheckIfRoot, func() bool { return false })
133
134 nonExistant := filepath.Join(c.MkDir(), "non-existant")
135 err := utils.ChownToUser(nonExistant)
136 c.Assert(err, gc.IsNil)
137}
138
139func (s *sudoSuite) TestChownToUserBadUid(c *gc.C) {
140 s.setupBadSudoUid()
141 err := utils.ChownToUser(c.MkDir())
142 c.Assert(err, gc.ErrorMatches, `invalid value "omg" for SUDO_UID`)
143}
144
145func (s *sudoSuite) TestChownToUser(c *gc.C) {
146 s.setupGoodSudoUid(c)
147 err := utils.ChownToUser(c.MkDir(), c.MkDir())
148 c.Assert(err, gc.IsNil)
149}

Subscribers

People subscribed via source and target branches

to status/vote changes: