dns: add TLSA record query support

[rithvikvibhu]

This PR adds resolveTlsa so that the resolver can query TLSA records.

c-ares added the parser in c-ares/c-ares#600 and @bradh352 (thanks!) provided some code to get started with: #39569 (comment)

Refs: #39569

P.S. I'm new to both node core as well as C++ so the code may be unideal, am open to any changes to be made.

Also, I'm not sure about the YAML markup in docs, what should the "Added in" say?

And is this considered a Notable Change?

[rithvikvibhu]

I've updated to fix linting and formatting, but not sure about the LeakSanitizer: detected memory leaks one.

Tried reproducing it locally with tools/test.py but could not, all tests always run without issues. And it seems to be complaining about dnsrec but I think it's handled properly? Would love some advice here.

[bradh352]

might be useful to define the known values for the TLSA records like we do in c-ares with ares_tlsa_match_t, ares_tlsa_selector_t, and ares_tlsa_usage_t

[rithvikvibhu]

Fixed the mem leak and confirmed the asan test passes (on a different branch, not in this PR: https://github.com/rithvikvibhu/node/actions/runs/9137751681/job/25128086198)

I think it's ready for review (assuming all checks pass).

---

@bradh352 I searched but couldn't find constants defined in dns for any other type, they are all stored and returned as regular objects. For ex, ParseMxReply:

node/src/cares_wrap.cc

Lines 301 to 311 in 559212e

	Local<Object> mx_record = Object::New(env->isolate());
	mx_record->Set(env->context(),
	env->exchange_string(),
	OneByteString(env->isolate(), current->host)).Check();
	mx_record->Set(env->context(),
	env->priority_string(),
	Integer::New(env->isolate(), current->priority)).Check();
	if (need_type)
	mx_record->Set(env->context(),
	env->type_string(),
	env->dns_mx_string()).Check();

[rithvikvibhu]

@lpinca can you approve the workflow again? I believe all check errors are fixed now. Also, any idea if someone or a group must be tagged for reviewing? (I don't mind if it takes time, just making sure I understand the process)

[Trott]

@nodejs/dns

[nodejs-github-bot]

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

[rithvikvibhu]

The only failing test is benchmark/test-benchmark-crypto, which this PR does not touch. There is an issue with the same error and a PR to mark it as flaky:

• flaky: benchmark/test-benchmark-crypto #52690
• test: mark test-benchmark-crypto as flaky #52955

I think we just wait till #52955 is merged, then I rebase?

[nodejs-github-bot]

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

[Trott]

This needs someone to review it. I already pinged @nodejs/dns 2 weeks ago, so now I'm going to try current collaborators who show up a lot in git blame src/cares_wrap.cc: @jasnell @addaleax @XadillaX @bnoordhuis @ShogunPanda @joyeecheung

[ShogunPanda]

I'm not really a C++ expert but it looks fine to me.

[abwesend890]

as I am really looking forward to these changes I wanted to kindly ask on what is missing until this PR can be finally included in a release.

Thank you!

[Trott]

as I am really looking forward to these changes I wanted to kindly ask on what is missing until this PR can be finally included in a release.

As of right now, there's a merge conflict in src/env_properties.h that needs to be resolved. Once that's done, tests will need to be re-run.

[codecov]

Codecov Report

Attention: Patch coverage is 25.75758% with 49 lines in your changes missing coverage. Please review.

Project coverage is 89.09%. Comparing base (79f96b6) to head (660774a).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
src/cares_wrap.cc	23.43%	46 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #52983      +/-   ##
==========================================
- Coverage   89.10%   89.09%   -0.02%     
==========================================
  Files         665      665              
  Lines      193203   193269      +66     
  Branches    37220    37228       +8     
==========================================
+ Hits       172158   172193      +35     
- Misses      13771    13814      +43     
+ Partials     7274     7262      -12     

Files with missing lines	Coverage Δ
lib/internal/dns/utils.js	99.44% <100.00%> (+<0.01%)	⬆️
src/cares_wrap.h	79.89% <ø> (ø)
src/cares_wrap.cc	63.05% <23.43%> (-1.99%)	⬇️

... and 39 files with indirect coverage changes

[Trott]

Lint cpp issues are probably fixable via rebase against current main, so I've done that and force-pushed.

[rithvikvibhu]

Thanks @Trott! I had the formatted commits ready to force push but was waiting to see if any other checks fail 😄

The internet test failing (internet/test-net-autoselectfamily-timeout-close) doesn't seem related to any change this PR makes.

I'll push one last commit for the open review comment once it gets a reply.

[rithvikvibhu]

@bradh352 @Trott review comments addressed and all checks are passing. Lemme know if anything else is required.

[nodejs-github-bot]

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

[Trott]

@nodejs/collaborators This is ready to land but needs one more approval since there was a change since the last approval. I could rubber-stamp it, but a careful review by someone knowledgable about DNS and/or C++ would be better. I've tried pinging the relevant smaller groups (@nodejs/dns, @nodejs/cpp-reviewers) and had a little success getting reviews, but I'd prefer more eyes on something like this, so pinging more widely now.

(If no one comes by in the next 48 hours or so with a review/approval, I'll rubber-stamp it and land it.)

[Trott]

Rubber-stamp LGTM based on previous reviews by others. Doing this to unlock Commit Queue.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/52983
✔  Done loading data for nodejs/node/pull/52983
----------------------------------- PR info ------------------------------------
Title      dns: add TLSA record query support (#52983)
Author     Rithvik Vibhu <[email protected]> (@rithvikvibhu, first-time contributor)
Branch     rithvikvibhu:dns-tlsa -> nodejs:main
Labels     c++, dns, semver-minor, lib / src, needs-ci, commit-queue-squash
Commits    2
 - dns: add TLSA record query and parsing
 - Update src/cares_wrap.cc
Committers 2
 - Rithvik Vibhu <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/52983
Refs: https://github.com/nodejs/node/issues/39569
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52983
Refs: https://github.com/nodejs/node/issues/39569
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 14 May 2024 18:28:18 GMT
   ✔  Approvals: 3
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/52983#pullrequestreview-2122736173
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52983#pullrequestreview-2619429908
   ✔  - Rich Trott (@Trott): https://github.com/nodejs/node/pull/52983#pullrequestreview-2624522158
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-02-16T05:12:15Z: https://ci.nodejs.org/job/node-test-pull-request/65245/
- Querying data for job/node-test-pull-request/65245/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @rithvikvibhu([email protected])
   ⚠  - commit 660774a9846c is authored by [email protected]
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/13397499730

[Ethan-Arrowood]

This lgtm - my only note is maybe the docs could be clearer on the return type of records. But wont block on that as its fine to get this landed.

[Trott]

Landed in ef91595

[Trott]

Aw, I did the force push after pushing to main, so the PR is marked as Closed rather than Merged. Sorry about that!

@rithvikvibhu Thanks for your patience and the contribution!

If anyone wants to improve the docs (per @Ethan-Arrowood's comment), please open a new PR to do it! Thanks in advance!

Apologies for all the exclamation points!!!!!!!!

[rithvikvibhu]

Haha np just glad it's finally done and merged!

Thanks to all the reviewers, especially @Trott and @bradh352 for all the help and follow ups