Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 21, 2025
Merged

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

merged 1 commit into from
Feb 21, 2025

Conversation

ThomasK33
Copy link
Member

@ThomasK33 ThomasK33 commented Feb 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 Kosiewski tk@coder.com

@ThomasK33 Graphite App
Copy link
Member Author

ThomasK33 commented Feb 19, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ThomasK33 ThomasK33 marked this pull request as ready for review February 19, 2025 17:35
@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from 47bcb17 to 92e2d6d Compare February 19, 2025 17:35
@ThomasK33 ThomasK33 changed the title fix(agentssh): pin random seed for RSA key generation fix(agent/agentssh): use deterministic host key for SSH server Feb 19, 2025
@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from 92e2d6d to f744aa5 Compare February 19, 2025 17:38
@ThomasK33 ThomasK33 mentioned this pull request Feb 19, 2025
Merged
@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from f744aa5 to d74cb78 Compare February 19, 2025 18:58
@ThomasK33 ThomasK33 marked this pull request as draft February 19, 2025 19:00
@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from d74cb78 to 38f9dfc Compare February 20, 2025 11:16
@ThomasK33 ThomasK33 marked this pull request as ready for review February 20, 2025 11:53
@ThomasK33 ThomasK33 requested a review from mafredri February 20, 2025 11:55
mafredri
mafredri reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@mafredri mafredri 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.

spikecurtis
spikecurtis reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@spikecurtis spikecurtis 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

@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from 38f9dfc to cab51d9 Compare February 20, 2025 12:53
spikecurtis
spikecurtis approved these changes Feb 20, 2025
View reviewed changes
@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from cab51d9 to 9c85cde Compare February 20, 2025 16:10
mafredri
mafredri approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@mafredri mafredri 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 the agent 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.

@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from 9c85cde to 658db5a Compare February 21, 2025 10:33
@ThomasK33 Graphite App
Copy link
Member Author

ThomasK33 commented Feb 21, 2025 •
edited
Loading

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

Since the agent package does not validate (or even skip) the SSH key validation, I've added a test to the cli ssh tests. I duplicated the TestSSH/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 about rsa.GenerateKey being non-deterministic, and they purposefully cut off the first byte in 50% of the cases, to ensure that trait. 1 and 2

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.

@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from 658db5a to d0206e7 Compare February 21, 2025 12:40
@ThomasK33
fix(agent/agentssh): pin random seed for RSA key generation
ad275dc 
Change-Id: I8c7e3070324e5d558374fd6891eea9d48660e1e9
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33 ThomasK33 force-pushed the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch from d0206e7 to ad275dc Compare February 21, 2025 13:06
@ThomasK33 ThomasK33 merged commit 6607464 into main Feb 21, 2025
31 checks passed
@ThomasK33 ThomasK33 deleted the thomask33/02-19-fix_agentssh_pin_random_seed_for_rsa_key_generation branch February 21, 2025 13:58
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@spikecurtis spikecurtis spikecurtis approved these changes

Assignees

@ThomasK33 ThomasK33

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