You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by arunmahadevan <gi...@git.apache.org> on 2018/06/15 23:38:15 UTC
[GitHub] storm pull request #2721: STORM-3110: Skip the user while checking isProcess...
GitHub user arunmahadevan opened a pull request:
https://github.com/apache/storm/pull/2721
STORM-3110: Skip the user while checking isProcessAlive
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arunmahadevan/storm STORM-3110-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2721.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2721
----
commit 1d43937a12d5632d1c64e5701b8443a179595062
Author: Arun Mahadevan <ar...@...>
Date: 2018-06-15T23:32:53Z
STORM-3110: Skip the user while checking isProcessAlive
----
---
[GitHub] storm pull request #2721: STORM-3110: Skip the user while checking isProcess...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2721
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by raghavgautam <gi...@git.apache.org>.
Github user raghavgautam commented on the issue:
https://github.com/apache/storm/pull/2721
👍
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2721
I'm not an expert of Supervisor V2 but just based on my understanding of this issue, I guess we may want to address this to two different situations:
1. `supervisor.run.worker.as.user` is set
Supervisor should check the process with `worker` account (can be read via worker state).
2. `supervisor.run.worker.as.user` is unset
Supervisor should check the process with `supervisor` account, as workers will be always launched with `supervisor` account.
In any cases, if the worker is running with different account, it might be considered as wrong process or out of sync. Ideally I think it should be corrected (kill the process) but it might open security issue (account A editing pid file for worker A1 and let Supervisor kill arbitrary process running with other account) so error or critical log message might be a best bet.
I'm not sure how many cases we can encounter above case, but unless the hole is huge, I guess checking with above logic would work.
@arunmahadevan @revans2 Please correct me if I'm missing here and/or there's better way to handle this.
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2721
So you are running with authentication, but run as user is disabled, because you are running on windows. That is a use case that I missed, sorry about that.
This is not a security issue as we kill the process as the user that owns the container. When run as user is enabled we do it as the user that launched the process, when it is disabled we do it as the supervisor user. This means the OS will prevent us from killing a process we shouldn't, even if a worker self reports a process that it should not.
The entire reason I put the user check in was to avoid a problem with PID reuse. OSes typically rotate through PIDs in a way to avoid situations where a process dies and its PID is quickly handed to another process, but with how storm self reports PIDs through the file system... I just wanted to be cautious.
Removing the user check entirely should be fine, the issue you might run into is that if a PID is reused too quickly by a different user you might be stuck trying to kill is process that you are not allowed to and end up stuck waiting for it to die forever. The simple fix for me would be to have the Container pass in `System.getProperty("user.name")` for the user name, with a comment about why, and have RunAsUserContainer pass the real user in.
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2721
My ideal thought depends on the assumption that we could get OS account for current JVM process in safe way. If there's no way to do it, I agree this PR might be a best bet.
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/2721
For security, I think we need to keep the if user check, we should fix it but not through deleting it.
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2721
Also note that supervisor might be killing the workers in response to an assignment change triggered by nimbus (may be due to user triggered kill or rebalance) and the supervisor has no idea of which end user/action might have triggered this. The authorization checks to see if the user has permissions to kill/rebalance the topology is already happening at nimbus so I think we are good.
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2721
@revans2 , @HeartSaVioR, can you take a look?
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2721
@revans2 , i have refactored the code so that we check for "user.name" in the normal container and the result of `getWorkerUser` in the RunAsUserContainer.
The `getWorkerUser` is used in other places to set the user in the workers local state and apparently used to do some authorization checks in the log viewer. I did not want to affect that behavior so I introduced a new function to return the OS user that the worker is expected to run as that invokes the `getWorkerUser` in the run as container.
This issue happens in posix environment as well and most of the users dont set the `supervisor.run.worker.as.user` config but run storm in secure mode.
---
[GitHub] storm issue #2721: STORM-3110: Skip the user while checking isProcessAlive
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2721
Unless `supervisor.run.worker.as.user` is set, the worker process runs as "storm" user. I guess the supervisor should always check "if all processes are dead" by just looking if the worker pids are alive than doing user comparison, since there is no mapping between the user that launched the topology (e.g kerberos user) and the actual OS user that worker is running as (this is always storm).. In the "run as user" container the "kill" command is launched by switching to the OS user that worker is actually running as here - https://github.com/apache/storm/blob/1.x-branch/storm-core/src/jvm/org/apache/storm/daemon/supervisor/RunAsUserContainer.java#L55 and that should take care of the security.
---