if we use trr to resolve a host we should send only ip address to the proxy

Honza, it will be nice if you have time to land this as well in 68.

Yes. If you can provide any code references where to start/get the necessary info, it might be helpful. Otherwise, I'll look around myself.

(In reply to Honza Bambas (:mayhemer) from comment #2)

Yes. If you can provide any code references where to start/get the necessary info, it might be helpful. Otherwise, I'll look around myself.

dns record has info if it was trr:
https://searchfox.org/mozilla-central/source/netwerk/dns/nsIDNSRecord.idl#104
We will need this info on nsHttpConnection :(

Also maybe this bug would be useful but has not landed yet: bug 1525640.

h2 sets origin at:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/Http2Stream.cpp#570 - this should be ip address.

for h1.1
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnection.cpp#2171

this nice to have in 68 if we can make it, otherwise it is not critical.

plinks:
https://searchfox.org/mozilla-central/rev/06578bfadb5bdc5faee81f7e9b3c3fed21e616e0/netwerk/dns/nsIDNSRecord.idl#104
https://searchfox.org/mozilla-central/rev/06578bfadb5bdc5faee81f7e9b3c3fed21e616e0/netwerk/protocol/http/Http2Stream.cpp#570
https://searchfox.org/mozilla-central/rev/06578bfadb5bdc5faee81f7e9b3c3fed21e616e0/netwerk/protocol/http/nsHttpConnection.cpp#2182

this shows to be a larger project which I obviously don't want to work on a day before feature freeze.

local env:

• nghttpx locally configured as an h2 proxy (using only slightly modified Example 2 of the default config at /etc/nghttpx, with valid certs for hosts-file domain)
• having a pac file pointing at it via an HTTPS result
• added proxy exception for: detectportal.firefox.com, mozilla.cloudflare-dns.com
• enabled TRR and made sure it's working

I don't see we would be doing any DNS/TRR resolution for a requested end-domain prior opening the tunnel (I specifically focus on h2 proxying here). Simply having sync logging of socket,dns,http and going to https://www.youtube.com/, while having breakpoint inside Http2Stream::GenerateOpen, I can see we are not doing any resolution attempt for www.youtube.com. We neither try dns prefetch because we explicitly (and logically) disable it when using http proxy.

hence, we need to add the dns resolution step as a blocking component into the opening path; this should also be only optional, maybe even having a UI preference, or this may be a flag exposed to extensions to set, similarly to bug 1545420.

this all is not that simple, also we need to be able to process multiple IPs from the query response as we do now in sockettransport (connect in parallel or one by one and handle connection failures with retry).

This somewhat relates to bug 1470411. We don't want to leak any DNS (prefetch or any attempt) when a sock proxy does the resolution (e.g. tor). But for this bug we want to enforce DNS resolution (+prefetch) locally when using TRR and block on it.

We have few variables here:

• type of the proxy (http, https, socks, socks with dns)
• setting for DNS (one of the trr modes, where one of them may be 'only')

These should be enough to establish the privacy level to do or not do DNS resolution at all.

• trr-only + no proxy:
  do as we do now
• trr-* + socks doing resolution:
  use the proxy to resolve (because users explicitly say they want the proxy to resolve, we could leak to the default system DNS server; bug 1470411)
• trr-first/trr-race + other type of proxy:
  block the connection on resolution, send IP if TRR was successful, otherwise pass the host name to the proxy (the IP is resolved by system DNS resolver only)
• trr-only + other type of proxy:
  block the connection on resolution, always send only IP to the proxy or fail

So:

• if proxy==socks && proxy-dns: prevent any DNS resolution by the browser
• if proxy==https && trr==only: resolve the host with TRR, if successful send IP to the proxy, otherwise fail
• if proxy==https && trr==race|first: resolve the host with TRR and system DNS, if TRR wins/succeeds send IP to the proxy, otherwise send the host name to the proxy
• if proxy==http/https && trr==disabled: do as we do now, no resolution by the browser and send the host to the proxy
• all other cases should behave as we do now (let proxy resolve)

The target is 69 and possible uplift. Will work on this next week w/o rushing.

never ran the patch, it just builds, nothing more.

Thanks for the feedback, from both!

(In reply to Dragana Damjanovic [:dragana] from comment #11)

Comment on attachment 9063918 [details] [diff] [review]
wip1

Review of attachment 9063918 [details] [diff] [review]:

::: modules/libpref/init/all.js
@@ +2348,5 @@

pref("network.proxy.no_proxies_on", "");
// Set true to allow resolving proxy for localhost
pref("network.proxy.allow_hijacking_localhost", false);
pref("network.proxy.failover_timeout", 1800); // 30 minutes
+pref("network.proxy.connect_request_ip_only", false);

add trr in the name or at a description... ad please add a description.

yup, always before r?.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +859,5 @@

• return DoConnectActual(aTransWithStickyConn);
  +}

+nsresult nsHttpChannel::DoConnectActual(

I starts to be fun to add new steps here. Find a name is a challenge :)

@@ +870,5 @@

• if (mDNSBlockingRecord) {
• nsAutoCString address;
• rv = mDNSBlockingRecord->GetNextAddrAsString(address);
• if (NS_FAILED(rv)) {

•  return NS_ERROR_CONNECTION_REFUSED;
  

is this a correct error? Shouldn't it be ip address not found?

I think it is the correct one, but I'm not 100% sure myself; here we just exhausted all IPs to try to connect to, all of them the proxy failed to connect to. I was thinking NET_RESET, but that has a special meaning. So rather REFUSED, not sure what error page we show to the user, tho.

@@ +2030,5 @@

mTransaction->DontReuseConnection();

• // Wish I've seen the future and know which code the proxy will return
• // when the host can't be connected by the provided IP.
• if (rv != NS_ERROR_PROXY_CONNECTION_REFUSED && mDNSBlockingRecord) {

we should try to find out.

I think this is a detail to be settled with any provider we end up with. I could not find anything in any spec..

Also be sure that it is send only for the
particular case. That we do not retry if unnecessary.

Particular case means? I think NS_ERROR_PROXY_CONNECTION_REFUSED means the proxy can be reached or it refuses to connect to anything we give it to connect to. Any other type of error can be considered as that the proxy tried to connect and failed. mDNSBlockingRecord is non-null only when we made the blocking trr resolve attempt - indicating "the particular case" - if it's what we both agree the case is.

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +66,5 @@

const nsCString& GetOrigin() const { return mOrigin; }
const char* Origin() const { return mOrigin.get(); }
int32_t OriginPort() const { return mOriginPort; }

• void SetOriginServer(const nsACString& host, int32_t port);

do we need additional mOriginForConnect? did you check that tls certificate
verification will work? I am afraid that we will compare cert origin with an
ip address.

Excellent point! No, I did not... I will need two tests: one for the failover thing (plain server handling (and refusing) CONNECT + handling TRR to have control over DNS) and one for actual ability to connect (plain CONNECT handling forwarding to the http2 server we run for some dummy request + plain TRR).

I hope TRR URL doesn't need to be https for testing purposes...

Next step: do an interop test and figure out what errors proxies return when the target host can't be connected.

There is one small issue: we need to know if the proxy is ipv6 capable if we send ipv6 addresses. And that now depends on how results of ipv4/6 network checks end up. Hence, those check should go via the proxy. OTOH, we also need to know how to connect the proxy itself, so we may need two checks - one for direct connections and another one through the proxy to know the proxy capabilities.

After talking to ekr, this is no longer considered a blocker for the secure proxy project.

P2, based on :erk's comment in a private thread.

Not actively working on this one now, but if this still is a priority I can take it back.