hildjj / dohdec

Lookup and decode DNS records using DNS-over-HTTPS (DoH)
MIT License
19 stars 5 forks source link

Proper support for ECS #14

Closed imcotton closed 2 years ago

imcotton commented 3 years ago

Need additional tweak of opts.ecs handling over other places since this PR changing its shape, or maybe use different fields.


resolves #9

hildjj commented 3 years ago

Can you do the following, please?

My assumption was that if the client doesn't specify an IP address, that the proxy would put in the source address masked by at most sourcePrefixLength bits at the front. In a world with lots of NATs, I expected that to be better than the IP supplied by the local OS, if you could even pick which of the several ones available is correct. I'm completely happy to let that IP be passed in to the library, at the caller's discretion, but want to preserve the original use case as well.

imcotton commented 3 years ago

In this draft am trying to get align the opts with support of ECS in dns-packet from mafintosh/dns-packet#47, which found out --ecs flag seems hasn't been mount correctly at the first place:

    if (opts.ecs != null) {
-     dns.additionals.options = [{
+     dns.additionals[0].options = [{

So command line --ecs option needs to adopt along with proper ECS support, fine with different form too.

ECS in practical to me, would be set by end user with static geo relevant address (i.e. DNS server IP of current ISP), which less privacy leaking yet gains CDN boosting.

It's fine to me to let this draft to be superseded by maturer one, glad to initialing the discussion tho.

hildjj commented 3 years ago

I'm happy to have you keep working on this, I'm looking forward to taking a patch from you. Please don't take anything I said above as anything but encouragement!

DNS server of the ISP is a good starting point. For some folks, dns.getServers() will provide some starting points, for others, those will be local caching recursive resolvers.

imcotton commented 3 years ago

I tried some ideas in the updates, looks on track?

hildjj commented 2 years ago

Sorry, I lost track of the this pull request. Would you like this to land before or after #18?

imcotton commented 2 years ago

Sorry I may have lost memory track too, are these changes looked good enough? Anything you would like to tweak further more in this batch?

codecov-commenter commented 2 years ago

Codecov Report

Merging #14 (4c6a024) into master (dadac2b) will increase coverage by 0.49%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   89.58%   90.08%   +0.49%     
==========================================
  Files           4        4              
  Lines         240      242       +2     
==========================================
+ Hits          215      218       +3     
+ Misses         25       24       -1     
Impacted Files Coverage Δ
lib/dnsUtils.js 97.67% <100.00%> (+1.24%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dadac2b...4c6a024. Read the comment docs.