Skip to content
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

feat: add ssh server #291

Merged
merged 10 commits into from
Feb 13, 2023
Merged

feat: add ssh server #291

merged 10 commits into from
Feb 13, 2023

Conversation

olevski
Copy link
Member

@olevski olevski commented Jan 10, 2023

Adds a ssh server that runs as non-root in the image.

This is the first step to allowing ssh access into a renku session.

@olevski olevski marked this pull request as draft January 10, 2023 20:31
@olevski olevski force-pushed the feat-add-ssh-server branch from be99825 to a6bdbc8 Compare January 10, 2023 20:39
@seanrmurphy
Copy link
Contributor

I know this is still draft and you're not looking for feedback/inputs per se, but has there been discussion of using a proper process manager inside the container? I'm specifically thinking of s6-overlay as I think that's v good but of course it's not the only solution...

https://github.com/just-containers/s6-overlay

Might require changes to the container build so might not really be the right solution, but if we're adding multiple services inside a container, I think it should be considered at some point...

@olevski
Copy link
Member Author

olevski commented Jan 11, 2023

@seanrmurphy can s6 be run with a non-root user?

@olevski olevski force-pushed the feat-add-ssh-server branch 2 times, most recently from a8a6e9d to 9ca7a0f Compare January 12, 2023 04:19
@olevski olevski force-pushed the feat-add-ssh-server branch from 9ca7a0f to 493bc35 Compare January 12, 2023 04:35
@seanrmurphy
Copy link
Contributor

@seanrmurphy can s6 be run with a non-root user?

I think so but I'm not certain; a common approach is that it runs as root in a container and drops privileges to run a process as a user. Is this in line with what you're thinking?

@olevski olevski force-pushed the feat-add-ssh-server branch from 1e066f8 to bf0cde5 Compare January 18, 2023 12:56
@olevski
Copy link
Member Author

olevski commented Jan 18, 2023

Is this in line with what you're thinking?

I am not sure. I would have to test things out. It may not be possible because the container runs with non-root uid, and privilege escalation is also turned off. So I am not sure if it will work or not. I know that turning off privilege escalation was causing a bunch of trouble with even having a fully functioning ssh server.

@Panaetius
Copy link
Member

I've changed the group id for all images for where we do chown or similar from 1000 to 100(users). We never actually added jovyan to group 1000, so it was completely useless, and the jupyterserver SecurityContext is hardcoded to run as group 100 anyways, so nothing would ever run as group 1000

@Panaetius Panaetius force-pushed the feat-add-ssh-server branch from d41b94c to 4d92ea3 Compare January 26, 2023 15:45
@rokroskar
Copy link
Member

@Panaetius
Copy link
Member

@rokroskar But that wouldn't have worked, no matter what you do in the Dockerfile. Since we do this for the Jupyter servers, K8s will ONLY set that group on the user, no matter what's in /etc/group or anywhere else. So you can add as many groups in the Dockerfile to the jovyan user as you'd like, it'll only ever be in 100.

Also note that groupadd creates the group, and does not add a user to a group (that'd be usermod -G group user), so I'm pretty sure that the above line also wouldn't do anything even if we didn't have that SecurityContext

@rokroskar
Copy link
Member

I can't remember the details anymore, but this was done initially because we had to have the same groups across different kinds of images for the purpose of running chown on repo clone in the init container. This may have changed since the init container has been refactored. It was a hack because the group just needed to exist, but jovyan user didn't actually need to be in the group... iirc

@Panaetius
Copy link
Member

@rokroskar do you know how it manifested? so we can test if it's still needed? And shouldn't the r image create the group if it creates the rstudio user as well?

@rokroskar
Copy link
Member

@Panaetius yes basically the repo permissions would be wrong in either the R or jupyter-based images. Easiest to confirm would be to launch both sets of images from this PR in a live deployment and verify that the repo is rw by the user. Lots of things have changed since that was put in there, so it's entirely possible it's not a limitation anymore.

@Panaetius
Copy link
Member

I tested it with the R image and all looked good (as far as I can tell as a non-R user...).

So I think this can be merged.

@olevski olevski marked this pull request as ready for review February 13, 2023 15:17
@Panaetius Panaetius merged commit 469a5d8 into master Feb 13, 2023
@Panaetius Panaetius deleted the feat-add-ssh-server branch February 13, 2023 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants