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

fix(ext/node): better dns.lookup compatibility #27936

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

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Feb 2, 2025

This PR improves the dns.lookup compatibility to Node.js.

Currently we use hickory-dns for looking up ipv4 and ipv6 addresses, but this seems causing lot of edge case issues about dns resolution (#27670, #27642, #27384, #27803). This PR changes it to use getaddrinfo to align it better to Node.js.

This fixes #27670
This could possibly have effect to #27642, #27384, and #27803


This PR also fixes the permission requirement for net.connect(hostname). Currently we require net permission of addresses of name servers and resolved ips. This PR change it to require only requested domain (now http.request(url) prompts the same permission as fetch(url)

To achieve that, op_node_getaddrinfo returns NetPermToken object. The token is passed to op_net_connect_tcp, and validates the ips in it. If token has a valid ip, then the permission is checked against domain name in the token, instead of the given ip address.

This fixes #27634


Remaining:

  • leak of op_getaddrinfo happens in unit_node/http_test in a flaky way
  • add net permission check for 'hostname'
  • skip net permission check for ip addressed if they are returned from dns.lookup result (because they are already granted via net permission check in dns.lookup (getaddrinfo))
  • port large dns response test from fix(ext/net): enable EDNS0 for Deno.resolveDns #27735
  • add check of permission requirement of http(s).request
  • op_tls_start perm issue

@kt3k kt3k added the ci-draft Run the CI on draft PRs. label Feb 2, 2025
@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch 3 times, most recently from c52102f to 99e4206 Compare February 3, 2025 05:01
@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch from 99e4206 to 1d63322 Compare February 3, 2025 05:09
@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch from 9b61e7d to 933b5a4 Compare February 3, 2025 07:15
@kt3k kt3k marked this pull request as ready for review February 4, 2025 14:51
@kt3k kt3k marked this pull request as draft February 5, 2025 08:47
@kt3k
Copy link
Member Author

kt3k commented Feb 5, 2025

I'll also try to implement net permission checks for dns.lookup (op_getaddrinfo) (which checks the net permission with hostnames, not name servers or config files).

@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch from da43e90 to 6480e07 Compare February 7, 2025 02:25
@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch from 6480e07 to d0a4740 Compare February 7, 2025 02:34
@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch from bc43984 to 9cde2d9 Compare February 7, 2025 06:01
@kt3k kt3k force-pushed the fix-node-dns-use-gai-resolver branch from 12435f9 to 9eb2828 Compare February 7, 2025 08:54
@kt3k kt3k marked this pull request as ready for review February 9, 2025 14:30
@kt3k kt3k marked this pull request as draft February 12, 2025 09:05
@kt3k kt3k marked this pull request as ready for review February 13, 2025 08:55
@kt3k
Copy link
Member Author

kt3k commented Feb 13, 2025

@bartlomieju @lucacasonato Now I think this is ready for review. PTAL. This now implements the token idea, which is created from op_node_getaddrinfo and validated in op_net_connect_tcp. If the token is valid, then op_net_connect_tcp checks the net permission against domain name instead of the resolved ip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-draft Run the CI on draft PRs.
Projects
None yet
1 participant