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)

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?

@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

@rithvikvibhu
Copy link
Author

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:

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

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jun 16, 2024

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

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

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

size_t data_len;
const unsigned char* data =
ares_dns_rr_get_bin(rr, ARES_RR_TLSA_DATA, &data_len);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably recommend checking this return value to not be NULL before attempting to memcpy it.

Copy link
Author

Choose a reason for hiding this comment

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

I'm looking at the possible codes and ARES_ENODATA seems to be the closest match status code to return in this case.
Or do you think we should just skip this record (if null data) without any errors?

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.

6 participants