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

Add session ssh support #26

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Add session ssh support #26

wants to merge 17 commits into from

Conversation

seanrmurphy
Copy link
Contributor

No description provided.

@seanrmurphy seanrmurphy marked this pull request as draft March 22, 2023 17:04
Copy link
Contributor

@aledegano aledegano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this RFC is very good and makes a good case for the MITM proxy.

I would spend a few words in the last section "Unresolved questions" about the "Renku data store" and how we could solve this together with the Resource Access Control RFC and its requirement for a backend.

Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @seanrmurphy! It's very useful to have all the options laid out like this. After reading through it, I can see that the MIMP is ultimately the lowest-friction solution, but it comes at a cost of implementing and maintaining a new service. There are, however, other components in this design that would already make the jumphost solution much better, namely the Renku Data Store and associated APIs + supporting services. My take on this is that we could start by implementing those pieces first since they can also be used more widely in the platform, and then re-assess the need to swap out the jumphost for the MIMP.

@seanrmurphy
Copy link
Contributor Author

I've gone through this again, making modifications based on the feedback received.

@seanrmurphy seanrmurphy marked this pull request as ready for review May 4, 2023 13:36
@rokroskar
Copy link
Member

Thanks @seanrmurphy for the update on this - I don't have any other substantive comments, other than somehow we need to decide if implementing a new service is worth the upkeep cost.

My personal take is that we should implement the key handling part no matter what; that is somehow a common piece between the current (interim) solution and the MITM approach suggested here. The majority of the user-facing part has already been implemented in the CLI; we would need to swap out the commit+push of a public key with an API call to the service. There are also some checks in place to determine if a project is "ssh-ready" which need to be adapted, but that seems pretty trivial.

Once that is done, the primary difference (in my view, please correct if that's wrong) between the MITM solution and the current one is the monitoring. What kind of monitoring would we want to have in this case? Can some of it be achieved by tweaking logging on the jump server? If not, how much do we care? If the answer is that we care a lot, then the MITM approach is probably worth the effort.

Maybe one other aspect to consider (I'm not sure exactly where it fits) is that we want to transition to having a single session per user per project (right now a user can launch many sessions from different commits of any project). This simplifies the session naming a lot and would mean that the ssh configs are basically created once and left alone (in the case of the current jumphost implementation) and in the case of MITM proxy, the routing is also simplified.

@rokroskar
Copy link
Member

I found another reason for using the MITM approach - using rclone for filesystem access (it doesn't use standard ssh so it doesn't respect the ssh config and does not have jump proxy support)

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.

3 participants