Skip to content

Connect My Computer: Derive agent label from username in main process#34302

Merged
ravicious merged 3 commits intomasterfrom
ravicious/agent-label
Nov 8, 2023
Merged

Connect My Computer: Derive agent label from username in main process#34302
ravicious merged 3 commits intomasterfrom
ravicious/agent-label

Conversation

@ravicious
Copy link
Copy Markdown
Member

When Connect sets up a Connect My Computer agent, it must execute teleport node configure with the arg --labels=teleport.dev/connect-my-computer/owner=<username>. The previous version would pass the labels like this:

  1. The renderer calls tshd (CreateConnectMyComputerNodeToken).
  2. tshd returns labels to the renderer (CreateConnectMyComputerNodeTokenResponse).
  3. The renderer passes the labels verbatim to the main process.
  4. The main process executes teleport node configure with the labels.

This PR makes it so that tshd returns no labels and instead the renderer passes just the username to the main process and the main process derives the agent label from that username.

The original flow was implemented so that the name of the label, teleport.dev/connect-my-computer/owner, is not hardcoded in the JS code and instead is taken directly from the Go code.

However, soon we'll have to hardcode a label in JS anyway. In order to implement polling for the Discover flow, I'll have to pass an extra hidden label anyway. The Go code won't even need to know about this label as it'll be shared exclusively between the Web UI and Connect. I'm going to create a constants.go equivalent in the JS code that's going to live in the shared package to accomplish that.

On top of that, the IPC message sent from the renderer to the main process which creates a config file accepts arbitrary labels, so it technically has a greater attack surface than an IPC message which does not implement. If an attacker compromises the renderer process, they'll be able to create nodes with arbitrary labels. The new implementation does not prevent that, you can still pass a username like alice,foo=bar which will result in the node also getting the foo=bar label. But at least the node will always have teleport.dev/connect-my-computer/owner set to some kind of value.


To completely get rid of the problem with that "injection" attack, we'd need to read the username from within the main process. However, currently the main process has no access to information from tshd. For the same reason, the empty state in Connect is not able to link to the Web UI.

@ravicious ravicious added backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry labels Nov 7, 2023
@ravicious ravicious requested review from avatus and gzdunek November 7, 2023 17:14

const labels = Object.entries({
// TODO(ravicious): Move this to a JavaScript version of constants.go.
'teleport.dev/connect-my-computer/owner': args.username,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As I wrote in the description, this is just temporary and the next commit will move this and another label to a better location. I just didn't want to spend time on thinking about it upfront since coming up with that new location wasn't needed for this particular PR.

@ravicious ravicious added this pull request to the merge queue Nov 8, 2023
Merged via the queue into master with commit 90e5fab Nov 8, 2023
@ravicious ravicious deleted the ravicious/agent-label branch November 8, 2023 17:40
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v14 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants