[MIB-DOCTORS] MIB Doctor review: mib content: draft-ietf-ipsp-ipsecaction-mib-02.txt

["Wijnen, Bert \(Bert\)" <[e-mail]>]

[I have copied Keith, because I understand he did partially review this
MIB module].

I have included Keith's comments in the review posting for the
ikeaction-mib.
Pls check there.

Also, Keith and I are not (yet) in complete agreement on some aspects of
MIB modules. I know that Keith has written to you about (for example)
the IPSEC-SPD-MIB after I had done MIB doctor (and I guess at the time
OPS AD review).
These are the two issues:

- I have advised, for objects of SYNTAX STorageType,
  to use some tekst aka

     For a storage type of permanent, none of the columns have
     to be writable."

- I have advised to write text aka

     An attempt to set this object to a value that does not
     exist in the ipiaSaNegotiationParametersTable MUST result
     in an inconsistentValue error."


Please seem my comment on ikeaction-mib for details.
The point is that I again have to warn that I may change my
earlier advise once this discussion closes on MIB doctors team.

General remark
I am not well enough versed/skilled in the security protocols and
mechanisms/algorithms that are being modeled in this MIB
module/document. So I have done a review with a SNMP/MIB view, but I
have not (and cannot) review it from a security-point of view. In fact,
after having spend more than a day on this MIB module, I cannot say that
I really understand it all.

So I am assuming that real security folk will do the securiy review and
check if this indeed correctly models the management information as
intended (or maybe already have done that). 
Also the grouping of Objects and the two MODULE-COMPLIANCE statements
are outside my competency to determine if they make sense or not.

The MIB documents are nearly clean (from a SYNTAX point of view) and
also many of the issues I had brought up in the review if the
IPSEC-SPD-MIB and also were rpesent in an earlier version of this MIB
document, have been resolved.
So a more detailed review made sense this time. Lots of comments and
questions still remain though.

Here are my comments on the IPSEC-IPSECACTION-MIB module.

1. I think this one looks better and is in better shape than the
   ikeaction-mib. This is good.

2. Several of my comments in the ikeaction-mib review apply,
   examples are:
   - use of "empty", "blank" or "NULL" for OCTET STRINGS
     you probably mean "zero-length string".
   - Use of capitalized terms from RFC2119 seem not
     always appropriate.
   - Use more REFERENCE clauses to point to 
     supporting/explaing/background documents
   - In various places where you use a SnmpAdminString TC
     with a SIZE (0..xx), pls make sure that you explain what
     the zero-length string means. Also make sure that a 
     zero-length string is always valid, if not, pls change
     the SIZE spec.
   - empty placeholders for NotificationVariables and Notifications.
   - possibly others, pls check   

2. I see
   ipsaSaPreActActionLifetimeSec OBJECT-TYPE
       SYNTAX      Unsigned32
       UNITS       "seconds"
       MAX-ACCESS  read-create
       STATUS      current
       DESCRIPTION
           "ipsaSaPreActActionLifetimeSec specifies how long in seconds
            the security association derived from this action is used.

            The default lifetime is 8 hours.

            Note: the actual lifetime of the preconfigured SA will be
            the lesser of the value of this object and of the value of
            the MaxLifetimeSecs property of the associated transform.

            A value of 0 indicates no time limit on the lifetime
            of the SA."
       DEFVAL      { 28800 }

   Which is good, because in a similar object in IKEACTION MIB, you used
   the value of zero to mean the default of 8 hours. I have suggested
   in my IPSEC-IKEACTION-MIB review that you use 28800 as the DEFAULT.
   Whatever you do, pls be consistent. And the spec here looks better to
   me than in IPSEC-IKEACTION-MIB.

3. ipsaSaPreActDFHandling
   You also have a DFHandling in ikeaction-mib.
   Maybe better to use a common TC ??

4. in 
   ipsaSaPreActESPTransformName OBJECT-TYPE
       SYNTAX      SnmpAdminString (SIZE(0..32))
       MAX-ACCESS  read-create
       STATUS      current
       DESCRIPTION
           "This object is the name of the ESP transform to use as an
            index into the ESPTransformTable.  A zero length value
            indicates no transform of this type is used."
       ::= { ipsaSaPreconfiguredActionEntry 14 }

   I see mention of ESPTransformTable. I cannot find that table.
   You probably/possibly mean ipsaEspTransformTable

5. ipsaSaPreActESPEncSecretName and ipsaSaPreActESPAuthSecretName
   claim to be usable as an index into ipsaCredentialTable.
   So Does a zero length string value mean there is no such info?
   Pls explain in DESCRIPTION clauses. 

6. For
     ipsaSaPreActIPCompSPI OBJECT-TYPE
         SYNTAX      Integer32
   I wonder of an SPI is not better represented as an Unsigned32.
   After all, the RFCs claim that SPI is an arbitrary 32bit value.
7. For these 2 objects:
     ipsaAhTranMaxLifetimeSec OBJECT-TYPE
      SYNTAX      Unsigned32
      UNITS       "seconds"
      MAX-ACCESS  read-create
      STATUS      current
      DESCRIPTION
          "ipsaAhTranMaxLifetimeSec specifies how long in seconds the
           security association derived from this transform SHOULD be
           used.

           A value of 0 indicates that the default lifetime of
           8 hours SHOULD be used."
      ::= { ipsaAhTransformEntry 2 }

     ipsaAhTranMaxLifetimeKB OBJECT-TYPE
      SYNTAX      Unsigned32
      MAX-ACCESS  read-create

      STATUS      current
      DESCRIPTION
          "ipsaAhTranMaxLifetimeKB specifies how long in kilobytes the
           security association derived from this transform SHOULD be
           used."
      ::= { ipsaAhTransformEntry 3 }

  I really get confused by the DESCRIPTION clause vs the descriptor
  (object name). 
  The name seem to indicate these are "maximum" life times.
  The DESCRIPTION (specifically since it uses capitalized SHOULD) 
  seems to tell me that they mmust be used at least this long (or
  may be exactly this long) ??
  I will admit that  English is not my native languange.

  The following is better in this respect:

    ipsaEspTranMaxLifetimeKB OBJECT-TYPE
       SYNTAX      Unsigned32
       MAX-ACCESS  read-create
       STATUS      current
       DESCRIPTION
           "ipsaEspTranMaxLifetimeKB specifies how long in kilobytes
            the security association derived from this transform is
            used."
       ::= { ipsaEspTransformEntry 3 }

   However now it seems to say (in the DESCRIPTION clause) it is to
   be exactly this value, where as I think you mean max so many
kilobytes.


8. Is this inconsistency (w.r.t. DEFVAL and/or default and the meaning
   of zero) done on purpose:

     ipsaSaPreActActionLifetimeSec

             .. snip ..

            The default lifetime is 8 hours.

             .. snip ..

            A value of 0 indicates no time limit on the lifetime
            of the SA."
       DEFVAL      { 28800 }

  
     ipsaAhTranMaxLifetimeSec OBJECT-TYPE

            .. snip ..

            A value of 0 indicates that the default lifetime of
            8 hours SHOULD be used."

     ipsaEspTranMaxLifetimeSec OBJECT-TYPE

            .. snip ..

           A value of 0 indicates that the default lifetime of
           8 hours SHOULD be used."

     ipsaIpcompTranMaxLifetimeSec OBJECT-TYPE
   
            .. snip ..

            A value of 0 indicates that the default lifetime of
            8 hours SHOULD be used."

9. IN 
   ipsaEspTranCipherKeyLength OBJECT-TYPE
       SYNTAX      Unsigned32
       MAX-ACCESS  read-create
       STATUS      current
       DESCRIPTION
           "This object specifies, in bits, the key length for
            the ESP cipher algorithm."

   I wonder if the value can really be zero. If so, what does that mean?
   And can it be weak key lengths like 1,2,3 and such?


10. ipsaEspTranReplayWindowSize 
   Is a zero value valid? I guess in case    ipsaEspTranReplayPrevention
   is 'false' that a zero value would be OK. Bit what does it mean
   (or is it allowed) when 'true' ??

   Same for ipsaAhTranReplayWindowSize (and any other xxxWindowSize)

11. In
      ipsaSaPreActAHSPI OBJECT-TYPE
        SYNTAX      Integer32
        MAX-ACCESS  read-create
        STATUS      current
        DESCRIPTION
          "This object represents the SPI value for the AH SA."

    And in other places where an SPI is defined, I wonder if
    Integer32 is the best representations.
    RFC4301 states:

      Security Parameters Index (SPI)

      An arbitrary 32-bit value that is used by a receiver to identify
      the SA to which an incoming packet should be bound.  For a unicast
      SA, the SPI can be used by itself to specify an SA, or it may be
      used in conjunction with the IPsec protocol type.  Additional IP
      address information is used to identify multicast SAs.  The SPI is
      carried in AH and ESP protocols to enable the receiving system to
      select the SA under which a received packet will be processed.  An
      SPI has only local significance, as defined by the creator of the
      SA (usually the receiver of the packet carrying the SPI); thus an
      SPI is generally viewed as an opaque bit string.  However, the
      creator of an SA may choose to interpret the bits in an SPI to
      facilitate local processing.

    So I'd say that an Unsigned32 is probably better. Or maybe even 
    an OCTET STRING SIZE(4). Not a fatal flaw though to use Interger32.
    I also found (based on a ptr from Sam Hartman) that in appendix
    C of RFC4301 it seems to be defined as an interger as well. Oh well.


11 Things about LiftimeSecs, LifetimeKB, KeyLength, and such of course
apply
   to all occurences in all tables.

12. When I read

   ipsaCredRemoteID OBJECT-TYPE
       SYNTAX      OCTET STRING(SIZE(0..256))
       MAX-ACCESS  read-create
       STATUS      current
       DESCRIPTION
           "This object represents the Identification (e.g. user name)
            of the user of the key information on the remote site.  If
            there is no ID associated with this credential, the value
            of this object SHOULD be the null string."

   Then I get the impression that this object is intended for human
   consumption (or representing humans). I'd think that a UTF-8 based
   string would be better so that a user name can be properly
represented
   also for non US-ASCII names.

13.
   ipsaCredSegIndex OBJECT-TYPE
       SYNTAX      Integer32 (1..65535)
       MAX-ACCESS  not-accessible
       STATUS      current

   RFC4181 strongly RECOMMENDs to use Unsigned32 for INDEX objects.

14.
   ipsaCredSegStorageType OBJECT-TYPE
       SYNTAX      StorageType
       MAX-ACCESS  read-only
       STATUS      current
       DESCRIPTION
           "The storage type for this row.  This object is
            read-only. Rows in this table have the same value as the
            ipsaCrendStorageType for the corresponding row in the
            ipsaCredentialTable.

            For a storage type of permanent, none of the columns have
            to be writable."
       DEFVAL { nonVolatile }
       ::= { ipsaCredentialSegmentEntry 4 }

  I suggest one could get rid of this object and just state the
persistency
  behaviour in the DESCRIPTION clause of ipsaCredentialSegmentEntry

15. I think I already mentioned in my IPSEC-IKEACTION-MIB review...
  I get completely confused by the 3 TCs:

    IpsaIdentityFilter, IpsaCredentialType and IpsecDoiIdentType

  I got confused again when I compared and tried to understand how these
  are used in the ipsaCredentialTable and ipsaPeerIdentityTable.
  Are those usesnon-consistent? If they are intended as used/specified, 
  pls try to help me clear my confusion.

16.
  --
  --
  -- Notification objects information
  --
  --

  ipsaNotificationVariables OBJECT IDENTIFIER ::=
     { ipsaNotificationObjects 1 }

  ipsaNotifications OBJECT IDENTIFIER ::=
     { ipsaNotificationObjects 0 }

  Seems empty and useless. Can be deleted as far as I am concerned.
  However, not a fatal flaw or serious error.
  I find Notifications to be defined under NotificationObjects
  somehwat strange as well.

17. MODULE-COMPLIANCE
  I see:

           -- OBJECT ipsaPeerIdAddressType
           -- SYNTAX InetAddressType { ipv4(1), ipv6(2) }
           -- DESCRIPTION
           -- Only support for global IPv4 and IPv6 address
           -- types is required.
           --
           -- OBJECT ipsaPeerIdAddress
           -- SYNTAX InetAddress (SIZE(4|16))
           -- DESCRIPTION
           -- Only support for global IPv4 and IPv6 address
           -- types is required.
           --"
   The above notation is done for INDEX objects (they cannot be
   lisyed in a real OBJECT clause, and so we put them as commnets
   in a DESCRIPTION clasue. For these objects though you can
   (and should use) real OBJECT clauses, not just comments.

   I now notice that this is also the case for the IKEACTION-MIB.
   I have added that comment to my IKEACTION-MIB review.



Bert

_______________________________________________
MIB-DOCTORS mailing list
[e-mail]
https://www1.ietf.org/mailman/listinfo/mib-doctors