Document: draft-ietf-nea-pb-tnc-04.txt
Reviewer: Spencer Dawkins
Review Date: 10 July 2009
IESG Telechat date: 16 July 2009

Summary: This document is almost ready for publication as a Proposed Standard. I have three minor issues that I wanted to put forward before next week's telechat. All are about 2119-language in the document.

And I apologize profusely for not sending my review before the end of IETF Last Call for this document.

4.1. PB-TNC Header

Spencer (minor): the style of error handling in this section bothers me - there are several occurances where one party MUST NOT send a batch type, but if it DOES send that batch type, the other party SHOULD ignore it and send an error message if received. Is there a reason why this is not also a MUST? 
(sample follows, but there are multiple occurances in this section). If it's really a SHOULD, what do you expect the second party to do instead? And is there any guidance you can give about why the handling described might not be appropriate?

   B-Type (Batch Type) (4 bits)

      This field is used to drive the state machine described in section
      3.2. This field MUST have one of the values from the following
      table.  If any other value is received, the recipient MUST send a
      Malformed Message error code in response.  In addition, if the
      value received is not permitted for the current state, according
      to the state machine in section 3.2. , the recipient MUST send an
      Unexpected Batch Type error code in response.

      Number   Name     Definition
      ------   ----     ----------

      1        CDATA    The Posture Broker Client may send a batch with
                        this Batch Type to convey messages to the
                        Posture Broker Server.  A Posture Broker Server
                        MUST NOT send this Batch Type.  If a Posture
                        Broker Client receives a batch with this Batch
                        Type, it SHOULD ignore the batch and send a
                        fatal Unexpected Batch Type error code in
                        response.  A CDATA batch may be empty (contain
                        no messages) if the Posture Broker Client has
                        nothing to send.
...

   Batch Length (32 bits)

      This length field contains the size of the full PB-TNC batch in
      octets.  This length includes the PB-TNC header and all the PB-TNC
      messages in the batch.  In other words, it includes the entire
      contents of the batch. This field MUST contain at least the value
      8 for the fixed-length fields in this header. Any Posture Broker
      Client or Posture Broker Server that receives a PB-TNC message
      with a PB-TNC Message Length field whose value is less than 8
      SHOULD send an Invalid Parameter error code in response.

Spencer (minor): is there any guidance you can give about why this reaction might not be appropriate? I note that similar phrasing for "PB-TNC Message Length (32 bits)" is MUST send an error in response... and I'm seeing similar SHOULDs in for other invalid length fields, so I'm actually asking a question about a number of SHOULDs in this specification.

...

      When a Posture Broker Client sets the EXCL flag for a PA message,
      the Posture Broker Client MUST set the Posture Validator
      Identifier field to the identifier of the Posture Validator that
      should receive the PA message.  If the EXCL flag is not set, a
      Posture Broker Client MAY still set the Posture Validator
      Identifier value for PA messages that it sends to indicate that
      the PA message is intended as a response to a message sent by the
      Posture Validator associated with the specified Posture Validator
      Identifier.  If the Posture Broker Server does not wish to
      indicate any Posture Validator in this manner, it SHOULD set this
      field to the reserved value 0xffff.

Spencer (minor): I don't think this is a 2119 SHOULD (about 
interoperability) - if you don't set the field to the reserved value, you've 
indicated a Posture Validator, which is what you didn't want to do, or am I 
confused? I'm thinking should be lowercased (the text is just saying how you 
do what you are trying to do).