[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
FW: MIB/PIB doctor review for draft-ietf-ipsp-ipsecpib-09.txt - part 2
Hi Bert,
Thanks for your comments. Suggestions on time period syntax are very helpful. Below is a description of modifications made according to each comment.
Best regards
Man
> -----Original Message-----
> From: Wijnen, Bert (Bert)
> Sent: dinsdag 16 maart 2004 15:49
> To: Man.M.Li@xxxxxxxxx; bwijnen@xxxxxxxxxx
> Cc: smb@xxxxxxxxxxxxxxxx; ho@xxxxxxxxxxxx; lsanchez@xxxxxxxxxxx;
> avri@xxxxxxxxxxxxxx; ipsec-policy@xxxxxxxx
> 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
OK. added display hint and removed zeroDotZero.
>
> 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.
Expanded the acronyms when they first appear in text and in PIB.
>
> 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).
OK. Used [RFCxxx] INSIDE the mib module.
>
> 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.
Defined many new TCs and also imported TCs from ipsec policy mibs. The few enumerations left are "un-typical" ones.
> 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.
Created a IpSecOrderTC and used 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.
Imported IkeGroupDescription from the ipsec MIB for the purpose
>
>
> 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.
Changed to [RFC3318]
>
> 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.
replaced "table" with "class" in all cases.
>
> 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.
Added labels in the packet logging description as follows
"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 (ipSecStaticActionAction = 1), IpsecDiscard (ipSecStaticActionAction = 2) or IKEReject (ipSecStaticActionAction = 3) actions are executed. "
>
> 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// ??
You are right. Made changes accordingly.
>
> 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?
Added a UNITS clause. Also changed the sentense to "It is assumed that the window size will take a value that is a power of 2."
>
> 19.ipSecEspTransformCipherKeyLength,
> ipSecEspTransformReplayPreventionWindowSize
> and may be others can also use a UNITS clause
Added UNITS clause when appropriate.
>
> 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.
Deleted ipSecCompTransformPrivateAlgorithm since there is no value defined.
>
> 21.ipSecIkeRuleTable is probably talking about IKEv1 ??
> Should that be made clear?
OK, used IKEv1 in the DESCRIPTION of IKE related classes
> What sort of change do we expect for IKEv2?
> WOuld it make sense to define this "optional" class in a separate
> document?
It seems to me that currently IETF does not want to see any more PIB drafts (and please let me know if I am wrong on this.)
In any case, as explained in the document, specifying the use of IKEv2 is quite simple -- direct the pointer to an IKEv2 class instead of the IKEv1 defined in this 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.
OK. removed optional from DESCRIPTION clauses.
>
> 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.
I looked through all the classes, in some cases (e.g., ipSecRuleTable) only a few attributes are needed in the UNIQUENESS. Unfortunately, ipSecIkeAssociationEntry does need all the attributes.
>
> 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!
Changed NULL to zero length OCTET STRING.
>
> 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.
Changed the DESCRIPTION to
"Specifies the Psuedo-Random Function (PRF) to propose for the IKE association. As indicated in [RFC2409], there are currently no negotiable pseudo-random functions defined in this document. Private use attribute values can be used for prf negotiation between consenting parties."
>
> 24.Similar question for ipSecIkeProposalIkeDhGroup
> And pls add REFERENCE clause to point to it.
Imported IkeGroupDescription TC from ipsec MIB for this usage.
>
> 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.
>
OK. Used 192.0.2.0/24 in the example instead.
> 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.
Changed to SnmpAdminString instead of octet string.
>
> 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.
As discussed in the document, the selector group is meant to specify a scalable way of define a large number of selectors. As indicated in the document (Section 3), to use other filters, simply direct the pointer to that class in a different PIB (e.g framework PIB).
>
> 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).
Expand the DESCRIPTION as follows
"Specifies IP Security Options (IPSO) filter sets. Each set contains an ordered list of IPSO filters. Please refer to [RFC1108] for details on IPSO."
>
> 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.
Defined a TC for this OCTET STRING encoded as UTF-8.
>
> 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.
Defined a TC as suggested.
>
> 31.Similar questions for ipSecRuleTimePeriodDayOfMonthMask.
> ipSecRuleTimePeriodDayOfWeekMask I have similar questions
Defined TCs for them.
>
> 32.For ipSecRuleTimePeriodTimeOfDayMask I ahve similar questions as
> in point 29 above
Defined a TC for this OCTET STRING encoded as UTF-8.
> 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.
Defined 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.
OK.
>
> 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?
I sent out an e-mail to see if the working group or anywhere in IETF is interested in having a separate PIB draft. No response which I take as "No".
>
> 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?
Modified. The PIB has four core groups and a few optional groups
>
> 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.
OK
>
> 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.
OK removed the sentence referring [9] and [12]
>
> 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.
Copied with modification the IANA section from RFC3317.
> 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
Added IPR ack as Section 12.
>
> Bert
>