fix: randomize webhook certificate serial numbers#1067
Conversation
Signed-off-by: pm-ju <pmdevops29@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces hardcoded certificate serial numbers with randomly generated 128-bit integers to improve security and compliance. It introduces a generateSerialNumber utility and updates the self-signed certificate generation logic to use it. Corresponding tests have been added to ensure serial numbers are positive and unique. Feedback suggests pre-calculating the serial number limit for better performance and using the Cmp method for more robust big.Int comparisons in the test suite.
| limit := new(big.Int).Lsh(big.NewInt(1), CertSerialNumberBits) | ||
| limit.Sub(limit, big.NewInt(1)) |
There was a problem hiding this comment.
Calculating the serial number limit on every call is slightly inefficient. Since CertSerialNumberBits is a constant, you could pre-calculate this limit once as a package-level variable or use a more direct approach to generate the random bits.
| assert.NotEqual(t, big.NewInt(1), firstCA.SerialNumber) | ||
| assert.NotEqual(t, big.NewInt(2), firstServer.SerialNumber) |
There was a problem hiding this comment.
While testify/assert handles *big.Int comparison by using reflect.DeepEqual (which compares the internal slices), it is generally more idiomatic and robust to use the Cmp method for big.Int values to ensure you are comparing the numeric values explicitly.
/kind bug
Webhook self-signed certificate generation used fixed certificate serial numbers:
Certificate serial numbers should be positive and unique per issuer. Reusing deterministic serial numbers for regenerated webhook certificates is weak certificate hygiene and can cause avoidable ambiguity for consumers that cache or inspect certificates.
This change:
Verification:
git diff --check
git diff --cached --check
Suggested test:
go test ./pkg/webhook/cert