tls: use options in getCACertificates() with X509Certificate #59349

haramj · 2025-08-04T16:43:57Z

This PR implements the TODO(joyeecheung) note to support X509Certificate output in tls.getCACertificates(). It introduces a new format option, enhancing the function's flexibility and aligning its API with Node.js's broader crypto module.

The function's behavior is now extended as follows:
API Enhancement: Adds an optional format parameter to tls.getCACertificates() to specify the output format.

Output Options:

'pem' (default): Returns an array of PEM-encoded certificate strings. This is the new default to align with the name used in other Node.js crypto APIs.
'der': Returns an array of certificate data as Buffer objects in DER format.
'x509': Returns an array of crypto.X509Certificate instances, providing direct access to certificate properties.

nodejs-github-bot · 2025-08-04T16:44:02Z

Review requested:

@nodejs/crypto
@nodejs/net

doc/api/tls.md

jasnell · 2025-08-04T17:15:13Z

doc/api/tls.md

@@ -2331,11 +2335,14 @@ Returns an array containing the CA certificates from various sources, depending
    trusted store.
  * When [`NODE_EXTRA_CA_CERTS`][] is used, this would also include certificates loaded from the specified
    file.
+


Suggested change

It seems the white spaces are still there? Can you remove them?

Okay. I'll remove the white space

@joyeecheung Removing that white space will cause a format error in the document..

doc/api/tls.md

lib/tls.js

test/parallel/test-tls-get-ca-certificates-x509-option.js

doc/api/tls.md

haramj · 2025-08-04T17:27:22Z

@jasnell Thank you very much for the thorough and detailed reviews.
I really appreciate the time and effort you’re putting into this.
I’ll carefully go through all the feedback and address them step by step.

Additionally,
When building the docs, the tooling failed to recognize the X509Certificate type and composite return types like Array<Buffer|X509Certificate>, causing errors.
I tried fixing type annotations, references, and escape characters, but I'd appreciate guidance on the correct way to document such composite types officially.

codecov · 2025-08-04T18:22:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.26%. Comparing base (7535aa1) to head (e2879a5).
⚠️ Report is 155 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59349      +/-   ##
==========================================
- Coverage   89.88%   88.26%   -1.62%     
==========================================
  Files         667      701      +34     
  Lines      195217   206815   +11598     
  Branches    38325    39788    +1463     
==========================================
+ Hits       175472   182553    +7081     
- Misses      12210    16289    +4079     
- Partials     7535     7973     +438

Files with missing lines	Coverage Δ
lib/tls.js	`96.57% <100.00%> (+0.32%)`	⬆️

... and 182 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

doc/api/tls.md

haramj · 2025-08-11T07:53:47Z

The CI is failing due to a missing documentation anchor:
link not found: all_tls_tlsgetcacertificatestype
This seems unrelated to the changes in this PR — I didn’t modify that anchor or the associated section..

doc/api/tls.md

Co-authored-by: James M Snell <[email protected]>

haramj · 2025-08-25T15:30:48Z

Hi, feedback has been addressed.
Is there anything else needed from me before this can move forward?

haramj · 2025-08-31T09:34:28Z

If you have some time, could you please review this? @nodejs/net, @joyeecheung

doc/api/tls.md

test/parallel/test-tls-get-ca-certificates-bundled.js

Co-authored-by: Daeyeon Jeong <[email protected]>

jasnell

LGTM! Great job. Let's ask @joyeecheung to also take a look since it was her TODO :-)

haramj · 2025-09-07T12:31:59Z

LGTM! Great job. Let's ask @joyeecheung to also take a look since it was her TODO :-)

Thank you so much for the review and the LGTM! I really appreciate your time.

During my final testing, I discovered an unexpected failure related to caching, and also found a minor inconsistency with the documentation. I would like to push a fix for those before the PR is ready to merge.

I'll push the updated changes shortly.

haramj · 2025-09-07T13:12:37Z

@jasnell @joyeecheung
I've confirmed that the logic using the map() method in the getCACertificates function is causing the caching-related assert.strictEqual test to fail, because it creates a new copy every time.

So I added the caching logic, but now other tests are failing.

nodejs-github-bot · 2025-09-07T14:44:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69114/

haramj · 2025-09-09T08:20:28Z

@jasnell @joyeecheung

I've successfully implemented support for new formats by clearly separating the logic for object input while maintaining the existing function's behavior.

All tests have passed, and I believe the PR is ready for review.

Thank you.

joyeecheung · 2025-09-10T14:08:04Z

lib/tls.js

+
+    validateString(type, 'type');
+
+    switch (type) {


It seems to me this can just reuse the original function; keep that as something like function getCACertificatesAsStrings(type = 'default'), and in function getCACertificates(options = undefined) you just normalize the options first, then get the array of strings, then process it from there.

joyeecheung · 2025-09-10T14:11:17Z

lib/tls.js

+      return certs.map((cert) => {
+        if (typeof cert === 'string') {
+          return cert;
+        }
+        return `-----BEGIN CERTIFICATE-----\n${cert.toString('base64').match(/.{1,64}/g).join('\n')}\n-----END CERTIFICATE-----`;
+      });


I don't think this branch is needed? It should just return the certs. They are already pem-encoded.

joyeecheung · 2025-09-10T14:30:02Z

lib/tls.js

+      return buffers;
+    }
+
+    return buffers.map((buf) => new X509Certificate(buf));


Parsing it after a base64 decoding from JS land is a lot of detour, this can just directly return certs.map(cert => new X509Certificate(buf)) first if the type is x509, the native land would just do the decoding and parsing at once, and it would be simpler in that case because the X509Certificate parser would actually attempt to decode it as PEM first before retrying with d2i_X509_bio anyway.

haramj · 2025-09-11T03:57:40Z

@joyeecheung I've applied the feedback. Could you take a look?

haramj · 2025-09-16T08:48:18Z

Could you please take a look at this when you have a moment? Thanks! @joyeecheung

nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Aug 4, 2025