RE: MIB/PIB doctor review for draft-ietf-ipsp-ipsecpib-09.txt - p art 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 protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: MIB/PIB doctor review for draft-ietf-ipsp-ipsecpib-09.txt -
> part 2
> 
> 
> 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
>