test: fixes for c-ares update to 1.21.0/1.22.0 #50743

bradh352 · 2023-11-15T14:37:45Z

c-ares has made intentional changes to the behavior of TXT records
to comply with RFC 7208, which concatenates multiple strings for
the same TXT record into a single string. Multiple TXT records
are not concatenated.

Also, response handling has changed, such that a response which is
completely invalid in formatting is thrown away as a malicious
forged/spoofed packet rather than returning EBADRESP. This is one
step toward RFC 9018 (EDNS COOKIES) which will require the message
to at least be structurally valid to validate against spoofed
records.

Fixes: #50741
Refs: #50444

Fix By: Brad House (@bradh352)

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fixes: nodejs#50741 Refs: nodejs#50444 Fix By: Brad House (@bradh352)

lpinca · 2023-11-18T19:48:55Z

I think we should apply changes from #50444 and land everything as a single commit.

nodejs-github-bot · 2023-11-18T19:51:09Z

Commit Queue failed

- Loading data for nodejs/node/pull/50743
✔  Done loading data for nodejs/node/pull/50743
----------------------------------- PR info ------------------------------------
Title      test: fixes for c-ares update to 1.21.0/1.22.0 (#50743)
Author     Brad House  (@bradh352)
Branch     bradh352:main -> nodejs:main
Labels     test, needs-ci
Commits    1
 - test: dns test case failures after c-ares update to 1.21.0+
Committers 1
 - Brad House 
PR-URL: https://github.com/nodejs/node/pull/50743
Fixes: https://github.com/nodejs/node/issues/50741
Refs: https://github.com/nodejs/node/pull/50444
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50743
Fixes: https://github.com/nodejs/node/issues/50741
Refs: https://github.com/nodejs/node/pull/50444
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 15 Nov 2023 14:37:45 GMT
   ✔  Approvals: 1
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/50743#pullrequestreview-1738469498
   ✘  This PR needs to wait 90 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

lpinca · 2023-11-18T19:52:31Z

Applied the commit-queue label instead of request-ci.

bradh352 · 2023-11-18T19:53:38Z

should pull in 1.22.0 instead of 1.21.0 in #50444

lpinca · 2023-11-18T19:55:42Z

The script to pull 1.22.0 should will run tomorrow. We can run it manually now but we have to reapply 0bd1e85.

nodejs-github-bot · 2023-11-18T19:56:58Z

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

lpinca · 2023-11-18T20:01:45Z

Shouldn't test/parallel/test-dns-resolveany.js fail at the moment? The c-ares version on main is 1.20.1.

lpinca · 2023-11-18T20:17:18Z

Shouldn't test/parallel/test-dns-resolveany.js fail at the moment? The c-ares version on main is 1.20.1.

Nevermind, the server answers with a single string now.

I think we should apply changes from #50444 and land everything as a single commit.

There is no need to land this and the c-ares update as a single commit. This is independent.

Still LGTM.

lpinca · 2023-11-18T20:32:07Z

test/parallel/test-dns-resolveany-bad-ancount.js

+      // May return EBADRESP or ETIMEOUT
+      // code: 'EBADRESP',


Suggested change

// May return EBADRESP or ETIMEOUT

// code: 'EBADRESP',

code: /^(?:EBADRESP|ETIMEOUT)$/,

gjasny · 2023-11-18T21:13:20Z

I think we should apply changes from #50444 and land everything as a single commit.

It would be helpful if the test fix is separate commit (but maybe same PR) and could also be backported to the v20 and v18 branch.
That would help me getting the fix into the Debian nodejs package.

Thanks,
Gregor

nodejs-github-bot · 2023-11-18T21:16:24Z

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

… list it appears.

nodejs-github-bot · 2023-11-19T20:35:34Z

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

nodejs-github-bot · 2023-11-20T18:23:12Z

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

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: #50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: #50741 Refs: #50444

lpinca · 2023-11-24T15:01:14Z

Landed in 345d16c.

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: nodejs#50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: nodejs#50741 Refs: nodejs#50444

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: nodejs#50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: nodejs#50741 Refs: nodejs#50444

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: #50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: #50741 Refs: #50444

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: #50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: #50741 Refs: #50444

@bradh352

c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: #50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: #50741 Refs: #50444

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 15, 2023

bradh352 force-pushed the main branch from 592bd5a to 48a995d Compare November 15, 2023 14:47

bradh352 mentioned this pull request Nov 15, 2023

dns test cases failures when upgrading to c-ares 1.21.0/1.22.0 #50741

Closed

lpinca approved these changes Nov 18, 2023

View reviewed changes

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 18, 2023

lpinca added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 18, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2023

lpinca approved these changes Nov 18, 2023

View reviewed changes

lpinca mentioned this pull request Nov 18, 2023

deps: update c-ares to 1.23.0 #50800

Open

bradh352 added 4 commits November 19, 2023 10:57

try to fix other outstanding issues

600a84f

fix reported linter issue

af82c55

fix reported linter issue

3d2185d

revert. c-ares is really going to need to support blanking the server…

d80295f

… list it appears.

jasnell approved these changes Nov 19, 2023

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2023

lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 19, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2023

RafaelGSS mentioned this pull request Nov 28, 2023

v21.3.0 proposal #50954

Merged

lpinca closed this Dec 3, 2023

test: fixes for c-ares update to 1.21.0/1.22.0 #50743

test: fixes for c-ares update to 1.21.0/1.22.0 #50743

bradh352 commented Nov 15, 2023 •

edited

lpinca commented Nov 18, 2023 •

edited

nodejs-github-bot commented Nov 18, 2023

lpinca commented Nov 18, 2023

bradh352 commented Nov 18, 2023

lpinca commented Nov 18, 2023 •

edited

nodejs-github-bot commented Nov 18, 2023

lpinca commented Nov 18, 2023 •

edited

lpinca commented Nov 18, 2023 •

edited

lpinca Nov 18, 2023

gjasny commented Nov 18, 2023

nodejs-github-bot commented Nov 18, 2023

nodejs-github-bot commented Nov 19, 2023

nodejs-github-bot commented Nov 20, 2023

lpinca commented Nov 24, 2023

	// May return EBADRESP or ETIMEOUT
	// code: 'EBADRESP',
	code: /^(?:EBADRESP\|ETIMEOUT)$/,

test: fixes for c-ares update to 1.21.0/1.22.0 #50743

test: fixes for c-ares update to 1.21.0/1.22.0 #50743

Conversation

bradh352 commented Nov 15, 2023 • edited

lpinca commented Nov 18, 2023 • edited

nodejs-github-bot commented Nov 18, 2023

lpinca commented Nov 18, 2023

bradh352 commented Nov 18, 2023

lpinca commented Nov 18, 2023 • edited

nodejs-github-bot commented Nov 18, 2023

lpinca commented Nov 18, 2023 • edited

lpinca commented Nov 18, 2023 • edited

lpinca Nov 18, 2023

Choose a reason for hiding this comment

gjasny commented Nov 18, 2023

nodejs-github-bot commented Nov 18, 2023

nodejs-github-bot commented Nov 19, 2023

nodejs-github-bot commented Nov 20, 2023

lpinca commented Nov 24, 2023

bradh352 commented Nov 15, 2023 •

edited

lpinca commented Nov 18, 2023 •

edited

lpinca commented Nov 18, 2023 •

edited

lpinca commented Nov 18, 2023 •

edited

lpinca commented Nov 18, 2023 •

edited