charm: fix symlink and file mode when bundling/expanding

https://code.launchpad.net/~niemeyer/juju/go-charm-symlinks/+merge/91960

(do not edit description out of merge proposal)

[niemeyer, 2012-02-08 01:29:33]

Please take a look.

[niemeyer, 2012-02-08 01:38:13]

Please take a look.

[fwereade, 2012-02-08 09:13:21]

LGTM: can't see anything to complain about here :).

[rog, 2012-02-08 09:56:55]

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode201
charm/bundle.go:201: err = os.MkdirAll(path, mode & 0777)
do we care about interaction with umask here?

in general i'm not sure about the file modes here (what do "group" permissions
mean when the group owner of the file isn't defined?) i'd be happy if we
continued to use 0755, or we could just copy the user mode bits to other and
group: mode&7 | mode&7<<3 | mode&7<<6

i think the permissions in the zip file (execute bits aside) are much more
likely to be a reflection of some dubious umask on the charm creator's side than
a deliberate decision.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode222
charm/bundle.go:222: if err != nil || strings.HasPrefix(reltarget, "..") {
i'm not sure the logic of the previous three lines is correct.

1) you're doing the join even when the target is already absolute (i think you
should check for target[0] == '/').
2) it allows absolute symlinks that happen to point into the charm directory.
3) it can disallow a symlink that points at a file name beginning with two dots,
which should be allowed.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode225
charm/bundle.go:225: return os.Symlink(string(data), path)
s/string(data)/target/

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode228
charm/bundle.go:228: f, err := os.OpenFile(path,
os.O_CREATE|os.O_EXCL|os.O_WRONLY, mode & 0777)
the same considerations about file mode apply here.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/5647047/diff/6001/charm/bundle_test.go#newcode115
charm/bundle_test.go:115: err := os.Symlink("../../target",
filepath.Join(charmDir, "hooks", "badlink"))
we should have a test with an absolute link.

[niemeyer, 2012-02-08 16:27:21]

PTAL

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode201
charm/bundle.go:201: err = os.MkdirAll(path, mode & 0777)
On 2012/02/08 09:56:56, rog wrote:
> do we care about interaction with umask here?

No, not here. This is the bundle content. If we want to alter the mode, we
should do at packing time. I'm happy to be more specific for the moment.

> in general i'm not sure about the file modes here (what do "group" permissions
> mean when the group owner of the file isn't defined?) i'd be happy if we
> continued to use 0755, or we could just copy the user mode bits to other and

This would give owner permission for everybody. I don't think we want that.

> group: mode&7 | mode&7<<3 | mode&7<<6

This isn't doing what you say it is.

> i think the permissions in the zip file (execute bits aside) are much more
> likely to be a reflection of some dubious umask on the charm creator's side
than
> a deliberate decision.

Yeah, probably. I've changed the bundling side to use fixed permissions.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode222
charm/bundle.go:222: if err != nil || strings.HasPrefix(reltarget, "..") {
On 2012/02/08 09:56:56, rog wrote:
> i'm not sure the logic of the previous three lines is correct.
> 
> 1) you're doing the join even when the target is already absolute (i think you
> should check for target[0] == '/').
> 2) it allows absolute symlinks that happen to point into the charm directory.
> 3) it can disallow a symlink that points at a file name beginning with two
dots,
> which should be allowed.

(1) Good catch, thanks.
(2) Not really. It's broken due to 1.
(3) That was conscious, but was silly. Will fix.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode225
charm/bundle.go:225: return os.Symlink(string(data), path)
On 2012/02/08 09:56:56, rog wrote:
> s/string(data)/target/

Will do.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle.go#newcode228
charm/bundle.go:228: f, err := os.OpenFile(path,
os.O_CREATE|os.O_EXCL|os.O_WRONLY, mode & 0777)
On 2012/02/08 09:56:56, rog wrote:
> the same considerations about file mode apply here.

Ditto.

https://codereview.appspot.com/5647047/diff/6001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/5647047/diff/6001/charm/bundle_test.go#newcode115
charm/bundle_test.go:115: err := os.Symlink("../../target",
filepath.Join(charmDir, "hooks", "badlink"))
On 2012/02/08 09:56:56, rog wrote:
> we should have a test with an absolute link.

Will do.

[niemeyer, 2012-02-08 16:37:52]

Please take a look.

[rog, 2012-02-08 17:14:29]

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go#newcode171
charm/dir.go:171: perm := os.FileMode(0644)
i think i'd be inclined to set all modes here, and let the extractor act as a
"umask" for them.
perm := os.FileMode(0666)
if fi.Mode() & 0100 != 0 {
    perm = 0777
}
h.SetMode(perm | (fi.Mode() & os.ModeSymlink))

we don't allow read-only files, but perhaps that's a good thing.

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go#newcode177
charm/dir.go:177: h.SetMode(fi.Mode()&^0777|perm)
do we want to be letting special modes other than symlink in here?

[niemeyer, 2012-02-08 17:21:54]

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go#newcode177
charm/dir.go:177: h.SetMode(fi.Mode()&^0777|perm)
On 2012/02/08 17:14:29, rog wrote:
> do we want to be letting special modes other than symlink in here?

No.

[niemeyer, 2012-02-08 17:32:01]

Please take a look.

[rog, 2012-02-08 17:40:18]

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go#newcode177
charm/dir.go:177: h.SetMode(fi.Mode()&^0777|perm)
On 2012/02/08 17:21:54, niemeyer wrote:
> On 2012/02/08 17:14:29, rog wrote:
> > do we want to be letting special modes other than symlink in here?
> 
> No.

sorry, i wasn't clear.
i think this line does let in special modes other than symlink, but perhaps i've
missed something.

[niemeyer, 2012-02-08 17:55:52]

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/5647047/diff/7001/charm/dir.go#newcode177
charm/dir.go:177: h.SetMode(fi.Mode()&^0777|perm)
On 2012/02/08 17:40:18, rog wrote:
> sorry, i wasn't clear.
> i think this line does let in special modes other than symlink, but perhaps
i've
> missed something.

People can't really get other file types onto Bazaar, but sure, it doesn't hurt
to check explicitly.

[niemeyer, 2012-02-08 18:47:29]

Please take a look.

[niemeyer, 2012-02-10 13:03:26]

On 2012/02/08 18:47:29, niemeyer wrote:
> Please take a look.

Anything else?

[fwereade, 2012-02-10 13:52:32]

Apart from 1 minor, LGTM.

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go#newcode243
charm/bundle.go:243: func hasDotDot(path string) bool {
Both clients appear to be using this in the same context, and might both be
happier with something like:

if linksOutside(root, path, target) {
  return fmt.Errorf(...)
}

[rog, 2012-02-10 15:20:13]

LGTM

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go#newcode244
charm/bundle.go:244: return len(path) >= 2 && path[:2] == ".." && len(path) == 2
|| path[2] == filepath.Separator
given that we're already using slashes above and we don't have symlinks on
windows, perhaps the more obvious:
return path == ".." || strings.HasPrefix("../")
would be better here.

https://codereview.appspot.com/5647047/diff/13/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/5647047/diff/13/charm/bundle_test.go#newcode126
charm/bundle_test.go:126: // Symlink targetting an absolute path.
s/targetting/targeting/

[niemeyer, 2012-02-10 16:29:47]

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go#newcode243
charm/bundle.go:243: func hasDotDot(path string) bool {
On 2012/02/10 13:52:32, fwereade wrote:
> Both clients appear to be using this in the same context, and might both be
> happier with something like:

That's a great idea. Refactored, thanks!

https://codereview.appspot.com/5647047/diff/13/charm/bundle.go#newcode244
charm/bundle.go:244: return len(path) >= 2 && path[:2] == ".." && len(path) == 2
|| path[2] == filepath.Separator
On 2012/02/10 15:20:13, rog wrote:
> given that we're already using slashes above and we don't have symlinks on
> windows, perhaps the more obvious:
> return path == ".." || strings.HasPrefix("../")
> would be better here.

That's cleaner indeed, and the optimization is pointless. Changed.

https://codereview.appspot.com/5647047/diff/13/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/5647047/diff/13/charm/bundle_test.go#newcode126
charm/bundle_test.go:126: // Symlink targetting an absolute path.
On 2012/02/10 15:20:13, rog wrote:
> s/targetting/targeting/

Fixed all, thanks.

[niemeyer, 2012-02-10 16:34:45]

*** Submitted:

charm: fix symlink and file mode when bundling/expanding

R=fwereade, rog
CC=
https://codereview.appspot.com/5647047

[kapil.foss, 2012-02-10 16:50:30]

It doesn't appear the de-referenced symlink has its type checked.

[gustavo_niemeyer.net, 2012-02-10 16:56:40]

On Fri, Feb 10, 2012 at 14:50,  <kapil.foss@gmail.com> wrote:
> It doesn't appear the de-referenced symlink has its type checked.

The link is never dereferenced. We just check its target, which is the
link's content, for validity.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.