client add Umask to the client config, use it to Chmod files #195
Conversation
|
Just want to note on quick review: using the |
| @@ -66,6 +71,25 @@ type Client struct { | |||
| Options []ClientOption | |||
| } | |||
|
|
|||
| func (c *Client) umask() os.FileMode { | |||
| if c == nil { | |||
There was a problem hiding this comment.
I find it a bit odd handling the receiver, c, being nil. It may mask bugs where callers are operating on a nil Client rather than a valid reference and getting inconsistent masks.
There was a problem hiding this comment.
agreed! get_file_test tests are written in such a way that the client is nil, and I'm not sure if consumers of this library are counting on the current behavior of FileGetter (or the other substructs). It may just be concealing bugs in the callers...
|
See also https://github.com/hashicorp/go-getter/tree/f-umask-v110-2, a much smaller set of changes that prevents setuid simply by avoiding direct calls to |
There was a problem hiding this comment.
Looks good, but I want to make sure we assert the intended behavior in tests. Feel free to merge when you've either verified its tested or added a couple tests.
Great work on this! I wish we could use the simpler approach, but it just seems to risky to leave the permissions up to go/libc/kernel to determine.
get_file_copy.go
Outdated
| } | ||
|
|
||
| // copyFile copies a file in chunks from src path to dst path, using umask to create the dst file | ||
| func copyFile(ctx context.Context, dst string, src string, mode os.FileMode) (int64, error) { |
There was a problem hiding this comment.
Let's unit test these 2 funcs.
There was a problem hiding this comment.
I don't think a direct unit test of these adds much value, they're consumed by all the other unit tested components. Since they're private, I thought separate tests might end up over-specifying them.
|
@jbardin pointed out in a discussion that this breaks backward compatibility for other users of go-getter as well as breaks our We could maintain backward compat and keep tar's behavior by defaulting to an empty mask and letting Nomad override it to strip setuid bits. |
That also addresses @mitchellh's concerns in passing by eliminating any dependency on the umask. We can avoid the syscall and the global default. |
96cca11 to
cb8c602
Compare
0ffc284 to
394ecaf
Compare
394ecaf to
b236ad3
Compare
|
This PR is superseded by #198, and can be closed. |
This adds a
Client.Umask os.FileModeand uses it to mask file permissions wherever files are created or permissions are set. This version of the patch is based on v1.1.0, it's to be followed by a pr that uses umask with the gcs getters.