ietf-wg-masque / draft-ietf-masque-connect-udp

Other
29 stars 9 forks source link

Response code for DNS resolve resulting in unsupported IP version #167

Closed gloinul closed 2 years ago

gloinul commented 2 years ago

I also want to point an issue due to this stance. The fact that we are not explicit and discuss the expectations and realities of different IP versions we have interesting failure cases due to this text from Section 3.1:

“However, if the target_host is a DNS name, the UDP proxy MUST perform DNS resolution before replying to the HTTP request.”

No where in the follow paragraphs in this section do it discuss the slight issue that a DNS resolution can return only an AAAA record and the MASQUE server may only support IPv4. What is the error response? Should this be a proxy-status response. I would not there are currently no response for “IP version not supported”.

LPardue commented 2 years ago

These concerns apply equally to TCP proxying via HTTP. We should strive to provide parity to how CONNECT treats these matters. HTTP semantics don't say anything

DavidSchinazi commented 2 years ago

+1 to Lucas' meta-comment about striving for parity with CONNECT

There is no such thing as a "unsupported IP version" DNS failure. Let's take an example where the client wants to CONNECT-UDP via the UDP proxy proxy.example.net to the target ipv6only.davidschinazi.com. This particular host is configured such that querying AAAA will get a valid address, and querying A will yield a "no answer no error" DNS result. Let's assume that in this example the UDP proxy is IPv4-only. In that scenario, the UDP proxy will only query A (because it doesn't know what AAAA is) and it will get "no result no answer" so it will send Proxy-Status: proxy.example.net; error=dns_error; rcode= NOERROR

DavidSchinazi commented 2 years ago

@gloinul in light of the previous comments, can you clarify what you think is missing from the document please?

gloinul commented 2 years ago

First of all, I think that just answering lets do like Connect is a poor answer. There are benefits to alignment, but also one should ask if one are preventing useful functionality. In this case I think there are a relevant difference between what could be answered depending on how one implement stuff. Your suggest road says that for a IPv4 only proxy with a DNS name that has AAAA record but no A record the answer is: a) There is no answer

For a node that would query both the answer could actually be: b) There is host, but I can't reach it as I don't support the right IP version, try another proxy.

Sure, limiting ourselves to a) is easy, but is it the most useful for the protocol?

LPardue commented 2 years ago

Disagree it's a poor answer. An HTTP proxy that inplements CONNECT for UDP is likely going to implement CONNECT for TCP. How to treat name resolution for both of these has to be consistent.

The Proxy-Status header field is the appropriate way to signal such failures.

Any text more complicated than what we have now, should be pursued in a separate document IMO. Not added at this late stage.

It would be straightforward to form up a document like "DNS considerations for HTTP proxying" that is additive to the specifications we have.

bemasc commented 2 years ago

I agree: Proxy-Status is the right place for this discussion, and CONNECT-UDP already recommends Proxy-Status.

Also, as a technical matter, this is not how dual-stack connectivity works. As noted above, an IPv4-only UDP client will never issue a AAAA query for a destination hostname.

DavidSchinazi commented 2 years ago

I agree with what Lucas and Ben said above. Let's move this discussion to an extension draft.

achernya commented 2 years ago

I agree that this discussion probably deserves to be in an extension draft. As is stands today, a DNS failure as described in this thread can just as easily result in a destination-unreachable / connection-closed error for the CONNECT-UDP stream, and an extension to provide detailed information as to why seems like the natural path forward.

tfpauly commented 2 years ago

Proxy status is absolutely the right thing to do here. The error=dns_error proxy-status is what we are already doing in production for CONNECT and CONNECT-UDP.

The current text seems entirely appropriate for this:

If errors occur during this process (for example, a DNS resolution failure), the UDP proxy MUST fail the request and SHOULD send details using an appropriate "Proxy-Status" header field [[PROXY-STATUS].

The only thing I could see saying further is to mention the error parameter, like this:

If errors occur during this process (for example, a DNS resolution failure), the UDP proxy MUST fail the request and SHOULD send details using an appropriate "Proxy-Status" header field with a relevant "error" parameter [[PROXY-STATUS].

I do not think we need any extension draft for this. This is very straightforward, and doesn't need to say much.

DavidSchinazi commented 2 years ago

Thanks Tommy, I think that's a useful clarification. I wrote up PR #169 based on your proposal to make this clearer

gloinul commented 2 years ago

Ok, I am fine with this resolution.

DavidSchinazi commented 2 years ago

Thank you for confirming Magnus!