Re: [IPsec] Éric Vyncke's Yes on draft-ietf-ipsecme-rfc8229bis-07: (with COMMENT)

["Eric Vyncke (evyncke)" <[e-mail]>]

Hello Valery,

Thank you for the prompt reply. 

It looks like Erik Kline could be my twin brother as we often emit the same comments. FYI, I never read other ADs' ballot to avoid being biased.

I agree with all your replies and explanations except when there is EV>

Regards

-éric


On 09/08/2022, 15:59, "Valery Smyslov" <[e-mail]> wrote:

    Hi Éric,

    thank you for your comments. Please see inline.

    > Éric Vyncke has entered the following ballot position for
    > draft-ietf-ipsecme-rfc8229bis-07: Yes
    > 
    > When responding, please keep the subject line intact and reply to all email
    > addresses included in the To and CC lines. (Feel free to cut this introductory
    > paragraph, however.)
    > 
    > 
    > Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-
    > ballot-positions/
    > for more information about how to handle DISCUSS and COMMENT positions.
    > 
    > 
    > The document, along with other ballot positions, can be found here:
    > https://datatracker.ietf.org/doc/draft-ietf-ipsecme-rfc8229bis/
    > 
    > 
    > 
    > ----------------------------------------------------------------------
    > COMMENT:
    > ----------------------------------------------------------------------
    > 
    > # Éric Vyncke, INT AD, comments for draft-ietf-ipsecme-rfc8229bis-07
    > CC @evyncke
    > 
    > Thank you for the work put into this document. There must be several use
    > cases
    > for it.
    > 
    > Please find below some non-blocking COMMENT points (but replies would be
    > appreciated even if only for my own education).
    > 
    > Special thanks to Tero Kivinen for the shepherd's detailed write-up including
    > the WG consensus, but it lacks the justification of the intended status.
    > 
    > Thanks as well to Brian Haberman for his INT directorate review, his review has
    > a 'ready' status.
    > 
    > I hope that this review helps to improve the document,
    > 
    > Regards,
    > 
    > -éric
    > 
    > ## COMMENTS
    > 
    > ### UDP blocked even with QUIC
    > 
    > As there are more and more traffic relying on QUIC (a wild guess of mine...),
    > is it still the case that middle boxes are blocking UDP ? Just out of
    > curiosity... feel free to ignore.

    Well, I don't have statistics for the current situation and tend to agree that wide adoption
    of QUIC may improve the situation. But I think blocking UDP still happens.
    Note also that some proposed extensions to IKEv2 rely on TCP transport
    to be able to reliably transfer very large public keys of PQ algorithms
    (draft-tjhai-ikev2-beyond-64k-limit).

    > ### Section 1
    > 
    > ```
    > Devices on these
    >    networks that need to use IPsec (to access private enterprise
    >    networks, to route Voice over IP calls to carrier networks, or
    >    because of security policies) are unable to establish IPsec SAs.
    > ```
    > 
    > The above appears like an exhaustive list while it is not. Suggest to add ",
    > etc.".

    OK.

    > ### Section 1
    > 
    > `Some peers default to using UDP encapsulation` is "peer" so well-defined in
    > the IPsec world ? 'some' is also rather vague, perhaps cite one implementation
    > ? or use "some peers may default to" ?

    "Peer" widely used in IPsec. But I seem to see your point. How about if we 

    s/peers/implementations

    ?

EV> perfect

    > ### Section 2
    > 
    > Should "Implementations of this specification" be used in `Implementations
    > MUST
    > support TCP encapsulation on TCP port 4500, which is reserved for IPsec NAT
    > traversal.` ?

    This was already caught by Erik. Currently changed to:

    Compliant implementations MUST support TCP encapsulation on TCP port 4500...

    > ### Section 3 No AH
    > 
    > Even if AH is nearly no more used, I wonder whether there is a reason why AH
    > is
    > not supported by this I-D.

    Well, it's a long story. AH has always been a headache for implementers 
    from its birth and when UDP encapsulation of IPsec was specified 
    (the work started 20 years ago), it was decided to not support AH for it 
    (I vaguely recall there was a draft for UDP encapsulation of AH 
    in the early stage of this work, but it appeared to be too complex).
    So, it would be strange to support TCP encapsulation for AH,
    when UDP encapsulation of it is not supported. 

    > The sentence about AH should really come earlier in the document.

    Hm, while not disagree, I'm a bit unsure where to put it.

     We can add a sentence:

    	Note that AH is not supported by this specification.

    at the end of the first para of Section 1. Is it OK?

EV> perfect

    > ### Section 3
    > 
    > ```
    >    Although a TCP stream may be able to send very long messages,
    >    implementations SHOULD limit message lengths to typical UDP datagram
    >    ESP payload lengths.
    > ```
    > 
    > What is the 'typical' length ?

    It's usually the size of PMTU.

EV> then it is worth mentioning


    > ### Section 3.1
    > 
    > Suggest to add a description of "Non-ESP" header field below the description
    > of
    > the "length" field rather than in the text above.

    OK.

    > ### Section 5.1
    > 
    > `Since UDP is the preferred method of transport for IKE messages,` hem...
    > previous text (and common sense) stated that direct is the preferred method.

    Direct transport only applicable to ESP. Here text is about IKE, which 
    uses UDP port 500/4500 by default.

EV> shame on me... I should try to learn how to read __

    > ### Section 6.1 what about IP address changes ?
    > 
    > ```
    >    Since new TCP connections
    >    may use different ports due to NAT mappings or local port allocations
    >    changing, the TCP Responder MUST allow packets for existing SAs to be
    >    received from new source ports.
    > ```
    > For some NAT devices (or IPv6 nodes w/o NAT but with temporary addresses),
    > the
    > IP address could also change. This document should describe what to do in this
    > case.

    The responder may have policy restricting source IP addresses. The whole point
    of this text is that it should not restrict source ports, with TCP they are usually ephemeral.

EV> then, would the document benefit with some text around "TCP responder MAY allow packets for existing SAs to be received from new IP addresses" ?

    > ### Section 6.5
    > 
    > The very last sentence deserves a paragraph on its own.

    OK.

    > ### Section 6.7
    > 
    > Please add that the DF bit is obvioulsy only for IPv4 (Hi, Tommy, I had to
    > insert my mandatory IPv6-related comment ;-) )

    Changed:

    s/IP header/IPv4 header

    is it OK?

EV> perfect

    > ### Section 9.3
    > 
    > I am not a transport person, but I would have used "MUST NOT" rather than
    > "MAY
    > NOT" in: ```

    Do you mean rather than SHOULD NOT?

EV> oups, indeed

    >    Individual packets
    >    SHOULD NOT use different markings than the rest of the connection,
    >    since packets with different priorities may be routed differently and
    >    cause unnecessary delays in the connection.
    > ```

    Well, MUST NOT implies that it is prohibited and the receiver must log a fatal error
    if this happens. The text meaning is that it is just makes no sense. So, you may 
    do it, but the performance could degrade.

EV> fair enough

    > Even if somehow obvious, should there be a statement about the IPv6 Flow-ID
    > header field ?

    Hm... Can you propose a text?

EV> say something around Flow-ID must remain constant to avoid ECMP load balancing

    > ### TCP Fast Open support
    > 
    > Is TCP fast open supported by this doc ?

    This was already asked by Erik, and the answer was that I see no reason why 
    TCP fast open cannot be used with this spec. Probably Tommy would add more.

EV> Tommy, worth mentioning ?

    Thank you,
    Valery.

    > ## Notes
    > 
    > This review is in the ["IETF Comments" Markdown format][ICMF], You can use
    > the
    > [`ietf-comments` tool][ICT] to automatically convert this review into
    > individual GitHub issues.
    > 
    > [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
    > [ICT]: https://github.com/mnot/ietf-comments
    > 
    >