Subject: MIB/PIB doctor review for draft-ietf-ipsp-ipsecpib-09.txt - part 2

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Tue, 16 Mar 2004 15:49:18 +0100

Steve, you may want to pay specific attention to items 34-37.

Addtional things I found when revieing revision 9

Comments 1-8 were in part 1 email of the review

9. Doing s smilint syntax check, I get the following:

      C:\smi\pibs\ietf>smilint -l 6 -s -m -inamelength-32 ./IPSEC-POLICY-PIB
         /IPSEC-POLICY-PIB:83: [5] {type-without-format} warning: type
         `Unsigned16TC' has no format specification
   simple to fix by adding a DISPLAY-HINT

         /IPSEC-POLICY-PIB:19: [5] {import-unused} warning: identifier
        `zeroDotZero' imported from module `SNMPv2-SMI's never used

   simple to fix by removing zeroDotZero from the imports

10. I see a lot of acronyms in the document without expansion (when used
    for the first time. These maybe well know to security folk, but not
    to new readers. RFC-Editor wants them to be exanded first time they 
    are used. Pls note that when PIB module is extracted, previous text is
    gone, and so it is probably wise to always expand in PIB DESCRIPTION 
    clauses.

11. All RFCs that contain PIB or MIB modules from which you IMPORT must
    be normatively referenced. The RFC-Editor recently also wanted such
    references to have citations. The notatation we agreed on is

      IMPORTS
      Unsigned32, Unsigned64, MODULE-IDENTITY,
      OBJECT-TYPE, TEXTUAL-CONVENTION, MODULE-COMPLIANCE,
      OBJECT-GROUP, pib
        FROM COPS-PR-SPPI                      -- [RFC3159]
      TruthValue
        FROM SNMPv2-TC                         -- [RFC2579]

    and so on. But.. your references seem to be just numbered.
    I Do not find 

      OBJECT-GROUP, pib
        FROM COPS-PR-SPPI                      -- [14]
   
    very acceptable. This is because when the PIB module gest extracted,
    then a "citation" of [14] does not help much. A Citation of [RFC3159]
    would still be clear for the reader (or so I think).

12. I see various ENUMERATIONS (e.g. INTEGERS) that probably would be
    better represented as a TC.
    For example:

       SYNTAX INTEGER {
         in(1),
         out(2),
         bi-directional(3)
         }
   
    and also 
        SYNTAX INTEGER {
          copy(1),
          set(2),
          clear(3)
          }
    there are others. Those are just 2 examples.

13.I wonder if this is wise:

      Unsigned16TC ::= TEXTUAL-CONVENTION
         STATUS       current
         DESCRIPTION
         "An unsigned 16 bit integer."
         SYNTAX    Unsigned32 (0..65535)

   In some places its use seems OK. In many places, it is used 
   to indicate an "order" as in:

      ipSecRuleOrder OBJECT-TYPE
        SYNTAX Unsigned16TC
        STATUS current
        DESCRIPTION
         "Specifies the precedence order of the rule within all the rules
          associated with {IfName, Roles}. A smaller value indicates a
          higher precedence order. "  
        ::= { ipSecRuleEntry  9 }

   I wonder if it would not be better to define a TC aka:

      IpSecOrderTC ::= TEXTUAL-CONVENTION
         STATUS        current
         DESCRIPTION  "An unsigned 16 bit integer that defines the order
                       of a Rule or Set. A smaller value indicates a
                       higher precedence order."
         SYNTAX    Unsigned32 (0..65535)

   And then use that TC for all "order" objects.

   In the case of 
      ipSecAssociationDhGroup OBJECT-TYPE
         SYNTAX Unsigned16TC

   I also wonder if a IpSecDhGroupTC would not be a wise thing.

14.I see citations in DESCRIPTION clauses, as in:

     ipSecRuleIfName OBJECT-TYPE
       SYNTAX SnmpAdminString
       STATUS current
       DESCRIPTION
      "The interface capability set to which this IPsec rule applies.
       The interface capability name specified by this attribute MUST
       exist in the frwkCapabilitySetTable [9] prior to association with
       an instance of this class."
       ::= { ipSecRuleEntry  2 }

   The [9] of course means nothing anymore when the PIB module gets
   extracted. If you used a notation ala [RFCxxxx] then it would still
   mean something in an extracted module.

15.In the ipSecXxxTable DESCRIPTION clauses you use text aka:
      "This table ...."
   In all other PIB modules that the IETF has published sofar, we did
   not talk about a table but either of a PRC (Policy Rule Class) or
   just class. See the DIFFSERV-PIB and the FRAMEWORK-PIB for example.
   I think that changing this so as to be consistent is a GOOD THING.

16.In the DESCRIPTION clause of:
     ipSecActionSetDoPacketLogging OBJECT-TYPE
        SYNTAX TruthValue
        STATUS current
        DESCRIPTION
      "Specifies whether to log when the resulting security association
       is used to process a packet. For ipSecStaticActions, a log message
       is to be generated when the IPsecBypass, IpsecDiscard or IKEReject
       actions are executed."
      ::= { ipSecActionSetEntry  5 }
  
   I wonder if "IPsecBypass, IpsecDiscard or IKEReject" are meant to
   mean the same thing as the enumerations in:

      ipSecStaticActionAction OBJECT-TYPE
         SYNTAX INTEGER {
            byPass(1),
            discard(2),
            ikeRejection(3),
            preConfiguredTransport(4),
            preConfiguredTunnel(5)
            }
         STATUS current

   If so, then you may want to sync up the labels.

17.I see:
     ipSecAssociationUseKeyExchangeGroup OBJECT-TYPE
       SYNTAX TruthValue
       STATUS current
       DESCRIPTION
       "Specifies whether or not to use the same GroupId for phase 2 as
        was used in phase 1.  If UsePFS is false, then this attribute is
        ignored.

        A value of true indicates that the phase 2 GroupId should be the
        same as phase 1.  A value of false indicates that the group number
        specified by the ipSecSecurityAssociationDhGroup attribute SHALL
        be used for phase 2. "
       ::= { ipSecAssociationEntry  7 }

   And wonder if ipSecSecurityAssociationDhGroup is mean to point to:

      ipSecAssociationDhGroup OBJECT-TYPE
        SYNTAX Unsigned16TC
        STATUS current
        DESCRIPTION
        "Specifies the key exchange group to use for phase 2 when the
         property ipSecSecurityAssociationUsePfs is true and the property
         ipSecSecurityAssociationUseKeyExchangeGroup is false."
        ::= { ipSecAssociationEntry  8 }

   If so, then better sync up the descriptors.
   If not, then where is ipSecSecurityAssociationDhGroup defined?
   By the way, in the last object, there are also a few descriptors
   that start with ipSecSecurityAssociation.... and that I cannot find.
   Again, probably s/Security// ??

18.I see:
     ipSecAhTransformReplayPreventionWindowSize OBJECT-TYPE
       SYNTAX Unsigned32
       STATUS current
       DESCRIPTION
      "Specifies, in bits, the length of the sliding window used by the
       replay prevention detection mechanism. The value of this property
       is ignored if UseReplayPrevention is false. It is assumed that the
       window size will be power of 2."
      ::= { ipSecAhTransformEntry  5 }
   Might be good to add a UNITS clause?
   But I also wonder what "window size will be power of 2" means?

19.ipSecEspTransformCipherKeyLength, ipSecEspTransformReplayPreventionWindowSize
   and may be others can also use a UNITS clause

20.In Table/Class ipSecCompTransformTable I see ipSecCompTransformAlgorithm
   and also ipSecCompTransformPrivateAlgorithm. Canthey both be specified
   for the same instance? If so, what happens? If not, can you make
   that clear? If yes, you may want to explain also what happens.

21.ipSecIkeRuleTable is probably talking about IKEv1 ??
   Should that be made clear?
   What sort of change do we expect for IKEv2?
   WOuld it make sense to define this "optional" class in a separate 
   document?
   In any event, you do not put in the DESCRIPTION clause that this table/class
   is optional. That is expressed (and even in machine readable form) in the
   MODULE-COMPLIACNE.

22.I am really wondering and worried about some of the UNIQUENESS clauses
   that require many columns/attributes to define uniqueness.
   But ipSecIkeAssociationEntry really seems excessive to me. It states
   that ALL attributes need to be checked for unqiueness !!???
   Is that really intended?
   You may want to check all tables/classes in fact.

23.ipSecIkeAssociationVendorId is an OCTET STRING and the descripiton clause
   talks about a vlaue of NULL. That is not clear! Better is to talk about
   a zero length OCTET STRING. That is clear!

24.I see:
     ipSecIkeProposalPrfAlgorithm OBJECT-TYPE
       SYNTAX Unsigned16TC
       STATUS current
       DESCRIPTION
      "Specifies the Psuedo-Random Function (PRF) to propose for the IKE
       association."
      ::= { ipSecIkeProposalEntry  7 }
   So where do I find what any value means?
   And would it not be better to have a separate TC fo such algorithm
   values?    And pls add REFERENCE clause to point to it.

24.Similar question for ipSecIkeProposalIkeDhGroup
   And pls add REFERENCE clause to point to it.

25.IN ipSecIkePeerEndpointIdentityValue you are using IP addresses as
   example. But you must use the ange of addresses that has been
   allocated for examples as per ID-NITs http://www.ietf.org/ID-nits.html
   which states:

        Addresses used in examples should prefer use of fully qualified
        domain names to literal IP addresses, and prefer use of example
        fqdn's such as foo.example.com to real-world fqdn's. See RFC 2606
        for example domain names that can be used. There is also a range
        of IP addresses set aside for this purpose. These are 192.0.2.0/24
        (see RFC 3330). Private addressess that would be used in the real
        world should be avoided in examples. 
 
26.Object ipSecCredentialFieldsValue is defined as an OCTET STRING.
   However, reading the DESCRIPTION clause, it feels as if this is
   a string that will be used/read by human beings. In that case, it
   must be a UTF-8 string. Possibly a SnmpAdminString or some other
   TC that defines UTF-8 text.

27.When I read the ipSecSelectorTable and related tables, then I really
   wonder if you guys have really evaluated the possible re-use of the
   frwkIpFilterTable in the FRAMEWORK-PIB !!??
   I thought that that framework filter table was meant to be re-used
   by many (if not all) filter/selector uses.

28.I have trouble understanding the ipSecIpsoFilterSetTable
   I bet it is cause by the very short DESCRIPTION clauses that do not
   mean much to me (and so probably do not mean much to other non-expert
   readers).

29.When I see ipSecRuleTimePeriodTimePeriod defined as an OCTET STRING
   then I wonder. It claims to be RFC2445 format (as RFC3060 also
   prescribes). There they talk about a string.
   I think it is US-ASCII, possibly represented as UTF-8 ??
   I need to go and check to be sure. In any event, if you say OCTET STRING
   and specify that THISANDPRIOR has special meaning, then it needs to be
   clear what encoding needs to be used for that text.
   A SIZE specification may also make sense (or so I think).
   This is a generic Policy construct, which probably even warrants a TC.

30.I do not understand 
     ipSecRuleTimePeriodMonthOfYearMask OBJECT-TYPE
       SYNTAX OCTET STRING
       STATUS current
       DESCRIPTION
      "An octet string that specifies which months the policy is valid
       for.  The octet string is structured as follows:

       - a 4-octet length field, indicating the length of the entire
         octet string; this field is always set to 0x00000006 for this
         property;
 
       - a 2-octet field consisting of 12 bits identifying the 12 months
         of the year, beginning with January and ending with December,
         followed by 4 bits that are always set to '0'.  For each month,
         the value '1' indicates that the policy is valid for that month,
         and the value '0' indicates that it is not valid.

       If this property is omitted, then the policy rule is treated as
       valid for all twelve months."
      ::= { ipSecRuleTimePeriodEntry  3 }

  My questions/thinking:
  - if indeed the length is ALWAYS 6, with only 2 octets of information,
    then why not define it as: OCTET STRING (SIZE(2))
  - But since you need to represent BITS, I wonder why you do not
    use the BITS construct aka:
     ipSecRuleTimePeriodMonthOfYearMask OBJECT-TYPE
       SYNTAX BITS {
          january(0),
          february(1),
          march(2),
          ...
          december(11)
       }
    In fact I would make it a TC. It is a generic construct for Policy
    information. As you probably know, BITS does translate to an
    octet string of the proper size on the wire.

31.Similar questions for ipSecRuleTimePeriodDayOfMonthMask.
  ipSecRuleTimePeriodDayOfWeekMask I have similar questions 

32.For ipSecRuleTimePeriodTimeOfDayMask I ahve similar questions as 
  in point 29 above

33.ipSecRuleTimePeriodLocalOrUtcTime
   I can live with it. It does not always result in a 16bit unint
   as per RFC3060 sect 6.5.6, but can easily be converted to that.
   Agin, it would be good if defined as a TC.

34.The reason I suggest to define TCs for points 29-33, is that the
   same TCs can/should be used by the MIB defintion. Since the MIB
   wants to (and can) go stds track, there probably should be a 
   separate POLICY-TC-MIB module that defines those TCs.

35.In fact, the whole TimePeriod... stuff seems so generic to any
   policy, that I would think it would also be in a separate module
   so that it can be re-used much easier.
   In fact the RFC3060 (and possibly RFC3460) should be separate
   modules that can be re-used by technology specific specifications
   and/or modules. I thought that that was the whole concept/idea of
   doing a PCIM and PCIMe. They would be CORE definitions upon which
   technology specific definitions (like IPsec and QoS) could/would
   build. Have we completely lost this idea/concept?

36.If I see it correctly, then every table has its own OBJECT-GROUP
   with a description clause of "Objects from the xxxTable."
   Is that the most logical grouping? I doubt it very much.
   Has any thought gone into this?

37.I will leave it to your security ADs to tell you about the security
   considerations section. But it seems to me that it is inadequate.

38.RFC-Editor considerations is incorrect. There are many more 
   normative references. In any event, [9] has been published as
   RFC3318, as you indicate yoruself in ref section. ANdf ref [12]
   has been published as RFC3385, as you can see on your WG charter page.

39.IANA Considerations section
   - the 1st para is fine, except I would add a ptr to the iana web page
     that has the registry. Makes it easier for everyone
   - the 2nd para need to be formulated in the same way as 1st para.
     So that it is also readable after IAN has done the assignment.
     Pls also add ptr to the cirrect iana web page.

Since this will result in a new revision, I propose to also add the
proper IPR statements as per the new RFCs 3667/8/9

Bert

---

From: <[email-masked]>
Date: Fri, 26 Mar 2004 09:32:41 -0500

Hi Bert,

Thanks for your comments and no need to worry since I am working on the draft. Will submit a new version when I finish.

Best regards
Man Li

-----Original Message-----
From: ext Wijnen, Bert (Bert) [mailto:[email-masked]]
Sent: Friday, March 26, 2004 8:59 AM
To: Li Man.M (Nokia-NRC/Boston)
Cc: [email-masked]'; [email-masked]'; [email-masked]';
[email-masked]'; [email-masked]'
Subject: RE: MIB/PIB doctor review for draft-ietf-ipsp-ipsecpib-09.txt -
part 2

I have to be very careful here (cause it took me many months
before I finished my review; for which I again appologize).

But once I have done the review, these things are kind of "fresh" in 
my mind/memory. If it takes a long time before you get to respond or
send a new revision... things get swapped out, sometimes into a 
sort of off-site store... 

Thanks,
Bert 

> -----Original Message-----
> From: Wijnen, Bert (Bert) 
> Sent: dinsdag 16 maart 2004 15:49
> To: [email-masked]; [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked];
> [email-masked]; [email-masked]
> Subject: MIB/PIB doctor review for draft-ietf-ipsp-ipsecpib-09.txt -
> part 2
> 
> 
> 19.ipSecEspTransformCipherKeyLength, 
> ipSecEspTransformReplayPreventionWindowSize