start jupyterhub-singleuser within docker spawer #95
Conversation
| self._ensure_system_user() | ||
| self._ensure_workspace_permission() | ||
|
|
||
| def start_single_user_server(self): |
There was a problem hiding this comment.
copied from customer_spawner.py
| self.log.info(f'Executing command: {" ".join(cmd)}') | ||
| subprocess.run(cmd, check=True) | ||
|
|
||
| def _ensure_system_user(self): |
There was a problem hiding this comment.
mostly copied from customer_spawner.py. But using pwd and grp to check existing user and group.
| finally: | ||
| fcntl.flock(lock, fcntl.LOCK_UN) | ||
|
|
||
| def _ensure_workspace_permission(self): |
There was a problem hiding this comment.
copied from custom_spawner.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
- Coverage 54.37% 47.75% -6.63%
==========================================
Files 6 7 +1
Lines 320 400 +80
==========================================
+ Hits 174 191 +17
- Misses 146 209 +63 ☔ View full report in Codecov by Sentry. |
| self.groupname = groupname # OS system group | ||
|
|
||
| # Inherit the current environment variables | ||
| self.environment = dict(os.environ) |
There was a problem hiding this comment.
This still makes me nervous...
There was a problem hiding this comment.
I will get a whitelist later once system is up and running.
| :param username: The name of the user for whom the environment is set up. | ||
| :param groupname: The name of the OS system group to which the user belongs. | ||
| """ | ||
| self.username = username # OS system username |
There was a problem hiding this comment.
I don't understand why the juipyterhub username and the linux username need to be the same
There was a problem hiding this comment.
somewhere down the line (I think it's auth) will need to check that system user exist with the same jupyterhub username.
There was a problem hiding this comment.
I don't quite get that - why would the linux username need to match the auth username?
There was a problem hiding this comment.
maybe for some spawner it's not required. But I don't see it concerns us.
There was a problem hiding this comment.
There was a problem hiding this comment.
Yarn is working fine with different users.
Not if it's bypassing fair share it isn't
There was a problem hiding this comment.
I will explore some yarn setting (spark.yarn.queue) on the client's side.
There was a problem hiding this comment.
If you just use the same user for the containers you don't have to worry about this
There was a problem hiding this comment.
I created another issue for this. #97
It's really not how Jupyterhub works as far as I understand.
There was a problem hiding this comment.
If you're saying that JH requires the linux username to match the JH username then yeah, that won't work and we'll have to look into modifying yarn or how the submit works
|
|
||
| self.log.info(f'Configuring workspace permissions for {self.username}') | ||
| subprocess.run(['sudo', 'chown', '-R', f'{self.username}:{group_name}', self.user_dir], check=True) | ||
| subprocess.run(['sudo', 'chmod', '-R', '777', self.user_dir], check=True) |
There was a problem hiding this comment.
Is this still needed or is 700 or 770 ok?
Also this is going to make all files executable, which I assume you don't want
There was a problem hiding this comment.
👍 changed to 770.
There was a problem hiding this comment.
Making all the files executable?
There was a problem hiding this comment.
👍 changed to 750. I am really still trying to make it work first, before considering security, etc.
There was a problem hiding this comment.
750 still makes the files executable for the owner but doesn't let the group traverse the directories. You'd probably need to do it in 2 steps, one handling directories and one handling files, unless there's some build in way in chmod to do that.
That said if you want to make an issue and move on that's fine
There was a problem hiding this comment.
But it also doesn't hurt. And really not our concerns now.
There was a problem hiding this comment.
But it also doesn't hurt.
It does hurt for the reasons in the link and general security principles. But as I said
That said if you want to make an issue and move on that's fine
There was a problem hiding this comment.
I really don't know what to put on the issue. Do you think you can make one? I just don't see any security concerns or principles being violated by assigning users 750 permissions on their own workspace within their isolated container.
There was a problem hiding this comment.
The security concerns are listed above in the SO post
For the ticket, just something like "Don't chmod non-executable files to executable" and link this PR, I'd say
No description provided.