[Softwires] FW: early MIB Doctor review for draft-ietf-softwire-map-mib-07

["Yu Fu" <[e-mail]>]

Just forward the discussion to the softwire WG.

-----Original Message-----
From: Bert Wijnen (IETF) [mailto:[e-mail]] 
Sent: Friday, May 12, 2017 11:05 PM
To: Yu Fu
Cc: [e-mail]; 'MIB Doctors'
Subject: Re: early MIB Doctor review for draft-ietf-softwire-map-mib-07

inline

On 12/05/2017 06:39, Yu Fu wrote:
> Hi Bert,
>
> Thanks a lot for your thorough MIB doctor review.
> Because I have a fever in the early of this week, sorry to reply you a little late.
>

no problem

> Please see my reply inline.
>
>>> -----Original Message-----
>>> From: [e-mail] [mailto:[e-mail]]
>>> Sent: Monday, May 08, 2017 10:56 PM
>>> To: [e-mail]; MIB Doctors
>>> Subject: early MIB Doctor review for draft-ietf-softwire-map-mib-07
>>>
>>> [not sure if [e-mail] also includes 
>>> the softwire WG mailing list. Anyways, I am not subscribed to that 
>>> WG list, so I probably can't post there. Pls WG chairs forward if needed/wanted].
>>>
>>> I did an early MIB doctor review for this document.
>>>
>>> The syntax check with SMICNG is clean. Great.
>>>
>>> My comments/questions:
>>>
>>> - section 4 states:
>>>     The MAP-E MIB provides a way to configure and monitor the MAP 
>>> devices
>>>     in MAP encapsulation mode through SNMP.
>>>   Yet there are no read-write or read-create objects defined in the MIB
>>>   module. So I don't think you can "configure" anything on the MAP 
>>> devices
>>>   via SNMP with this MIB module
>>>
>>> - section 4.1.1 states:
>>>     The mapRule subtree describes managed objects used for managing 
>>> the
>>>     multiple mapping rules in the MAP encapsulation mode.
>>>   So, since the MIB module is read-only, I think I would change
>>>   s/managing/monitoring/. But maybe that is just a wording choice.
>
> [fuyu] : We will change the "configure and monitor" to "manage and monitor". Thanks.
>
ok
>>> - section 4.1.2
>>>       - The BR MUST perform a validation of the consistency of the 
>>> source
>>>         IPv6 address and source port number for the packet using BMR.
>>>
>>>       - The Customer Edge (CE) SHOULD check that MAP received 
>>> packets'
>>>         transport-layer destination port number is in the range 
>>> configured by
>>>         MAP for the CE.
>>>    Mmmm... what is BR ? Please expand the acronym the first time you 
>>> use it in your document. Same for BMR.
>
> [fuyu] : I will add the explanation for acronym in the updated version.
>
great
>>>    I am not sure that the MIB document is the place to tell 
>>> (normatively
>>> (MUST/SHOULD)
>>>    a BR that it MUST perform a certain protocol function. That is (I
>>> assume) specified
>>>    in the RFC7597 itself, right?
>>>    Maybe you mean to say that the BR (and CE) MUST and SHOULD report 
>>> any invalid
>>>    packets as found per the rules in RFC7579. They MUST/SHOULD 
>>> report them via this
>>>    MIB module (as per your MODULE Conformance).
>>>    Oh well, again this may be a wording issue.
>>>    If my interpretation here is correct, I can live with your 
>>> wording although I think
>>>    it can be clearer and that the proper compliance language is in 
>>> the proper document.
>
> [fuyu] Yes, it is a good suggestion. The protocol function requirement is specified in RFC7597.
>      So we will change the description and delete MUST/SHOULD language.
>>>
ok
>>> - section 5.
>>>   I think I would change:
>>>
>>>      5.  Definitions
>>>
>>>       MAP-E-MIB DEFINITIONS  ::=  BEGIN
>>>
>>>
>>>       IMPORTS
>>>          MODULE-IDENTITY, OBJECT-TYPE, mib-2,
>>>          Integer32, Unsigned32, Counter64
>>>             FROM SNMPv2-SMI
>>>          ifIndex
>>>             FROM IF-MIB
>>>          InetAddressType, InetAddress,
>>>          InetAddressPrefixLength
>>>             FROM INET-ADDRESS-MIB
>>>          OBJECT-GROUP, MODULE-COMPLIANCE
>>>             FROM SNMPv2-CONF;
>>>
>>>   Into
>>>
>>>      5.  Definitions
>>>
>>>      The following MIB module imports definitions from [RFC2578], 
>>> [RFC2580],
>>>      [RFC2863], and [RFC4001].
>>>
>>>       MAP-E-MIB DEFINITIONS  ::=  BEGIN
>>>
>>>
>>>       IMPORTS
>>>          MODULE-IDENTITY, OBJECT-TYPE, mib-2,
>>>          Integer32, Unsigned32, Counter64
>>>             FROM SNMPv2-SMI                   -- RFC2578
>>>          ifIndex
>>>             FROM IF-MIB                       -- RFC2863
>>>          InetAddressType, InetAddress,
>>>          InetAddressPrefixLength
>>>             FROM INET-ADDRESS-MIB             -- RFC4001
>>>          OBJECT-GROUP, MODULE-COMPLIANCE
>>>             FROM SNMPv2-CONF;                 -- RFC2580
>>>
>>>      That way, you have a reference to each document (they are all
>>> normative)
>>>      and in the MIB module itself (where you cannot put
>>> references/citations) you
>>>      would still have document which RFC the imports are from. Handy 
>>> when a MIB module
>>>      has been extracted from an RFC.
>
> [fuyu]: Yes, it is more clear. Thanks, I will change it in the updated version.
>
ok
>>> In the MIB module itself:
>>>
>>>   mapRuleID OBJECT-TYPE
>>>           SYNTAX Integer32 (1..2147483647)
>>>           MAX-ACCESS not-accessible
>>>           STATUS current
>>>           DESCRIPTION
>>>              "An identifier used to distinguish the multiple mapping
>>>               rule which is unique with each CE in the same BR."
>>>           ::= { mapRuleEntry 1 }
>>>
>>>   Since this is an index object, it be better defined as unsigned 32.
>>>   See RFC4181, section 4.6.1.1. Specifically page 15, which states
>>>      - Unsigned32 with a range that excludes zero is RECOMMENDED for
>>>        most index objects.  It is acceptable to include zero in the
>>>        range when it is semantically significant or when it is used as
>>>        the index value for a unique row with special properties.  Such
>>>        usage SHOULD be clearly documented in the DESCRIPTION clause.
>
> [fuyu]:  I will change it into unsigned 32.
>
Pls also think about the question if the value can ever be zero for a good reason.
Normally INDEX object do not have a value of zero.

>>>
>>>    mapRuleIPv6PrefixType OBJECT-TYPE
>>>           SYNTAX     InetAddressType
>>>           MAX-ACCESS read-only
>>>           STATUS     current
>>>           DESCRIPTION
>>>               "This object MUST be set to the value of ipv6(2) to
>>>                present the IPv6 address.It describes the
>>>                address type of the mapRuleIPv6Prefix and
>>>                mapRuleBRIPv6Address."
>>>
>>>    Such MUST language is not recommended. The way to specify such 
>>> mandatory
>>>    value you best use the MODULE COMPLIANCE statement. Such is 
>>> RECOMMENDED
>>>    as per RFC4181, page 26
>
> [fuyu]: I will delete the MUST in the updated version.
>
But if you change to InetAddressIPv6 and InetAddressIPv4 (as you state below), then there is no need for an InetAddressType.

>>>
>>>    mapRuleIPv6Prefix OBJECT-TYPE
>>>           SYNTAX     InetAddress(SIZE (0..16))
>>>           MAX-ACCESS read-only
>>>           STATUS     current
>>>           DESCRIPTION
>>>              "The IPv6 prefix defined in mapping rule which will be
>>>               assigned to CE. The address type is given by
>>>               mapRuleIPv6PrefixType."
>>>           ::= { mapRuleEntry 3 }
>>>
>>>    mmmmm, when the InetAddressType is ipv6(2), then my understanding 
>>> of RFC4001
>>>    is that the SIZE for the InetAddress MUST be 16. Maybe Juergen 
>>> can chime in here?
>>>
>>>    And if it is ALWAYS an IPv6 address, then maybe it is even better 
>>> to use
>>>    InetAddressIPv6 as the OBJECT-TYPE.
>>>
>>>    I see the same set of 2 objects for IPv4.
>>>
>>>    In my view, the idea of InetAddressType and InetAddress were/are 
>>> to allow yopu to specify
>>>    one or a single pair that can hold each of the 2 (or even more if
>>> needed) AddressTypes.
>>>
>>>    So why are there separate objects for IPv4 and IPv6 ???
>
> [fuyu]:OK, I will change it into
>
>      mapRuleIPv6Prefix OBJECT-TYPE
>            SYNTAX     InetAddressIPv6
>            MAX-ACCESS read-only
>            STATUS     current
>            DESCRIPTION
>               "The IPv6 prefix defined in mapping rule which will be
>                assigned to CE. The address type is given by
>                mapRuleIPv6PrefixType."
>            ::= { mapRuleEntry 3 }
>
>      mapRuleIPv4Prefix OBJECT-TYPE
>           SYNTAX     InetAddressIPv4
>           MAX-ACCESS read-only
>           STATUS     current
>           DESCRIPTION
>              " The IPv4 prefix defined in mapping rule which will be
>                assigned to CE. The address type is given by
>                mapRuleIPv4PrefixType."
>           ::= { mapRuleEntry 6 }
>
> Do you think it OK?
>
Yes. And you then do not need the InetAddressType object.

>>>
>>>    mapRulePSID  OBJECT-TYPE
>>>           SYNTAX     Integer32
>>>           MAX-ACCESS read-only
>>>           STATUS     current
>>>           DESCRIPTION
>>>              "The PSID value algorithmically identifies a set of
>>>               ports assigned to a CE."
>>>           REFERENCE
>>>                "PSID: section 3 of RFC 7597."
>>>           ::= { mapRuleEntry 9 }
>>>
>>>     Mmmm... section 3 of RFC7597 only defines the term. The 
>>> algorithm is in section 5.1
>>>     Maybe that is a better place to point to.
>>>     Reading that section 5.1 in RFC7597, I wonder if "Integer32" is 
>>> the best representation.
>>>     In section 5.1, I see a PSID that is 6 bits (figure 2 on page 
>>> 10). But there is also
>>>     text about a PSID of 0x00 and 0xFF, which does not fit in 6 bits.
>>>     I am not an expert (basically have no knowledge about) on RFC7597.
>>> But sofar I cannot
>>>     determine what the value range might be and how Integer32 is a 
>>> good representation for
>>>     the PSID. Please explain (not just to me, but adding text to the 
>>> internet drafts
>>>     and the DESCRIPTION clause of this object)
>
> [fuyu]:Different PSID values guarantee non-overlapping port sets.
>      The length of PSID is k bits, and the default value of PSID offset is 6 bits.
>      So the bit length of PSID is variable. Thank you for your question.
>      I have reconsidered the SYNTAS of the PSID. It can never be a negative value.
>      I think Unsigned32 will be better.
>
So do you just map the 4 octets of the PSID into this object?
Is it not better to then use OCTET-STRING with a SIZE(4) ??
Maybe with a DISPLY-HINT added as well

Do you normally display the value as an (unsigned) integer?
Or do you normally display it as 0Xxxxxxxxx ??
Or maybe as bits?

>>>
>>>     mapRulePSIDLen  OBJECT-TYPE
>>>           SYNTAX     Integer32
>>>
>>>     My understanding sofar is that this object can never take a 
>>> negative value,
>>>     so Unsigned32 would be better I think.
>
> [fuyu]: I will change it into Unsigned32 in the updated version.

Does it also make sense to add a range? (what are the possible values?) ??

>>>
>>>     mapRuleType OBJECT-TYPE
>>>           SYNTAX     Integer32
>>>           MAX-ACCESS read-only
>>>           STATUS     current
>>>           DESCRIPTION
>>>              "The type of the mapping rule. A value of 0 means it
>>>
>>>               is a BMR; a non-zero value means it is a FMR."
>>>          REFERENCE
>>>                "BMR, FMR: section 5 of RFC 7597."
>>>           ::= { mapRuleEntry 13 }
>>>
>>>     That probably works. But it is probably much better to do a 
>>> ENUMERATION, Which
>>>     has a SYNTAX of INTEGER and then lists enumberated values. That 
>>> makes it much
>>>     more extensible in the future (if needed).
>
> [fuyu]: Thanks for your suggestion. I will updated it in the next version.
>
ok
>
>>>   mapMIBSecurityGroup OBJECT-GROUP
>>>         OBJECTS {
>>>            mapSecurityCheckInvalidv4,
>>>            mapSecurityCheckInvalidv6 }
>>>        STATUS current
>>>        DESCRIPTION
>>>        " The collection of this objects are used to give the
>>>        information on MAP security checks."
>>>        ::= { mapMIBGroups 2 }
>>>
>>>    These objects are defined with ACCESS accessible-for-notify. I 
>>> think that means
>>>    you have to define this group as a NOTIFICATION-GROUP instead of 
>>> an OBJECT-GROUP.
>
> [fuyu]: mapSecurityCheckInvalidv4 and mapSecurityCheckInvalidv6 indicate the number of the invalid IPv4 or IPv6 packets received by the
>       MAP-E. So I think define this group as OBJECT-GROUP will be better.
>       I will change the ACCESS of these objects from accessible-for-notify into read-only.
>
OK

>>>  - Section 7. Security considerations:
>>>
>>>     These are the objects and their sensitivity/vulnerability:
>>>
>>>       mapRuleIPv6PrefixType
>>>
>>>       mapRuleIPv6Prefix
>>>
>>>       ... etc (quite a list of objects).
>>>
>>>     But nowhere do I see text about "their sensitivity/vulnerability". ??
>>>     Still to be added?
>
> [fuyu]: Some of the readable objects in this MIB module may be considered sensitive or
>    vulnerable in some network environments. These objects are readable,
>    so maybe they are considered sensitive or vulnerable in some use case.
>    We have a description why they are sensitive or vulnerable above the list of objects.
>
Mmmm... can you point me to that text? I do not see that you describe the vulnerability at any place. You do describe what the objects are for, but that does not clearly explain what the security risks are if some unauthorize person would see them.

If you describe that at some other place in the document, then at least I would suggest to add a pointer to that text in the Security COnsiderations Secion.

Bert
>>> Bert
>
> Thanks again for your MIB doctor review.
>
> Cheers
> Yu
>
>
>