Re: [OPSAWG] AD Review of " A YANG Module for Network Address Translation (NAT) and Network Prefix Translation (NPT)" draft-ietf-opsawg-nat-yang-13

[<[e-mail]>]

Hi Warren,

Thank you for the review. 

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Warren Kumari [mailto:[e-mail]]
> Envoyé : vendredi 23 mars 2018 10:00
> À : [e-mail]; [e-mail]
> Objet : AD Review of " A YANG Module for Network Address Translation (NAT)
> and Network Prefix Translation (NPT)" draft-ietf-opsawg-nat-yang-13
> 
> AD Review of " A YANG Module for Network Address Translation (NAT) and
> Network Prefix Translation (NPT)"  draft-ietf-opsawg-nat-yang-13
> 
> Note: I started while I was the Responsible AD for OpsAWG; Igans has
> taken this over, but these may still be helpful.
> 
> -------
> Hi there,
> 
> Apologies for how long this AD review took -- various travels got in
> the way and this got delayed.
> 
> Thank you to the editors and WG for your efforts on this document,
> it's a well written and easy to understand
> draft.  I do have a few comments that I’d like addressed  before I
> start IETF LC — addressing these now will avoid
> issues later in the process.
> 
> 1:  Section Abstract, Terminology
> "NAT44, Network Address and Protocol Translation from IPv6 Clients to
> IPv4 Servers (NAT64), ..."
> NAT44 is not defined, nor is it in the RFC Editor Well Known Acronyms
> list - I think RFC7857 might work or just add something like "Network
> Address Translation from IPv4
> to IPv4 (NAT44)" to terminology.
> 

[Med] I added "Network Address Translation from IPv4 to IPv4 (NAT44)"

> 2: Section 2.1.  Overview
> "This YANG module allows to instruct a NAT function to enable the
> logging feature"
> This is missing some words -- perhaps "This YANG module provides a
> method to instruct a NAT function to enable the logging feature" (or
> similar)
> 

[Med] Works for me. Fixed.


> 3: Section 2.2.  Various Translation Flavors
> "The NAT YANG module allows to retrieve the capabilities of a NAT
> instance " -- same as above
> 

[Med] Fixed.

> 4: Section 2.4.  Other Transport Protocols
> "The module is structured to support other protocols than UDP, TCP, and ICMP.
> "
> s/other protocols than/protocols other than/  (readability / flow)
> 

 [Med] OK

> 5: "The mapping table is designed so that it can indicate any
>    transport protocol.  For example, this module may be used to manage a
>    DCCP-capable NAT that adheres to [RFC5597].
>    Future extensions can be defined to cover NAT-related considerations
>    that are specific to other transport protocols such as SCTP
>    [I-D.ietf-tsvwg-natsupp]."
>  The above sounds confusing (to me at least) - the mapping table is
> designed so it can indicate any transport protocol. Future extensions
> can be defined to make it do so? (not sure how to word it better, but
> the above sounds unclear as to if the mapping table can actually
> indicate any transport protocol or if it itself needed to be extended)
> 

[Med] The mapping table can indicate any transport protocols. Nevertheless, if some transport needs to manipulate some specific information, then the mapping entry needs to be extended. 

I changed the text to "Future extensions may be needed to cover ..."

>  6: "Also, the module allows to enable translation for these protocols
> when required"
>  Similar to #2, #3 -- perhaps "the module allows the operator to
> enable translation" or "the module enables translation for" (I think
> the former, or reword).
> 

[Med] Fixed.

> 
> 7: Section 2.6.  Port Set Assignment
>    "Port numbers can be assigned by a NAT individually (that is, a single
>    port is assigned on a per session basis).  Nevertheless, this port
>    allocation scheme may not be optimal for logging purposes"
>    I would suggest combining these into a single sentence -- "... on a
> per session basis), but this port..." - purely a readability nit
> 

[Med] Deal.


> 8: "Therefore, a NAT function should be able to assign
>    port sets (e.g., [RFC7753]) to optimize the volume of the logging
>    data (REQ-14 of [RFC6888])."
>    "Therefore" sounds like it is a new requirement on NATS - can you
> reword to make it clear it isn't.
> 

[Med] I deleted "Therefore" to avoid that misinterpretation.

> 9: Section 2.7.  Port-Restricted IP Addresses
>  "Some NATs require to restrict the source port numbers"
>  I'd suggest s/require to//
> 

[Med] Fixed.

>  10: Section 2.8.  NAT Mapping Entries
>    "A TCP/UDP mapping entry maintains an association between the
> following information:"
>    It this true for all types of NATs? For example, a 1:1 NAT /
> rewriting doesn't need to track ports, because 192.0.2.1:xxx ->
> 10.10.10.10:xxx (internal port == external port, so no need to track
> port state)
> 

[Med] TCP/UDP mapping does make sense only when ports are rewritten. For the case you cited, mappings are not tracked at the transport level.

>  11: "In order to cover both NAT64 and NAT44 flavors in particular,
> the NAT mapping structure allows to include an IPv4"
>   I think you can drop the "in particular"
> 

[Med] Works for me.

> 12: "In order to cover both NAT64 and NAT44 flavors in particular, the
> NAT mapping structure allows to include an IPv4"
>   "allows to include an" parses oddly - perhaps "allows for the
> inclusion of an..." (or similar)
> 

[Med] Fixed.

> 13: Table 5 is formatted oddly / weird justification - presumably the
> RFC Editor would fix this, but if you can, it would make review
> easier.
> 
[Med] Will check and fix as appropriate.

> 14: "In order to prevent from generating frequent notifications"
> This is missing a word or words.
> 

[Med] I added "...prevent a NAT implementation ... "

> 15: "The NAT YANG module allows to set parameters to prevent a user from"
> Similar to #2, #3.
> 

[Med] Fixed.

> 16: "Nevertheless, an attacker who is able to access to the NAT can
> undertake"
> s/to//
> 

[Med] Fixed. 

> 
> Thank you.
> W
> 
> --
> I don't think the execution is relevant when it was obviously a bad
> idea in the first place.
> This is like putting rabid weasels in your pants, and later expressing
> regret at having chosen those particular rabid weasels and that pair
> of pants.
>    ---maf