Re: [Masque] Tsvart last call review of draft-ietf-masque-connect-ip-08

[David Schinazi <[e-mail]>]

Hi Bob,

Alex and I sat down to go through your thorough review in detail. I'm
sending our responses below marked with "EDITORS:". Important note: in this
email, EDITORS refers only to Alex and myself, not all document
editors/authors. Apologies if the email formatting comes out strange.

Thanks,
David


On Wed, Mar 29, 2023 at 2:48 PM Magnus Westerlund <
[e-mail]> wrote:

> Hi Bob,
>
>
>
> Thanks for the review. We authors will have to go though your comments in
> details. It is unfortunate that you did not review HTTP Datagram
> specification (RFC 9297) prior as that is allowing sending of datagrams
> over HTTP/3. In my initial read through of the comments I think most still
> applies as they are not directly related to if the tunneled IP are in
> datagrams (unreliable in delivery order) or in streams (reliable in order
> delivery).  I do know some of the issues you bring up have been discussed
> but was not documented. We will have to consider if the choice to not
> document them should be changed or not.
>
>
>
> Cheers
>
>
>
> Magnus
>
>
>
> *From: *Bob Briscoe via Datatracker <[e-mail]>
> *Date: *Wednesday, 29 March 2023 at 09:04
> *To: *[e-mail] <[e-mail]>
> *Cc: *[e-mail] <
> [e-mail]>, [e-mail] <
> [e-mail]>, [e-mail] <[e-mail]>
> *Subject: *Tsvart last call review of draft-ietf-masque-connect-ip-08
>
> Reviewer: Bob Briscoe
> Review result: Not Ready
>
> This document has been reviewed as part of the transport area review team's
> ongoing effort to review key IETF documents. These comments were written
> primarily for the transport area directors, but are copied to the
> document's
> authors and WG to allow them to address any issues raised and also to the
> IETF
> discussion list for information.
>
> When done at the time of IETF Last Call, the authors should consider this
> review as part of the last-call comments they receive. Please always CC
> [e-mail] if you reply to or forward this review.
>
> -----------------------------------------------------------------------
> ==Overall comments==
>
> How can I say this politely? The HTTP aspects of this document are great
> work
> and near-complete. However, the tunnelling aspects, in particular the
> layering
> interactions need a lot of work - possibly even a partial redesign in
> places.
> Also, the security aspects seem to have been viewed through rose-coloured
> spectacles; sthg like "Most people who like privacy are nice".
>
> Well-written, but not an easy read in a single pass (not clear what the
> point
> is for quite a few pages and lots of forward x-references). Early overview
> of
> the scheme would help. Need to bear in mind that cross-layer work has to be
> addressed at a wider community who don't necessarily all subscribe to the
> same
> mind-share.
>
> -----------------------------------------------------------------------
> ==Document Structure==
>
> I believe §§3-4 are about the tunnel config, and §§5-7 are the per-packet
> aspects of the tunnel. Might be worth explaining that in a doc roadmap.
>
> EDITORS: That's a reasonable editorial ask, we'll look into it as editors
> and see what we can do.
>
> §6 "Payload Format"
> This section needs to be split and/or re-titled, 'cos in the middle (from
> "Client MAY optimistically start sending...") it switches to operation, not
> format. At "Note that endpoints will decrement..." it switches to TTL
> handling. Then at "IPv6 requires that.." it switches to MTU discovery,
> which is another aspect of operation.
> Just because TTL and packet size are fields of an IP packet, doesn't mean
> these aspects fall under the heading of "Payload Format" - these paras are
> about handling these fields, not defining their format.
>
> EDITORS - We agree, filed an issue to track adding a new Performance
> Considerations section:
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/144
>
> -----------------------------------------------------------------------
> ==Gaps==
>
> ===A) No discussion of packet ordering ===
>
> Although one of the variants of HTTP (http/3) is over UDP, which is
> unordered, QUIC still delivers an ordered stream to the app (which, in this
> case, is the inner of the tunnel). So, presumably as packets arrive at the
> proxy, if there has been misordering or a loss, QUIC will buffer until the
> next packet in order can be delivered, i.e. head-of-line blocking. Hence,
> it's not really advisable to forward connectionless packets through a
> connection-oriented tunnel. The privacy benefit might make the poor latency
> performance worth it. But this trade off at least needs to be stated,
> perhaps in an applicability section.
>
> I don't think an http3 stream is any less granular than an http2 stream or
> an http1.1 connection in this proxy arrangement. But if it is, each need to
> be discussed separately.
> If there is a vision to use an unordered variant of QUIC, then there could
> be problems with Context IDs being processed out of order, which could
> produce all sorts of unexpected side-effects, given Context IDs are
> potentially stateful (caveat: you will see that I don't really understand
> what Context IDs might be used for).
>
> EDITORS: As discussed, this section is incorrect.
>
> ===B) Conflicting Congestion Controllers (CCs) ===
>
> Needs a section sthg like §12.1 of RFC8229, except:
>
> That text mostly just listed the problems, but the implication of
> publishing such a section in a spec is essentially, "We've told you the
> problems, but now go ahead anyway." This is not just a TCP in TCP problem.
> It's any nesting of two or more CC-controlled protocols (e.g. TCP within
> QUIC, QUIC within QUIC, etc)
>
> As above, if this is a trade off between privacy and performance, that
> needs stating.
>
> EDITORS: This makes sense, we'll add some text. Tracking in this issue:
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/145
>
> ===C) Overlapping connection life-cycles ===
>
> If a proxy crashes, powers down, etc. can it be brought back up with all
> the connection state restored that it held before it failed? Otherwise, all
> the connections within the tunnel inner depend on the survival of their
> outer connection. There is no reason to believe that all the inner
> connections are dispensable.
>
> Would a soft-state design be more resilient than the hard-state design
> described?
>
> EDITORS: This is true of all tunnels, for example IKEv2/IPsec. Luckily
> here we can use ADDRESS_REQUEST to recreate the same state and avoid
> connections breaking
>
> [The above points (A-C) are examples of the problems I mentioned when
> using a connection-oriented protocol to tunnel a connectionless protocol.]
>
> ===D) Applicable Next Headers (aka IP Protocols) ===
>  No discussion of whether some IPv6 'Next Headers' (or IPv4 Protocols) are
> not applicable / unsafe / poor performing.
>
> The last time that I proposed a generic UDP tunnel to the Intarea as an
> alternative to GUE, the criticism was that it was "too generic". I was told
> that the choice of tunnel protocols is about whether they are sufficiently
> limiting; the aim is not totally generic.
>
> EDITORS: This document is about creating an IP tunnel over HTTP so we have
> a tight scope based on the underlying protocol instead of on the high layer
> protocols.
>
> === E) Applicable Address Ranges ===
>
> Are anycast addresses in scope? IP multicast?
> Link-local multicast is mentioned. What about ARP? Service Discovery?
>
> EDITORS: We've documented the address types that require implementation
> involvement. Anycast and (non-link-local) unicast are equal to unicast as
> far as a single link is concerned. ARP operates at L2 so it doesn't apply
> to an L3 tunnel. Similarly, service discovery protocols don't need any
> special guidance.
>
> ===F) Propagation of IP headers and header fields between inner and outer.
> ===
>
> Some IPv6 extension headers are copied from inner to outer on
> encapsulation; others aren't [RFC2473; §5.1]. The outer IPv6 flow label is
> often zeroed. The contents of the DSCP, and ECN fields can be propagated
> from inner to outer and the ECN outer is propagated back to the forwarded
> header [RFC2983], [RFC6040].
>
> Because there's a whole stack of headers between the inner and outer IP
> headers (e.g. IP, UDP, QUIC, HTTP, IP), the draft needs to make it clear
> that these copying and propagation rules still apply between the two IP
> headers.
>
> EDITORS: This is a tunnel operating at L7, not an L3/L4 encapsulation
> format. Copying these fields doesn't make sense because it is possible to
> have multiple IP packets inside a single IP packet.
>
> ===G) No explanation of pros & cons relative to other packet tunnelling ===
> Yes, we can tunnel anything over anything, but why this one? AFAIK, it's
> to hide traffic in the HTTP crowd for privacy. But what are the merits
> relative to hiding in other crowds? And the drawbacks (e.g. HoL blocking as
> mentioned above). In particular, the pros & cons vs UDP in HTTP.
>
> EDITORS: There are many motivations for tunneling IP over HTTP, one of
> them being that it allows traveling through HTTP load balancers. This is
> discussed in our charter and doesn't need to be added to the document.
>
> ===H) Role reversal? ===
>
> It's not made clear whether there's anything intrinsically 'clientish'
> about a client. Or 'proxyish' about a proxy. Especially in a bump in the
> wire topology (like the site-site VPN example), does it matter which is
> which? Is it just determined by the administrator setting a proxy up in
> listen mode? Is there anything that one or the other cannot do by virtue of
> its role, other than initiate a connection to the other one? Shouldn't the
> draft speak about this if it requires correct manual set up? Particularly
> for tunnels consisting of chains (or trees?) of intermediaries.
>
> EDITORS: The terms "client" and "proxy" are HTTP terminology. The client
> sends the request, the proxy responds. Once the tunnel is up, the draft
> talks about endpoints and peers because then it's symmetric.
>
> -----------------------------------------------------------------------
> ==Section by Section==
>
>  1. Introduction (and Abstract)
>
> "...updates RFC9298"
> It's not at all clear to me what aspect of RFC9298 this RFC updates. It
> seems unlikely, given IP Proxying is an alternative to UDP Proxying, not an
> update. But if it does, the draft needs to say what it updates. And, if
> RFC9298 were updated by this draft, it would surely be a normative
> reference, not informative.
>
> EDITORS: Fair enough, we'll add text. Tracked in this issue:
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/146
>
>  3. Configuration of Clients
>
> Reasoning for each each URI template rule?
> I can't really judge whether this set of rules leaves the URI template
> flexible enough, without knowing the reason for each constraint. Is this
> explained in "Proxying UDP in HTTP"? If so, a ref would be useful.  "IP
> proxy deployments SHOULD offer service at this location if they need to
> interoperate with such clients." Surely "MUST" to interoperate?
>
> EDITORS: That's right, the MUST was to simplify interoperating. We made
> these decisions for CONNECT-UDP as a WG and then decided to keep the same
> ones for CONNECT-IP.
>
>  4. Tunnelling IP over HTTP
>
> "When sending its IP proxying request, the client SHALL perform URI
> template expansion..." Surely just "... the client performs..." (just a
> necessary process step, not an interop requirement)
>
> EDITORS: We disagree, making this normative is clearer. This matches our
> editorial style in 9298.
>
> "IP proxying requests [responses] do not carry any message content." Is it
> an error if they do?
>
> EDITORS: The stream is taken over by the capsule protocol. Data on the
> stream is not message content as the data stream has been taken over. See
> 9297 for details.
>
>  4.1. IP Proxy Handling
>
> "...the IP proxy MUST perform DNS resolution..." Surely "... the IP proxy
> performs..." (just a necessary process step, not an interop requirement)
>
> EDITORS: We disagree, making this normative is clearer. This matches our
> editorial style in 9298.
>
> "IP proxies MAY choose to tear down the tunnel due to a period of
> inactivity" Better? "IP proxies MAY choose to tear down the tunnel, e.g.
> due to a period of inactivity" Rationale: there are other possible reasons,
> e.g responding to an attack.
>
> EDITORS: It's already clear that the proxy can tear down the tunnel, this
> bit is about lifetime
>
> "Any response other than a successful response indicates that the request
> has
> failed; thus, the client MUST abort the request." This is repeated at each
> instance in the subsequent sections. Either not needed here as well, or not
> needed in each section as well.
>
> EDITORS: This isn't repeated, they're separate requirements.
>
>  4.6. Limiting Request Scope
>
> "When the IP proxy knows that a request is scoped to a target prefix or
> protocol, it can leverage this information to optimize its resource
> allocation..." This highlights that the protocol relies on a perverse
> incentive for the client to scope its request so that the server can
> optimize its resources. That smells like potentially the opposite of
> resource optimization, i.e. a vulnerability; clients can scope their
> requests to force a server to frag its resources. Perhaps consider
> redesigning the protocol so that the server initiates the scoping (or at
> least it can somehow negotiate the outcome)?
>
> EDITORS: The server is always able to abort the tunnel if it doesn't want
> to spend resources. The protocol as currently designed is what we need in
> practice, your proposed redesign is not.
>
> "target: ...the IP proxy is expected to perform DNS resolution ..." Might
> this potentially amplify a resource exhaustion vulnerability, esp. if the
> client continually gives the proxy DNSSEC work?
> "IP proxies MAY perform access control using the scoping information
> provided by the client: if the client is not authorized to access any of
> the destinations included in the scope, then the IP proxy can immediately
> fail the request." I think the clause after ':' ought to be tagged as an
> example. I think it's intended to be a degenerate/extreme example.
> Whatever, its relation with the earlier words is unclear. It's possible
> this would be better in §4.1, which is where it would fit into the process
> steps. However, at that point in the draft, scope limiting hasn't been
> explained. I'm in two minds.
>
> EDITORS: I agree with you that this change doesn't really improve things,
> so we'll keep them as-is.
>
>  4.7. Capsules
>
> Are all control messages to the tunnel on one stream, or can each capsule
> arrive in a separate stream, or is it up to the operator? IOW, do capsules
> have to be acted on in the order they are sent, or does it depend?
>
> EDITORS: Please read RFC 9297 § 3. Capsules are sent on the one (only)
> stream, and are sent in order. Capsule protocol does not require they be
> processed in order, but in almost all cases should be.
>
>  §4.7.1 The last para starting "Note that the IP forwarding tunnels
> described in this document..." seems more like an architectural point that
> ought to be stated earlier. It's related to ADDRESS_ASSIGN, but is it
> really appropriate to bury it here?
>
> EDITORS: Agreed, tracked in this issue:
> https://github.com/ietf-wg-masque/draft-ietf-masque-connect-ip/issues/144
>
>  §4.7.2
> IP version:
> Address assignment syntax presumes that two request prefixes will return
> two addresses. I think  this means that an endpoint cannot request just one
> address, when it doesn't mind whether it's within a certain IPv4 address
> range or a certain IPv6 address range. In the happy eyeballs example, this
> returns two addresses. Similarly, I don't think the syntax can express no
> preference for a prefix length. Only a specific prefix length is possible.
> Correct? Should these limitations of the syntax be pointed out?
>
> EDITORS: If a client wants exactly one address, but doesn't care which
> address family, it should send an ADDRESS_REQUEST for v4 as 0.0.0.0/32,
> and if it does not get one assigned, then send a request for v6 as ::/128
> (or other mask). (This algorithm can be run in the reverse order as well)
>  If it wants _at least_ one address of either address family, it should
> send a single ADDRESS_REQUEST asking for both, and simply not use the
> assigned address of the address family it is uninterested in.
>
>  §4.7.3
> "...the most recently received ROUTE_ADVERTISEMENT capsule..." I assume
> this means most recently received after reordering has been corrected. See
> my first point under Capsules about control message ordering.
>
> EDITORS: Please read 9297. These capsules are on the reliable stream and
> are delivered in order.
>
>  §5 Context Identifiers
>
> The point below is in a similar vein to the earlier point about whether
> generic support of IP Protocols (Next Headers) is a boon or a bane...
>
> This section seems like it's early research. It's vague, with no examples
> and no clear purpose (to me). So one cannot reason about what security
> vulnerabilities it might open up (let alone what flexibility it potentially
> affords). There is no citation of [HTTP-DGRAM] in this section, nor
> explanation of the purpose of a Context ID.
>
> EDITORS: The first paragraph explains the rationale. This feature was
> discussed at length in the WG and we decided to add context IDs to both
> CONNECT-UDP and CONNECT-IP.
>
> "The Context ID value of 0 is reserved for IP payloads, " This clearly
> means that non-IP payloads cannot use 0, but does it also mean that IP
> payloads cannot use non-zero values? It's not clear, but none of the
> examples (that all use IP) show a non-zero Context ID. A later para (2nd
> para under Fig 13 in §6) seems to more unambiguously define a 1:1 mapping
> between IP and Context ID=0. If so, I don't understand the point of a
> Context ID in these tunnelling cases. What am I missing?
>
> EDITORS: Context IDs are registered and can be for any purpose. Context ID
> 0 is pre-registered for full IP payloads. Future extensions can perform
> registration to create new context IDs with different payload types
>
> "it is possible for datagrams to be received with Context IDs that have
> not yet been registered. For instance, this can be due to reordering of the
> packet containing the datagram and the packet containing the registration
> message during transmission." So..., what happens then?
>
> EDITORS: Datagrams referring to context ids that are not yet registered
> can either be buffered or dropped by the receiver, at their discretion. We
> don't provide specific guidance because this behavior will be specified by
> extensions that use the context ID and that guidance might vary between
> extensions.
>
>  6. HTTP Datagram Payload Format
>
> "Context ID:
>     A variable-length integer that contains the value of the Context ID.
> If an HTTP/3 datagram which carries an unknown Context ID is received, the
> receiver SHALL either drop that datagram silently or buffer it temporarily
> (on the order of a round trip) while awaiting the registration of the
> corresponding Context ID."
>
> A number of problems here:
> * undefined what happens if an http/2 or http/1.1 datagram carries an
> unknown Context ID?
> *  For TCP, the app layer doesn't know the RTT.
>
> EDITORS: This won't happen in HTTP/2 or HTTP/1.1, because everything is
> delivered in order by TCP.
>
> *  Isn't   this a resource exhaustion vulnerability? the sender contrives
> packets with   Context IDs that cause the receiver to buffer everything,
> exhausting the   memory available for other senders.
>
> EDITORS: We're still bound by TCP flow control. Additionally, the receiver
> is allowed to drop so it'll do that.
>
> "Endpoints MAY implement additional filtering policies on the IP packets
> they forward." Eh? This is the first mention of filtering policies.
>
> EDITORS: Agreed, this is the first mention. Implementers of IP tunnels are
> familiar with the concept.
>
>
>  7. Error Signalling
>
> For a bump in the wire topology, can an ICMP error be sent back, from one
> IP side to the other, through the HTTP tunnel and out the other side? That
> will probably work. However, presumably an ICMP error raised by the network
> within the extent of the HTTP tunnel will be directed back to the ingress
> tunnel endpoint but no further.
>
> EDITORS: CONNECT-IP conveys ICMP inside the tunnel only and is not
> responsible for any ICMP messages on the outer connection. Existing
> handling rules apply and are out of scope of this document.
>
>  8.4. Proxied Connection Racing
>
> It might be worth pointing out in a note at the end of this example that,
> as well as the proxy deciding to set up parallel tunnels, the approach
> would also allow the client to initiate parallel tunnels.
>
> EDITORS: We don't see value in adding this note to this example. The
> proposed note is a completely different example. The proxy cannot set up
> parallel tunnels; only the client issuing CONNECT-IP can set up a tunnel.
> The right-hand side are not tunnels in this example.
>
>  10. Security Considerations
>
> I find it quite lame/naïve that this section essentially says, "This Proxy
> design gives arbitrary clients great power, both to toast proxies and to
> make them appear to be the senders of arbitrary messages to arbitrary
> destinations" BUT, "That's not a problem, because a proxy SHOULD restrict
> its use to authorized users."
>
> This massive "get out of jail free" card begs the question, "What
> management
> processes are necessary for this proxy to monitor users to determine
> whether to remove their authorization?" It's akin to saying, "We're going
> to issue automatic assault rifles to everyone over the age of 6, but it's
> not a problem, because it's illegal to shoot someone without a permit."
>
> EDITORS: Your comment is both ableist and violent. Please rephrase this to
> be professional and actionable and then we'll discuss more.
>
> -----------------------------------------------------------------------
> ==Nits==
>
> EDITORS: We've looked through these nits and made minor changes to the
> document for the ones we agree with.
>
> "HTTP provides the CONNECT method (see Section 9.3.6 of [HTTP]) for
> creating a
> TCP [TCP] tunnel..."
>     Citation of IETF core protocols like [TCP] is unnecessary. And
> definitely
>     not normative - this sentence is informatively comparing this draft
> with a
>     TCP-based alternative.
>
> §3
> s/an URI/a URI/
> (unless we're meant to pronounce URI like Cockney "In an 'urry luv?" :)
>
> §4
> Para 1: "To allow negotiation..."
> Para 3: "To initiate an IP tunnel..."
> I think these two paras repeat each other. But para 3 is better: a)
> initiate is
> a better word than negotiate (AFAICT there is no negotiation), and b) it
> explains that it's a /single/ HTTP stream.
>
> "IPv6 scoped addressing zone identifiers..." ref? [RFC6874]?
>
> §4.7.3
> Using 'IP Protocol' rather than 'Next Header' as the keyword within an IP
> Address Range is rather politically incorrect for IPv6 zealots, isn't it?
> :)
>
> Is 'equal than' (2 occurrences) correct in US English? It's certainly not
> in
> British. I can't find any reference to it.
>
> §6
> s/since receiving addresses and routes is/
>  /since receipt of addresses and routes is/
>
> "When an endpoint receives an HTTP Datagram containing an IP packet, it..."
> a tunnel endpoint?
>
> MTU sometimes ought to be PMTU.
>
> "HTTP Datagrams with payloads of at least 1280 bytes"
> Fig 13 potentially labels two things as HTTP Datagram Payload. Which is
> meant
> here? I assume the inner one, but it needs to be clear.
>
> "...the endpoints can pad the QUIC INITIAL packets of the underlying QUIC
> connection that IP proxying is running over. (Assuming QUIC version 1 is in
> use,..." The parenthesis needs to be tagged as an example.
>
> §8.1
> In the format defined in §1.3 of [QUIC] and used in the examples, a value
> after
> '=' is normally numeric. the following format threw me:
>     Payload = Encapsulated IP Packet
> I thought that must imply a field announcing that the type of the payload
> is
> the string "Encapsulated IP Packet". Any chance this could be bracketed off
> somehow, e.g.
>     Payload = <Encapsulated IP Packet>
> Is there any precedent in RFC9000 for an unquoted/unbracketed description
> of a
> binary blob on the RHS of '='?
>
> §8.4
>     "The IP proxy assigns the client both an IPv4 address (192.0.2.3) and
> an
>     IPv6 address (2001:db8:1234::a) to the client."
> Delete first occurrence of 'the client'.
>
> §10
> s/IP proxies SHOULD restrict its use to authenticated users./
>  /IP proxies SHOULD restrict their use to authorized users./
> Reason: as well as the incorrect singular, following the literal rule as
> originally written would give access to authentic criminals.
>
> §11.1
> There's a few places in the IANA section where editor's marks might be
> useful
> to identify text that will need to be updated once approved.
>
> Cheers
>
> Bob
>
>