Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix(agent/agentssh): use deterministic host key for SSH server#16626

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

ThomasK33
Copy link
Member

@ThomasK33ThomasK33 commentedFeb 19, 2025•
edited
Loading

Fixes:#16490

The Agent's SSH server now initially generates fixed host keys and, once it receives its manifest, generates and replaces that host key with the one derived from the workspace ID, ensuring consistency across agent restarts. This prevents SSH warnings and host key verification errors when connecting to workspaces through Coder Desktop.

While deterministic keys might seem insecure, the underlying Wireguard tunnel already provides encryption and anti-spoofing protection at the network layer, making this approach acceptable for our use case.

Change-Id: I8c7e3070324e5d558374fd6891eea9d48660e1e9
Signed-off-by: Thomas Kosiewskitk@coder.com

@ThomasK33Graphite App
Copy link
MemberAuthor

ThomasK33 commentedFeb 19, 2025•
edited
Loading

@ThomasK33ThomasK33 marked this pull request as ready for reviewFebruary 19, 2025 17:35
@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from47bcb17 to92e2d6dCompareFebruary 19, 2025 17:35
@ThomasK33ThomasK33 changed the titlefix(agentssh): pin random seed for RSA key generationfix(agent/agentssh): use deterministic host key for SSH serverFeb 19, 2025
@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from92e2d6d tof744aa5CompareFebruary 19, 2025 17:38
@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch fromf744aa5 tod74cb78CompareFebruary 19, 2025 18:58
@ThomasK33ThomasK33 marked this pull request as draftFebruary 19, 2025 19:00
@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch fromd74cb78 to38f9dfcCompareFebruary 20, 2025 11:16
@ThomasK33ThomasK33 marked this pull request as ready for reviewFebruary 20, 2025 11:53
Copy link
Member

@mafredrimafredri 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 you found a great approach here, nice work! Just one minor nit wrt the seed source, otherwise LGTM.

I would still like to see a test that ensures the this behaves as expected.

Copy link
Contributor

@spikecurtisspikecurtis left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Couple comments inline

@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from38f9dfc tocab51d9CompareFebruary 20, 2025 12:53
@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch fromcab51d9 to9c85cdeCompareFebruary 20, 2025 16:10
Copy link
Member

@mafredrimafredri left a comment•
edited
Loading

Choose a reason for hiding this comment

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

It might be worthwhile to add one more test in theagent package to ensure the signer is set to the expected key via the manifest, but I won't block on its addition.

Context: Right now the tests verify that the host key change works since a host key has been omitted by default, so if update isn't called, tests will fail. An unlikely scenario would be that someone added back the default key (for whatever reason), and broke the update method. No test would catch this currently, i.e. there's nothing to guard/verify the change we wanted to achieve in this PR.

@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from9c85cde to658db5aCompareFebruary 21, 2025 10:33
@ThomasK33Graphite App
Copy link
MemberAuthor

ThomasK33 commentedFeb 21, 2025•
edited
Loading

Adding the test was a great idea; you should have blocked the PR for its addition. 😅

Since theagent package does not validate (or even skip) the SSH key validation, I've added a test to the cli ssh tests. I duplicated theTestSSH/Stdio test and added a host key validation, which worked fine on the first run locally, but then failed in CI and then worked again locally until it didn't.

As it turns out, Go's stdlib is very specific aboutrsa.GenerateKey being non-deterministic, and they purposefully cut off the first byte in 50% of the cases, to ensure that trait.1 and2

Long story short, we're now computing RSA keys by hand to overcome this limitation, and we now consistently generate the same key because we're not cutting off that first byte.

On a side note, we should consider rolling out this host key validation for all CLI SSH tests in the future, but I don't want to blow the scope of this PR.

@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from658db5a tod0206e7CompareFebruary 21, 2025 12:40
Change-Id: I8c7e3070324e5d558374fd6891eea9d48660e1e9Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33ThomasK33force-pushed thethomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch fromd0206e7 toad275dcCompareFebruary 21, 2025 13:06
@ThomasK33ThomasK33 merged commit6607464 intomainFeb 21, 2025
31 checks passed
@ThomasK33ThomasK33 deleted the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branchFebruary 21, 2025 13:58
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mafredrimafredrimafredri approved these changes

@spikecurtisspikecurtisspikecurtis approved these changes

Assignees

@ThomasK33ThomasK33

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Agent SSH server should use a consistent key over workspace restarts
3 participants
@ThomasK33@mafredri@spikecurtis
[8]ページ先頭
©2009-2025 Movatter.jp