Skip to content

Conversation

@errorxyz
Copy link
Member

@errorxyz errorxyz commented Jun 29, 2024

Description

Uses mitmproxy_rs.DnsResolver to resolve domain names that need to be checked in the hosts file first and forwards any other type of query to the server specified in the options
Fixes #6739

Checklist

  • I have updated tests where applicable.
  • I have added an entry to the CHANGELOG.

TODO: check and fix tests after we release latest version of mitmproxy_rs

@errorxyz
Copy link
Member Author

How do I tell the tests to use the updated mitmproxy_rs?

@errorxyz errorxyz changed the title Use mitmproxy_rs's getaddrinfo to resolve domain names Use mitmproxy_rs.getaddrinfo to resolve domain names Jun 30, 2024
@mhils
Copy link
Member

mhils commented Jun 30, 2024

How do I tell the tests to use the updated mitmproxy_rs?

This currently requires us to ship a new release of mitmproxy_rs. There is an upper bound in https://github.com/mitmproxy/mitmproxy/blob/main/pyproject.toml#L45 though, so we can safely do that without affecting our users.

@mhils
Copy link
Member

mhils commented Jun 30, 2024

Regarding this PR: Could we just expose a get_system_dns_server() from Rust (that uses https://docs.rs/hickory-resolver/latest/hickory_resolver/system_conf/fn.read_system_conf.html internally) instead? We can then treat DNS mode just like we treat reverse DNS mode.

(We still need our getaddrinfo for mitmproxy.proxy.server - but the addon here we can maybe simplify dramatically 😃)

@errorxyz
Copy link
Member Author

errorxyz commented Jul 1, 2024

that uses https://docs.rs/hickory-resolver/latest/hickory_resolver/system_conf/fn.read_system_conf.html internally

This is available for UNIX only

@mhils
Copy link
Member

mhils commented Jul 1, 2024

that uses https://docs.rs/hickory-resolver/latest/hickory_resolver/system_conf/fn.read_system_conf.html internally

This is available for UNIX only

The docs are misleading here - it's available for Windows as well if you look at the sources. :)

@errorxyz errorxyz changed the title Use mitmproxy_rs.getaddrinfo to resolve domain names Use mitmproxy_rs.DnsResolver to resolve domain names Jul 6, 2024
Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I'm curious whether we will run into any issues with overly long records or so, but for now this is a great way forward. 👍

@errorxyz
Copy link
Member Author

errorxyz commented Jul 6, 2024

I'm curious whether we will run into any issues with overly long records or so, but for now this is a great way forward

I don't think we'll run into this issue practically since we're only handling A/AAAA records(they typically won't cross the UDP limit). That TODO is a remnant of the old code and theoretically speaking, we may have a response size greater than the UDP limit that would need to be handled.

@mhils mhils marked this pull request as ready for review July 7, 2024 11:16
@mhils mhils force-pushed the resolver branch 2 times, most recently from 307d66e to f649b1a Compare July 7, 2024 11:40
@mhils mhils marked this pull request as draft July 7, 2024 12:01
@errorxyz errorxyz changed the title Use mitmproxy_rs.DnsResolver to resolve domain names Support all query types in DNS mode Jul 12, 2024
@errorxyz errorxyz force-pushed the resolver branch 2 times, most recently from e4d5e4c to 3c2e150 Compare July 12, 2024 17:02
@errorxyz errorxyz marked this pull request as ready for review July 12, 2024 18:39
Copy link
Member

@mhils mhils left a comment

Choose a reason for hiding this comment

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

LGTM % nits! 😃

@errorxyz errorxyz requested a review from mhils July 16, 2024 07:14
@mhils
Copy link
Member

mhils commented Jul 16, 2024

#6975 (comment) (unresolved comment above), otherwise LGTM :)

@mhils mhils enabled auto-merge (squash) July 16, 2024 14:50
@errorxyz errorxyz disabled auto-merge July 16, 2024 14:55
@errorxyz
Copy link
Member Author

Sorry, messed up the commit history a bit while hopping between windows and linux. Should be fixed now

@mhils
Copy link
Member

mhils commented Jul 16, 2024

No worries, this is why we squash! :)

@mhils mhils merged commit 317f5b9 into mitmproxy:main Jul 16, 2024
@errorxyz errorxyz deleted the resolver branch August 6, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add dns_server option.

2 participants