Dynamic Link Exchange Protocol (DLEP)
draft-ietf-manet-dlep-29

This is generally a well written document and I enjoyed reading it.

Section 14 (Security Considerations) now says:

   At the transport layer, when TLS is in use, each peer SHOULD check
   the validity of credentials presented by the other peer during TLS
   session establishment.  Implementations following the "dedicated
   deployments" model attempting use of TLS MAY need to consider use of
   pre-shared keys for credentials

I think you need an reference here, such as RFC 5487.

   , and provide specialized techniques
   for peer identity validation.

Section 4 of 7925 has some discussion of credential types for IoT, some of which might be useful in DLEP. But I am not sure whether this is the great reference. (If you add it, you probably should make it Informative.)

   Implementations following the
   "networked deployment" model described in Implementation Scenarios
   SHOULD refer to [RFC7525] for additional details.

For the Latency and Relative Link Quality metrics, it seems like allowing their measurement methods to be implementation-dependent reduces their usefulness. If two different implementations calculate these in different ways, then the results may not be comparable. Are these not meant to be compared between different destination implementations?

I agree with Alexey's discuss point about the security considerations.

-5.1: Is discovery support optional?
Can you offer any guidance on a reasonable maximum interval for peer discovery signals?

- 5.2: It's probably worth mentioning that the messages from here onward go over the TCP connection established as a result of discovery.
Can you offer any guidance on how long a modem should wait for the Session Initiation Message before abandoning a connection?

-7, paragraph 2: "they will need to
   be standardized either as an update to this document, or as an
   additional stand-alone specification."
I'm not sure what the distinction is between an update vs a stand-alone specification in this context. But in any case, it's unusual to require that a spec update an RFC to exercise  defined extension points.

- 8, first paragraph: "At large scale, implementations SHOULD
   consider employing techniques to prevent flooding its peer with a
   large number of Messages in a short time."

The SHOULD consider is an odd use of a 2119 SHOULD. If the intent is to tell implentors to think about this, then the SHOULD is probably not appropriate. Or does this mean "implementations SHOULD employ techniques" (which would be perfectly reasonable)?

-10.3: This seems to forbid the configuration of non-standard ports. Is that the intent?

- 10.5, 2nd to last paragraph: Can you offer any sort of guidance at all for Session Initiation timeout values?

-11.5: Do heartbeat messages occur in parallel to other active traffic, or only when things are quiet? Or in other words, do messages other than heartbeats reset the heartbeat interval timer?

Can you offer any guidance on reasonable heartbeat interval values, or guidance on how to choose a value? It seems like there could be a lot of heartbeat messages if people choose low values.

-12 Is eavesdropping a concern? Could inserted messages be used to direct user traffic over a compromised link?
What is the reasoning to not at least require implementation of TLS?
Do you imagine situations where it might make sense not to use TLS, other than the mentioned? ("deployments where the DLEP router and modem are the only devices on a physical Layer 2"). Would a "MUST use TLS unless..." construction make sense here?

Editorial Comments:

- General: Neither the abstract or the first few paragraphs of the introduction mention that this draft creates a protocol. That sort of buries the lede. Please consider mentioning that both in the abstract and near the top of section 1.

- 10.5, 2nd to last paragraph: "DLEP Heartbeats are not fully established "

I'm not sure what it means to fully establish heartbeats. Would it be reasonable to say "... not started"?

Matt's Gen-ART  comments need to be addressed.

linda dunbar performed the opsdir review

Thanks, Alexey for asking the question on validating credentials in your discuss.  It would be good to see more clarity on how that will be done and I see the response stating it is an outstanding problem in Manet - I'd like to know the plan to solve this.

Technical comments:
1) You say
"If a Signal is received with a TTL value that is NOT equal to 255,
   the receiving implementation MUST ignore the Signal."
and 
"If a packet in the TCP stream is received with a TTL value other than
   255, the receiving implementation MUST immediately transition to the
   Session Reset state."
However, there is no requirement that the initial TTL must be 225. I guess you can just add a sentence to require this from the sender.

2) You say:
"If an unrecognized, or unexpected Signal is received, or a received
   Signal contains unrecognized, invalid, or disallowed duplicate Data
   Items, the receiving implementation MUST ignore the Signal."
Would it make sense to also send an error signal in this case or was this omitted as it could be used as a DoS reflector? If so, maybe add a sentence. 

3) in section 10.5:
"DLEP Heartbeats are not fully established until receipt of the
   Session Initialization Response Message (Section 10.6), and therefore
   implementations MUST use their own timeout and retry heuristics for
   this Message."
I'm not sure if re-trying makes sense given that TCP should deliver the data reliably here. I guess time-out to terminate the TCP and a local error log would be more appropriate.

4) section 11.16:
"The Latency value is reported as transmission delay.  The calculation
   of latency is implementation dependent.  For example, the latency may
   be a running average calculated from the internal queuing."
Not sure if this is very useful. I would either recommend to define to report the average (or max?) latency (where the calculation is still of course implementation specific) or have three different items for min, max, and current average.

Editorial comment: 
The 2119 boilerplate has a weird position as part of a subsection. I assume that this is intended to say that the normative text starts here but it could also be interpreted to be just part of the subsection. Also having 'Requirements' (3.1) as subsection of 'Extension' (3) seems to be an error. I would suggest to have the 2119 boilerplate as well as the Requirements section both as own main level sections. And sec 3 and sec 7 are both called 'Extensions'...?

Further:
I didn't see a reply to the tsv-art review by Micheal Scharf. Have those comments be addressed?
Here is his review on -25:
This draft is basically ready for publication, but has nits that should be fixed before publication.

TSV-ART review comments:

* I think the DLEP protocol makes an implicit assumption that the 1-hop link between the router and the modem is unlikely to become a bottleneck, e.g., because its bandwidth is larger than the maximum possible bandwidth of the modem. I assume that in typical deployments this condition can be fulfilled, and the hop count limitation provides some safety measures. Yet, the link between the router and modem could possibly run over a tunnel, with unknown performance characteristics (e.g., another wireless backhaul link). It is unclear what a router would indeed learn from the information provided by DLEP in such a case. This scenario is not the target environment for the protocol, but it would make sense to more explicitly spell out that assumption, e.g., in Section 1.  

Other comments:

* Page 9: "If the router and modem support both IPv4 and IPv6, the IPv6 transport MUST be used for the DLEP session." seems inconsistent with page 21: "For routers supporting both IPv4 and IPv6 DLEP operation, it is RECOMMENDED that IPv6 be selected as the transport."

* I am not an IANA expert but at first sight the IANA section does not comprehensively describe the policy for modifying the IANA registries (Section 4 in RFC 5226). Is it "Standards Action"? This in particular matters for the extensions in Section 13.6.

- 5.1: Is a modem supposed to ignore peer discovery
signals from routers with which the modem does not have a
TCP connection?

- 10.7: This says: "If the modem is capable of
understanding and forwarding this information (via
mechanisms not defined by DLEP)," I don't get why that's
here, can you explain? If modem-modem comms is part of
DLEP deployments (even if not fully spec'd) then that does
change the security model.

- 10.9 and elsewhere: none of these messages have anything
like a cookie. Why not? That'd help mitigate potential off
path attacks, where we currently depend solely on TTL=255
(and TLS, which seems to not be some people's favourite;-)

- 11.8/9: are there any special addresses that MUST NOT
occur here? E.g. ::1, 127.0.0.10? What about the addresses
IANA allocates for you from 13.14/15?

- 11.10/11: what does prefix=0 mean?

- I agree with Alexey's DISCUSS#3, the TLS stuff needs
more work to be usable. Maybe recommend PSK?

Thanks for addressing my DISCUSS and COMMENT points.