Subject: Comments on draft-ietf-tsvwg-ecn-tunnel-07

From: Gorry Fairhurst <[email-masked]>
Date: Wed, 03 Mar 2010 21:32:09 +0000

Bob, WG,

I've completed a review of the above draft, with a view to starting a 
WGLC. I have some comments, please see below, and have contacted Bob to 
see if he can make a new revision.

Best wishes,

Gorry Fairhurst
 
----------------------------------------------------------------------------
Issues:

---
1)
634 be followed.  However, alternate ECN tunnelling schemes are NOT
635 RECOMMENDED as the deployment burden of handling exceptional PHBs in

- The exact meaning of the all-uppercase expression 'NOT REQUIRED' is 
not defined in RFC 2119.  If it is intended as a requirements 
expression, it should be rewritten using one of the combinations defined 
in RFC 2119; otherwise it should not be all-uppercase.
- I wonder if this may be written /SHOULD NOT be.../?
---
2)
741 Alarms should be rate-limited so that the anomalous combinations
            ^^^^^^
- Is this a place where the IETF should make a strong requirements 
recommendation, and use /SHOULD/?
---
3)
907 will never need the OPTIONAL compatibility mode as explained in
908 Section 4.3 (except in one corner-case described below).

910 {ToDo: Question to Security Directorate: Although this corner-case
911 theoretically exists, it would be preferable to delete any mention
912 of it for simplicity & clarity.  Agree?}

914 One corner case can exist where an RFC4301 ingress does not use
915 IKEv2, but uses manual keying instead.  Then an RFC4301 ingress
916 could conceivably be configured to tunnel to an egress with
917 limited functionality ECN handling.  Strictly, for this corner-
918 case, the requirement to use compatibility mode in this
919 specification updates RFC4301.  However, this is such a remote
920 possibility that RFC4301 IPsec implementations are NOT REQUIRED to
921 implement compatibility mode.

- I'd really rather the author decided whether the block of text is 
included or delete the text (or if need by, clearly mark a set of 
sentences for RFC-Ed deletion). At the moment, I'm not sure of the 
suggested text and what the WG has decided. The WGLC needs to confirm 
specific text.

-----
4)
1285 transport will not understand other ECN codepoints.  If the
1286 outer header carries an explicit congestion marking, the
1287 alternate scheme will probably need to drop the packet--the

- I realise that section 7 is informative. However the language 
/probably need/ is particularly vague. Is it possible the WG /should/ be 
a recommended action (lower case, informative)?

-----
5)
1298 3.  Any alternate scheme MUST define a behaviour for all

- I see the use of normative language here, please use lower case /must/ 
or the WG should decide that to make the section normative.

----
6)
1315 needed tomorrow in future standards actions.

- Please consider whether point 3 should also note that rate limiting is 
needed for any Alarms raised?

----
7) Please can you check if this warning, and address or confirm if you 
think it is not important:

   == The document seems to lack a disclaimer for pre-RFC5378 work, but 
was first submitted before 10 November 2008.
(See the Legal Provisions document at 
http://trustee.ietf.org/license-info for more information.) -- however, 
there's a paragraph with a matching beginning. Boilerplate error?

----------------------------------------------------------------------------
Editorial NiTs:

24 affecting pre-existing uses of the ECN field (backward compatible).
                                                  ^^^^^^^^^^^^^^^^^^^
- it may be clearer to say /providing backwards compatibility/
---
379 element to notify the onset of congestion without having to drop
            ^
- I think it would be helpful here to provide an example, e.g. say (e.g. 
router) or something similar?
---
438 congestion states to the Internet.  Existing three
                                         ^
- insert /The/
---
441	   In fact, this document is the opposite of a fork.
- This statement is a repeat of the statement in line 432. Could you 
omit this second statement?
---
617  RFC3168 also made it an auditable event for an IPsec tunnel "if the
618  ECN Field is changed inappropriately within an IPsec tunnel...".
- It's usually better to also state to the section number when providing 
a direct citation from an RFC.
---
685 +---------+------------------------------------------------+
686 |Incoming |            Incoming Outer Header               |
687 |   Inner +---------+------------+------------+------------+
688 |  Header | Not-ECT | ECT(0)     | ECT(1)     |     CE     |
689 +---------+---------+------------+------------+------------+
690 | Not-ECT | Not-ECT |Not-ECT(!!!)|Not-ECT(!!!)|   drop(!!!)|
691 |  ECT(0) |  ECT(0) | ECT(0)     | ECT(1)     |     CE     |
692 |  ECT(1) |  ECT(1) | ECT(1) (!) | ECT(1)     |     CE     |
693 |    CE   |      CE |     CE     |     CE(!!!)|     CE     |
694 +---------+---------+------------+------------+------------+
695           |               Outgoing Header                  |
696           +------------------------------------------------+

- I see this is the only remaining use of a diagram with the bottom box, 
here at lines 695,696. This format caused me some difficulty 
understanding the tables in earlier drafts. Do you think it would be 
possible to remove these lines and replace them by an explanation in the 
figure legend saying the table defines the value in the outgoing header?
---
753 approach is discussed in Appendix D and in the discussion of the ECN
                              ^^^^^^^^^^
- This appendix is to be deleted.
---
754 nonce [RFC3540] in Section 8, which in turn refers to Appendix F.
                                                           ^^^^^^^^^^
- This appendix needs to be renumbered, as does citation on line 1357.
---
770 We can categorise the way that an ingress tunnel endpoint is paired
771 with an egress as either:

773 static:   those paired together by prior configuration or;

775 dynamically discovered:  those paired together by some form of tunnel
776 endpoint discovery, typically finding an egress on the path taken
777 by the first packet.
- To me, this seems odd to use a format of a pair of hanging paragraphs 
followed by more detailed expansion on each at line 779 and 797. Would 
it be possible to replace 773-777 with a single sentence that explains 
the two categories, and then just use one set of hanging paragraphs to 
expand on each in turn?
---
865 continue without raising any error or warning as its egress
                                                   ^^^
- I think this reads oddly. I wonder if you could replace /as/, possibly 
by starting a new para with /This/
---
996 ingress (see Appendix E for a full explanation).
                  ^^^^^^^^^^
- cites an Appendix to be deleted.
---
878 Ingress:  An RFC4301 IPsec encapsulator is not changed at all by the
                                                            ^^^^^^
- Maybe omit /at all/?
---
1081 convoluted work-rounds to this problem (see Appendix D).  But by
                                              ^^^^^^^^^^^^^^
- This text is marked for deletion. I'd rather see this cross-reference 
removed prior to WGLC. I'd prefer to see the appendix also removed at 
this stage - but if necessary RFC-ed could remove it.
---
1657 "controls are provided to manage the bandwidth of this [covert]
                                                             ^      ^
- Cross-referencing tools will interpret this as a reference. Consider 
using round brackets instead?
---
- Appendix B uses the language of "us" and "We" - given this is an IETF 
draft, it would be better to say the WG considers, or more abstractly 
the /present document/ considers etc.

1698, 1709, 1712, 1748,  etc.
---
1732 better control system designs.  For instance, Appendix E describes
                                      ^^^^^^^^^^^^^^^^^^^^^^
- Appendix E is marked for deletion this and following sentences need to 
be reworked.
---
- Appendix D was marked for deletion. Can we delete this prior to WGLC?
---
- Appendix E was marked for deletion. Can we delete this prior to WGLC?
---

---

From: Bob Briscoe <[email-masked]>
Date: Thu, 04 Mar 2010 10:02:29 +0000

Gorry,

Thanks for this careful (as always) review.

I've dealt with all your points, responding to 
some where necessary inline. The new draft was 
posted yesterday as draft-ietf-tsvwg-ecn-tunnel-08.

Here's a summary of the changes made as a 
consequence (I omitted to add this text to the 
change log within the doc - if it goes to another draft, I will add it):

 From ietf-07 to ietf-08:

       *  Changes to standards actions:

          +  Section 4: Changed non-RFC2119 phrase 'NOT RECOMMENDED' to
             'SHOULD be avoided', wrt alternate ECN tunnelling schemes.

          +  Section 4.2: Used upper-case in 'Alarms SHOULD be rate-
             limited'.

          +  Section 7: Made bullet #1 in the decapsulation guidelines
             for alternate schemes more precise.  Also changed any upper-
             case keywords in this informative section to lower case.

       *  Editorial changes:

          +  Changed copyright notice to allow for pre-5378 material.

          +  Shifted supporting text intended for deletion on publication
             into editorial comments.

          +  Explained how to read the decapsulation matrices in their
             captions.

          +  Minor clarifications throughout.

Specific responses inline...

At 21:32 03/03/2010, Gorry Fairhurst wrote:
>Bob, WG,
>
>I've completed a review of the above draft, with 
>a view to starting a WGLC. I have some comments, 
>please see below, and have contacted Bob to see if he can make a new revision.
>
>Best wishes,
>
>Gorry Fairhurst
>- The exact meaning of the all-uppercase 
>expression 'NOT REQUIRED' is not defined in RFC 
>2119.  If it is intended as a requirements 
>expression, it should be rewritten using one of 
>the combinations defined in RFC 2119; otherwise it should not be all-uppercase.

Changed to:
"...alternate ECN tunnelling schemes SHOULD be 
avoided whenever possible as ..."

>- Is this a place where the IETF should make a 
>strong requirements recommendation, and use /SHOULD/?
>---

Done

>- I'd really rather the author decided whether 
>the block of text is included or delete the text 
>(or if need by, clearly mark a set of sentences 
>for RFC-Ed deletion). At the moment, I'm not 
>sure of the suggested text and what the WG has 
>decided. The WGLC needs to confirm specific text.

Demoted to an editorial comment (to allow the 
security directorate to see it) and marked for removal by the RFC Editor.

>- I realise that section 7 is informative. 
>However the language /probably need/ is 
>particularly vague. Is it possible the WG 
>/should/ be a recommended action (lower case, informative)?

We cannot say 'should', because even the default 
scheme no longer drops such packets in all cases 
(i.e. Not-ECT inner; ECT(1) outer: forwarded as Not-ECT).

I've changed the whole bullet to the following:

"If the arriving inner header is Not-ECT it 
implies the transport will not understand other 
ECN codepoints. If the outer header carries an 
explicit congestion marking, the alternate scheme 
would be expected to drop the packet—-the only 
indication of congestion the transport will 
understand. If the alternate scheme recommends 
forwarding rather than dropping such a packet, it 
must clearly justify this decision. If the inner 
is Not-ECT and the outer carries any other ECN 
codepoint that does not indicate congestion, the 
alternate scheme can forward the packet, but probably only as Not-ECT."

>- I see the use of normative language here, 
>please use lower case /must/ or the WG should 
>decide that to make the section normative.

Done: s/MUST/must/

>- Please consider whether point 3 should also 
>note that rate limiting is needed for any Alarms raised?

On balance I don't think this is necessary, but you were right to raise it.

>----
>7) Please can you check if this warning, and 
>address or confirm if you think it is not important:
>
>   == The document seems to lack a disclaimer 
> for pre-RFC5378 work, but was first submitted before 10 November 2008.
>http://trustee.ietf.org/license-info for more 
>information.) -- however, there's a paragraph 
>with a matching beginning. Boilerplate error?

Thanks. I've switched the boilerplate to

pre5378Trust200902

All done as requested, except where stated otherwise...

>- I think it would be helpful here to provide an 
>example, e.g. say (e.g. router) or something similar?
>---

Doesn't make sense with a plural. Left unchanged.

>- This statement is a repeat of the statement in 
>line 432. Could you omit this second statement?

432 ... this is not a fork in the RFC series.
441 ... In fact, this document is the opposite of a fork.

The former says it doesn't introduce a split. The 
latter says it actually heals a pre-existing 
split. But I didn't want to make it sound as if I 
was blaming anyone for creating an earlier split. 
So, I'd rather leave these two carefully worded 
sentences unchanged - hope this is OK.

>- It's usually better to also state to the 
>section number when providing a direct citation from an RFC.
>---
>- I see this is the only remaining use of a 
>diagram with the bottom box, here at lines 
>695,696. This format caused me some difficulty 
>understanding the tables in earlier drafts. Do 
>you think it would be possible to remove these 
>lines and replace them by an explanation in the 

Removed citation of Appx D, and changed text to the following:

"Before the present specification was written, 
the PCN working-group had  proposed a number of 
work-rounds to the problem of a tunnel egress not 
propagating two severity levels of congestion. 
Without wishing to disparage the ingenuity of 
these work-rounds, none were chosen for the 
standards track because they were either somewhat 
wasteful, imprecise or complicated. Treating 
ECT(1) as either the same as ECT(0) or a higher 
severity level is explained in the discussion of the ECN nonce ..."

>754 nonce [RFC3540] in Section 8, which in turn refers to Appendix F.

I will supply XML to the RFC Editor, so that 
removal of Appendices will automatically renumber all cross-references.

>- To me, this seems odd to use a format of a 
>pair of hanging paragraphs followed by more 
>detailed expansion on each at line 779 and 797. 
>Would it be possible to replace 773-777 with a 
>single sentence that explains the two 
>categories, and then just use one set of hanging 
>paragraphs to expand on each in turn?
>---
>- I think this reads oddly. I wonder if you 
>could replace /as/, possibly by starting a new para with /This/

s/ as/, because/

>996 ingress (see Appendix E for a full explanation).

Left unchanged, as the emphasis was deliberate.

>- This text is marked for deletion. I'd rather 
>see this cross-reference removed prior to WGLC. 
>I'd prefer to see the appendix also removed at 

I'd rather leave them in for now. These 
appendices were provided mainly so that the 
Security Directorate and other reviewers outside 
the transport area had more info on the 
interactions between PCN and tunnelling if they 
needed it, without having to read PCN RFCs/drafts.

>- Cross-referencing tools will interpret this as 
>a reference. Consider using round brackets instead?

Removed completely and altered start of sentence instead:
"With respect to using the DS or ECN fields as covert channels..."

>---
>- Appendix B uses the language of "us" and "We" 
>- given this is an IETF draft, it would be 
>better to say the WG considers, or more 
>abstractly the /present document/ considers etc.
>

I've reworded all these cases into the passive.

>- Appendix E is marked for deletion this and 
>following sentences need to be reworked.
>---

See justification above for keeping these during review process.

Bob

________________________________________________________________
Bob Briscoe,                                BT Innovate & Design