-
Notifications
You must be signed in to change notification settings - Fork 30k
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
crypto: add extensions property to X509Certificate #48780
Conversation
Review requested:
|
This needs tests. |
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.
May I suggest to rename certificateExtensions
to just extensions
. Also then update the first commit message (and the PR title) to crypto: add extensions property to X509Certificate
?
I believe the tests belong better to Not being familiar with extensions myself so apologizies if the following questions are just plain irrelevant. I wonder if an extension can be present multiple times, in which case how would such extension be returned? Are there structured extensions? Or extensions who's value is not a string? @tniessen is there a reason you didn't expose this in the first place? |
11d9028
to
6916509
Compare
When I look before the test, I get the output of x509.extensions undefined. Could it be because I cannot access it because it is undefined? |
I am unfamiliar with the underlying implementation. |
Nice work. Extensions can be of various different types and content. Now they only appear one in terms of key (an OID see for example: https://access.redhat.com/documentation/en-us/red_hat_certificate_system/9/html/administration_guide/standard_x.509_v3_certificate_extensions) but can have multiple values in a form of an array. |
@mscdex @panva @tniessen @jeffsec-aws hello I will be very happy if you review it, thank you very much |
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.
Why are there hardcoded values in the implementation?
@panva How can I generate a certificate with extensions? I will use it in my test instead of hardcoded. i will be using it in my test code |
@mertcanaltin I am really not able to be of that kind of assistance. |
What method do you want to use to generate those certificates? Would OpenSSL commands help? |
yes it will help |
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'll quote my earlier concern which needs to be settled before this can progress.
Not being familiar with extensions myself so apologizies if the following questions are just plain irrelevant. I wonder if an extension can be present multiple times, in which case how would such extension be returned? Are there structured extensions? Or extensions who's value is not a string?
These examples would need tests added and the documentation for this method also has to be added.
Yes, I added this certificate to fixtures instead of hardcoding it and tried to fix my test @panva |
And I answered to you. Extensions are referenced by OID so the key always appears only once. Now the value can be array of value for some extensions like OCSP Responder URL |
Hello @jeffsec-aws,@panva I would appreciate it if you could review this Pull Request. Thank you! |
@nodejs/crypto This could probably use some reviews (or re-reviews for the people that have already reviewed it). |
greetings I triggered you guys again very sorry I wonder if I have a chance to get an update here |
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 am unable to review the implementation, aside of that this lacks sufficient test coverage and update to documentation.
thank you very much for your suggestion, I updated the document @panva |
greetings I updated the document and I think I got stuck on a flakky test @panva |
Aside from my above suggestion to fix and simplify the docs entry I believe this lacks sufficient test coverage. Note that I can not review the c++ implementation |
thank you very much for your suggestions, here I created a crypto-extensions.pem certificate for testing and tested the subjectAltName in the extension, in fact, I would be very happy if you have a suggestion. |
dismissing one review, i still think test coverage is not there and someone else than me has to review the c++ code, there is no further point in asking me to review this,
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.
Node.js has made the mistake of representing structured ASN.1 data as strings in the past, which has led to various issues. Most notable, perhaps, CVE-2021-44532 and CVE-2021-44533, which were a nightmare to fix without breaking the ecosystem.
At this point, we should be extremely careful about exposing any human-readable information provided by OpenSSL for anything besides debugging purposes. These text formats are not meant to be processed automatically, and OpenSSL generally provides no stability guarantees.
doc/api/crypto.md
Outdated
|
||
Returns: {Object} | ||
|
||
Returns an object representing the extensions of the certificate. |
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.
This needs a lot more information, especially w.r.t. the exact structure of the returned object.
@@ -0,0 +1,23 @@ | |||
-----BEGIN CERTIFICATE----- |
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.
You should modify test/fixtures/keys/Makefile
such that it can be used to generate this certificate, unless that is not feasible for some reason.
thank you very much for your suggestions I have applied a few changes and I hope I have followed the correct steps |
36fb0c0
to
2486934
Compare
@tniessen greetings, I would be very happy if you could take a look when you have time, again many thanks for your support |
Greetings, I want to improve this place very much, do not hesitate to make suggestions when appropriate ❤️ |
There are still a bunch of tests failing and some comments from @tniessen that have not been addressed |
@tniessen Hello, I would like to move this place forward, do you have an update? |
2486934
to
b171afe
Compare
hello this is my old first attempt to contribute, I will try to fix this place if you have any suggestions please feel free |
This pull request adds an enhanced property to the X509Certificate class, providing additional functionality for X.509 certificates. The new property improves the handling of certificate subjects, issuers, and other related information. This enhancement enhances the overall capabilities and usability of the X509Certificate class
issue:#48730
fyi @jeffsec-aws