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]

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

[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

[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!

[panva]

@tniessen can you take a look?

[mertcanaltin]

@anonrig Thank you very much for your suggestion, it gave me a new perspective

[mertcanaltin]

Hello, I've added a GetCertificateExtensions method on the C++ side to populate extensions. After using this method, the extensions object looks like this:

 extensions: {
    subjectKeyIdentifier: '\x04\x14\x16J`?15x���K\x0Fq\x1D ����5',
    authorityKeyIdentifier: '0\x16�\x14\x16J`?15x���K\x0Fq\x1D ����5',
    basicConstraints: '0\x03\x01\x01�',
    subjectAltName: '0\x12�\x10evil.example.com'
  }

However, I'm not sure if I need to convert this to UTF-8. I would greatly appreciate your feedback and advice on this matter. Thank you once again

[mertcanaltin]

I think I have a character encoding issue, and I need to solve this.

NOTE: The test started as a child_process using these flags: [ '--expose-internals' ] Use NODE_SKIP_FLAG_CHECK to run the test with the original flags.
node:assert:125
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '0\x12�\x10evil.example.com'
- 'DNS:example.com, DNS:www.example.com'
    at Object.<anonymous> (/Users/mert/Desktop/nodejs/node/test/parallel/test-crypto-x509.js:370:10)
    at Module._compile (node:internal/modules/cjs/loader:1233:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '0\x12�\x10evil.example.com',
  expected: 'DNS:example.com, DNS:www.example.com',
  operator: 'strictEqual'
}

Node.js v21.0.0-pre
➜  node git:(dev-48730) ./node test/parallel/test-crypto-x509.js
NOTE: The test started as a child_process using these flags: [ '--expose-internals' ] Use NODE_SKIP_FLAG_CHECK to run the test with the original flags.
node:assert:125
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '0\x1E�\x0Bexample.com�\x0Fwww.example.com'
- 'DNS:example.com, DNS:www.example.com'
    at Object.<anonymous> (/Users/mert/Desktop/nodejs/node/test/parallel/test-crypto-x509.js:370:10)
    at Module._compile (node:internal/modules/cjs/loader:1233:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '0\x1E�\x0Bexample.com�\x0Fwww.example.com',
  expected: 'DNS:example.com, DNS:www.example.com',
  operator: 'strictEqual'
}
Node.js v21.0.0-pre

[mertcanaltin]

Hello, I've solved the issue and improved my code. I'm very happy about it!

• Implemented BIO objects for secure data handling.
• Used OpenSSL functions for proper extension data printing.
• Added error handling for potential issues.
• Ensured careful memory management.
• Checked and adjusted character encoding settings as needed.

[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

[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.

[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?