Subject: [Softwires] Comments on draft-sivakumar-yang-nat-05

From: "Senthil Sivakumar (ssenthil)" <[email-masked]>
Date: Wed, 8 Feb 2017 18:15:30 +0000

I had reached out to Dan Wing and some others, as suggested by Ian to get reviews on the 
https://tools.ietf.org/html/draft-sivakumar-yang-nat-05.

I am just posting the response from Dan here, I will address his comments in a separate email and copy the alias.

Thanks
Senthil

Use [email-masked] as my email address, to ease sorting the email if someone happens to follow up.  
    
>> was going to ask you for a favor to review a I-D for me. The recommendation from OPS WG was to get
    >> Some expert reviews from Behave members, finding any behave members willing to do a review is hard these days.
    >> 
    >> https://tools.ietf.org/html/draft-sivakumar-yang-nat-05
    > 
    >    My comments -- do you want them public somewhere?
    > 
    >    1. Why not also cover NAT66 and NPTv6?  It seems the design allows those, but the text doesn't mention that they can also be expressed in this model.
    > 
    >    2. "   A NAT function can either assign individual port numbers or port
    >       sets.  Both features are supported in the YANG data model."  ->  can you provide citation or definition of "port set"?
    > 
    >    3. "   To accommodate deployments where [RFC6302] is not enabled, the NAT
    >       function can be configured to log the destination port number." ->  that logging is not a "NAT function" (whereas the previous paragraph does describe a NAT function).  Logging is a function of YANG, right?  Perhaps instead how about text like "... this defined Yang model can be configured to ..."
    > 
    >    4.    "This data model assumes that pools of IPv4 addresses can be
    >       provisioned to NAT function.  These pools may be contiguous or non-
    >       contiguous."
    > 
    >    First sentence does not read well, would be improved with "... can be provisioned to *the* NAT function".  Also, please provide description or citation to what is meant by "pool".
    > 
    >    5. "A NAT device can enabled multiple NAT instances;"
    > 
    >    does not read well.
    > 
    >    6. "   o  Exclude/include ports (e.g.; system port) from the port assignment
    >          pool."
    > 
    >    Nit: That should be "system portS" (plural).
    > 
    >    Serious:  that is a significant deficiency.
    > 
    >    7. "Deterministic NAT assignment scheme" -> provide description or citation about what that means.
    > 
    >    8. "module: ietf-nat
    >       +--rw nat-config
    >       |  +--rw nat-instances
    >       |     +--rw nat-instance* [id]
    >       |        +--rw id                                 uint32
    >       |        +--rw enable?                            boolean
    >       |        +--rw external-ip-address-pool* [pool-id]
    >       |        |  +--rw pool-id             uint32
    >       |        |  +--rw external-ip-pool?   inet:ipv4-prefix"
    > 
    >    How is IPv6 expressed for NAT64?
    > 
    > 
    >    9.  "   |        +--rw subscriber-mask-v6?                uint8
    >       |        +--rw subscriber-mask-v4* [sub-mask-id]
    >       |        |  +--rw sub-mask-id    uint32
    >       |        |  +--rw sub-mask       inet:ipv4-prefix"
    > 
    >    Why are IPv6 masks expressed differently than IPv4 mask?
    > 
    >    Is "subscriber" the same as "internal"?  I mean, this whole Yang model seems to use "subscriber" and "external", rather than "internal" and "external".  What if the "internal" side isn't """subscribers""", such as a NAT in front of a datacenter, like a NAT64 in front of an IPv6-only datacenter, or a NAT44 in front of an IPv4-only datacenter; in that case the "subscriber" is now confusingly the server.
    > 
    > 
    >    10.     |        +--rw port-randomization-enable?         boolean
    >       |        +--rw port-preservation-enable?          boolean
    > 
    >    both of those could be set to TRUE, creating opportunity for configuration and implementation conflict, creating interoperability problems.  Can you instead define a trinary value, or does Yang like to have booleans so much, even when they can cause interop problems?
    > 
    >    11.    |        +--rw udp-timeouts?                      uint32
    > 
    >    Have you considered per-port timeouts?  Some NATs are configurable with short timeouts for certain ports, such as 10 seconds on port 53 (DNS) and NTP (123) and longer timeouts on other ports.  This Yang model doesn't allow that.  Seems a port list might be better to handle such configurations.
    > 
    >    12. I noticed there isn't any reference in the text to RFC7857, which updates and clarifies a lot of things.  Is the Yang model compliant with the changes caused by RFC7857?  The text should certainly cite it where it makes sense, but more importantly if additional settings are required by RFC7857, they need to be part of the yang model.
    > 
    >    13.    |        +--rw logging-info
    >       |        |  +--rw destination-address    inet:ipv4-prefix
    >       |        |  +--rw destination-port       inet:port-number
    > 
    >    this doesn't indicate UDP or TCP, and doesn't seem able to log ICMP or other protocols that lack ports, which NATs might NAT (e.g., IPsec ESP protocol 50).  Should be highlighted in security considerations.
    > 
    >    14.    |        +--rw connection-limit
    >       |        |  +--rw limit-per-subscriber?   uint32
    >       |        |  +--rw limit-per-vrf?          uint32
    >       |        |  +--rw limit-per-subnet?       inet:ipv4-prefix
    > 
    >    Can only list one subnet?
    > 
    >    This doesn't allow different limits per VRF (e.g., VRF 1 is limited to 100 mappings, VRF 2 is limited to 5555 mappings).  Seems restrictive.
    > 
    >    15.    |        +--rw ftp-alg-enable?                    boolean
    >       |        +--rw dns-alg-enable?                    boolean
    >       |        +--rw tftp-alg-enable?                   boolean
    >       |        +--rw msrpc-alg-enable?                  boolean
    >       |        +--rw netbios-alg-enable?                boolean
    >       |        +--rw rcmd-alg-enable?                   boolean
    >       |        +--rw ldap-alg-enable?                   boolean
    >       |        +--rw sip-alg-enable?                    boolean
    >       |        +--rw rtsp-alg-enable?                   boolean
    >       |        +--rw h323-alg-enable?                   boolean
    >       |        +--rw all-algs-enable?                   boolean
    > 
    >    OMG.  ALGs, really?  Do those need to be per-subscriber or per-VRF or per-subnet?  How are new ALGs added to Yang, like if I want an ALG for, um, I dunno, WebRTC or ssh.
    > 
    >    16.  In mapping-table, I see:    "rw lifetime                uint32".  Would this track the lifetime while the TCP connection is becoming fully-formed and hits the various Yang-defined 'timeout' values (tcp-idle-timeout, tcp-trans-open-timeout, and so on)?".  I suppose it does.  Text should say so.
    > 
    >    17.             |  +--ro nat44-support?                              boolean
    >                |  +--ro nat64-support?                              boolean
    > 
    > 
    >    NAT46?  NAT66?  NPTv6?  XLAT64?  CLAT?
    > 
    >    18.  stealth-mode-support needs better description than "Indicates whether to respond for unsolicited traffic.", and needs to align with IETF host requirements and IETF router requirements RFCs.
    > 
    > 
    >    -d
    > 
    > 
    > 
    > 
    >