chore: replace rand.Reader with nil in keygen and signing calls#8802
chore: replace rand.Reader with nil in keygen and signing calls#8802mkhandker19 wants to merge 12 commits into
Conversation
|
Hi, thanks for the PR! Looks like you edited the files in vendor/ too, which is incorrect. Could you please revert that part? Also, please let us know to what extent you used AI coding tools to generate this PR. It's fine to use them, we just like to be clear. Also, my preference is for PR comments to be written in your own words, not AI. Thanks! |
| "context" | ||
| "crypto/ecdsa" | ||
| "crypto/elliptic" | ||
| "crypto/rand" |
There was a problem hiding this comment.
There are many other files touched by this PR in which this same removal can (and in fact must) be done. Ensure you remove this import from all files which no longer reference the rand package.
Hello! I actually used Claude to help me set up Go, understand the issue, and plan the steps of action. All the code changes were done by me. |
mkhandker19
force-pushed
the
fix-issue-8540
branch
from
5022b2b to
5ca44dd
Compare
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
I've taken the liberty of fixing up this PR: there were significantly more instances of import crypto/rand that needed to be removed, and I've addressed two spots where a rand reader was removed that it shouldn't have been (one where we were using it for true randomness, not signing; and one where we actually didn't need it at all).
With those compile and runtime errors fixed, it looks like this set of changes works. Asking @letsencrypt/boulder-developers for review since this PR now includes changes authored by me.
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Generally looks good; one question I think we need to answer before merging.
| crlBytes, err := x509.CreateRevocationList( | ||
| rand.Reader, | ||
| nil, |
There was a problem hiding this comment.
CreateRevocationList doesn't have documentation saying that the rand argument is ignored. Can you confirm whether that is the case?
There was a problem hiding this comment.
Oof. As far as I can tell, it sort of is, sort of isn't. It depends on the underlying Signer. At the end of the day, the rand is just there to be passed into the Signer's Sign method, but that's about twelve layers of abstraction away from this callsite.
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
Ok, having done further researcch:
- crypto/rsa, crypto/ed25519, and crypto/ecdh now ignore the source of randomness for key generation
- crypto/ecdsa now ignores the source of randomness for key generation and for signing
- (Also, x509.CreateCertificate definitely still needs randomness in the general case, because it uses it to generate a serial number if you don't provide one. So the PR description mentioning CreateCertificate is incorrect.)
So because we use both ECDSA and RSA, we cannot omit the source of randomness to any of our signing operations. That means CreateCertificate, CreateCertificateRequest, CreateRevocationList, etc. We should only be omitting the source of randomness to GenerateKey calls.
In effect, I think that means that this change should be reduced to be test-only. We do lots of key generation in our tests, and we can remove this argument there, but this change shouldn't touch anything else.
@mkhandker19 can you reduce the scope of this PR appropriately?
Right, and to clarify: the PR description says "Go 1.26 deprecated the rand argument to functions like ... x509.CreateCertificate". That's inaccurate. However, I think it would be accurate to say that Boulder supplies an explicit serial number to CreateCertificate in all cases, and so we don't need to pass rand. |
Oops, I posted this too fast and was going off this, from the x509.CreateCertificate documentation:
However, that doesn't say that's all I think as a practical matter it's the case that we don't need rand, since we sign with either ECDSA or RSA, and we don't use RSA-PSS. But I'd prefer not to plumb through that not-needing without some more explicit guards and reference to documentation. Let's omit CreateCertificate from this PR. |
mkhandker19
force-pushed
the
fix-issue-8540
branch
from
6256b4d to
cbd4e9a
Compare
|
@aarongable I've reduced the scope of the PR let me know if any changes are needed. |
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
Thanks, the main code changes look good to me now. However, there are again many files in which "crypto/rand" is now imported but not used.
Please run the tests locally before requesting review; they will immediately show these errors as compiler failures. You can see them in the github action logs (e.g. https://github.com/letsencrypt/boulder/actions/runs/28068067946/job/83764215570?pr=8802) but you should see them locally too.
|
@aarongable Thank you for the feedback. I will work on this and have it resubmitted. |
mkhandker19
force-pushed
the
fix-issue-8540
branch
from
cbd4e9a to
dbca4d5
Compare
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
Hi, several comments on the latest commits:
- Please avoid force-pushing, as it makes it much harder to review. Instead just add new commits addressing the comments we provide.
- You've added some test output files that should not be included in this PR.
- There are still many many files from which the
crypto/randimport needs to be removed.
…t#8853) This reverts PR letsencrypt#8606, which was merged as commit abfceb5. The revert is clean with the exception of one line in cmd/shell.go, as a result of the function signature change in letsencrypt#8833. We are reverting because automated analysis found several issues (mostly, but not all, minor) with the sweeping changes made in the slog PR. We will reland when the identified issues have been fixed. Part of letsencrypt#8557
…etsencrypt#8856) I missed these checks in the WFE in stage 1, while making sure that all components exchanged _both_ string-type Authz IDs _and_ int64-type Authz IDs. Because stage 1 has been deployed for a while, this pivots only the WFE around the type change so that a follow-up change (and deployment cycle) can successfully remove the string-type ID support.
…crypt#8863) The un-anchored pattern was also matching "boulder/ratelimits"
Cert-checker failing when it finds bad certs has undesirable behavior on the deploy side (e.g. job failure due to bad cert discovery leads to unnecessary redeployment of the job and re-firing of associated alerts) and IMO finding bad certs is not a failure of the cert-checker (in fact, it did it's job, woohoo!)
…#8864) The redis RingOptions config knob for DialerRetries was introduced by a recent `go-redis` vendored library update (letsencrypt#8815). This change allows for the value of the option to be loaded from component config files.
This change ensures Boulder never trusts a CAA DNS query response when the TC (truncated) bit is set. Because the truncated portion may contain a valid CAA record that forbids issuance, it is not safe to trust a partial response. A truncated record like `CAA 0 issue ";"` could be safely ignored given a non-truncated `CAA 0 issue "letsencrypt.org"`; but if a record with an unknown/unsupported Property tag and the critical bit set, e.g. `CAA 128 unknown "foo"`, is ever truncated, that's a definitive misissuance per RFC 8659, [Section 4.1](https://datatracker.ietf.org/doc/html/rfc8659#section-4.1-6.2.2.2). > __Bit 0, Issuer Critical Flag:__ If the value is set to "1", the Property is critical. A CA __MUST NOT__ issue certificates for any FQDN if the Relevant RRset for that FQDN contains a CAA critical Property for an unknown or unsupported Property Tag. In practice, it would require a freak accident DNS misconfiguration or a malicious construction of records (an exercise left to the reader) to trigger this bug. And in any case, doing so doesn't grant an attacker any issuance capabilities they don't already have. If you have access to a domain's DNS zone to exploit this bug, you can already edit CAA records and validate DNS-01 challenges sufficient to issue a legitimate certificate. Nevertheless, this bug represents an attacker-controlled compliance risk and we should patch it.
7 participants
What does this PR do?
Replaces all
crypto/rand.Readerarguments in keygen and signing callsites with
nil, in preparation for Go 1.26 compatibility. In Go 1.26,cryptographic functions ignore the
randargument and use a secureinternal randomness source instead. This PR removes the now-redundant
explicit
rand.Readerreferences and cleans up the resulting unused"crypto/rand"imports.Why was this PR needed?
Go 1.26 deprecated the
randargument to functions likeecdsa.GenerateKey,rsa.GenerateKey,rsa.SignPKCS1v15,ecdsa.Sign, andx509.CreateCertificate. Passingrand.Readerexplicitly is now misleading since the argument is silently ignored.
Replacing it with
nilaligns the codebase with the new Go standardand removes unnecessary imports.
Reference: golang/go#70942
What are the relevant issue numbers?
Closes #8540
Does this PR meet the acceptance criteria?