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): ensure RSA key generation always produces valid keys#16694

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 25, 2025•
edited
Loading

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 whenPrecompute is called.

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

@ThomasK33Graphite App
Copy link
MemberAuthor

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

@ThomasK33ThomasK33 marked this pull request as ready for reviewFebruary 25, 2025 08:48
@ThomasK33ThomasK33 changed the titlefix(agentssh): ensure RSA key generation always produces valid keysfix(agent/agentssh): ensure RSA key generation always produces valid keysFeb 25, 2025
Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

TIL aboutbig.ProbablyPrime() and the source code is interesting reading!
Approving to unblock, but a couple of things:

  1. Might be good to have this in its own package
  2. Might also be good to add a benchmark (not for benchmarking purposes, but for proving non-panicky purposes).

@ThomasK33Graphite App
Copy link
MemberAuthor

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.

@ThomasK33ThomasK33force-pushed thethomask33/02-25-fix_agentssh_ensure_rsa_key_generation_always_produces_valid_keys branch from2914761 to77d1812CompareFebruary 25, 2025 12:25
@ThomasK33Graphite App
Copy link
MemberAuthor

ThomasK33 commentedFeb 25, 2025•
edited
Loading

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

❯ gotest -fuzz=FuzzGenerateDeterministicKey ./agent/agentrsafuzz: elapsed: 0s, gathering baseline coverage: 0/3 completedfuzz: elapsed: 0s, gathering baseline coverage: 3/3 completed, now fuzzing with 11 workersfuzz: 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)PASSok      github.com/coder/coder/v2/agent/agentssh/agentsshrsa    249.082s
johnstcn reacted with rocket emoji

@ThomasK33ThomasK33force-pushed thethomask33/02-25-fix_agentssh_ensure_rsa_key_generation_always_produces_valid_keys branch 2 times, most recently froma2eafe3 tof0fe6cdCompareFebruary 25, 2025 12:36
@ThomasK33ThomasK33force-pushed thethomask33/02-25-fix_agentssh_ensure_rsa_key_generation_always_produces_valid_keys branch fromf0fe6cd tod95abcdCompareFebruary 25, 2025 13:06
Change-Id: I0a453e1e1f8c638e40e7a4b87a6d0d7299e1cb5dSigned-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33ThomasK33force-pushed thethomask33/02-25-fix_agentssh_ensure_rsa_key_generation_always_produces_valid_keys branch fromd95abcd tocd3a104CompareFebruary 25, 2025 13:28
@ThomasK33ThomasK33 merged commit38c0e8a intomainFeb 26, 2025
31 checks passed
@ThomasK33ThomasK33 deleted the thomask33/02-25-fix_agentssh_ensure_rsa_key_generation_always_produces_valid_keys branchFebruary 26, 2025 10:45
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 26, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

Assignees

@ThomasK33ThomasK33

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@ThomasK33@johnstcn
[8]ページ先頭
©2009-2025 Movatter.jp