fix(agent/agentssh): ensure RSA key generation always produces valid keys #16694
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
johnstcn
approved these changes
Feb 25, 2025
There was a problem hiding this comment.
TIL about big.ProbablyPrime() and the source code is interesting reading!
Approving to unblock, but a couple of things:
- Might be good to have this in its own package
- Might also be good to add a benchmark (not for benchmarking purposes, but for proving non-panicky purposes).
|
I don't believe I need to get unblocked here. It’s a somewhat rare and random occurrence for this panic to be triggered, so I think it's reasonable to take the time to clean this up. |
2914761 to
77d1812
Compare
|
So, I've added a benchmark for benchmarking, and a fuzz test for making sure that the function won't randomly panic given some funny seed value. I let it run/marinate for some time and no panics were found ❯ go test -fuzz=FuzzGenerateDeterministicKey ./agent/agentrsa
fuzz: elapsed: 0s, gathering baseline coverage: 0/3 completed
fuzz: elapsed: 0s, gathering baseline coverage: 3/3 completed, now fuzzing with 11 workers
fuzz: elapsed: 3s, execs: 61 (20/sec), new interesting: 55 (total: 58)
fuzz: elapsed: 6s, execs: 123 (21/sec), new interesting: 105 (total: 108)
fuzz: elapsed: 9s, execs: 185 (21/sec), new interesting: 139 (total: 142)
fuzz: elapsed: 12s, execs: 252 (22/sec), new interesting: 172 (total: 175)
fuzz: elapsed: 15s, execs: 316 (21/sec), new interesting: 185 (total: 188)
fuzz: elapsed: 18s, execs: 382 (22/sec), new interesting: 199 (total: 202)
fuzz: elapsed: 21s, execs: 439 (19/sec), new interesting: 209 (total: 212)
fuzz: elapsed: 24s, execs: 501 (21/sec), new interesting: 219 (total: 222)
fuzz: elapsed: 27s, execs: 560 (20/sec), new interesting: 227 (total: 230)
fuzz: elapsed: 30s, execs: 620 (20/sec), new interesting: 231 (total: 234)
fuzz: elapsed: 33s, execs: 693 (24/sec), new interesting: 235 (total: 238)
fuzz: elapsed: 36s, execs: 761 (23/sec), new interesting: 240 (total: 243)
fuzz: elapsed: 39s, execs: 822 (20/sec), new interesting: 241 (total: 244)
fuzz: elapsed: 42s, execs: 890 (23/sec), new interesting: 243 (total: 246)
fuzz: elapsed: 45s, execs: 949 (20/sec), new interesting: 245 (total: 248)
fuzz: elapsed: 48s, execs: 1007 (19/sec), new interesting: 246 (total: 249)
fuzz: elapsed: 51s, execs: 1063 (19/sec), new interesting: 247 (total: 250)
fuzz: elapsed: 54s, execs: 1116 (18/sec), new interesting: 247 (total: 250)
fuzz: elapsed: 57s, execs: 1186 (23/sec), new interesting: 248 (total: 251)
fuzz: elapsed: 1m0s, execs: 1259 (24/sec), new interesting: 249 (total: 252)
fuzz: elapsed: 1m3s, execs: 1335 (25/sec), new interesting: 251 (total: 254)
fuzz: elapsed: 1m6s, execs: 1411 (25/sec), new interesting: 251 (total: 254)
fuzz: elapsed: 1m9s, execs: 1478 (22/sec), new interesting: 252 (total: 255)
fuzz: elapsed: 1m12s, execs: 1556 (26/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m15s, execs: 1632 (25/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m18s, execs: 1709 (26/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m21s, execs: 1782 (24/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m24s, execs: 1857 (25/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m27s, execs: 1912 (18/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m30s, execs: 1992 (27/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m33s, execs: 2068 (25/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m36s, execs: 2136 (23/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m39s, execs: 2205 (23/sec), new interesting: 253 (total: 256)
fuzz: elapsed: 1m42s, execs: 2274 (23/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 1m45s, execs: 2349 (25/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 1m48s, execs: 2417 (23/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 1m51s, execs: 2485 (23/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 1m54s, execs: 2550 (22/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 1m57s, execs: 2624 (25/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m0s, execs: 2694 (23/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m3s, execs: 2773 (26/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m6s, execs: 2838 (22/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m9s, execs: 2918 (27/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m12s, execs: 2989 (24/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m15s, execs: 3066 (26/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m18s, execs: 3138 (24/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m21s, execs: 3212 (25/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m24s, execs: 3244 (11/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m27s, execs: 3280 (12/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m30s, execs: 3327 (16/sec), new interesting: 254 (total: 257)
fuzz: elapsed: 2m33s, execs: 3375 (16/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m36s, execs: 3432 (19/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m39s, execs: 3504 (24/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m42s, execs: 3575 (24/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m45s, execs: 3637 (21/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m48s, execs: 3697 (20/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m51s, execs: 3748 (17/sec), new interesting: 255 (total: 258)
fuzz: elapsed: 2m54s, execs: 3787 (13/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 2m57s, execs: 3843 (19/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m0s, execs: 3904 (20/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m3s, execs: 3963 (20/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m6s, execs: 4033 (23/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m9s, execs: 4099 (22/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m12s, execs: 4164 (22/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m15s, execs: 4232 (23/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m18s, execs: 4304 (24/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m21s, execs: 4373 (23/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m24s, execs: 4447 (25/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m27s, execs: 4520 (24/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m30s, execs: 4593 (24/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m33s, execs: 4670 (26/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m36s, execs: 4737 (22/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m39s, execs: 4817 (27/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m42s, execs: 4891 (25/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m45s, execs: 4967 (25/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m48s, execs: 5042 (25/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m51s, execs: 5119 (26/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m54s, execs: 5199 (27/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 3m57s, execs: 5267 (23/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 4m0s, execs: 5338 (24/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 4m3s, execs: 5409 (24/sec), new interesting: 256 (total: 259)
fuzz: elapsed: 4m6s, execs: 5472 (21/sec), new interesting: 256 (total: 259)
^Cfuzz: elapsed: 4m9s, execs: 5519 (18/sec), new interesting: 256 (total: 259)
PASS
ok github.com/coder/coder/v2/agent/agentssh/agentsshrsa 249.082s |
a2eafe3 to
f0fe6cd
Compare
johnstcn
approved these changes
Feb 25, 2025
johnstcn
reviewed
Feb 25, 2025
f0fe6cd to
d95abcd
Compare
Change-Id: I0a453e1e1f8c638e40e7a4b87a6d0d7299e1cb5d Signed-off-by: Thomas Kosiewski <tk@coder.com>
d95abcd to
cd3a104
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Modify the RSA key generation algorithm to check that GCD(e, p-1) = 1 and
GCD(e, q-1) = 1 when selecting prime numbers, ensuring that e and φ(n)
are coprime. This prevents ModInverse from returning nil, which would
cause private key generation to fail and result in a panic when
Precomputeis called.Change-Id: I0a453e1e1f8c638e40e7a4b87a6d0d7299e1cb5d
Signed-off-by: Thomas Kosiewski tk@coder.com