Add 'allow-different-user' flag and configuration option #471

[sjakobi]

No description provided.

[sjakobi]

I'd be interested in opinions on the flag name allow-sudo and the error message:

Preventing execution under sudo in order to protect file permissions.
Override this safety measure with '--allow-sudo' at your own risk.

Edit: Please comment on everything else, too! ;) The flag name and message are just the things I'm feeling particularly unsure about.

[mgsloan]

Yeah, I don't think --allow-sudo because not all posix systems have sudo. Even though the issue mentions --as-root, I think --allow-root is a better name. To me, "as-root" implies that it must be run as root, and might even run something to escalate to root privlidges. "allow-root", on the other hand, implies the intended meaning "allow running this as root, but it's fine if it isn't".

Another reason is that it looks better as a stack.yaml configuration field - allow-root: true vs as-root: true. Could you please add and document this configuration option? I think for the cases where people want --allow-root, they'll want it all the time, so best to allow it to be configurable.

[sjakobi]

Yeah, I don't think --allow-sudo because not all posix systems have sudo. Even though the issue mentions --as-root, I think --allow-root is a better name. To me, "as-root" implies that it must be run as root, and might even run something to escalate to root privlidges. "allow-root", on the other hand, implies the intended meaning "allow running this as root, but it's fine if it isn't".

The problem with --allow-root is that running as root is permitted anyway.

The flag I'm implementing here is only concerned with sudo. That's why it's looking for the SUDO_UID environment variable.

Another reason is that it looks better as a stack.yaml configuration field

Given that there don't seem to be so many use cases for the new flag I'm wondering whether there's even a point in making adding it to the configuration:

2. Occasionally you might want to poke around in a container as the root user, in which case sudo stack docker exec bash can be handy. But that should be rare so needing an override argument would be just fine.

[borsboom]

I guess a question is why prevent sudo in particular, as opposed to the more general case of preventing running as root when the .stack directory (or the directory where .stack will be put, if it doesn't already exist) is not owned by root and having it mess up permissions, which should also cover the sudo case (described in more details at #471 (comment)).

[sjakobi]

I guess a question is why prevent sudo in particular, as opposed to the more general case of preventing running as root when the .stack directory (or the directory where .stack will be put, if it doesn't already exist) is not owned by root and having it mess up permissions, which should also cover the sudo case (described in more details at #471 (comment)).

Can you explain how root could execute stack using another user's .stack directory without using sudo? I do agree though that a more general solution would be better.

[borsboom]

A couple I can think of: su -c or another program with the setuid bit set (and here's a list of sudo alternatives). Another case is setting STACK_ROOT to another user's directory while running as root.

[sjakobi]

A couple I can think of: su -c or another program with the setuid bit set (and here's a list of sudo alternatives). Another case is setting STACK_ROOT to another user's directory while running as root.

Ah ok, thanks! Somehow I missed STACK_ROOT.

To summarize the necessary changes:

• Check ownership of stackRoot instead of relying on SUDO_* env vars
• Change flag name from allow-sudo to allow-root
• Add and document configuration option allow-root. I assume that means the global config, not the project config, right?

Does this sound right?

I should get around to this in the next 1-2 weeks.

[borsboom]

That sounds right. One other thing would be to check stackRoot's parent directory before creating it (if the stackRoot doesn't already exist), to avoid putting a new root-owned .stack in a user's home directory.

[mgsloan]

Would it make sense to generally error out when .stack or .stack-work is owned by a different user? This flag would be called --allow-different-user.

[borsboom]

Yes, I think that probably would make sense and would cover the root case as well.

[mgsloan]

Misc note: For a moment, I thought it'd be tricky to get this working right with docker, since the current user is stack. However, when I ls -la, turns out everything that's normally owned by mgsloan is now owned by stack. Docker magic! Makes sense, as otherwise permissions would get really annoying with docker.

[borsboom]

Docker didn't actually make that easy at all, the magic is all in Stack itself :)

[sjakobi]

I haven't actually done any testing with Docker yet. (I haven't actually used Docker at all yet!)

What would be a typical workflow that is changed by this PR?

[sjakobi]

Is there anything that we can do about this? Would there be a point in splitting Stack.Constants into a part that is actually constant and a part that depends on other stack modules?

[borsboom]

Yes, I think such a change is probably overdue.

[borsboom]

The one Docker case that I think this could effect is if you're running Stack on a different machine than the Docker Engine. When that's the case, the in-container Stack runs as root because the assumption is that however you've mounted the "client" OS's filesystem to the Docker host will take care of any user ID mapping (the common cases being boot2docker or Vagrant "synced" filesystems). This is an obscure and not officially supported corner case and I'm not really sure how best to handle it, so I'm leaning toward just requiring anyone doing this to set the flag that allows another user.

[sjakobi]

The one Docker case that I think this could effect is if you're running Stack on a different machine than the Docker Engine. When that's the case, the in-container Stack runs as root because the assumption is that however you've mounted the "client" OS's filesystem to the Docker host will take care of any user ID mapping (the common cases being boot2docker or Vagrant "synced" filesystems). This is an obscure and not officially supported corner case and I'm not really sure how best to handle it, so I'm leaning toward just requiring anyone doing this to set the flag that allows another user.

Sounds like it would be rather difficult for me to test this.

For more conventional cases with root and sudo I've run tests locally and the behaviour seems to be as intended.

Should #1678 be fixed and Stack.Constants.stackRootEnvVar be imported before this can be merged?

Is there anything else that I need to take care of?

[mgsloan]

Can you please wrap up this "search parent directories till a predicate is met" into a utility in Path.IO? Another place this crops up is https://github.com/fpco/stack/blob/54fa4999905f9469c87ffefaeaddbd6bf2308c4f/src/Stack/Config.hs#L653https://github.com/fpco/stack/blob/54fa4999905f9469c87ffefaeaddbd6bf2308c4f/src/Stack/Config.hs#L653https://github.com/fpco/stack/blob/54fa4999905f9469c87ffefaeaddbd6bf2308c4f/src/Stack/Config.hs#L653

I'm thinking something with the signature findParent :: MonadIO m => (Path Abs Dir -> Maybe a) -> Path Abs Dir -> m (Maybe a)

It'd best to avoid this nubOrd / iterate pattern, because it cannot leverage laziness, and will always compute the 100 intermediate paths.

[sjakobi]

Can you please wrap up this "search parent directories till a predicate is met" into a utility in Path.IO?

Will do. There's something wrong with your link though. Is this the spot you were referring to?

stack/src/Stack/Config.hs

Line 674 in 0347cd1

search dir = do

Regarding the type, we'd need to allow monadic predicates, so that would be MonadIO m => (Path Abs Dir -> m (Maybe a)) -> Path Abs Dir -> m (Maybe a).

It'd best to avoid this nubOrd / iterate pattern, because it cannot leverage laziness, and will always compute the 100 intermediate paths.

I'm not quite sure I understand this correctly but wouldn't the following result in an infinite loop if it were true?

λ> let xs = nubOrd $ iterate succ (1 :: Int)
λ> head xs
1
λ> :sprint xs
xs = 1 : _

But terminating when we the directory is the same as the previous one, as the other implementation does, is much better anyways.

[mgsloan]

Hi! Sorry for letting this languish!

Should #1678 be fixed and Stack.Constants.stackRootEnvVar be imported before this can be merged?

I don't think this is necessary.

Is there anything else that I need to take care of?

The comment I added, and one more thing: I think #471 specifies that this check would also happen for .stack-work folders. This is a bit trickier, since there's a .stack-work for the project, as well as for the packages, and it usually ends up getting created by Cabal.

How about having a check when loading the config, for the project .stack-work. Then, when building, within Stack.Build.Execute.withSingleContext maybe, do the check for the package-specific .stack-work folders.

[sjakobi]

The comment I added, and one more thing: I think #471 specifies that this check would also happen for .stack-work folders. This is a bit trickier, since there's a .stack-work for the project, as well as for the packages, and it usually ends up getting created by Cabal.

How about having a check when loading the config, for the project .stack-work. Then, when building, within Stack.Build.Execute.withSingleContext maybe, do the check for the package-specific .stack-work folders.

You're right. I'll look into this in the next few days.

[sjakobi]

I think this should do the trick.

I still need to do some testing, maybe even write some integration tests – this should be possible with System.PosixCompat.User.setUserID, right?

I should adjust the commit message, too.

[sjakobi]

On a second look, I wonder if this couldn't cause some hard to find Windows-only bugs.

I guess we should better get the file status on Windows, too, so it will cause an exception when the file or directory doesn't exist.

[mgsloan]

Seems fine to me!

[mgsloan]

LGTM, thanks! There's a GHC 7.8 build error, but I'll fix it.

Feel free to commit tests and tweaks directly to the repo.