Subject: [MIB-DOCTORS] early MIB Doctor review for draft-ietf-softwire-map-mib-07

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Mon, 8 May 2017 16:55:47 +0200

[not sure if [email-masked] 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.

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

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

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.

    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

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

    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)

     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.

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

   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.

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

Bert

---

From: "Yu Fu" <[email-masked]>
Date: Fri, 12 May 2017 12:39:45 +0800

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. 

Please see my reply inline.

>>-----Original Message-----
>>From: [email-masked] [mailto:[email-masked]]
>>Sent: Monday, May 08, 2017 10:56 PM
>>To: [email-masked]; MIB Doctors
>>Subject: early MIB Doctor review for draft-ietf-softwire-map-mib-07
>>
>>[not sure if [email-masked] 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].
>>

[fuyu] : We will change the "configure and monitor" to "manage and monitor". Thanks.

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

>>    it can be clearer and that the proper compliance language is in the

[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.
>>      has been extracted from an RFC.

[fuyu]: Yes, it is more clear. Thanks, I will change it in the updated version.

>>        usage SHOULD be clearly documented in the DESCRIPTION

[fuyu]:  I will change it into unsigned 32.

>>    value you best use the MODULE COMPLIANCE statement. Such is

[fuyu]: I will delete the MUST in the updated version.

>>    So why are there separate objects for IPv4 and IPv6 ???

[fuyu]:OK, I will change it into 

           SYNTAX     InetAddressIPv6

     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?

>>     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 Unsigned32 would be better I think.

[fuyu]: I will change it into Unsigned32 in the updated version.
>>     more extensible in the future (if needed).

[fuyu]: Thanks for your suggestion. I will updated it in the next version.

>>    you have to define this group as a NOTIFICATION-GROUP instead of

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

>>     But nowhere do I see text about "their sensitivity/vulnerability". ??

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

>>Bert

Thanks again for your MIB doctor review.

Cheers 
Yu

---

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Fri, 12 May 2017 17:05:11 +0200

inline

> Because I have a fever in the early of this week, sorry to reply you a little late.

no problem

> [fuyu] : We will change the "configure and monitor" to "manage and monitor". Thanks.
ok
> [fuyu] : I will add the explanation for acronym in the updated version.
great
>      So we will change the description and delete MUST/SHOULD language.
ok
> [fuyu]: Yes, it is more clear. Thanks, I will change it in the updated version.
ok
> [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.

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

>                assigned to CE. The address type is given by
Yes. And you then do not need the InetAddressType object.

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

> [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?) ??

> [fuyu]: Thanks for your suggestion. I will updated it in the next version.
ok
>       I will change the ACCESS of these objects from accessible-for-notify into read-only.
OK

>    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

---

From: "Yu Fu" <[email-masked]>
Date: Tue, 16 May 2017 15:10:08 +0800

Hi Bert,

I have cc the email to the softwire WG so that the WG could see the MIB Doctor's comments.

>Normally INDEX object do not have a value of zero.

[fuyu] Yes, since it is an index object and the value range is not include zero. I think unsigned 32 is better.

>>    Such MUST language is not recommended. The way to specify
>>such
>> mandatory
>But if you change to InetAddressIPv6 and InetAddressIPv4 (as you state
>below), then there is no need for an InetAddressType.

[fuyu] :Yes, I will delete the object of mapRuleIPv6PrefixType and mapRuleIPv4PrefixType too. Thank you for your remind.

>>    mmmmm, when the InetAddressType is ipv6(2), then my
>>understanding
>> of RFC4001
>>    In my view, the idea of InetAddressType and InetAddress
>>were/are
>> to allow yopu to specify

[fuyu] : Yes, I will delete the objects of mapRuleIPv6PrefixType and mapRuleIPv4PrefixType too. Thank you for your remind.

>>     I am not an expert (basically have no knowledge about) on
>>RFC7597.
>> But sofar I cannot

[fuyu] We always describe a Basic mapping rule as below:

  A MAP node (CE or BR) can, via the BMR or equivalent FMR,
   determine the IPv4 address and port set as shown below:

   EA bits offset:       40
   IPv4 suffix bits (p)  Length of IPv4 address (32) -
                         IPv4 prefix length (24) = 8
   IPv4 address:         192.0.2.18 (0xc0000212)
   PSID start:           40 + p = 40 + 8 = 48
   PSID length:          o - p = (56 - 40) - 8 = 8
   PSID:                 0x34 (1232)

So I think display the values as (unsigned) integer or display it as 0Xxxxxxxxx both will be OK.
Do you have the surggstions?

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

[fuyu]  The value range of mapRulePSIDLen is greater than or equal to 0 and no more than 16, so do you think define as below
     
           SYNTAX     Unsigned32(0..16)
    
  Will it make sense?

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

[fuyu]: Sorry. I should point it more clearly. It is in the paragraph 2 of the section 7.
     From the first sentence that "Some of the readable objects in this MIB module may be
   considered sensitive or vulnerable..."
>Bert

Thanks a lot

Yu

---

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Tue, 16 May 2017 10:15:33 +0200

inline

> [fuyu] Yes, since it is an index object and the value range is not include zero. I think unsigned 32 is better.
OK, but then the definition would better be:

     SYNTAX Unsigned32 (1..4294967295)
That wat it is machinereadably clear that valu 0 is not valid.

OK
ok

>    PSID length:          o - p = (56 - 40) - 8 = 8

Not sure I understand: 0x34 (1232) ??? 0x34 in my mind is 52, no?

In fact I am not sure I understand much/any of the above.
But that is OK, if people who are familiar with (or must use/implement) this and
if they understand it then fine.

> Do you have the surggstions?
My personal feeling/thinking is that it is probably best displayed as 0Xxxxxxxxx.
That (I would think) makes it easier to see bit positions as opposed to an (unsigned)
integer. But in princople I can live with either. The idea/hope is that if you add
a DISPLAY HINT that everyone displays it in the same way.

>       mapRulePSIDLen  OBJECT-TYPE
I think that makes sense. Now you can see (machine readable) what the range
of valid values is.

>    considered sensitive or vulnerable..."

It states that they " may be considered sensitive of vulnerable".
But it does not state WHY or IN WHAT CIRCUMSTANCES they might be vulnerable.
What bad things are there that an intruder can do (or find out) when he/she
gets read access to these objects?
I.e. does he get to see confidential or private information? Does he learn
about a competitors internal network structure? Or something else?

Bert

---

From: "Yu Fu" <[email-masked]>
Date: Wed, 17 May 2017 15:15:57 +0800

Hi Bert,

>That wat it is machinereadably clear that valu 0 is not valid.

[fuyu] :OK, I will updated it. Thanks.

>Not sure I understand: 0x34 (1232) ??? 0x34 in my mind is 52, no?

 [fuyu] : Ops, sorry .....it's my fault.  1232 is the port number, not the integer. 

>But that is OK, if people who are familiar with (or must use/implement) this
>and if they understand it then fine.
>
>>>>
>That (I would think) makes it easier to see bit positions as opposed to an
>(unsigned) integer. But in princople I can live with either. The idea/hope is
>that if you add a DISPLAY HINT that everyone displays it in the same way.

[fuyu]:Thank you for your suggestions. I will define it as below:

RulePSID ::= TEXTUAL-CONVENTION
    DISPLAY-HINT "0x:"
    STATUS       current
    DESCRIPTION
        "Represents ..."
    SYNTAX       OCTET STRING (SIZE (4))

mapRulePSID  OBJECT-TYPE
    SYNTAX     RulePSID
    MAX-ACCESS read-only
    STATUS     current
    DESCRIPTION
       "The PSID value algorithmically identifies a set of
               ports assigned to a CE."
          REFERENCE
                "PSID: section 5.1 of RFC 7597."
           ::= { mapRuleEntry 9 }

Do you think it will be Ok?

>>>> 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.
>>>>
>What bad things are there that an intruder can do (or find out) when
>he/she gets read access to these objects?

[fuyu] : I will update it in more detail in this paragraph.Thanks.
>Bert

Thanks again
Yu

---

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Wed, 17 May 2017 09:17:36 +0200

thank you. I am happy mow

Bert

---

From: Terry Manderson <[email-masked]>
Date: Wed, 17 May 2017 14:56:01 +0000

Bert, Yu,

Thank you very much for your review and activity on this!

Cheers
Terry

On 17/05/2017, 5:17 PM, "Bert Wijnen (IETF)" <[email-masked]> wrote:

    Bert

---

From: "Yu Fu" <[email-masked]>
Date: Thu, 18 May 2017 09:53:23 +0800

Thanks Terry and Bert,

An updated version will be submitted soon.

Cheers

Yu

>From: [email-masked] [mailto:[email-masked]]
>Sent: Wednesday, May 17, 2017 10:56 PM
>To: Bert Wijnen (IETF); Yu Fu
>Cc: [email-masked]; 'MIB Doctors';
>[email-masked]
>Subject: Re: early MIB Doctor review for draft-ietf-softwire-map-mib-07
>
>Bert, Yu,
>