[MIB-DOCTORS] FW: MIB Doctor review of draft-ietf-behave-nat-mib-11

["ietfdbh" <[e-mail]>]

Hmmm; behave-ads doesn't work. So I'll send direct.
Forgot to include mib doctors list.

David Harrington
[e-mail]
[phone]

> -----Original Message-----
> From: David Harrington [mailto:[e-mail]]
> Sent: Tuesday, June 17, 2014 2:22 AM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]
> Subject: MIB Doctor review of draft-ietf-behave-nat-mib-11
> 
> Hi,
> 
> I have done a MIB Doctor review of draft-ietf-behave-nat-mib-11.
> I have reviewed this document as part of the MIB Doctor directorate's
> ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments
> were written primarily for the benefit of the operational area directors.
> Document editors and WG chairs should treat these comments just like any
> other
> last call comments.
> 
> My impression is that this document is not ready for publication.
> 
> Technical issues:
> The nature of the realm usage in NAT-MIB could have serious consequences
> to
> existing deployments of other MIB modules.
> The use of calculated thresholds, and calculated values to compare to
> thresholds, could be problematic, especially to resource-constrained agent
> implementations.
> 
> Editorial issues:
> A significant cause for concern is the naming, which doesn't follow RFC
4181
> guidelines. This will make the MIB harder for operators to use in the
field.
> Another significant concern is the lack of REFERENCE clauses. This will
make
> the MIB harder for operators to use in the field.
> The description clauses are sometimes rather sparse. This will make the
MIB
> harder for operators to use in the field.
> Some of the descriptions in the module are sparse, while the surrounding
> text contains a better description. But MIB modules are frequently
> distributed without the surrounding RFC text. This will make the MIB
harder
> for operators to use in the field.
> The IMPORTS clauses don't identify the RFC that contains the MIB module
> being imported from. This will make the MIB harder for operators to use in
> the field.
> The redefinition of realm in the middle of a bis could be a problem,
> especially for managers. This will make the MIB harder for operators to
use
> in the field.
> 
> 
> Detailed review:
> 
> 1) We have discussed whether the new objects really should be a new MIB
> module, published under a new MIB module name. Reading through this
> document, I feel that deprecating the whole previous NAT-MIB within the
> same
> document is a mistake. It makes the document harder to deal with. This
> really should be two separate documents and two separate MIBs - with
> different module names.
> 
> The current approach might be perfectly viable for an agent
implementation,
> where the NAT-MIB is likely to be an implementation of EITHER the first
half
> of this document, or the second half of this document.
> But it seems problematic for an NMS implementation that has to support
> BOTH
> implementations based on RFC4008, and implementations based on this
> document.
> I especially think about the fact that NMSs import MIB modules, including
> the Description clauses for the managed objects, and can display the
> description clauses so operators can see what the definition of the object
> is.
> With this document, NMS applications MUST parse all the deprecated
> objects,
> even if, at some future time, there are no devices that implement the
> RFC4008 NAT-MIB.
> If the deprecation was handled in a separate RFC from the new MIB module,
> and an NMS no longer had to support the "old" NAT-MIB, then NMSs in the
> future would only need to import the new document.
> 
> I found it especially irritating to review, because I am used to finding
> Textual Conventions at the beginning of the MIB module, which is usually
at
> the beginning of the document containing the MIB module, but here, I had
> to
> look for the T-Cs in the middle of a long MIB module, because I had to
work
> past the first 54 pages of deprecations to find the T-Cs of the new
module.
> Every user of this MIB document will be similarly inconvenienced, over and
> over.
> 
> 2) This new I-D is redefining a term "realm" that is defined in the
original
> NAT-MIB. (see section 3.3).
> RFC4008 defined realm objects to be of type INTEGER, but the new NAT-MIB
> defines realm objects as SnmpAdminStrings.
> An NMS (and operators) needs to be able to support devices that implement
> the "old" NAT-MIB and devices that implement the "new" NAT-MIB.
> With this new NAT-MIB, an NMS user must be aware when they look at the
> description of a realm-related object, whether that object was defined in
> the RFC4008 NAT-MIB or in the <new RFC> NAT-MIB, in order to understand
> which meaning and datatype of the term "realm" applies.
> But since RFC4008 is updated by being deprecated in the same document, in
> the same-named MIB module, the reader of the new NAT-MIB must
> determine
> whether the object was defined in the first half of this document, i.e.,
the
> first half of the NAT-MIB, i.e., the "old" deprecated NAT-MIB, or in the
> second half of this document,  to understand the meaning of the word
> realm.
> I think this is problematic.
> 
> 3) Section 3.3 explains that implementations of MIB modules that
implicitly
> support realms (e.g.,OSPFv2-MIB), and utilize SNMPv3 contexts (or
> presumably
> communities in community-based SNMP) in an SNMP-standard compliant
> manner,
> will no longer be able to do things in those standard-compliant manners,
> because the NAT-MIB design spans realms.
> 
> This I-D should list the names and references for the "many MIBs" that
> "implicitly support realms in one form or another" that may be broken by
> this NAT-MIB's usage of realm. I suggest an "Operational Considerations"
> section for this information. I think the IETF community needs to better
> understand this claim before approving this document.
> 
> 4) I will assume all the RFC4008 objects that are being deprecated have
been
> included here, and each has had its status changed to deprecated. I have
not
> gone through the document looking at each and every deprecation.
> 
> 5) ProtocolNumber is a rather generic name for this textual convention. It
> doesn't start with "nat". I fear there is a potential for conflict.
> This does not follow the conventions recommended in RFC4181 Appendix C:
>    - Names of TCs that are specific to the MIB module and names of
>      SEQUENCE types that are used in conceptual table definitions should
>      start with a prefix Xxx that is the same as xxx but with the
>      initial letter changed to uppercase.  OPT-IF-MIB uses the prefix
>      OptIf on the names of TCs and SEQUENCE types.
> 
> 6) ProtocolNumber is dependent on an IANA registry.
> There should be a REFERENCE clause with a link to the specific IANA
> registry, so implementers/users can see the list of valid values.
> 
> 7) This document does not provide REFERENCE clauses; it really should.
> 
> 8) natPoolID - the description does not describe what a pool is, or
provide
> a REFERENCE clause.
> 
> 9) natBehaviorType has only a reference in the description. In general it
is
> better to actually have a description in the description clause and
provide
> the reference in the REFERENCE clause. This is not merely an academic nit.
> Because GUI-based NMSs frequently display the Description clause in a
> window
> so the user can understand the meaning of the object, it is important that
> the description actually explain the meaning. Providing a reference in the
> description implies that the user would then need to go find the RFC and
> find the section of the RFC, and read the whole section to understand what
> this object means. Not very helpful.
> On top of that, an NMS might know how to track down an RFC and provide a
> link to the document in the REFERENCE clause, but it wouldn't necessarily
> know to parse the Description clause to find the reference.
> 
> 10) NatPoolingType is only used to define the type of one object.
> The description clause in the object is indirect - " "Type of address
> pooling behavior that was used to create this mapping."
> To understand this object, you need to look at the textual convention.
> But the textual convention is also indirect - "Pooling type as described
in
> [RFC4787] sections 4.1."
> Even looking at RFC4787 section 4.1 isn't immediately helpful, because the
> discussion of arbitrary versus paired is a subset of section 4.1 (that is
> not called out as a subsection); it is found in the one-paragraph
> explanatory text for REQ-2. A short description in the OBJECT description
> clause could be made from a couple sentences from section 4.1,
> supplemented
> with a REFRERENCE clause, which would be much easier for users than the
> current indirect-indirect-someplace-in-this-text  approach.
> 
> 11) SubscriberIdentifierType concerns me. We are supposed to be
> standardizing things here. But this appears to be a standardized "bucket"
> which implementers can stuff with their own proprietary identifiers,
> especially the "other" value. How is a vendor-neutral NMS supposed to know
> how to handle the "other" value?
> I feel uncomfortable with this whole identifierType, which implies that an
> implementation needs to go parse the packet to pull out additional
> information to identify the subscriber, without standardizing how this
> information should be extracted from the message in a standardized
> vendor-neutral manner.
> 
> I feel uncomfortable that for the "other" type, each implementation
defines
> the semantics of AND and OR combinations. How should a vendor-neutral
> NMS
> handle the implementation-specific variations; where is the AND/OR
> semantic
> decision documented by each implementer, to make this vendor-
> interoperable?
> 
> For the interfaces choice, how is the set of interface indexes
constructed?
> I'm confused by this. The classifying information is in the packet
(identify
> the subscriber from an incoming packet); so does the packet contain a set
of
> interface indexes that the receiving implementation is supposed to
extract?
> There is no reference to the type of packet involved, and I haven't had to
> unpack a set of interface indices from a packet before. What type of
packet
> contains a set of one or more ifIndexes? The RFC2863 textual convention
> doesn't seem to describe this.
> 
> 12) SubsInterfaceIdRowIndex - unique within what scope? Within the agent?
> 
> 13) I am concerned about abbreviations is naming. RFC4181 Appendix C
> suggests
>    - When abbreviations are used, then they should be used consistently.
>      Inconsistent usage such as
> 
>        xxxYyyDestAddr
>        xxxZzzDstAddr
> 
>      should be avoided.
> 
> But sometimes, object names in this module use Identifier and other times
> ID: natSubscriberVpnIdentifier vs. natSubscriberIPEncapsIdType
> Sometimes it's subscriber, other times it's "subs".
> natSubscriberVpnIdentifier vs. natSubsInterfaceIndex
> Sometimes it's threshold, sometimes it's "thresh":
> natMappingsNotifyThreshold  vs. natSubscriberMapNotifyThresh
> For an operator, especially one under stress, using a command-line SNMP
> tools to query a value, it is really irritating to have some objects
> abbreviated one way and other objects abbreviated other ways.
> 
> 14) This is made worse by inconsistent name construction, such as a set of
> nat limits being named nat + "Limit" + <specific>, as in natLimitMappings
> but the set of notify thresholds are constructed as nat + <specific> +
> "NotifyThreshold", as in natMappingsNotifyThreshold. So operators need to
> recall which way you chose to construct the names, on a case-by-case
basis,
> because you are not being consistent. Why not do nat + <specific> +
"Limit"
> to match the way you construct NotifyThreshold names, or use
> nat+"NotifyThreshold"+<specific> to match how you construct the Limit
> names?
> 
> 15) Once again, I will point out consistency in naming, following Appendix
C
> conventions:
> "   - The descriptor associated with a conceptual table should be of the
>      form xxxZzzTable; the descriptor associated with the corresponding
>      conceptual row should be of the form xxxZzzEntry; the name of the
>      associated SEQUENCE type should be of the form XxxZzzEntry; and the
>      descriptors associated with the subordinate columnar objects should
>      be of the form xxxZzzSomeotherName.  An example from the OPT-IF-MIB
>      is the OTMn table.  The descriptor of the table object is
>      optIfOTMnTable, the descriptor of the row object is optIfOTMnEntry,
>      the name of the associated SEQUENCE type is OptIfOTMnEntry, and the
>      descriptors of the columnar objects are optIfOTMnOrder,
>      optIfOTMnReduced, optIfOTMnBitRates, optIfOTMnInterfaceType,
>      optIfOTMnTcmMax, and optIfOTMnOpticalReach."
> 
> But we find in this NAT-MIB module inconsistency:
> natCountersTable OBJECT-TYPE
> ...
>  natCountersEntry OBJECT-TYPE
> ...
> NatCountersEntry ::=
>        SEQUENCE {
>            natTranslations             Counter64,
>            natOutOfPortErrors          Counter64,
>            natResourceErrors           Counter64,
>            natQuotaDrops               Counter64,
>            natMappingCreations         Counter64,
>            natMappingRemovals          Counter64,
>            natAddressMappingCreations  Counter64,
>            natAddressMappingRemovals   Counter64
>        }
> 
> And
> natLimitsTable OBJECT-TYPE
> ...
> natLimitsEntry OBJECT-TYPE
> ...
>    NatLimitsEntry ::=
>        SEQUENCE {
>            natLimitMappings            Unsigned32,
>            natMappingsNotifyThreshold  Unsigned32,
>            natLimitAddressMappings     Unsigned32,
>            natAddrMapNotifyThreshold   Unsigned32,
>            natLimitFragments           Unsigned32,
>            natLimitSubscribers         Unsigned32
>        }
> (notice Limits vs Limit, as well as objects with no natLimit prefix, as
well
> as "AddressMappings" vs "AddrMap".)
> 
> 16) If there is a concern that descriptor length can become problematic,
> "Threshold" is never used without the "Notify" prefix. You could probably
> shorten the NotifyThreshold names to just use Threshold.
> 
> 17) Consistency. Watermark vs Threshold.
> 
> 18) natLimitMappings - this is in a table of limits for a NAT instance.
> What does "Global" mean in this context? See others in this table as well.
> 
> 19) natMappingsNotifyThreshold - the description is "See
> natNotifMappings."
> Which says "This notification is generated when the number of active
>             mappings exceeds the value of natMappingsNotifyThreshold."
> Why not write the (modified) description into both places rather than
using
> indirection?
> 
> 20) natNotifMappings
> Typically we try to not derive values when defining objects. For example,
> RFC4188 states as a principle " Exclude objects that are simply derivable
> from others in this or
>       other MIB modules."
> But natNotifMappings is triggered when " the number of active mappings
> exceeds the value of natMappingsNotifyThreshold"
> But we don't actually track active mappings; we track creations and
> removals, and we calculate active by subtracting removals from creations
> (active=creations-removals).
> If you want to use active against the threshold, why not simply track
active
> and skip tracking creations and removals?
> 
> Is there a reason to track creations and removals?
> If the maximum number of creations performed  is important (maybe for
> memory
> management or something?), then why not track creations and active and let
> somebody else calculate the removals (removals=creations-active)
> If the number of removals is important (I have no idea why), then you can
> track active and removals, and let somebody else calculate
> (creations=active+removals)
> This way a notification doesn't have to wait to calculate active.
> 
> 21) natNotifMappings - the relationship between the notification and
> thresholds is clear.
> The relationship to "which mappings" is a bit less clear.
> I see a natMapIntAddrTable, and a natMappingTable; which one is related to
> natNotifMappings?
> (and is the savings from dropping the y from notify significant?)
> 
> I assume the fact we are sending a notification means it is important for
> the operator to know right away when "active exceeds threshold" happens.
> I have to guess that this might be related to memory/resource management.
> But I have to guess because nothing is said about why these particular
> values are tracked.
> Nothing is said about table row re-use when a mapping is removed.
> Is it really important to know the creation and removal counts, rather
than
> just the active count?
> Is it important to not reuse an index value to make sure that the NMS
> doesn't associate previously acquired data with a new mapping.
> (This is important, for example, if ifIndex changes during a reboot,
because
> an NMS could take data associated with an old ifIndex and confuse it as
> being associated with a new ifIndex, and totally misinterpret trends, and
so
> on. Other MIB modules need to protect against reuse of an index value, at
> least for some period of time, or until rollover.)
> 
> 22) It's a good thing the description of natPoolTable is "Table of pools"
> because I might have confused it as being something else. 8-ball anyone?
> Description please?
> 
> 23) natpoolRealm - "Realm to which this pool's addresses belong."
> MIB modules are often distributed without the surrounding text from the
> RFC.
> 
> So it is important to be sure the description is meaningful without the
> whole RFC being present.
> At a minimum, especially since you are defining realm in this document,
you
> should provide a REFERENCE to the text in this RFC that contains the
> definition for realm.
> 
> 24) natNotifPoolWatermarkLow and WatermarkHigh
> 
> we usually try to not use derived values in MIB modules. We try to keep
the
> agent simple, and push complexity toward the NMS, which normally has far
> more resources available.
> When you set a watermark/threshold as a percentage, then the system
> needs to
> use CPU cycles to calculate the percentage - CPU cycles that might be
better
> used forwarding packets.
> 
> The calculations you expect are rather complex - you require summing
> (allocations - deallocations) over all address ranges, then dividing by
the
> total number of ip addresses in the poll and by the size of the port range
> (calculated by portmax-portmin+1).
> 
> I'm not sure that that formula is unambiguous; is that
> (sum(allocations-deallocation)/ipaddresses)/portrange or am I calculating
> something with (ip addresses and size of portrange) before dividing? Can
you
> use parentheses or something?
> 
> How often should this complex calculation be done to monitor the
> percentage
> versus the threshold? Should this be re-calculated ***every time*** one of
> the following changes: allocation or deallocation across any address
range,
> total number of ip addresses, or size of the port range? (and of course,
the
> watermark setting).
> 
> Wow! That seems like a lot of CPU cycles.
> 
> 25) natpoolIndex - the description doesn't mention the scope of
uniqueness.
> Is this unique across all address pools in all instances? Unique within a
> nat instance, but not necessarily across other nat instances?
> 
> 26) natPool watermarks
> There is mention of low usage mode and high usage mode. Is there a
> REFERENCE
> you can make, maybe to NAT requirements or something, that explains this a
> bit more?
> Is there a reason why BEHAVE is not trying to standardize low/high usage
> behaviors? (isn't standardizing behaviors what behave is all about?)
> 
> Are the "may" terms here RFC2119-compliant? Usually RFC2119 "may" terms
> are
> capitalized.
> 
> 27) It is very helpful to add comments to your IMPORTS identifying the RFC
> that contains the MIB you are importing from.
> 
> 28) natPoolRangeType - can this support any of the address types allowed
by
> this textual convention? Or should the text constrain the acceptable
values?
> I'm not sure which ways to limit this are acceptable. Let me know if all
the
> address types cannot be supported, and I'll check with other mib doctors
to
> determine the best way to constrain it.
> 
> 29) natMappingTable - "Table of mappings indexed by external 3-tuple."
> I'm not sure this is accurate, given
>        INDEX { natInstanceIndex,
>                natMappingProto,
>                natMappingExtRealm,
>                natMappingExtAddressType,
>                natMappingExtAddress,
>                natMappingExtPort }
> 
> 30) natMappingExtAddress - can you point me to the REFERENCE for "the
> undefined address"? I didn't find it in RFC4001.
> Ditto for natMappingIntAddress
> 
> 31) natMappingMapBehavior and natMappingFilterBehavior - for reasons
> mentioned above, please include a description, not just a reference in the
> description text. Put the reference in a REFERENCE clause.
> 
> 32) natMappingSubsIndex - you don't really need the Index suffix. This
> would
> be more meaningful as natMappingSubscriber
> 
> 33) natSubscribersTable, natSubscribersEntry, and natSubscriberIndex (et
al)
> are not consistent. I recommend losing the plural.
> 
> 34) do the existence of fields natSubscriberVlanIdentifier,
> natSubscriberVpnIdentifier, natSubscriberIPEncapsIdType, and
> natSubscriberIPEncapsIdAddr depend on the value of
> natSubscriberIdentifierType? If so, then it might be better to define this
> as a sparse augments table. Let me know, and I'll check with the MIB
Doctors
> list about the best way to do this.
> 
> 35) natSubscriberIdentifierType - " Unused identifier values MUST be zero
or
> equivalent".
> I think this is under-specified.
> I think the zero-equivalents should be specified here for each type.
> When the type is none(0), and the identifier is "none", does this mean the
> identifier field is not implemented, or has a zero value or empty string
or
> what?
> Which of these is the zero equivalent?
> The TC SubscriberIdentifierType says the interfaces(1) type means the
> identifier is a set of **one or more** interface indexes. What is the
> zero-equivalent of a set of one or more interface indexes?
> VPNIdorZero says "           The semantics of the value zero-length OCTET
> STRING are
>             object-specific and must therefore be defined
>             as part of the description of any object that uses this
>             syntax."
> So presumably, the zero equivalent is a zero-length OCTET STRING. But the
> description here does not define the semantics of the zero-length string;
it
> only implies the semantics.
> And for other(5), we have no idea what a zero-equivalent is.
> 
> 36) natSubscriberIntPrefixType is a "Subscriber's internal prefix type."
> While the corresponding address is a "Prefix assigned to a subscriber's
> CPE."
> I tend to think of a CPE as being the border between a subscriber's
network
> and an ISP's network.
> If the subscriber entry represents a host served by a managed enterprise
> NAT, is this still the prefix type for the CPE?
> Please clarify if the type is the internal prefix type for the
subscriber's
> CPE, or simply the subscriber's internal prefix type.
> 
> 37) natSubscriberLimitMappings - again, we should avoid calculated
objects.
> This depends on "active" mappings, which is a calculated value.
> 
> 38) natSubscriberMapNotifyThresh - consistency in naming please; this
> seems
> to be a real problem throughout the subscriber table.
> (natSubscribers vs natSubscriber, thresh, mappngs vs Map)
> 
> 39) natSubscriberIPEncapsIdType -  I find the "when not unknown(0)" to be
> an
> odd description. The InetAddressType description is " A value that
> represents a type of Internet address." I think that should suffice here
as
> well, even though unknown(0) has special semantics described in the T-C.
> 
> 40) natSubsInterfaceIdentifierTable - the description contains a sentence
> fragment; I don't know what it means - " 'OR' semantics if multiple
> interface indexes
>             are present." I find the other sentences a bit short; I
> recommend rewriting the description to make it clearer.
> 
> 41) natSubsInterfaceIdSubsIndex - I have some concerns about the use of
> Index as parts of object descriptors and part of object descriptions, for
> objects that are not INDEXes, when INDEX is a SMI reserved word (RFC2578,
> section  3.7). I don't find the "Index" suffix necessary. I think
> SubscriberIndex would be fine as Subscriber, and SubsInterfaceIdRowIndex
> would be fine as SubsInterfaceIdRow, and natSubsInterfaceIdRowIndex
> would be
> fine as natSubsInterfaceIdRow, etc.
> 
> 42) natSubsInterfaceIdRowIndex - the description is "Row index."  There is
> no description of why this is being defined, or what it is expected to be
> used for.
> The T-C has a better explanation of its uniqueness and its assignment to
> rows, but no description of why it is needed.
> It might have been better explained in the natSubsInterfaceIdentifierEntry
> description, where it is used in the INDEX. But I don't understand the
> description there - "Each entry provides a single interface index."
> Can you provide an example of how this table works?
> 
> 43) I am a bit concerned about the compliance options. If we are
> standardizing behaviors, why do we have five compliance levels? Lots of
> compliance levels = lack of standardization.
> 
> --- Security considerations ---
> 44) What objects reveal host identities? Do  you mean host addresses? I
> think the community has made a statement about identity versus address in
> efforts like HIP and SNMPv3.
> 45) If a disgruntled former employee can break into private hosts, can
> attackers who never worked for the employer do so as well? I don't have
any
> employees, and no former employees; does that mean this is an attack
> vector
> I can totally ignore?
> 
> 
> Hope this helps,
> 
> David Harrington
> [e-mail]
> [phone]