[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
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