tls: use options in getCACertificates() with X509Certificate by haramj · Pull Request #59349 · nodejs/node

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

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

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

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

❌ Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.17%. Comparing base (695f01f) to head (2680887).
⚠️ Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/tls.js	91.17%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59349      +/-   ##
==========================================
- Coverage   90.19%   90.17%   -0.03%     
==========================================
  Files         771      771              
  Lines      264628   265482     +854     
  Branches    50237    50463     +226     
==========================================
+ Hits       238683   239396     +713     
- Misses      16953    17034      +81     
- Partials     8992     9052      +60

Files with missing lines	Coverage Δ
lib/tls.js	`93.29% <91.17%> (-0.19%)`	⬇️

... and 75 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.

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

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

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.

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

haramj · 2026-03-31T05:30:52Z

Hi @joyeecheung,

I've refactored getCACertificates() to follow a clean, linear flow:

Normalize options to handle both string and object inputs.

Single retrieval using getCACertificatesAsStrings().

Process results based on the requested format.

This eliminates redundant calls and preserves caching for the default format. I also updated the tests to strictly verify data integrity using tlsCommon.assertEqualCerts()

github-actions · 2026-07-28T00:22:44Z

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

haramj · 2026-08-20T06:33:30Z

Hi @jasnell @joyeecheung @daeyeon,

I've rebased this PR onto the latest main and applied the review feedback — getCACertificates() now normalizes the options and reuses getCACertificatesAsStrings() as suggested, and the doc changes entry was cleaned up. All CI checks pass.

Would you mind taking another look when you have a moment? Thanks!

Signed-off-by: haramjeong <04harams77@gmail.com>

haramj · 2026-09-07T05:52:49Z

Hi @jasnell, I've rebased this PR onto the latest main and all CI checks pass. Could you please take another look when you have a moment? The previous approval became stale after the code changes. Thanks!

- Change API to use two separate arguments: getCACertificates(type, options) instead of polymorphic single options argument, per reviewer feedback - Remove unnecessary '= undefined' from function parameter - Restore removed test case for default type caching - Update documentation and tests to match new signature Signed-off-by: haramjeong <04harams77@gmail.com> Assisted-by: opencode/mimo-v2.5-free

Address markdown lint failure and review feedback: - Add blank line between parameter entries to satisfy remark-lint rules - Remove extra blank lines between type description list items to match original formatting style

Add assertions to bundled, default, and system test files to verify that the options object form with format: 'pem' returns the same results as the string argument form, ensuring backward compatibility.

haramj · 2026-09-08T03:33:32Z

@jasnell Thanks for the review! I've addressed the feedback in the latest commits, including the API signature, tests, and documentation updates.

I've also added coverage to verify that the options form remains consistent with the string form.

Thanks again!

The X509Certificate class imported from require('crypto') in lib/tls.js does not work correctly during bootstrap - it returns a broken object instead of proper X509Certificate instances. Use InternalX509Certificate from require('internal/crypto/x509') instead, which is the pattern used by other internal modules like lib/internal/tls/wrap.js. Signed-off-by: opencode <opencode@opencode.ai> Assisted-by: opencode <opencode>

The X509Certificate class imported from require('crypto') in lib/tls.js does not work correctly during bootstrap - it returns a broken object instead of proper X509Certificate instances. Use InternalX509Certificate from require('internal/crypto/x509') instead, which is the pattern used by other internal modules like lib/internal/tls/wrap.js. InternalX509Certificate expects a Buffer (DER-encoded) rather than a PEM string, so wrap the PEM string with Buffer.from() to match the pattern used by the user-facing X509Certificate constructor. Signed-off-by: opencode <opencode@opencode.ai> Assisted-by: opencode <opencode>

Use the public X509Certificate class instead of InternalX509Certificate for the x509 format in getCACertificates(). The public API handles PEM string-to-Buffer conversion internally and properly sets up the prototype chain, simplifying the code and addressing reviewer feedback.

getCACertificatesAsStrings already validates the type parameter, so the duplicate validateString call in getCACertificates is unnecessary.

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