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

[Warren Kumari <[e-mail]>]

On Fri, Mar 23, 2018 at 10:35 AM,  <[e-mail]> wrote:
> 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.

Win!

Thank you,
W

>
>>
>> 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



-- 
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