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

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.

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)

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

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)

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)

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


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

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.

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

 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)

 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"

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)

13: Table 5 is formatted oddly / weird justification - presumably the
RFC Editor would fix this, but if you can, it would make review
easier.

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

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

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


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