Re: [Int-dir] [Last-Call] Intdir telechat review of draft-ietf-masque-connect-ip-10

[David Schinazi <[e-mail]>]

Thanks Joe and Magnus for the replies.
Some more responses inline.
David

On Tue, Apr 18, 2023 at 1:23 AM Magnus Westerlund <
[e-mail]> wrote:

> Hi,
>
>
>
> Please see inline. Prefix with “MW:”
>
>
>
> I have added some issues to our github tracker for this review.
>
>
>
> *From: *[e-mail] <[e-mail]>
> *Date: *Tuesday, 18 April 2023 at 07:56
> *To: *David Schinazi <[e-mail]>
> *Cc: *[e-mail] <[e-mail]>,
> [e-mail] <
> [e-mail]>, [e-mail] <
> [e-mail]>, [e-mail] <[e-mail]>
> *Subject: *Re: [Last-Call] Intdir telechat review of
> draft-ietf-masque-connect-ip-10
>
> Hi, David,
>
>
>
> Responses below inline…
>
>
>
> Joe
>
>
>
> On Apr 17, 2023, at 1:49 PM, David Schinazi <[e-mail]>
> wrote:
>
>
>
> Hi Joe, thank you for your review. Responses inline.
>
> David
>
>
>
> On Fri, Apr 14, 2023 at 8:29 PM Joseph Touch via Datatracker <
> [e-mail]> wrote:
>
> Reviewer: Joseph Touch
> Review result: Not Ready
>
> This review focuses on the behavior of the tunnel from an IP perspective.
> The
> HTTP aspects are not considered.
>
> In summary, the document presents a method for tunneling IP packets over an
> HTTP connection. Its terminology and discussion is confusing, being
> presented
> largely from the perspective of the HTTP mechanism and not sufficiently
> from
> the perspective of the resulting IP tunnel that is provided. Details are
> provided below.
>
> --
>
> The terminology is confusing. Because HTTP is a client/server protocol, it
> makes sense to refer to a web (or HTTP) proxy, a single entity that acts as
> both server and client, relaying requests received on the server side and
> initiating them on the client side. An IP proxy would relay datagrams
> received
> on one IP address and issue them with at least some new addressing. What
> appears to be described here is way to create an IP tunnel over a web
> connection, which is not a proxy. It is (and should be called) just an IP
> tunnel. The web client and server are not co-located as they are in a
> proxy;
> the term proxy is nonsensical in this context.
>
>
>
> The target audience for this document is HTTP implementers, since you need
>
> an HTTP stack in order to implement this. Because of this, it is more
> helpful
>
> to use HTTP terminology instead of tunneling terminology.
>
>
>
> The target audience needs to consider both implementers and intended users.
>
>
>
> Sec 4.6 indicates that ipproto can be any number in [IANA-PN]; this is not
> consistent with the document title or remainder of the document
> description.
> That list is of protocols that can be payloads of IP packets, not of IP
> protocols - e.g., IPv6 is not 6 but 41, and when it is 41 it describes
> IPv6 as
> a payload of an outer IP packet, not an IPv6 packet. There does not appear
> to
> be a way to indicate the outermost IP packet protocol version, e.g.,
> https://www.iana.org/assignments/version-numbers/version-numbers.xhtml.
> The
> claim that implementations MUST walk the chain of extensions to find the
> protocol is unboundeded as stated; it needs to be limited, e.g., it needs
> to
> stop at the first non-IP protocol (e.g., it should not “find” TCP in IP in
> UDP
> in IP, if limited to protocol=6).
>
>
>
> I don't understand what you mean here. IPv6 extension headers all contain
> the
>
> Next Header field, but IP protocols like TCP and UDP do not. So the
> scenario
>
> that you describe of "UDP in TCP" isn't possible given the wire format.
>
>
>
> Let’s break this into separate parts.
>
>
>
> The first issue is that you never specify the IP header - IPv4 or IPv6.
>
> The [IANA-PN] could be 41. Here are two packets you could receive:
>
>
>
> a) IPv4 containing UDP
>
> b) IPv4 containing IPv4 containing UDP
>
> c) IPv4 containing IPsec containing IPv4 containing UDP
>
>
>
> Does 4 match a), b), c), or some combination?
>
>
>
> What about these:
>
>
>
> d) IPv6 containing UDP
>
> e) IPv6 containing IPv6 containing UDP
>
> f) IPv6 containing IPsec containing IPv6 containing UDP
>
>
>
> Does 41 match e), f), or both?
>
>
>
> And how do you match a) and not d) [given only IP protocols are checked,
> not the first IP header)?
>
>
>
>
>
> MW: So the outermost IP version that will be given by the use of the
> Target address. So if that is an explicit IP address it will be restricted
> to that IP version. If it is a hostname then it can be for either IP
> versions. If target is *, then it will depend on what address was assigned
> by the address capsule. If both families where given it can be either.
>
>
>
> Thus, to answer your question if IP ipproto is 41, then this packet will
> need to be either IPv4/IPv6 or IPv6/IPv6 depending on value of Target
> variable and what address families was assigned. And out of your
> alternatives only e) is valid for 41, and for a case with Ipproto=4 then
> only b will match.
>
>
>
> So based on this discussion I see the source of confusion. We should
> clarify that the IPproto field value are matched against what is present in
> the outermost IP header of the packet to be encapsulated.
>
>
>
> New Issue:
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/166
>

I also now understand, let's add some clarifying text to the spec.

—
>
>
>
> As to the chain issue, it might suffice to specify “chain of IPv6
> extension headers”.
>
>
>
>
>
> Note that this is also inconsistent with the ROUTE_ADVERTISEMENT capsule
> as described in Sec 4.7.3, which asserts that ICMPs are exceptions to the
> indicated protocol (at a minimum, this needs to be noted here as well).
>
>
>
> Good catch! This is somewhat mentioned in security considerations but I
> added a
>
> sentence to the Scoping section:
>
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/commit/d8a95d53cfb49914e6c1e99a45deece1330cda12
> <https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-8da719b533a07b0b&q=1&e=93c74e8e-16d0-455d-b2e4-e65598f856d7&u=https%3A%2F%2Fgithub.com%2Fietf-wg-masque%2Fdraft-ietf-masque-connect-ip%2Fcommit%2Fd8a95d53cfb49914e6c1e99a45deece1330cda12>
>
>
>
> Sec 4.7, although defining new capsules, is really about tunnel
> configuration.
> It should be moved to a top-level section and presented as such.
>
> Except for Sec 4.7, which explains how IP addresses and routing can be
> obtained
> over the HTTP connection, the document views the necessary behavior from
> only
> the perspective of the tunnel ingress as an HTTP client and the tunnel
> egress
> as an HTTP server.
>
>
>
> That's not right, most of the document is written in terms of "IP proxying
> endpoints"
>
> because most of the mechanisms are symmetric. Do you have a specific
> example?
>
>
>
> The abstract, which talks about the server acting a proxy but not the
> client:
>
>
>
> ...this document defines a protocol that allows an HTTP client to create
> an IP tunnel through an HTTP server that acts as an IP proxy
>
>
>
> Sec 2 definitions, again focusing on the behavior of the server, not the
> client:
>
>
>
> In this document, we use the term "IP proxy" to refer to the HTTP server
> that responds to the IP proxying request.
>
>
>
> MW: So the HTTP usage does enforce a client server behavior. That can’t be
> ignored. Only a client can request the establishment of a tunnel and what
> potential filtering and other parameters/functions to apply. The tunnel is
> then created and then it becomes symmetric in that tunnel context.
>
>
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/167
>
>
>
> Sec 7 describes clients sending packets to servers; I could not easily
> locate the converse:
>
>
>
> Clients *MAY* optimistically start sending proxied IP packets ...
>
>
>
> MW: So there is no potential for the converse to happened. The client
> requests a tunnel, and might have packets to send for that tunnel, they can
> be sent prior to the HTTP response as well as any capsule arrives back at
> the client. But the server will have the request to create a tunnel, and
> will answer that before sending anything in that tunnel context. Thus,
> there are asymmetry here.
>

There is some asymmetry (only the client can initiate the tunnel), but most
of the document speaks in terms of "IP proxying endpoints" - e.g. the
section about encapsulation. That makes it clear that both of them can send
encapsulated packets.

It is missing the way in which these ingress/egress
> components are viewed a their endpoints, e.g., to be useful as an IP
> tunnel,
> these need to appear as attached to (possibly virtual) network interfaces,
> i.e., to appear as a link, which allows them to then be used for local
> processes (via sockets), packet forwarding, etc.
>
>
>
> That's an implementation detail that doesn't belong in this document. Most
>
> implementations will indeed use virtual TUN interfaces, but it's not a
> requirement.
>
> There is a known implementation with transport protocols in userspace that
> doesn't
>
> do what you describe.
>
>
>
> Whether a TUN interface is used or some other method, there needs to be a
> method by which these applications (client, server) present something that
> accepts IP packets.
>
>
>
> That aspect of how this is actually used is ignored and needs to be
> addressed. It does not need to be implementation specific, but it would not
> hurt to give an example like TUNs.
>
>
>
> (The fact that other user-space IP systems ignore this issue is not
> rationale for this document also ignoring it)
>
>
>
> MW: I don’t see how any text can be other than informational. Considering
> the below discussion. Are you asking for a general discussion of the
> boundaries between the routing and the link the tunnel that this construct
> results in, especially as it puts some traffic filtering rules in front of
> the encapsulation that affects the routing?
>
>
>
>
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/165
>

I think I now understand what Joe was saying. We didn't make it clear
enough that this document specifies a (virtual) link with routers attached
to it. The IP proxying endpoints both act as routers that are connected by
this virtual link. That's why we talk about having them send ICMP. Adding
some text to clarify this should help. We'll cover this in issue 165.

E.g., the mechanism in Sec 4.7
> is only one way that endpoints may be configured; others should be
> discussed –
> including using DHCP or IPv6 RAs over the tunnel itself.
>
>
>
> We support assigning addresses out of band (e.g., in a config file) but
> don't see
>
> the need for DHCP or RAs through the tunnel.
>
>
>
> As an IP tunnel, it needs to consider the possibility that it would
> transit any IP traffic.
>
>
>
> As an IP device, there needs to be a description of how existing IP
> configuration would interact with the additional mechanism supported here.
>
>
>
> Sec 7 explains many aspects of IP packet handling that are already
> sufficiently
> described in RFCs 1122 and 1812 (for IPv4) and 8200 (for IPv6).  That
> section
> unnecessarily repeats that detail and is also vague as to where particular
> behaviors are to be realized. I.e., parsing the IP header, hopcount
> processing,
> and packet forwarding. The document should just clearly state that tunnels
> behave as links (as explained in draft-ietf-intarea-tunnels) and the rest
> of
> processing happens *outside* the HTTP server as defined for a host or
> router.
>
>
>
> Where the code lives is an implementation detail that we don't need to
> constrain.
>
>
>
> Sec 7 implies that the tunnel does the TTL decrementing, relaying between
> addresses, and IP packet checking.
>
>
>
> Those are functions defined in RFC1122 and 1812 as happening in endpoints
> and/or routers, not inside tunnels or links. Those documents do constrain
> when those functions happen in Internet devices; this document needs to
> adhere to those constraints. This isn’t an implementation detail; it’s an
> architectural constraint.
>
>
>
> It seems incorrect to design these tunnels as something less than a
> typical IP
> interface, esp. for IPv6 (autoconfig, neighbor discovery). Doing so only
> serves
> to invite incompatibility and/or undermine existing mechanisms that are
> useful
> in detecting misconfiguration (e.g., duplicate address detection). If
> there is
> believed to be a technical justification for this limitation, the argument
> needs to be presented and its implications reviewed (e.g., an IPv6 tunnel
> that
> isn’t a true IPv6 interface may not be useful as an IPv6 tunnel).
>
>
>
> The WG members who are implementing this don't have a need for a true IPv6
>
> interface. IKEv2 made the same design choice and it's been working fine.
>
> https://www.rfc-editor.org/rfc/rfc7296#section-3.15.3
>
>
>
> IPsec has made other decisions that undermine the way in which tunnels
> interact with routing (RFC3884), so it’s not an example of tunnels I use
> other than as a cautionary tale.
>
>
>
> At a minimum, this document needs to address the lack of these
> capabilities and the rationale for making that decision.
>
>
>
>
>
> The end of section 7 on link MTUs needs to address the additional
> requirement
> of IPv6 EMTU_R of 1500 bytes.
>
> Source fragmentation and reassembly need to be
> addressed for both IPv4 and IPv6, and path-fragmentation for IPv4 (unless
> it is
> not supported, which needs to be noted).
>
>
>
> I'm not sure we need to go into these details that are well covered by IP
> RFCs.
>
>
>
> This document doesn’t explain where this would happen - similarly to TTL
> decrement, NOT inside the client or server. That needs to be made clear.
>
>
>
>
>
> Sec 8 should be clear on what entity is responsible for ICMP packet
> generation
> and receipt. This presumably should be the HTTP endpoint device, not the
> HTTP
> client or server. I.e., the tunnel transits ICMP packets but tunnel
> endpoints
> should neither generate nor consume them, just like any other link.
>
>
>
> Similar to above, where the code lives is an implementation detail.
>
>
>
> If you implement the code inside the application, it will never handle
> ICMPs properly for packets that could be routed between this tunnel and
> other interfaces the way it should.
>
>
>
> See Sec 3.2 of draft-ietf-intarea-tunnels
>
>
>
> This is about the difference between a tunnel, an endpoint, and a router.
> A tunnel connects to and endpoint or a router; a tunnel itself is neither
> an endpoint nor a router. Only endpoints and routers emit ICMPs. There’s no
> way to handle ICMP properly inside the tunnel - it does matter where the
> code resides because the code context is inside the tunnel.
>
>
>
>
>
> As other reviewers have noted, Sec 10 on nested congestion control is quite
> thin. The current statement is equivalent to “if you KNOW congestion is
> nested,
> turn it off” – it should be the opposite, i.e., “turn congestion ON only
> if you
> KNOW congestion is NOT nested”.
>
>
>
> Fair enough. We're tweaking that section to be more permissive:
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/pull/162
> <https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-b250abaed7197763&q=1&e=93c74e8e-16d0-455d-b2e4-e65598f856d7&u=https%3A%2F%2Fgithub.com%2Fietf-wg-masque%2Fdraft-ietf-masque-connect-ip%2Fpull%2F162>
>
>
>
> It’s not about allowing congestion control to be disabled; that needs to
> be a SHOULD, with the caveat that when it is not, performance can suffer in
> ways that are difficult to predict.
>
>
>
> MW: I agree, and we could actually say SHOULD in that text under those
> constraints rather than MAY. However, doing this disabling first of all
> requires support of the DATAGRAM extension and will require changes to the
> QUIC stack that might not be possible in all deployment scenarios. And it
> can’t be done in general as some usage of this tunnel specification might
> happen over other HTTP versions than HTTP/3 that uses TCP.
>
>
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/164
>

I personally don't think a SHOULD is reasonable here. We don't have enough
data to demonstrate that this advice is always sound. My personal
experience is that nested congestion control loops work fine in practice.
(Note that this is nested congestion control, not nested loss recovery.)
Whether this should be done or not is very dependent on the deployment
environment. Adding some text warning of the risks is always good, but a
normative recommendation is a step too far.



> Section 11.1 refers to fragmented packets; it should refer to them as not
> being
> able to be “re-fragmented”; source-generated fragments are still
> fragmented and
> can cross the tunnel subject to the tunnel MTU.
>
>
>
> The use of "fragmented" in that section refers to QUIC datagram frames,
> which
>
> cannot be fragmented - this isn't about IP fragmentation.
>
>
>
> The section talks about whether IP packets can fit inside QUIC datagram
> frames.
>
>
>
> Fragmentation of those packets can - and will - happen when those packets
> are generated on the host where the packets enter the tunnel, unless the
> host decides to force “don’t fragment” on those packets. That’s a decision
> that happens (could happen or should happen, depending on your viewpoint)
> before the packets ever get to the tunnel ingress.
>
>
>
> On-path fragmentation of IPv4 packets relayed to the IP proxy happens
> (could happen or should happen, again depending on your viewpoint) before
> those packets ever get to the tunnel ingress.
>
>
>
> Either of those can happen - even if QUIC datagrams sit inside IP packets
> with DF=1 (or IPv6) that are also not source fragmented.
>
>
>
>
>
> MW: So I think this may need a bit of wording clean up and also
> clarification of the conceptual model. If I understand Joe correct here a
> reasonable way of looking on this is that when a packet arrive at the
> router part with this tunnel as one of its interfaces, the router part will
> have some knowledge of the current tunnel MTU. The tunnel itself will not
> refragement the data, but it will support a particular MTU. Thus, the
> router part can actually fragment an IPv4 packet that doesn’t have the DF
> bit set and send it into the tunnel. And in relation to discussion about
> ICMP generation. It will be the router part that generates the ICMP when
> the routing decision says send it over the tunnel, but the tunnel MTU is
> too small for the packet to fit.
>
>
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/165
>

I agree with Magnus, if we clarify the split between link and router then
this becomes natural.

Section 11.1 should require that the IP tunnel be created with a given MTU
> and
> to indicate that to the endpoint where the client/servers reside; issuing
> ICMP
> PTBs is the responsibility of those endpoints when they are seeking to use
> those tunnel endpoints, NOT of the tunnel endpoint itself. Tunnels are
> links;
> links do not issue ICMPs.
>
>
>
> This use of ICMP is a pragmatic solution to ensure that the link doesn't
> violate
>
> the minimum IPv6 link MTU. Some working group members felt strongly that
>
> we needed a solution for this.
>
>
>
> It is incorrect. ICMPs can be issued by hosts or routers, but not by
>  links (tunnels). Again, see draft-ietf-intarea-tunnels sec 3.2 as to why.
>
>
>
> The solution is to explain how this IP tunnel ties into the end hosts or
> router - again, that’s the gap in this document.
>
>
>
> MW: I do believe I see your point here. There are parts of this
> specification that impacts the router part. The scoping is basically
> traffic filters for routing decision as well as what you brought up with
> how ICMP is handled. To address this can have some significant impact on
> the document and I think we need further discussion among the authors for
> how to handle this.
>
>
>
>
>
> Sec 12 should indicate how access to the HTTP proxy itself should be access
> limited, i.e., to avoid presenting an arbitrary traffic injection point.
>
>
>
> Are you talking about things like <<don't set your root SSH password to
>
> "password">>?
>
>
>
> Not quite; it’s more like “don’t run root SSH with no password”.
>
>
>
> Regardless, it creates a huge DOS opportunity - how is the endpoint
> protected?
>
>
>
> MW: So from my perspective the whole first paragraph talks about this.
>
>
>
> There are significant risks in allowing arbitrary clients to establish a
> tunnel that permits sending to arbitrary hosts, regardless of whether
> tunnels are scoped to specific hosts or not. Bad actors could abuse this
> capability to send traffic and have it attributed to the IP proxy. HTTP
> servers that support IP proxying *SHOULD* restrict its use to
> authenticated users. Depending on the deployment, possible authentication
> mechanisms include mutual TLS between clients and proxies, HTTP-based
> authentication via the HTTP Authorization header [HTTP
> <https://ietf-wg-masque.github.io/draft-ietf-masque-connect-ip/draft-ietf-masque-connect-ip.html#HTTP>
> ], or even bearer tokens. Proxies can enforce policies for authenticated
> users to further constrain client behavior or deal with possible abuse. For
> example, proxies can rate limit individual clients that send an excessively
> large amount of traffic through the proxy. As another example, proxies can
> restrict address (prefix) assignment to clients based on certain client
> attributes such as geographic location.¶
> <https://ietf-wg-masque.github.io/draft-ietf-masque-connect-ip/draft-ietf-masque-connect-ip.html#section-12-1>
>
>
>
> This is about limiting access to the functionality of establishing
> tunnels, as well as potential having additional resource constraints on a
> tunnel.
>
>
>
> It should also address the potential use of HTTP-level encryption (e.g.,
> TLS,
> DTLS) to protect the tunnel contents and tunnel configuration exchanges.
>
>
>
>  We require the scheme to be https, which means that QUIC or TLS is
> mandatory.
>
>
>
> I see that in the examples, but it doesn’t appear to be a requirement
> anywhere. Can you indicate where it explains that “http://…”
> <https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-c4adc279bc355fbe&q=1&e=93c74e8e-16d0-455d-b2e4-e65598f856d7&u=http%3A%2F%2F...-9o0a%2F>
> would be considered invalid or that ONLY “https” can be used to start?
>
>
>
> MW: I think Joe is correct that this is only implicit required when using
> HTTP/3. We don’t actually have a requirement to use HTTPS.
>
>
>
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/163
>

Oh you're right, the use of the https scheme isn't actually mandated. That
said, we shouldn't mandate use of HTTPS, since there can be other future
schemes. Instead we should RECOMMEND running HTTP over TLS or QUIC to
provide confidentiality, integrity, and authentication. Given the existence
of HTTP intermediaries, there will be scenarios where the
client-to-intermediary security is provided by QUIC and the
intermediary-to-proxy security is provided by other mechanisms such as
IPsec or proprietary hardware-offloaded security.