Aggregation and Fragmentation Mode for Encapsulating Security Payload (ESP) and Its Use for IP Traffic Flow Security (IP-TFS)
draft-ietf-ipsecme-iptfs-19

144 -- thanks very much!

Nit:

I just happened to notice this bit in the Intro:

"While directly
   obscuring the data with encryption [RFC4303], fully"

There seem to be words missing before "fully"?

The first question of the shepherd writeup was not completely answered.  Question 18, which is particularly relevant here, was not answered at all.

Section 7.1 creates an IANA registry with "Expert Review" rules.  Of such a registry, Section 4.5 of RFC 8126 says, among other things:

   The required documentation and review criteria, giving clear guidance
   to the designated expert, should be provided when defining the
   registry.

This document doesn't do so.  After discussion, I'm told this is typical for IPSec registries; it's just understood that whoever might be assigned as a DE for this new registry will have the knowledge, context, and vision to do a good job.  I have some broad concerns about how much our succession planning in general needs improvement, and I think this sort of thing is one aspect of that problem.  I urge the WG and the AD to give this some more thought.

Thanks for including Section 8.  This is always helpful to read.

[ Chat with Christian Hopps on the telechat -- I explained my concerns and Christian will add some text around how to deploy this safely / some background context. Even if you are the network admin and in complete control of the network, having some "here are some things to keep in mind when deploying, like that that will ALWAYS use the configured bandwidth so make sure you will always have that free during failures and congestion and things like that..." type warnings are helpful. ]

Clearing my discuss.


Much thanks to Bo Wu for the OpsDir review, and to the authors for addressing / incorporating the comments.

Thanks for addressing my discuss points. The resolution looks good.

However, I have small regret that the following comments were not addressed which I believe would add more clarity to the specification. 

# Section 2.4: I would like to have rational behind the two mode of operations. what are the pros and cons and when would an implementer select one over another? if this is written somewhere else then having a point would be great benefit. 

# Section 2.4.1: The failure correction due to the expected bandwidth under estimation, where loss seems to be an indication, seems like a serious matter and but still there is no normative language requirements on the reporting the loss. I wonder how useful this could be. If the reporting is that important to have a note in this specification then it is better to use normative langues to enforce it.

# Éric Vyncke, INT AD, comments for draft-ietf-ipsecme-iptfs-13
CC @evyncke

Thank you for the work put into this document and the quick reactions on my original DISCUSS ballot on -14. Having a proper IP protocol number is the right way to proceed. I am still uneasy with the ICMP text though, but not to point of blocking this document any further.

I have now cleared my DISCUSS. I was about to ballot ABSTAIN because I am still concerned about the ICMP processing, about not specifying a generic tunnel rather than something specific to IPsec, but this does not fit the ABSTAIN per https://www.ietf.org/standards/process/iesg-ballots/, hence my no objection.

My previous COMMENTS are still there as most of them have not been replied/addressed AFAIK, this is non blocking, but the intent of those COMMENTS is to improve the document quality.

The same applies for Antoine Fressancourt’s review https://mailarchive.ietf.org/arch/msg/ipsec/4u9zP9mkITwUWPfjPBg_abo4r54/ A reply by the authors will also be welcome.

Please find below *for archiving only* my DISCUSS points, some non-blocking COMMENT points, and some nits.

Special thanks to Tero Kivinen for the shepherd's detailed write-up including the WG consensus, alas the justification of the intended status is missing :-(

Please note that Tatuya Jinmei is the Internet directorate reviewer (at my request) and you may want to consider this int-dir review as well:
https://datatracker.ietf.org/doc/review-ietf-ipsecme-iptfs-13-intdir-telechat-jinmei-2022-08-18/

I hope that this review helps to improve the document,

Regards,

-éric


## Previous DISCUSS kept for archiving

As noted in https://www.ietf.org/blog/handling-iesg-ballot-positions/, a DISCUSS ballot is a request to have a discussion on the following topics:

### Section 6.1

```
   An AGGFRAG payload is identified by the ESP Next Header value
   AGGFRAG_PAYLOAD which has the value 0x5.  The value 5 was chosen to
   not conflict with other used values.  The first octet of this payload
   indicates the format of the remaining payload data.
```
This is in direct conflict with RFC 4303 (see below) IMHO as 5 is already allocated to ST (RFC 1819, which is still 'current' even if it was never used).

But ESP RFC 4303 section 2.6 says that this is an IP protocol number (and 5 is already allocated by the IANA):
```
   The Next Header is a mandatory, 8-bit field that identifies the type
   of data contained in the Payload Data field, e.g., an IPv4 or IPv6
   packet, or a next layer header and data.  The value of this field is
   chosen from the set of IP Protocol Numbers defined on the web page of
   the IANA, e.g., a value of 4 indicates IPv4, a value of 41 indicates
   IPv6, and a value of 6 indicates TCP.
```

I.e., either this document needs to formally update RFC 4303 by allowing any number or another IP protocol number must be requested to the IANA.


### Section 2.1, generic tunnel capability 

```
   Other non-IP-TFS uses of this AGGFRAG mode have been suggested, such
   as increased performance through packet aggregation, as well as
   handling MTU issues using fragmentation.  These uses are not defined
   here, but are also not restricted by this document.
```

Moreover, while IPSECme charter includes:
```
The demand for Traffic Flow Confidentiality has been increasing in the user
community, but the current method defined in RFC4303 (adding null
padding to each ESP payload) is very inefficient in its use of network
resources. The working group will develop an alternative TFC solution that
uses network resources more efficiently.
```
it says nothing about a generic tunnelling protocol, which is usually INTAREA topic, and I cannot refrain from thinking that this tunnelling mechanism could be used on any connection-less transport, e.g., UDP or IP.

Hence, this DISCUSS point is only to initiate a discussion with IPSECME chairs and AD; Christian Hopps has already given some explanations when I deferred this I-D. I understand that I am in the rough here (no reaction on int-area and int-dir review is positive).


### Section 2.2.6

Please also mention hop-limit and RFC 8200.

### Absence of ICMP considerations

Should there be an equivalent of section 6 of RFC 4301 about ICMP ? As several unprotected packets can be bundled together, some guidance to the implementers will be welcome.

## COMMENTS

### Yet another fragmentation above layer 3

No need to reply on this comment, but I cannot refrain from wondering how many fragmentation mechanisms above layer 3 have been specified by the IETF... We could end up running this specification over IPsec over TCP ;-)

### draft-templin-intarea-parcels

Are the authors/WG aware of draft-templin-intarea-parcels ? This draft was not adopted by intarea (lack of interest mainly) but also aggregates packets into "parcels".

Christian has already replied to this comment when I deferred the IESG evaluation. This is only kept for archiving.

### Section 1

s/(indicated using protocol 59)/(indicated using IP protocol 59)/ ?

### Section 2.1

```
   Padding is only added
   to the tunnel packets if there is no data available to be sent at the
   time of tunnel packet transmission, or if fragmentation has been
   disabled by the receiver.
```

The reader will welcome explanation about why a receiver disabling fragmentation is influencing padding at the sender side.

### Section 2.2

As ESP next header expects an IP protocol number and this one should probably be an IPv6 extension header, then the format proposed on figure 1 does not fit the usual extension header format. Did the authors analyze the potential use of the usual IPv6 extension header ? (at the expense of wasting bytes of course...)

### Section 2.2 dual-stack blocks ?

Should there be a note about having a mix of IPv4 and IPv6 data blocks inside a single payload ?

### Section 2.2.5.1

Please add text about this section being IPv4-only as IPv6 header does not have a DF bit.

Is there a recommended value for the DF bit for IPv4 outer headers ?


## NITS

### Section 1

```
   While directly
   obscuring the data with encryption [RFC4303], fully, the patterns in
   the message traffic may expose information due to variations in its
   shape and timing ([RFC8546], [AppCrypt]). 
```

Possible wrong places for ',' around 'fully' ?

## 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

# GEN AD review of draft-ietf-ipsecme-iptfs-14

CC @larseggert

Thanks to Peter E. Yee for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/VKlfYh3uoGomO4_Lv8e6kltl36g).

## Comments

### Section 2, paragraph 0
```
  2.  The AGGFRAG Tunnel
```
This description in this section doesn't seem to accurately reflect the
availability of a congestion-controlled mode of operation, it's only about CBR.

### Section 2.2.3.1, paragraph 1
```
     When the tunnel bandwidth is not being fully utilized, a sender MAY
     pad-out the current encapsulating packet in order to deliver an inner
     packet un-fragmented in the following outer packet.  The benefit
```
If this MAY is not followed, the tunnel isn't a CBR tunnel anymore. Permitting
that seems to contradict one of the main premises of this approach?

### Section 2.2.5.2, paragraph 0
```
  2.2.5.2.  ECN value
```
RFC6040 has updated the guidance in RFC4301 for ECN (see above, too.)

### Section 2.4.2, paragraph 1
```
     The required inputs for the TCP friendly rate control algorithm
     described in [RFC5348] are the receiver's loss event rate and the
     sender's estimated round-trip time (RTT).  These values are provided
     by IP-TFS using the congestion information header fields described in
     Section 3.  In particular, these values are sufficient to implement
     the algorithm described in [RFC5348].
```
RFC5348 like most IETF congestion control mechanisms are sender-side. Is there a
good reason to flip this around and do the computation on the receiver, which
complicates the actual use of RFC5348?

### Section 2.4.2, paragraph 2
```
     the available tunnel bandwidth.  An implementation choosing to always
     send the data MAY also choose to only update the LossEventRate and
     RTT header field values it sends every RTT though.
```
Why? Sending known stale data is worse than not sending any data.

### Section 4.1, paragraph 1
```
     Bandwidth is a local configuration option.  For non-congestion-
     controlled mode, the bandwidth SHOULD be configured.  For congestion-
     controlled mode, the bandwidth can be configured or the congestion
     control algorithm discovers and uses the maximum bandwidth available.
     No standardized configuration method is required.
```
For congestion-controlled mode, what is the point of configuring bandwidth? The
CC algorithm will not use this at all.

### Section 4.3, paragraph 1
```
     Congestion control is a local configuration option.  No standardized
     configuration method is required.
```
I don't understand what this is supposed to express?

### Section 6.1.2, paragraph 8
```
     E:
        A 1-bit value that if set indicates that Congestion Experienced
        (CE) ECN bits were received and used in deriving the reported
        LossEventRate.
```
What is the reason for signaling this? CC does not depend on this.

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditional`; alternatives might be `classic`, `classical`, `common`,
   `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`
 * Term `native`; alternatives might be `built-in`, `fundamental`, `ingrained`,
   `intrinsic`, `original`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Section 2.4.2, paragraph 1
```
     practice RECOMMENDS that the algorithm conform to [RFC5348].
```
"RECOMMENDS" is not an RFC2119 keyword.

### Grammar/style

#### Section 2.2.3, paragraph 5
```
mbers will not work for this document so the sequence number stream MUST incr
                                     ^^^
```
Use a comma before "so" if it connects two independent clauses (unless they are
closely connected and short).

#### Section 2.2.3, paragraph 5
```
w sizes. This is because packets outside of the smaller window but inside the
                                 ^^^^^^^^^^
```
This phrase is redundant. Consider using "outside".

#### Section 2.2.3.1, paragraph 3
```
load is called an empty payload. Currently this situation is only applicable
                                 ^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Currently".

#### Section 2.2.3.1, paragraph 4
```
[RFC4301], AGGFRAG mode may and often will be encapsulating more than one IP
                                ^^^^^^^^^^^^^
```
The adverb "often" is usually put between "will" and "be".

#### Section 2.2.7, paragraph 1
```
e tunnel will take. This is required so the user can guarantee the bandwidth
                                    ^^^
```
Use a comma before "so" if it connects two independent clauses (unless they are
closely connected and short).

#### Section 2.5, paragraph 3
```
to have a round-trip time estimate. Thus the sender communicates this estimat
                                    ^^^^
```
A comma may be missing after the conjunctive/linking adverb "Thus".

#### Section 6.1.1, paragraph 7
```
value of zero indicates no loss. Otherwise the loss event rate is 1/LossEven
                                 ^^^^^^^^^
```
A comma may be missing after the conjunctive/linking adverb "Otherwise".

#### Section 6.1.3.3, paragraph 3
```
ffecting network congestion in a predictable way, and if it would be, then n
                            ^^^^^^^^^^^^^^^^^^^^
```
Consider replacing this phrase with the adverb "predictably" to avoid
wordiness.

## 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. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Thanks for addressing my DISCUSS.

Thanks to Joe Touch for 2 TSVART reviews, and for addressing his comments. Also thanks for the very literate discussion of congestion control.

(2.2.3) It would be nice to at least suggest a default number for the reordering window. In TCP, we traditionally use 3, but really any suggestion for the clueless is fine.

(3) Please clarify: is TsVal the actual tranmission time, or the time the packet is queued for the next transmission opportunity?

(3) This probably just needs a bit more explanation, but reading this document, and not knowing much about ESP, I could not figure out the case where the return path does not support AGGFRAG_PAYLOAD. IIUC, IKEv2 negotiates this for the pair explicitly, so this case cannot arise. Otherwise, how is this negotiated? Why would a tunnel endpoint support just AGGFRAG without payloads but not with?

NITS
(2.4.1) update the [RFC8229] reference to RFC8229bis?

(6.1) "The value 5 was chosen to not conflict with other used values." IIUC the values here are just Protocol numbers from the registry. So maybe it's better to be more explicit and say that this cannot be used with RFC1819 streams?