Skip to content

[v17] Fix Desktop Connection Error When HostID Is Not a UUID (#63908)#64546

Open
rhammonds-teleport wants to merge 2 commits intobranch/v17from
rhammonds/backport-63908-branch/v17
Open

[v17] Fix Desktop Connection Error When HostID Is Not a UUID (#63908)#64546
rhammonds-teleport wants to merge 2 commits intobranch/v17from
rhammonds/backport-63908-branch/v17

Conversation

@rhammonds-teleport
Copy link
Contributor

@rhammonds-teleport rhammonds-teleport commented Mar 12, 2026

Backports #63908

Changelog: Fixed bug that causes Windows desktop connection errors on EC2 joined nodes.

Manual Test Plan

Test Environment

  • Local cluster with Windows Desktop Service and a reachable Windows Desktop instance in EC2.

Test Cases

  • Unit test coverage validates proper handling of UUID strings, non-UUID strings, and empty strings.
  • Manual validation of RDP client connection.

* Fix windows desktop connection error when the configured HostID is not a UUID.

* Rename 'hostID' to 'rdpClientID'
@rhammonds-teleport rhammonds-teleport force-pushed the rhammonds/backport-63908-branch/v17 branch from 7a21677 to 103237d Compare March 12, 2026 03:12
@rhammonds-teleport rhammonds-teleport marked this pull request as ready for review March 12, 2026 19:07
@github-actions github-actions bot requested review from probakowski and zmb3 March 12, 2026 19:08
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 103237d7b6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".


// Fall back to taking a hash of the rdpClientID
hash := [16]byte{}
copy(hash[:], md5.New().Sum([]byte(id)))

Choose a reason for hiding this comment

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

P1 Badge Hash non-UUID host IDs before truncating to 16 bytes

For non-UUID HostID values, md5.New().Sum([]byte(id)) does not compute md5(id); Hash.Sum appends the current digest to the provided slice, so this returns id || md5(""). Because the next line copies only 16 bytes, any non-empty IDs are effectively truncated/prefixed rather than hashed, which can cause client ID collisions (for example IDs sharing the same first 16 bytes) and undermines the fallback uniqueness this change is meant to preserve.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct. I misunderstood the Hash interface. I've updated both backports and will need to get this change into master as well.

Side note: It would be nice to have the codex run its review in the draft stage so that we can find and fix things before pinging reviewers.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant