crypto: add extensions property to X509Certificate

[mertcanaltin]

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

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[mscdex]

This needs tests.

[panva]

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?

[panva]

I believe the tests belong better to test/parallel/test-crypto-x509.js. A test for accessing the property when there are no extensions would also be welcome.

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?

[mertcanaltin]

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?

[panva]

I am unfamiliar with the underlying implementation.

[jeffsec-aws]

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.

[mertcanaltin]

@mscdex @panva @tniessen @jeffsec-aws hello I will be very happy if you review it, thank you very much

[panva]

Why are there hardcoded values in the implementation?

[mertcanaltin]

@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

[panva]

@mertcanaltin I am really not able to be of that kind of assistance.

[jeffsec-aws]

What method do you want to use to generate those certificates? Would OpenSSL commands help?

[mertcanaltin]

What method do you want to use to generate those certificates? Would OpenSSL commands help?

yes it will help

[panva]

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.

[mertcanaltin]

Yes, I added this certificate to fixtures instead of hardcoding it and tried to fix my test @panva

[jeffsec-aws]

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.

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

[mertcanaltin]

Hello @jeffsec-aws,@panva I would appreciate it if you could review this Pull Request. Thank you!

[Trott]

@nodejs/crypto This could probably use some reviews (or re-reviews for the people that have already reviewed it).

[mertcanaltin]

greetings I triggered you guys again very sorry I wonder if I have a chance to get an update here

[panva]

I am unable to review the implementation, aside of that this lacks sufficient test coverage and update to documentation.

[mertcanaltin]

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

[mertcanaltin]

greetings I updated the document and I think I got stuck on a flakky test @panva

[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

[mertcanaltin]

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,

[tniessen]

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.

[tniessen]

This needs a lot more information, especially w.r.t. the exact structure of the returned object.

[tniessen]

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.

[mertcanaltin]

thank you very much for your suggestions I have applied a few changes and I hope I have followed the correct steps

[mertcanaltin]

@tniessen greetings, I would be very happy if you could take a look when you have time, again many thanks for your support

[mertcanaltin]

Greetings, I want to improve this place very much, do not hesitate to make suggestions when appropriate ❤️

[marco-ippolito]

There are still a bunch of tests failing and some comments from @tniessen that have not been addressed

[mertcanaltin]

@tniessen Hello, I would like to move this place forward, do you have an update?

[mertcanaltin]

hello this is my old first attempt to contribute, I will try to fix this place if you have any suggestions please feel free