Skip to content

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

Open
haramj wants to merge 34 commits into
nodejs:mainfrom
haramj:haramjeong-patch-250805
Open

tls: use options in getCACertificates() with X509Certificate#59349
haramj wants to merge 34 commits into
nodejs:mainfrom
haramj:haramjeong-patch-250805

Conversation

@haramj

@haramj haramj commented Aug 4, 2025

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot 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
Comment thread doc/api/tls.md
Comment thread doc/api/tls.md Outdated
Comment thread doc/api/tls.md
trusted store.
* When [`NODE_EXTRA_CA_CERTS`][] is used, this would also include certificates loaded from the specified
file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll remove the white space

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread doc/api/tls.md
Comment thread doc/api/tls.md
Comment thread lib/tls.js
Comment thread test/parallel/test-tls-get-ca-certificates-x509-option.js Outdated
Comment thread test/parallel/test-tls-get-ca-certificates-x509-option.js Outdated
Comment thread test/parallel/test-tls-get-ca-certificates-x509-option.js Outdated
Comment thread test/parallel/test-tls-get-ca-certificates-x509-option.js Outdated
Comment thread doc/api/tls.md
Comment thread doc/api/tls.md Outdated
@haramj

haramj commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

@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

codecov Bot commented Aug 4, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.70%. Comparing base (2263b4d) to head (625faaa).
⚠️ Report is 102 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59349      +/-   ##
==========================================
+ Coverage   89.68%   89.70%   +0.01%     
==========================================
  Files         676      694      +18     
  Lines      206710   214253    +7543     
  Branches    39584    41123    +1539     
==========================================
+ Hits       185398   192188    +6790     
- Misses      13441    14109     +668     
- Partials     7871     7956      +85     
Files with missing lines Coverage Δ
lib/tls.js 93.61% <100.00%> (+0.50%) ⬆️

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

Comment thread doc/api/tls.md Outdated
@haramj

haramj commented Aug 11, 2025

Copy link
Copy Markdown
Member Author

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

Comment thread doc/api/tls.md Outdated
@haramj haramj changed the title tls: add 'as' option to getCACertificates() for X509Certificate output tls: use options in getCACertificates() with X509Certificate Aug 20, 2025
@haramj haramj force-pushed the haramjeong-patch-250805 branch from 2e19981 to d978079 Compare August 20, 2025 15:29
@haramj

haramj commented Aug 25, 2025

Copy link
Copy Markdown
Member Author

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

@haramj

haramj commented Aug 31, 2025

Copy link
Copy Markdown
Member Author

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

Comment thread doc/api/tls.md Outdated
Comment thread doc/api/tls.md Outdated
Comment thread test/parallel/test-tls-get-ca-certificates-bundled.js Outdated

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@haramj

haramj commented Sep 7, 2025

Copy link
Copy Markdown
Member Author

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

haramj commented Sep 7, 2025

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Collaborator

@haramj

haramj commented Sep 9, 2025

Copy link
Copy Markdown
Member Author

@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 haramj force-pushed the haramjeong-patch-250805 branch from e2879a5 to e787797 Compare March 24, 2026 05:24
@haramj

haramj commented Mar 31, 2026

Copy link
Copy Markdown
Member Author

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()

@haramj haramj requested review from daeyeon and joyeecheung March 31, 2026 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants