-
Notifications
You must be signed in to change notification settings - Fork 108
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: use ext helper in more places, test custom exts. #154
Conversation
Previously there was no test coverage for custom extensions in certificates or CSRs. This commit adds a simple example of encoding a custom extension, and then demonstrating that it can be parsed with `x509-parser`, both in a serialized certificate and in a CSR. There's no support in webpki, openssl-rs or botan-rs for handling custom extensions so no test coverage for those libraries is possible at this time.
Codecov Report
@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 70.28% 71.90% +1.62%
==========================================
Files 7 7
Lines 1898 1876 -22
==========================================
+ Hits 1334 1349 +15
+ Misses 564 527 -37
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Previously when writing CSR DER from `CertificateParams` that specified custom extensions, but did not specify any SANs, the serialization code would skip over writing the PKCS9 extension request attribute. This commit updates the serialization logic to ensure the attribute is written when either SANs are provided, or custom extensions are present. Prior to this update, the modified `test_x509_custom_ext` test fails, reproducing the problem reported in the issue tracker: ``` 'test_x509_custom_ext::custom_ext' panicked at 'missing requested extensions' ``` With the update, it passes again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only gave this a cursory glance due to limited time but the general idea of the PR is good, thanks. Maybe it would be good to get another review?
Nice! |
This branch updates a couple places that were writing x509 extensions by hand to use the existing
write_x509_extension
helper instead.Since custom extensions are a bit trickier a unit test for custom extensions in CSRs and certificates is added before refactoring the existing code to use the helper. This helps ensure the change is a no-op from the perspective of users.
With the updated test, it was possible to demonstrate a bug similar to #122. The CSR serialization logic requiring SANs to be present in order to emit the custom extensions into the CSR PKCS9 extension request attribute. This branch fixes the issue.