Subject: [Gen-art] Gen-art last call review of draft-ietf-mpls-psc-updates-03.txt (review type corrected).

From: Elwyn Davies <[email-masked]>
Date: Fri, 04 Apr 2014 19:58:04 +0100

(Sorry...  got the wrong review type in the subject line).

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-mpls-psc-updates-03.txt
Reviewer: Elwyn Davies
Review Date: 4 April 2014
IETF LC End Date: 2014-04-09
IESG Telechat date: (if known) -

Summary:
Almost ready.  

Major Issues:

Minor Issues:
General: 
As discussed during 
Last Call/IETF review of draft-ietf-mpls-tp-psc-itu, RFC 6378 is missing
some details regarding 
- the on-the-wire format of messages
- detection of and behaviour in the event of reception of malformed
messages 
- behaviour in the event of receiving unknown TLV items
- behaviour in the event or receiving unexpected TLV items in a
particular mode.

Specifics:
On-the-wire format: 
- The encoding of the TLV Length field is not specified (unsigned binary
integer in network bit order).
- The encoding of the individual TLV length and Type fields in s2 should
also be specified (unsigned binary integer in network bit order). 
- The value of TLV Length should be more precisely specified.  Suggest:
  The TLV Length is the sum of the lengths of all TLVs in the message, 
  where the length of a TLV is the sum of the lengths of the three
  TLV fields, i.e., the the length of the value field + 4.

Malformed messages check:
- Check values of fields prior to TLV Length are consistent with s4.2 of
RFC 6378.
- Check overall length of message matches value in TLV Length + 12.
- Check Sum of lengths of TLVs matches value in TLV Length. 
- Check all TLV types received are recognized.

Behaviour in the event of receiving a malformed message:
- There has been discussion of appropriate behaviour on the MPLS mailing
list.

Behaviour in the event of receiving a well-formed but inappropriate TLV
in a message:
- This needs to be specified. (might be mode specific)

Nits/Editorial Comments:
General: s/i.e. /i.e., /g, s/e.g. /e.g., /g

Abstract/s1: Make the count of changes consistent (four/five currently).
Might be better just to say 'a number of changes' and leave the reader
to count.

s1: Since the number of items has grown to five and maybe six (depending
on how the above changes are inserted), it would helpful to link the
categorization to the actual section numbers.)

s2: It would be good to have the conventional picture here - just grab
the one from draft-mpls-tp-psc-itu.

s2, Value field: Better to say that this has the number of octets
specified in the length field rather than talking about multiples of 4
again.  The discussion of padding seems superfluous.

s4, next to last para: Should 'A remote No Request message SHALL be ignored' be
'A remote NR message SHALL be ignored' for consistency?

s5,para 8: s/In both cases the request which was driving/In both cases the request that was driving/

s6: It might be worth pointing out that extensions of PSC (like tp-psc-itu) may 
introcuce additional capabilities and state that it is up to these sepcifications to 
say how capability mismatch in this areas is/are handled.

_______________________________________________
Gen-art mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/gen-art

---

From: Eric Osborne <[email-masked]>
Date: Wed, 9 Apr 2014 09:52:02 -0400

Hi Elwyn-

  Thanks for the comments.  I'll make sure they're addressed before
publication.  I also have other comments but they're along the same
lines as yours, and those will be integrated too.

eric

---

From: Eric Osborne <[email-masked]>
Date: Fri, 11 Apr 2014 16:07:59 -0400

Hi Elwyn-

  A few questions and comments inline with EO#.

>   TLV fields, i.e., the the length of the value field + 4.

EO#
Your comments are around the on-wire format for PSC TLVs.  It seems
weird to specify the encoding *only* for TLVs and not for the rest of
PSC.  So the first thing I thought of was adding a section that says
"all of PSC is network byte order".  This seems like overkill, though,
as no other MPLS-TP RFCs discuss byte order.  Would any reasonable
implementer get everything else right in all other protocols and then
get it wrong in PSC TLVs?

> - Check all TLV types received are recognized.

EO#  Same kind of question - is this necessary?  It seems like an
implementation issue.  I'd hate to see an implementation that lets
other errors pass by uncaught because psc-updates didn't tell the
implementer what other sorts of errors to look for.

For both of these questions I looked at a few big RFCs as examples -
2328, 5340, 2460, 6427, 4271 - and got very little out of them.

4271 just says "All multi-octet fields are in network byte order."
None of the rest say anything about network bit/byte ordering or the
error checking an implementation must have in order to be compliant.

And of course rfc6378 itself doesn't cover this much either, for any
of its own fixed fields.

It took me longer to go do that research than it would have to just
write the section you want, so I'm not really wedded to leaving that
sort of stuff out.  It just seems like overkill to put it only in PSC
TLVs when it's nowhere else.

Is this level of detail really necessary?

> - This needs to be specified. (might be mode specific)

EO#  ACK - these two need to be specified as per discussion on the list.

> again.  The discussion of padding seems superfluous.

EO#  It seems odd that you want me to take our four words around
padding while adding a section on implementation-level error
checking...

eric

---

From: Elwyn Davies <[email-masked]>
Date: Mon, 14 Apr 2014 16:58:07 +0100

Hi, Eric.
> get it wrong in PSC TLVs?

The other parts of the PSC don't involve integers and so aren't
affected.

The usual solution is to add a single statement to the effect that all
integer fields are encoded as unsigned integers in network bit order.

Assuming things is always dangerous in standards.

> Is this level of detail really necessary?

Doubtless that is a matter of opinion, but in my experience of code
inspection and functional specifications/standards, providing a
checklist while we are thinking about it can help to avoid the sort of
security doom that OpenSSL is coping with at the moment.

> padding while adding a section on implementation-level error

You are not obliged to follow the comment. Anything here is designed to
make it easier for people to follow what is going on after memories have
faded... and we all have our views of how this is best achieved.

Regards
Elwyn

---

From: Eric Osborne <[email-masked]>
Date: Wed, 16 Apr 2014 11:07:53 -0400

Hi Elwyn-

  Inline and trimmed a bit.

...

>> > On-the-wire format:
...
>> PSC
.....
> Assuming things is always dangerous in standards.

All true.  OK, I'll add a such a statement.

...

>> > - Check all TLV types received are recognized.
...
> security doom that OpenSSL is coping with at the moment.

EO#  OK.  I'll add these cases and make clear that they are not the
only ones.  I don't want to make it too heavy as I'm wary of crossing
that line between protocol spec and implementation spec.

I will put something together today or tomorrow and get it out to this
list for review.

thanks!

eric