[manet] AD Review of draft-ietf-manet-credit-window-04

["Alvaro Retana (aretana)" <[e-mail]>]

Stan:

Hi!  How are you?

I just finished reading this document.  First of all, thanks for taking this work on!

Please see my comments below.  I think many of the comments (even the Major ones) should be easy to address.  However, I am specially concerned about the actions (terminate) to be taken in some of the cases — which, I think, make DLEP vulnerable.

After you address the Major comments and provide a revised ID, I will want to get a review from the Transport Area, probably in parallel with the IETF Last Call (depending on where we are with the DLEP document).

Thanks!

Alvaro.


Major:

M1. "Credit Use Rejected" Status.  When should this Status be used?  I'm guessing that maybe windowing is not supported (the router understands the feature, but doesn't support it) -- what else?  I suspect that it is up to the receiver to use this status as it sees fit, is that the intent?  If so, please clearly say so.  Are there any well-known reasons that might be beneficial to mention?


M2. Both Sections 8.1. (DLEP Destination Up Message) and 8.2. (DLEP Destination Announce Message) say that "MUST use the value in Credit Grant as the initial value…".  But given that the credit use can be rejected, the "MUST" is misleading.  s/MUST/SHOULD or clarify "…if accepted…"


M3. It is not a requirement that Credit Windows be always implemented in both directions, right?

M3.1. Sections 5. (Operation), 8.3. (DLEP Destination Up Response Message) and 8.4. (DLEP Destination Announce Response Message) all say that after receiving a Credit Grant data item "the receiver MUST respond…with a response message that contains a Credit Grant data item".  However, Section 9.1. (Credit Grant) says that "the receiver MUST respond with a message containing a Credit Window Status data item", which seems to be the right thing.

M3.2. It seems to me that (in 8.3 and 8.4) this text should also talk about the Credit Window Status data item:
M3.2.1. (in the response): "…if the corresponding…message did not contain a Credit Grant…Response message MUST NOT contain a Credit Grant (Section 9.1) data item".
M3.2.2. "receiver of…Response MUST use the received Credit Grant value to initialize…".

M3.3. Related to this point, Section 9.1. (Credit Grant) says that the "Credit Grant data item MAY appear in the DLEP Destination Up, Destination Announce, and Destination Update messages."  No  response messages are listed, which is ok.  I would suggest you stress that those are the only messages which can contain the data item.  Either s/MAY/MAY only, or (even better) s/MAY/MUST only   [I think the "MUST" is ok because the Operation section already talks about not every destination needing credit windowing.]


M4. Credit Window Out of Sync status code.  Both 8.3 and 8.4 say that if a peer "detects a mismatch in the presence or absence of credit window data items…MUST terminate the session".

M4.1. Section 7. (DLEP Status Codes for Credit-Window Extension) says lists the failure mode as "Continue".  Please correct that and make sure that the registration request explicitly tells IANA which range to assign the status codes from.

M4.2. It seems to me that using a "terminate" failure mode may be overkill.  It also seems to me that it would be straight forward to require a Credit Grant data item to be sent after receiving this status code and let the session resync on its own (or send a Credit Grant if the lack of sync is detected locally) — it looks like the potential impact of resetting the session may not be justified for a single stream.  I note in the DLEP spec that even refusing to do something (Request Denied) has a failure mode of Continue.


M5. Section 9.2. (Credit Window Status) says that if the "local credit counts are not synchronized…MAY either…or o Issue a DLEP Destination Down message, to clear credit counts on the session."

M5.1. [nit] was this intended to be a bulleted list?

M5.2. The "MAY" indicates that both actions are optional.  IOW, it says that neither action has to be performed.  If that is so, then how do we get the session back in sync?

M5.3. That second option (Destination Down) could also result in the routing protocol (or anyone else listening to DLEP) to reset their adjacency.  As I mentioned above, this action seems to be too drastic because of the effect.  Or am I missing something?

M5.4. Going back to the "MAY either" part.  When (Why) would an implementation chose not to use the least intrusive option?

M5.5. Given the potential problems with falling out to sync, why is this section not more prescriptive: "It is recommended that implementations issue a DLEP Destination Update with a Credit Window Status data item at a configurable multiple of the DLEP Heartbeat timer…"  Maybe s/recommended/RECOMMENDED


M6. Section 9.3. (Credit Request)

M6.1. "If the corresponding…message for this session did not contain a Credit Grant data item…then receipt of the Credit Request data item MUST be considered as an error by the receiver, requiring termination of the DLEP peer session."  Same comment as before; very disruptive!

M6.2. There's no guidance as to what should be done when receiving a Credit Request.


M7. Section 10. (Security Considerations)

M7.1. "The extension does not introduce any additional threats above those documented in [DLEP]."  I disagree.  The extra opportunities to reset the DLEP session create a potential DoS attack: a peer can (for example) send a Credit Request for any destination not using credits and the whole session is reset, potentially affecting other flows.

M7.2. "The mitigation strategy documented in that document is sufficient to secure operation of this extension."  Neither "mitigation" nor "strategy" show up in the DLEP document — you'll have to be more specific.



Minor:

p1. For ease of tracking, please number the TBD values (for example TBD1, TBD2, etc.) throughout the document to match the requests to IANA.

p2. Section 6 is superfluous, please delete it.

p3. "Descriptions of the data items are included below."  Please put a reference.

p4. The format in Section 9.1. (Credit Grant) shows two "Credit Increment" fields.  From the explanation which indicates a "64-bit quantity), I'm assuming that these are really not 2 independent fields, but one big (64-bits) "Credit Increment" field.  Please fix the figure.
P4.1. The MRW/RRW in the figure in Section 9.2 have the same problem.

p5. I don't think the reference to RFC5578 is needed.  Pointing to another scheme may create confusion.



Nits:

n1. Please expand on first mention: DLEP (in the abstract), RF.

n2. s/Dynamic Link Event Protocol/Dynamic Link Exchange Protocol

n3. s/draft/document

n4. Change the order of the sections putting "Credit Window Data Item Definitions" before "DLEP Data Items for Credit-Window Extension".