-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] LocalProcessProxy Kernels: user impersonate & working_dir #971
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,8 @@ | |
import signal | ||
import re | ||
import uuid | ||
|
||
from pwd import getpwnam | ||
import grp | ||
from tornado import web | ||
from ipython_genutils.py3compat import unicode_type | ||
from ipython_genutils.importstring import import_item | ||
|
@@ -19,6 +20,7 @@ | |
from enterprise_gateway.mixins import EnterpriseGatewayConfigMixin | ||
|
||
|
||
|
||
def get_process_proxy_config(kernelspec): | ||
""" | ||
Return the process-proxy stanza from the kernelspec. | ||
|
@@ -147,7 +149,7 @@ async def start_kernel(self, *args, **kwargs): | |
username = KernelSessionManager.get_kernel_username(**kwargs) | ||
self.log.debug("RemoteMappingKernelManager.start_kernel: {kernel_name}, kernel_username: {username}". | ||
format(kernel_name=kwargs['kernel_name'], username=username)) | ||
|
||
self.connection_dir=os.path.join(os.path.expanduser('~'+username),".local/share/jupyter/runtime/") | ||
# Check max kernel limits | ||
self._enforce_kernel_limits(username) | ||
|
||
|
@@ -571,8 +573,17 @@ def write_connection_file(self): | |
self.hb_port = ports[3] | ||
self.control_port = ports[4] | ||
|
||
return super(RemoteKernelManager, self).write_connection_file() | ||
cf = super(RemoteKernelManager, self).write_connection_file() | ||
username=self.user_overrides.get('KERNEL_USERNAME',None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a better way to get the username at this point? like: It would be the best to write_connection_file to the correct user rather than chown-ing it later. But idk https://github.com/jupyter/jupyter_client/blob/98faaf91d00b4fbf17b2fbf0b08b1fd23adc27bf/jupyter_client/connect.py#L42 |
||
if username: | ||
uid=getpwnam(username).pw_uid | ||
guid=grp.getgrnam(username).gr_gid | ||
os.chown(self.connection_file,uid,guid) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to change ownership of connection files. Because connection files are created by the gateway process (root) and when the kernel is launched as a user |
||
self.log.debug(f"Local connection file chowned:{self.connection_file} to {username}") | ||
else: | ||
raise RuntimeError("Unrecognized user") | ||
|
||
return cf | ||
def _get_process_proxy(self): | ||
""" | ||
Reads the associated kernelspec and to see if has a process proxy stanza. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ | |
from zmq.ssh import tunnel | ||
|
||
from ..sessions.kernelsessionmanager import KernelSessionManager | ||
|
||
from jupyterhub.spawner import set_user_setuid | ||
# Default logging level of paramiko produces too much noise - raise to warning only. | ||
logging.getLogger('paramiko').setLevel(os.getenv('EG_SSH_LOG_LEVEL', logging.WARNING)) | ||
|
||
|
@@ -911,7 +911,18 @@ def __init__(self, kernel_manager, proxy_config): | |
|
||
async def launch_process(self, kernel_cmd, **kwargs): | ||
await super(LocalProcessProxy, self).launch_process(kernel_cmd, **kwargs) | ||
|
||
user=kwargs["env"].get("KERNEL_USERNAME",None) | ||
self.log.info(f"KARGS {kwargs}") | ||
|
||
if user: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can check here if EG_IMPERSONATION_ENABLED & user. (The current EG behavior ignores the KERNEL_USERNAME) btw,I will also remove unnecessary logging, it was to help me understand how EG works. |
||
kwargs["preexec_fn"]=set_user_setuid(user,chdir=False) | ||
else: | ||
raise RuntimeError("Trying to run as root on host, unrecognized user") | ||
# launch the local run.sh | ||
working_dir=kwargs["env"].get("KERNEL_WORKING_DIR",None) | ||
if working_dir: | ||
kwargs["cwd"]=working_dir | ||
|
||
# launch the local run.sh | ||
self.local_proc = self.launch_kernel(kernel_cmd, **kwargs) | ||
self.pid = self.local_proc.pid | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.connection_dir is normally determined by JUPYTER_RUNTIME_DIR but if this is not in user Paths in which to search for connection files it breaks so I did another hacky update. https://github.com/jupyter/jupyter_client/blob/98faaf91d00b4fbf17b2fbf0b08b1fd23adc27bf/jupyter_client/connect.py#L191
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to call
jupyter_core.paths.get_home_dir()
to, presumably get/root
, then replace that portion ofjupyter_runtime_dir()
with the target user's home directory? Otherwise, this isn't producing the correct path relative to the platform.We should also ensure the user's home directory exists, since KERNEL_USERNAME is not necessarily reflective of a user. (I suppose setuid will enforce that constraint though.)
How will these kinds of files be cleaned up?
self.connection_dir
is on the multi kernel manager which spans all kernel manager instances - so this isn't going to work. I'm not sure the connection information needs to be isolated on a per user level. It isn't today at least.