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

Feature/dns caa #35466

Closed
wants to merge 11 commits into from
Closed

Feature/dns caa #35466

wants to merge 11 commits into from

Conversation

lxdicted
Copy link
Contributor

@lxdicted lxdicted commented Oct 2, 2020

net: Added support for resolving DNS CAA records (RFC 8659)

This adds support for DNS Certification Authority Authorization (RFC 8659) to nodejs.

Fixes: #19239
Refs: #14713

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This adds support for DNS Certification Authority
Authorization (RFC 6844) to nodejs.

This closes nodejs#19239 and possibly affects nodejs#14713.
@lxdicted lxdicted requested a review from a team as a code owner October 2, 2020 16:45
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 2, 2020
doc/api/dns.md Outdated Show resolved Hide resolved
deps/cares/cares.gyp Outdated Show resolved Hide resolved
@lxdicted
Copy link
Contributor Author

lxdicted commented Oct 3, 2020

@rvagg @nodejs/modules please review the c-ares backport.

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 3, 2020
@Trott
Copy link
Member

Trott commented Oct 3, 2020

@nodejs/dns

doc/api/dns.md Outdated Show resolved Hide resolved
doc/api/dns.md Outdated Show resolved Hide resolved
env->type_string(),
env->dns_caa_string()).Check();

ret->Set(context, i + offset, caa_record).Check();
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a todo comment to do proper exception handling in this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no exception handling in cares_wrap.cc. If this was needed, it would have been added for all parsing the other record types, too. I don't see a todo here.

Copy link
Member

Choose a reason for hiding this comment

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

There is no exception handling in cares_wrap.cc.

Right, it’s missing, so it would be nice to have a comment about that somewhere in the file

test/internet/test-dns.js Show resolved Hide resolved
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. review wanted PRs that need reviews. labels Oct 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@nodejs-github-bot
Copy link
Collaborator

doc/api/dns.md Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Oct 16, 2020

Landed in 6f34498

@aduh95 aduh95 closed this Oct 16, 2020
aduh95 pushed a commit that referenced this pull request Oct 16, 2020
This adds support for DNS Certification Authority Authorization
(RFC 8659) to Node.js.

PR-URL: #35466
Fixes: #19239
Refs: #14713
Reviewed-By: Anna Henningsen <[email protected]>
@AdamMajer
Copy link
Contributor

FWIW, this pulls c-ares patch that is not yet released which will break Node on systems where c-ares is linked as an external dependency. For this reason, I think this should not be backported to currently released versions.

@AdamMajer
Copy link
Contributor

The don't-land-on-v15.x actually already happened. It's in 15.0.0+ in 6f34498

@aduh95
Copy link
Contributor

aduh95 commented Oct 22, 2020

Indeed! Do you think that should be reverted on v15 or do you think it's fine as is?

@AdamMajer
Copy link
Contributor

For me it's OK since we will not be shipping 15 with anything and by the time 16 rolls around we can either patch our c-ares for the extra functionality or have a new c-ares version by then.

@Trott
Copy link
Member

Trott commented Nov 4, 2020

Unfortunately, this broke a test in test/internet (which only get run nightly and I only got around to bisecting today):

node:internal/process/promises:218
          triggerUncaughtException(err, true /* fromPromise */);
          ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(Array.isArray(result[0]))

    at validateResult (/Users/trott/io.js/test/internet/test-dns.js:404:12)
    at test_resolveCaa (/Users/trott/io.js/test/internet/test-dns.js:411:3) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Bug in the test? Bug in this change? Something else?

@Trott
Copy link
Member

Trott commented Nov 4, 2020

Oh, interesting, the test that is failing was added in this PR.

Is the bug that it should be checking Array.isArray(result) and not Array.isArray(result[0])?

@Trott
Copy link
Member

Trott commented Nov 4, 2020

Oh, interesting, the test that is failing was added in this PR.

Is the bug that it should be checking Array.isArray(result) and not Array.isArray(result[0])?

Yeah, looking at the surrounding code, that's gotta be it.

@Trott
Copy link
Member

Trott commented Nov 4, 2020

Fix in #35969

Trott added a commit to Trott/io.js that referenced this pull request Nov 5, 2020
Refs: nodejs#35466 (comment)

PR-URL: nodejs#35969
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
Refs: #35466 (comment)

PR-URL: #35969
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@lxdicted
Copy link
Contributor Author

@AdamMajer , @aduh95 : c-ares 0.17.1 has been released recently. It there any change to get this into v10, v12, or v14?

@AdamMajer
Copy link
Contributor

I think no problem here to backport this to older versions now.

@richardlau
Copy link
Member

@AdamMajer , @aduh95 : c-ares 0.17.1 has been released recently. It there any change to get this into v10, v12, or v14?

10.x is in maintenance so this won't land there. The last planned 12.x release before it enters maintenance at the end of the month is due on Tuesday (#35950 ) so the chance of getting this into 12.x is low.

For 14.x the path forwards should be to update c-ares in master to 0.17.1 and then backport that together with this PR to 14.x.

@lxdicted
Copy link
Contributor Author

I've created a pull request for c-ares 1.17.1 here: #36207

@lxdicted
Copy link
Contributor Author

For 14.x the path forwards should be to update c-ares in master to 0.17.1 and then backport that together with this PR to 14.x.

@richardlau c-ares 1.17.1 has landed in 3bd9b81, any other obstacles to backport this to 14.x?

@richardlau
Copy link
Member

For 14.x the path forwards should be to update c-ares in master to 0.17.1 and then backport that together with this PR to 14.x.

@richardlau c-ares 1.17.1 has landed in 3bd9b81, any other obstacles to backport this to 14.x?

Our general policy is that for LTS release lines (e.g. 14.x) a change should live on the current release (i.e. 15.x) for two weeks before being backported (https://github.com/nodejs/Release#lts-staging-branches). 3bd9b81 hasn't gone out in 15.x yet (the next 15.x release is planned for this week, nodejs/Release#621). The next 14.x release is going to be a security release (https://nodejs.org/en/blog/vulnerability/january-2021-security-releases/) so any backports of features would have to wait until after that. We haven't firmly sketched out the release plan for 14.x for next year, but have discussed planning a semver-minor for mid to late January.

So timeline wise:

  1. 3bd9b81 goes out in a 15.x release.
  2. Two weeks after the 15.x release the c-ares update lands on v14.x-staging.
  3. This PR is backported, minus the c-ares floating patch (as the intention is to use the proper c-ares update) and including test: fix error in test/internet/test-dns.js #35969 (fixing up one of the testcases). I've added a backport-requested-v14.x label.

@targos targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v14.x review wanted PRs that need reviews. labels Apr 30, 2021
targos pushed a commit that referenced this pull request May 1, 2021
This adds support for DNS Certification Authority Authorization
(RFC 8659) to Node.js.

PR-URL: #35466
Fixes: #19239
Refs: #14713
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request May 1, 2021
Refs: #35466 (comment)

PR-URL: #35969
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. 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.

Add support for resolving CAA records
9 participants