Subject: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-nat-yang-06

From: Jürgen Schönwälder <[email-masked]>
Date: Fri, 27 Oct 2017 11:14:15 -0700

Reviewer: Jürgen Schönwälder
Review result: Not Ready

Summary

>From a YANG modeling point of view, the module is rather straight
forward and I did not discover anything that seems fundamentally
problematic. That said, there are a number of details where I doubt
the model is correct and things where I believe things are incomplete.
Given that there are many different NAT functions, I also expected to
see usage of YANG features.

One fundamental question one could raise is whether this model should
have been broken down into a generic NAT core model plus extensions of
the core model for the different types of NATs. Well, a single model
with YANG features is an inline version of that as this still requires
to mark clearly which parts are specific to certain types or NATs.

I did not compile the module myself since the IETF tracker says no
errors using pyang and yanglint (and I would not have run anything
different). (Is it OK for YANG doctors to trust the tracker tools?
Well, since this is labelled as an early review and I expect more
updates, I assume this is fine.)

I can't tell whether the model makes sense from a NAT point of view. I
am not an expert in the various types of NATs and surely more reviews
of NAT experts would be good (and they would have likely spotted some
things I have spotted, so I guess the usual problem of getting enough
substantial reviews).

Details

- We use revision statements only for published modules. Hence, please
  replace the revision statements with

  // RFC Ed. update to match the date of the latest edits
  revision 2017-xx-yy {
    description
      "Initial revision.";
    reference
      "RFC XXXX: A YANG Data Model for Network Address Translation
                 (NAT) and Network Prefix Translation (NPT)";
  }

- We usually follow a different template for the contact statement
  that has more context and just a list of names and email addresses.

- We usually write

     reference
       "RFC 3022: Traditional IP Network Address Translator
                  (Traditional NAT)";

  instead of just

     reference
       "RFC 3022.";

  since not everybody remembers all the numbers equally well.

- Is there a reference for dst-nat?

- What is the vrf-routing-instance identity good for? Its used only in
  the leaf external-vrf-instance and the description of that leaf is
  not telling me how this is going to be used. Who is going to derive
  identities? I fear you are trying to achieve something where using
  identities is really the wrong approach. Section 2.10 does not tell
  how this is supposed to work either. I assume this needs to be
  checked by the routing area experts to make sure things fit with
  their modeling of VRFs.

- s/start-port-numbert/start-port-number/

- Are comments like

  // port numbers: single or port-range

  necessary given that there are description statements? Note that
  comments may be removed by tools while description statements
  usually are preserved. So it is important to have anything important
  covered in description statements.

- What is a PSID algorithm? Spell out acronyms on first usage.

- The leafs start-port-number and end-port-number are commented out in
  port-range, probably in favour of the uses port-number. If they are
  not needed anymore, they should be removed.

- I do not understand port-set-algo from the descriptions. What is the
  psid-offset applied? What is a 'sharing ration for an IPv4 address'?
  Is this stuff IPv4 specific? Is the psid something that has a
  certain meaning outside of this YANG module? If so, where do I find
  more details (missing reference statement?)?

- mapping-entry/index: is this an arbitrary identifier? how are these
  identifiers allocated? Are they created by the NAT or are they created
  via configuration? Or even both? Well, I guess it is both given the
  mapping-entry/type leaf.

- mapping-entry/type: instead 'manually configured', I would write
  'explicitly configured' (configuration does not have to be a manual
  process). I also find the other enum labels at least a bit confusing.

        enum "dynamic-explicit" {
          description
            "This mapping is created by an
             outgoing packet.";
        }
        
        enum "dynamic-implicit" {
          description
            "This mapping is created by an
             explicit  dynamic message.";
        }

  Note the 'implicit' vs 'explicit' clash here. Perhaps this should be
  aligned with the definition of dynamic and implicit mappings:

   o  Dynamic implicit mapping: is created implicitly as a side effect
      of traffic such as an outgoing TCP SYN or an outgoing UDP packet.
      A validity lifetime is associated with this mapping.

   o  Dynamic explicit mapping: is created as a result of an explicit
      request, e.g., PCP message [RFC6887].  A validity lifetime is
      associated with this mapping.

  But then it seems you really messed things up here. I would even
  write these definitions differently since 'outgoing' is unclear and
  potentially confusing.

   o  Dynamic implicit mapping: is created implicitly as a side effect
      of processing a packet (e.g., an initial TCP SYN packet) that
      requires a new mapping. A validity lifetime is associated with
      this mapping.

   o  Dynamic explicit mapping: is created as a result of an explicit
      request, e.g., PCP message [RFC6887].  A validity lifetime is
      associated with this mapping.

   Similarly, I would change the description of

        enum "dynamic-implicit" {
          description
            "This mapping is created implicitely as a side effect
             of processing a packet that requires a new mapping.";
        }

        enum "dynamic-explicit" {
          description
            "This mapping is created as a result of an explicit
             request, e.g., a PCP message.";
        }

- We should perhaps have a type for an IP protocol number, ideally in
  inet-types. (Just a reminder to myself.) That said, I do not
  understand what this sentence means:

    No transport protocol is indicated if a mapping applies for
    any protocol.

  Perhaps you wanted to say this:

    If this leaf is not instantiated, then the mapping applies to any
    protocol.

- container internal-src-port:
- container external-src-port:
- container internal-dst-port:
- container external-dst-port:

         It is used also to carry the internal
         source ICMP identifier.";

  I think details are lacking here. What is the ICMP identifier? What
  does 'carry' mean and how does this relate to the port-number
  grouping?

  See above, I do not understand the ICMP identifier statement.

- lifetime:

  Spell out 3WHS. I think there should be a units statement. And is
  this a ticking lifetime, i.e., this changes on every get request? Is
  this useful? Have alternatives been considered such as reporting the
  point in time when the mapping was established? Or is the idea that
  the lifetime reports the time left until the mapping will be garbage
  collected? What does "tracks the connection" really mean here?

- I suggest to remove empty lines in say leaf definitions. Instead

      leaf id {
        type uint32;

        description
          "NAT instance identifier.";

        reference
          "RFC 7659.";
      }

  write

      leaf id {
        type uint32;
        description
          "NAT instance identifier.";
        reference
          "RFC 7659: Definitions of Managed Objects for Network
                     Address Translators (NATs)";
      }

- It seems you try to be aligned with the NATV2-MIB module but there
  is no explicit discussion about this in the document. I suggest that
  you a section (for example before the tree diagram) where you
  discuss how this YANG module coexists with the NATV2-MIB module.

  For example, I see that Natv2InstanceIndex in the MIB module
  excludes 0 as an instance identifier while your id leaf above allows
  the usage of 0.

- container nat-capabilities:

  Here we find a bunch of "config true" leafs and I wonder how these
  work. There are things a NAT implementation is capable to do, and
  there are things that are enabled in a NAT deployment and of course
  you can't enable something in a deployment that has not been
  implemented. So are these 'capabilities' more like NAT features
  enabled (since we have "config true" nodes) or are these more
  implemented capabilities (but then "config true" may be wrong)?
  Depending on the answer, did you consider using YANG features?

- nat-flavor and nat44-flavor:

  Does it make sense to have two objects here? Can I put basic-nat
  into nat-flavor? Are the differences between lets say napt and
  basic-nat really nat44 specific?

  Note that you also derive restricted-nat from nat44 but this option
  is not mentioned in the description of nat44-flavor. I think the
  description should be more open since in principle I can derive even
  more identities in the future.

- boolean capability flags

  Do these need references so one can lookup what the exact meaning of
  these 'capabilities' are? It would surely help me and it will help
  implementors that need to decide whether a certain vendor feature
  fits any of these.

- nat-pass-through-pref

  Perhaps spell out nat-pass-through-prefix (pref might also be
  understood as preference). And perhaps change the wording in the
  description

  OLD
	    "The IP address subnets that match
             should not be translated. According to

  NEW
	    "IP addresses matching this prefix are
             not be translated. According to

- Sometimes you repeat prefixes in leaf definitions (e.g.,
  nat-pass-through-port) while at other times you do not.

- nat-pass-through-port

  You seem to have copy pasted the description of
  nat-pass-through-pref.  Is this a single port? So I need multiple
  nat-pass-through entries for multiple ports. Fine. But is there a
  special meaning if both nat-pass-through-pref and
  nat-pass-through-port are configured in a single list entry? Does
  this mean the port is scoped to the prefix?

- Some nat type specific parameter lists are flat, for others there is
  an additional container. Is this by design?

- I meanwhile think there really should be features. Certain lists
  only make sense for implementations that support certain NAT
  types. Having features defined allows code generators to easily
  generate stubs that actually match the capabilities of an
  implementation. And you automatically benefit from feature
  announcements.

- s/attachedto/attached to/

- s/prefixs./prefixes./

- nat64-prefix

  What is the purpose of //default "64:ff9b::/96"; ??

- stateless-enable

  Does this enable leaf make sense on a list entry? Perhaps it does
  but the description is fairly general and hence I am asking.

- external-ip-address-pool/pool-id

  This seems to relate to a Natv2PoolIndex, which again excludes the
  value 0. I have not checked all such related things, so please go
  and check yourself.

- external-ip-address-pool

  The description says

            Both contiguous and non-contiguous pools
            can be configured for NAT purposes.";

  but it seems more accurate to say that a pool is a set of prefixes
  since this is what the model suggests.

- supported-transport-protocols

  I am again not clear whether you are reporting implementation
  capabilities here or enabled features or something else. Is the
  configuration of transport-protocol-id and transport-protocol-name
  mutually exclusive? If so, should this be a choice? If I can
  configure both, I assume they have to be consistent.

- transport-protocol-name

  Is this restricted to the acronyms that are used in the IANA
  protocol numbers registry?

- s/masck/mask/

- subscriber-mask-v6

  The name seems to indicate that this only applies to IPv6 prefixes
  handed out to CPEs. Perhaps this should be stated explicitely (but
  yes it is unlike to ever get an IPv4 prefix). But then, the
  subscriber-match has an IPv4 prefix example.

- quota-type

  Is this a good name for the leaf? This seems to indicate for which
  transport protocol a port-limit is enforced. And this list is
  restricted to TCP, UDP, and ICMP while other parts of the model are
  more flexible in supporting additional transport protocols. So is it
  consistent to a rather restricted enum here?

- port-set-timeout

  Needs a units statement.

- timeouts

  Can I make the timeouts arbitrarily small, in the extreme case 0
  seconds? In some cases I find text like "for at least 6 seconds".
  Does this mean that the range is really uint32 { range "6..max"; }?
  Or is it still OK to configure the value 3 and the 'must' is really
  a 'should'?

- port-timeout

  I doubt this is of type inet:port-number and there likely needs to
  be a units statement.

- alg-name

  Is there a list of well-known ALG names? IANA? Or is this a random
  string that one needs to guess form the vendor's documentation,
  i.e., this is potentially not interoperable out of the box? Is there
  a way to obtain the number of ALGs supported or is the idea to do
  trial and error probing to find out (which is even harder if there
  are no well-known ALG names)?

- all-algs-enable

  This says "Enable/disable all ALGs." but I _assume_ that I set this
  to false and to enable specific ALGs. Perhaps this interaction needs
  to be spelled out.

- Is there any throttling mechanism needed in case my NAT is
  constantly operating around the notification thresholds?

- Add units to the mapping limit definitions ("subscribers",
  "mappings", ...)

- limit-per-subnet

  This is type inet:ip-prefix? I guess you wanted something else here.

- Why are some limits mandatory, others not?

- If you write 'limit per subscriber', how is a subscriber identified?
  I assume these are global per subscriber limits, i.e., all
  subscribers receive the same limit.

- limit-per-subnet

          leaf limit-per-subnet {
            type inet:ip-prefix;

            description
              "Rate-limit the number of new mappings
	       and sessions per subnet.";
          }

  I do not see how this works. The other limit-per-XXX objects are
  numbers - presumably defining the limit. This is a prefix?

- logging-info

  Is this information complete? Perhaps it is for plain syslog
  (without any security) but I doubt the info is complete for the
  other transports mentioned, at least not for FTP. And surely, if you
  want to protect the logging information, then you will neeed way
  more parameters. And what does 'retrieving' logging entries mean?  I
  assume with syslog and ipfix you push log messages. I am less sure
  about FTP. Perhaps less is more and simply provide support for
  syslog and leave the other options for extensions. You have a choice
  in place, so not need to go and deal with all possible complexity
  here.

- For the statistics, we used to use plural form for counters back in
  SNMP land and I think this was good practice. So go with
  sent-packets, sent-bytes, rcvd-packets, rcvd-bytes, dropped-packets,
  dropped-bytes, ...

- total-mappings, total-tcp-mappings, total-udp-mappings,
  total-icmp-mappings: These may use yang:gauge32.

- address-allocated, address-free -> addresses-allocated, addresses-free
  (may also be yang:gauge32)

- Some of these gauges might fluctuate fast (maybe consider
  exponentially smoothed gauges, but this might also be left for an
  extension).

- The security considerations text should follow the new boilerplate
  that also covers RESTCONF. I think it would help to be more specific
  about the security aspects of this data model. This text is quite
  generic. With a NAT, things even have privacy aspects. If I can
  force a specific NAT mapping, I can track a subscriber.

- At least one example has syntax errors. Examples should ideally be
  validated by tools so that the examples are syntactically correct and
  valid regarding the data model.

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sun, 29 Oct 2017 09:36:36 +0100

One additional nit: You may want to think about the name of the toplevel
node. Here is what is currently published:

    +--rw interfaces            ietf-interfaces         RFC 7223
    +--rw ipfix                 ietf-ipfix-psamp	RFC 6728
    +--rw key-chains            ietf-key-chain		RFC 8177
    +--rw lmap                  ietf-lmap-control       RFC 8194
    +--ro modules-state         ietf-yang-library	RFC 7895
    +--rw nacm                  ietf-netconf-acm        RFC 6536
    +--ro netconf-state         ietf-netconf-monitoring RFC 6022
    +--rw routing               ietf-routing            RFC 8022
    +--rw snmp                  ietf-snmp               RFC 7407
    +--rw system                ietf-system		RFC 7317
    +--ro system-state          ietf-system     	RFC 7317

    +--ro interfaces-state      ietf-interfaces         RFC 7223 (to be deprecated)
    +--ro routing-state         ietf-routing            RFC 8022 (to be obsoleted)

Calling the toplevel node simply 'nat' may be more inline with the
existing names (which are not all perfect but if we ignore the -state
nodes there is a certain level of consistency - a not toplevel node
uses 'module').

/js

On Fri, Oct 27, 2017 at 11:14:15AM -0700, Jürgen Schönwälder wrote:
> _______________________________________________
> yang-doctors mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/yang-doctors

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: <[email-masked]>
Date: Mon, 30 Oct 2017 14:41:19 +0000

Hi Jürgen,

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Jürgen Schönwälder [mailto:[email-masked]]
> Envoyé : vendredi 27 octobre 2017 20:14
> À : [email-masked]
> Cc : [email-masked]; [email-masked]
> Objet : Yangdoctors early review of draft-ietf-opsawg-nat-yang-06
> 
> problematic.

[Med] OK.

 That said, there are a number of details where I doubt
> see usage of YANG features.

[Med] We used to make use of "features", but there was a comment during the WG call for adoption that requested us to make use of identities, instead.

> things I have spotted, so I guess the usual problem of getting enough

[Med] FYI, the module was reviewed by experts of each of translation flavors covered in the document. All the reviews were shared on the list. 

>                  (NAT) and Network Prefix Translation (NPT)";

[Med] Fixed. Thanks.

>   that has more context and just a list of names and email addresses.

[Med] Fixed.

>   since not everybody remembers all the numbers equally well.

[Med] Fixed. 

> - Is there a reference for dst-nat?

[Med] I don't have an RFC to cite here. 

> 
> - What is the vrf-routing-instance identity good for?

[Med] This is used to bind a NAT function to an external VRF routing instance. Please check https://www.ietf.org/mail-archive/web/opsawg/current/msg05117.html 

 Its used only in
> - s/start-port-numbert/start-port-number/

[Med] Fixed.

>   covered in description statements.

[Med] Cleaned all these comments. 

> - What is a PSID algorithm? Spell out acronyms on first usage.

[Med] PSID stands for Port Set Identifier. This is an algorithm to assign non-overlapping port sets to users serviced with the same shared IPv4 address. References are added to the module.

>   not needed anymore, they should be removed.

[Med] Removed. 

>   more details (missing reference statement?)?

[Med] I updated the description and added pointers to the authoritative spec. 

>   via configuration? Or even both? Well, I guess it is both given the

[Med] Both can be supported. 

>   'explicitly configured' (configuration does not have to be a manual

[Med] Manually is derived from RFC6887, which says: 

      *  Explicit static mappings are created by manual configuration
         (e.g., via command-line interface or other user interface) and
         persist until the user changes that manual configuration.

>   Note the 'implicit' vs 'explicit' clash here. 

[Med] Good catch. Thanks.

Perhaps this should be
>       associated with this mapping.

[Med] Works for me.

>              request, e.g., a PCP message.";

[Med] Deal!

>     If this leaf is not instantiated, then the mapping applies to any

[Med] You got it. I used your proposed wording. 

>   See above, I do not understand the ICMP identifier statement.

[Med] 'ICMP Identifier' usage is the same as in RFC6146. 

I added this NEW text:

         As a reminder, all the ICMP Query messages contain 
         an 'Identifier' field, which is referred to in this 
         document as the 'ICMP Identifier'.";

>   Spell out 3WHS.
[Med] Replaced with three-way handshake. 

 I think there should be a units statement.
[Med] Agree. Fixed.

 And is
>   collected? What does "tracks the connection" really mean here?

[Med] The initial value indicates the duration to maintain a mapping. When retrieved in a get operation, it indicates the remaining validity lifetime.  
I updated the text to clarify this.

>                      Address Translators (NATs)";
[Med] Fixed.

>   discuss how this YANG module coexists with the NATV2-MIB module.

[Med] I will Senthil (one of the co-author of the NATV2-MIB module) comment on this.  

>   implemented capabilities (but then "config true" may be wrong)?

[Med] These are about what an implementation is able to do. When I set config to "false", pyang is complaining.

>   Depending on the answer, did you consider using YANG features?

[Med] Yes, we considered the YANG features.

>   basic-nat really nat44 specific?

[Med] These are specific to characterize a NAT44. For example:
- NPTv6 is by design acting on prefixes; ports are not touched
- NAT64 can be stateless or statefull
- EAM relies on address rewriting

>   is not mentioned in the description of nat44-flavor. 

[Med] That one is removed, because it is conflicting with the use of "restricted-port-support" that is valid for both state full NAT64 and NAT44. 

I think the
>   implementors that need to decide whether a certain vendor feature

[Med] I added references as appropriate. 

>              not be translated. According to

[Med] Works for me.

>   nat-pass-through-port) while at other times you do not.

[Med] Fixed.

>   nat-pass-through-pref.  Is this a single port?

[Med] Yes, this is a single port that can be supplied with or without a prefix

 So I need multiple
>   nat-pass-through entries for multiple ports.

[Med] Yes.

 Fine. But is there a
>   this mean the port is scoped to the prefix?

[Med] Yes. The text was updated accordingly. 

>   an additional container. Is this by design?

[Med] Yes, the design was built with NAT/NAT64 as the core functionality. Other specific-techniques are added as containers for clarity. 

>   implementation. And you automatically benefit from feature

[Med] As I mentioned earlier, we used to make use of features in early versions of the draft. I’m really neutral on this.  

> - s/attachedto/attached to/
[Med] Fixed.

> - s/prefixs./prefixes./
[Med] Fixed.

>   What is the purpose of //default "64:ff9b::/96"; ??

[Med] This is now removed given that there is no recommendation to use a Well-known Prefix or a Network-Specific Prefix (RFC6052). 

>   but the description is fairly general and hence I am asking.

[Med] Both stateless and statefull NAT64s are implementing the same address translation algorithm (and consequently the same configuration data). stateless-enable is an explicit indication to a NAT64 to indicate whether it should consider source IPv6 addresses as IPv4-translatabke IPv6 addresses (RFC6052). 

>   value 0. I have not checked all such related things, so please go

[Med] I will let Senthil comment on this one. 

>   since this is what the model suggests.

[Med] Yes, that's it. I changed the text as suggested. 

>   capabilities here or enabled features or something else. 

[Med] This one is to configure explicitly a transport protocol that will be handled by the NAT.

Is the
>   mutually exclusive? 

[Med] No. A list of protocols can be enabled.

If so, should this be a choice? If I can
>   protocol numbers registry?

[Med] Yes.  

> - s/masck/mask/
[Med] Fixed. 

>   subscriber-match has an IPv4 prefix example.

[Med] This is IPv6-specific. The text is now updated. 

>   consistent to a rather restricted enum here?

[Med] Good point. Updated accordingly. 

>   Needs a units statement.
[Med] Done.

>   Or is it still OK to configure the value 3 and the 'must' is really

[Med] Only recommended values are provided. An administrator is free to set the values it sees appropriate for his/her deployment.  

>   I doubt this is of type inet:port-number and there likely needs to

[Med] Good catch. Fixed.

>   are no well-known ALG names)?

[Med] There is no "standardized/IANA" list of ALGs. There is a list of widely deployed/implemented ones.
FWIW, we used to have this list: 

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

We changed the design to this one because it is easily extensible. 

>   to false and to enable specific ALGs. Perhaps this interaction needs

[Med] Updated.

>   constantly operating around the notification thresholds?

[Med] This is deployment- and implementation-specific. We don't have a standard document to cie. 

> - Add units to the mapping limit definitions ("subscribers",
[Med] Will be checked. 

>   This is type inet:ip-prefix? I guess you wanted something else here.

[Med] I confirm. A subscriber is assumed to be identified by a prefix. 

> - Why are some limits mandatory, others not?
[Med] Because those that are required for most of existing NAT implementations. 

>   subscribers receive the same limit.

[Med] A subscriber can be identified by a subscriber-mask or an explicit prefix. 

>   numbers - presumably defining the limit. This is a prefix?

[Med] Yes. 

>   in place, so not need to go and deal with all possible complexity

[Med] It is out of scope of specify the exact logging information. Those considerations are out of scope. We only focus on the protocol and the server information. 

>   sent-packets, sent-bytes, rcvd-packets, rcvd-bytes, dropped-packets,

[Med] Actually, we used both of them. I updated when required. 

>   total-icmp-mappings: These may use yang:gauge32.
[Med] OK.

>   (may also be yang:gauge32)
[Med] OK.

>   exponentially smoothed gauges, but this might also be left for an

[Med] We can leave those for a future extension. 

>   that also covers RESTCONF.

[Med] Updated.

 I think it would help to be more specific
>   generic.

[Med] The text Acks that all information are sensitive. They must be protected and secure means must be used. 

 With a NAT, things even have privacy aspects. If I can
>   force a specific NAT mapping, I can track a subscriber.

[Med] IMHO, this is not specific to the NAT. Mandating secure means to control the NAT is a guard against such attack.  

>   valid regarding the data model.
[Med] Will double check.

---

From: <[email-masked]>
Date: Mon, 30 Oct 2017 14:42:36 +0000

Re-,

OK, thanks.

Cheers,
Med

> De : Juergen Schoenwaelder [mailto:[email-masked]]
> Envoyé : dimanche 29 octobre 2017 09:37
> Objet : Re: [yang-doctors] Yangdoctors early review of draft-ietf-opsawg-
> nat-yang-06
>

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 30 Oct 2017 17:22:10 +0100

> [Med] We used to make use of "features", but there was a comment during the WG call for adoption that requested us to make use of identities, instead.

But these are not the same thing. If I am not a nat64, then apparently
several objects to not apply to me. This is what features allow you to
express. Perhaps someone wanted identities for something different, I
do not know. I think it is reasonable to assume that not all NAT
implementations will support all types of NATs that the model supports
and hence the model should reflect this.

> [Med] This is used to bind a NAT function to an external VRF routing instance. Please check https://www.ietf.org/mail-archive/web/opsawg/current/msg05117.html 

I very much doubt that an identity identifies a VRF instance. I am not
questioning the need to identify a VRF, I am questioning that the
solution put in place achieves this.

> >   checked by the routing area experts to make sure things fit with

And this was my explanation why I believe this identity thing does not
work. You need to make work this out with the people modeling VRFs.
 
>          persist until the user changes that manual configuration.

Note the word 'explicit' there. I assume this boils down in NMDA terms
to configuration ending up in <intended> and I prefer explicit
configuration since configuration ending up in <intended> can very
well come from automated systems, i.e., there is not necessarily a
manual process.

> I updated the text to clarify this.

This means this is constantly changing. If you would return the
timestamp when the lifetime ends the value would change rarely. The WG
needs to think about the choice. Note that constantly changing values
are potentially a burden with telemetry interfaces and caches but this
may not apply here. The WG should discuss what the right choice is
here.

> [Med] These are about what an implementation is able to do. When I set config to "false", pyang is complaining.

If leafs are not configurable, then they should be config false.
 
> [Med] Yes, we considered the YANG features.

I do believe you want features. Now, if an implementation supports
multiple different NATs, how do I tell the NAT which type of NAT
function applies to a given packet? Is this always clear?

> [Med] Yes, the design was built with NAT/NAT64 as the core functionality. Other specific-techniques are added as containers for clarity. 

Hm. Perhaps a bit subtle without a description.
 
> >   protocol numbers registry?

So please state this.
 
> We changed the design to this one because it is easily extensible.

Well, as long as it is clear what an ALG name is...
 
> [Med] It is out of scope of specify the exact logging information. Those considerations are out of scope. We only focus on the protocol and the server information.

I think my comment was saying that the information is not sufficient to
talk to a server.

> [Med] The text Acks that all information are sensitive. They must be protected and secure means must be used.

A generic statement - in the past you there usually was a problem
getting such generic statements through the approval process since
security people want authors to think through the security aspects of
individual objects or groups of related objects. But you can try of
course, perhaps the mindset has changed - and I am not a secdir
reviewer. ;-)

> [Med] IMHO, this is not specific to the NAT. Mandating secure means to control the NAT is a guard against such attack.  

I think this is actually more subtle than one might think but this is
the job of the security directorate people to think about.

/js

---

From: <[email-masked]>
Date: Tue, 31 Oct 2017 09:55:46 +0000

Hi Jüergen,

Please see inline.

Cheers,
Med

> Envoyé : lundi 30 octobre 2017 17:22
> À : BOUCADAIR Mohamed IMT/OLN
> Cc : [email-masked]; [email-masked];
> [email-masked]
> Objet : Re: Yangdoctors early review of draft-ietf-opsawg-nat-yang-06
> 
> > [Med] We used to make use of "features", but there was a comment during
> the WG call for adoption that requested us to make use of identities,
> instead.
> 
> But these are not the same thing.

[Med] You are right. Sorry for not being clear. 

What I meant is that the module used, for example, "if-feature nat64" and so on to refer to NAT flavors. We replaced those with "when" statements that points to the actual capabilities of an implementation. 
Another change we made, is that we used to have "Boolean" to indicate the support of a given tarnation scheme (e.g., nat64-support, nat44-support), but we updated those to make use of identities. 

 If I am not a nat64, then apparently
> several objects to not apply to me.

[Med] Yes. We tried to cover this by "when" statements. 

 This is what features allow you to
> and hence the model should reflect this.

[Med] The module indicates what a given implementation has to support based on its capabilities. This is handled by means of "when" statements.   

> > [Med] This is used to bind a NAT function to an external VRF routing
> instance. Please check https://www.ietf.org/mail-
> archive/web/opsawg/current/msg05117.html
> >
> 

[Med] Thank you for clarifying. This is actually a good point. The options we considered are as follows: 

(1) Use an Identity to avoid struggling with semantics of how a VRF can be defined.
(2) Point to draft-ietf-rtgwg-ni-model, likely (?) as a normative reference. 
(3) Remove the VRF thing from the draft; future extensions can be defined.

Given that "VRFs" are not required for all NATs and some reviewers asked to add VRFs in addition to interfaces, we went for option (1). Having a dependency on a YANG module under development is not justified for all implementations. 

> well come from automated systems, i.e., there is not necessarily a

[Med] I hear you. FYI, there is no "manual" mention in the new revision of the draft. 

> > [Med] The initial value indicates the duration to maintain a mapping.
> When retrieved in a get operation, it indicates the remaining validity
> lifetime.
> timestamp when the lifetime ends the value would change rarely.

[Med] That's another option which has implications on the NAT implementation itself. We modeled the YANG module to be aligned with current NAT implementations.  

 The WG
> If leafs are not configurable, then they should be config false.

[Med] I will revert config to false as we used to have in previous versions. As I said earlier, pyang will cry. I don't know how to fix that. 

> function applies to a given packet? Is this always clear?

[Med] The current module allows for implementations that support many translation flavors. In particular, it allows for a same NAT instance to enable many features in the same time.

There is no need to indicate explicitly the translation scheme(s) to enable because this will be implicitly deduced from the configuration parameters. For example: 
- an implementation supplied with an external IPv4 address pool only, will automatically behave in the base NAT mode.
- an implementation supplied with an external IPv4 address pool and port-related parameters will behave in the NAPT mode.
- an implementation supplied with an external IPv4 address pool, port-related parameters, and NAT64 prefixes will behave in the statefull NAT64 mode
- an implementation supplied with an external IPv4 address pool, port-related parameters, dst-nat-enable=true, and dst-ip-address-pool will behave in the NAPT mode + destination NAT

and so on. 

> > [Med] Yes, the design was built with NAT/NAT64 as the core
> functionality. Other specific-techniques are added as containers for
> clarity.
> >
> 

[Med] Fixed in my local copy. 

> Well, as long as it is clear what an ALG name is...

[Med] Actually, what is more important is the protocol/port. 

BTW, I updated the module to indicate src and/or dst ports. 

> > [Med] It is out of scope of specify the exact logging information. Those
> considerations are out of scope. We only focus on the protocol and the
> server information.
> 

[Med] It is only about the minimal set of information. The information is not complete on purpose. Protocol-specific information is out of scope of this document. 

> > [Med] IMHO, this is not specific to the NAT. Mandating secure means to
> control the NAT is a guard against such attack.
> 

[Med] You are probably right, but I prefer to leave those for the secdir reviews.