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

RFD 133 - Connect My Computer #27815

Merged
merged 12 commits into from
Jul 12, 2023
Merged

RFD 133 - Connect My Computer #27815

merged 12 commits into from
Jul 12, 2023

Conversation

ravicious
Copy link
Member

@ravicious ravicious commented Jun 13, 2023

Rendered version

The RFD is missing the Security section – we didn't start with the RFD 0000 template and we forgot about including a separate Security section. I'll add it tomorrow.

@ravicious ravicious force-pushed the rfd/0133-connect-my-computer branch from 802101e to d5588aa Compare June 13, 2023 17:58
@ravicious ravicious marked this pull request as ready for review June 13, 2023 17:59
@github-actions github-actions bot requested review from jakule and Joerger June 13, 2023 18:00
@github-actions github-actions bot added rfd Request for Discussion size/md labels Jun 13, 2023
@ravicious ravicious requested review from klizhentas, zmb3 and xinding33 and removed request for jakule and Joerger June 13, 2023 18:01
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Great detail here, thanks for the effort on this!

rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
@ravicious ravicious mentioned this pull request Jun 15, 2023
8 tasks
rfd/0133-connect-my-computer.md Outdated Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
rfd/0133-connect-my-computer.md Show resolved Hide resolved
Comment on lines +477 to +478
The feature is supported only on macOS and Linux because the Teleport SSH agent does not work on
Windows.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would it take to extend CMC to Windows (perhaps via something like Windows Subsystem for Linux)? The reason I'm asking is because Windows is actually the biggest use case for Teleport Connect. By not supporting Windows, we are missing out on potentially closer to 50% of all Connect users.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zmb3 @gzdunek Thoughts on supporting Connect My Computer on Windows through WSL?

I don't know why we haven't considered WSL all this time, I think it's a viable option.

I just ran a node agent through WSL and it appears to be working just fine. There's a few failing unit tests, but they might as well be due to me not interacting with Windows firewall prompts fast enough.

The FAQ doesn't list any downside between WSL and a regular Linux distro which would be a showstopper for us. The WSL team did some tests to ensure WSL compatibility.

The only downside is that setting up WSL is an interactive process done through CLI. You need to select a distro (which we can preselect for you) and provide a password for the new unix account. But we can just open a new tab with a terminal, let the user set up WSL and then continue the regular Connect My Computer setup. I bet some part of the target audience will even have WSL installed.

I think we could spend a little bit more time on the MVP if it means reaching twice as much users. We could first build a version that works on Unix only and then see how we can fit it into WSL.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works, I'm all for supporting it.

The only downside is that setting up WSL is an interactive process done through CLI. You need to select a distro (which we can preselect for you) and provide a password for the new unix account.

Is this accurate? AFAIK the distro is preselected be default (Ubuntu) and then you have to provide a username and password for the account (but I failed to verify that on a Parallels VM, it looks like WSL2 doesn't work there).

I think we could spend a little bit more time on the MVP if it means reaching twice as much users. We could first build a version that works on Unix only and then see how we can fit it into WSL.

+1
It seems to me that after setting up WSL, we would have to take care of two things:

  • spawning wsl teleport instead of teleport,
  • making the mechanism that checks if the parent PID hasn't changed work on Windows.

But probably there would be something more :)

Copy link
Member Author

@ravicious ravicious Jun 28, 2023

Choose a reason for hiding this comment

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

Is this accurate? AFAIK the distro is preselected be default (Ubuntu) and then you have to provide a username and password for the account

Oh, I've been installing WSL on a couple of machines during Rails Girls and the guides explicitly specified Ubuntu. But you still need to provide a password, so the install is still interactive.

There seems to be a way to set up WSL without user interaction (microsoft/WSL#3369 (comment)). But as WSL is a system-wide thing and not specific to Teleport or Connect, I'd rather have the user install it in a separate interactive step. Running the interactive installer would also show the progress bars and errors OOTB.

but I failed to verify that on a Parallels VM, it looks like WSL2 doesn't work there

Yeah, you can downgrade it to WSL1 for now and use it with Parallels, that's how I tested everything.

  • making the mechanism that checks if the parent PID hasn't changed work on Windows

There's a couple of issues mentioning some inconsistencies between WSL1 and WSL2, but there's a slight chance that the binary running on WSL we'll be able to use the same PID monitoring mechanism as Unix binaries, without having to implement Windows-specific handling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no problem with trying to support Windows via WSL, but I think we should ship mac/Linux support first.

We don't have enough experience running Teleport on WSL to know what type of issues we might encounter, and I don't want those unknowns from preventing us from shipping something for mac/Linux.

Copy link
Member Author

Choose a reason for hiding this comment

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

@xinding33 Are you fine with us shipping macOS & Linux version first and following up with Windows after? We'd prioritize this over the app access features we discussed when we last met.

Zac is right that we don't have much experience with running Teleport itself on WSL, let alone setting up WSL and running a WSL process from Connect. It might take an extra week or two to add support for that.

I talked briefly with Zac about it yesterday. If the timelines align, that is we finish working on the MVP of Connect My Computer and there's still a week or two weeks of development time left before the next major or minor version of Teleport, we'll focus on shipping WSL in that next version. Otherwise, the Unix version would be a priority.


## Details

After signing into a Team plan cluster, which currently has no connected nodes, the user sees a
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for reviving this thread but I think it'll actually be rare for a user to download Teleport Connect and log in to a completely empty cluster. I believe the ultimate goal of CMC to be virality. The feature is meant to bring more users to Teleport. To that end, I think we should show the "Connect your Computer" button as long as the current user's computer is NOT connected. Of course, we don't have to make it as visible as when there are no resources connect at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's more or less the plan with regards to the UI. See the new designs made by Kenny, the empty state and the button to manage Connect My Computer in the top right of the resource table.

The button will always be visible, as long as the user's account meets all the requirements to run Connect My Computer. The copy in the empty state will be shown only if there's no resources at all.

Comment on lines +402 to +406
#### Keeping the agent up to date

Upgrading the agent is done by upgrading Connect. On the first agent launch after a Connect upgrade,
Connect sees that the agent matching its version is missing and downloads a matching version of the
agent. This follows from the behavior described in the “Downloading the agent” section.
Copy link
Member Author

Choose a reason for hiding this comment

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

@roraback Question about whether the update process of an agent should be left interaction-free or if we should force the user to manually start the agent update after an app gets updated.

We imagined that the update process of the agent is going to look like this:

  1. The user is logged in to a cluster and closes Connect 13.0.0 without manually stopping Connect My Computer.
  2. The user upgrades Connect to 13.0.1 and launches Connect again.
  3. Since Connect My Computer was not manually stopped and the user has a valid cert, Connect attempts to start the agent soon after the app starts.
  4. Before actually starting the agent, Connect sees that the agent version 13.0.1 is missing from the cache.
  5. Connect downloads the agent 13.0.1.
  6. The new agent is launched as usual.

Is it fine if this is kept interaction-free? There should be no prompt about running a new program. The only downside is that downloading the new agent might take a minute or two or even fail (depending on the internet connection).

Of course this would be accompanied by a separate state of Connect My Computer which would clearly indicate that the download is in progress, perhaps even on the level of the button in the resource table.

The alternative

Alternatively, after a successful app upgrade we could show an extra icon next to the Connect My Computer button indicating that a manual intervention is needed. Connect would start downloading the agent only after the user navigates to the status page and clicks a button.

logins: [bob]
```

Connect creates this role and adds it to the current user. Since the user cert in Connect at the
Copy link
Member Author

Choose a reason for hiding this comment

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

@zmb3 @xinding33 @klizhentas Could we limit Connect My Computer to only local users? This likely won't impact our target audience much, as initially Team plan clusters consist solely of local users until SSO is set up.

The context is that Nic pointed out that adding roles to SSO users isn't straightforward, as their roles are dictated by the auth connector setup. An SSO user is an ephemeral resource on the backend, so we cannot just add a role to it.

We need to create a role in the first place to allow users to log into the Connect My Computer node with their system user. For example, [email protected] should be able to log in as bob.

I looked at the SSO docs and I don't see a way around it, at least not without significant effort past what we have originally planned. We'd likely need to treat Connect My Computer as a first-class citizen on the backend, while so far we wanted to create the MVP by using existing cluster features, without pushing changes to the backend.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's just limit to local users for now. For our hobbyist/home lab use case it will almost always be the first local user who signed up, especially if we get the onboarding/invite flow right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we limit Connect My Computer to only local users

Are you looking to enforce this? I think any enforcement we can make here would be good. IP Pinning comes to mind, but it's an enterprise only feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we will enforce it by only showing the option for local users.

IP pinning won't be necessary here.


The setup steps are displayed in a similar fashion to Cloud account setup steps when the cluster is
being spun up for the first time. Once the setup is done, the setup tab automatically transitions to
showing the status of the agent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will Teleport Connect be usable while this setup is happening, or will the user be effectively blocked at a loading screen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will Teleport Connect be usable while this setup is happening, or will the user be effectively blocked at a loading screen?

The app will be usable. In theory this means you can switch to another workspace and set up Connect My Computer there at the same time too. The MVP will simply ensure that the second setup won't corrupt the first one.

logins: [bob]
```

Connect creates this role and adds it to the current user. Since the user cert in Connect at the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's just limit to local users for now. For our hobbyist/home lab use case it will almost always be the first local user who signed up, especially if we get the onboarding/invite flow right.

@ravicious
Copy link
Member Author

@jentfoo @xinding33 @klizhentas Anything to add? If not we're going to merge it on Wednesday.

@xinding33 @klizhentas Reminder that for the MVP, we'll most probably have to make Connect My Computer work with local users only (no SSO): #27815 (comment)

@jentfoo I think I addressed all comments from your review.

Copy link
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

Approving as I believe this is in a state were we can do an MVP. That said there are some risks that we need to articulate to any customers adopting this feature, and hopefully in the future improve.

Future improvements with IP pinning, limiting what role can be created, and a more role based join token can make this feature more secure and easier to use by a larger team.

@ravicious ravicious added this pull request to the merge queue Jul 12, 2023
Merged via the queue into master with commit 4c680b5 Jul 12, 2023
@ravicious ravicious deleted the rfd/0133-connect-my-computer branch July 12, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfd Request for Discussion size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants