Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: add TLSA record query support #52983

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rithvikvibhu
Copy link

@rithvikvibhu rithvikvibhu commented May 14, 2024

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)

Related to #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?

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 14, 2024
@lpinca lpinca added dns Issues and PRs related to the dns subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels May 14, 2024
@rithvikvibhu
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Member

Trott commented Jun 1, 2024

@nodejs/dns

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 1, 2024
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants