Subject: [ntpwg] My review of: draft-ietf-ntp-ntpv4-mib-05.txt

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Sun, 8 Feb 2009 21:31:40 +0100

Note: I am not subscribed to the ntpwg mailing list and do not plan to
subscribe either. So if you want me to see any responses, please
copy me explicitly

I did a review for:
    http://www.ietf.org/internet-drafts/draft-ietf-ntp-ntpv4-mib-05.txt

Here are my findings:

SMICng results:

  C:\bw\smicng\work>smicng ntpv4.inc
  E: f(ntpv4.mi2), (272,14) Default value for "ntpEntStatusCurrentModeVal"
      must be a name and not a number

   *** 1 error and 0 warnings in parsing

This means changing
    DEFVAL { 99 }
into
    DEFVAL { unknown }
will fix it.

But I also find:

- writable objects for which I see no text in the DESCRIPTION clauses
  about expected persistency behaviour

- Counter32 objects with no test in the DESCRIPTION clauses about
  possible discontinuities and which timer object would indicate such.
  Specifically, the fact that (I think) the ntpd keeps track of the 
Counters,
  it seems that there may be discontinuities at other times than when
  the snmpd restarts (e.g. at other times than reset of sysUpTime)

- I see a whole list of REVISION clauses. Normally we only have a
  single revision clause at RFC publication time. Not a list of revisions
  that took place during I-D revisions.

- I am missing the Copyright statement in the DESCRIPTION clause
  of the MODULE_IDENTITY
- I am missing that this revision of the document is published as RFC xxxx
  (xxxx to be filled in by RFC-editor)

- I see a lot of objects with a SYNTAX of DisplayString.
  I would like to point out "note 2" on web page
       http://www.ops.ietf.org/mib-common-tcs.html
   Note 2.  DisplayString does not support internationalized text.
            It MUST NOT be used for objects that are required to
            hold internationalized text (which is always the case
            if the object is intended for use by humans [RFC2277]).
            Designers SHOULD consider using SnmpAdminString,
            Utf8String, or LongUtf8String for such objects.

- Instead of
     ntpEntNotifPrefix  OBJECT IDENTIFIER ::= { ntpSnmpMIBObjects 6 }
     ...
     ntpEntNotifications OBJECT IDENTIFIER ::= { ntpEntNotifPrefix 0 }

  I propose/suggest to do
     ntpEntNotifications OBJECT IDENTIFIER ::= { ntpSnmpMIB 0 }

  This makes it more inline with what we often do in MIB module.
  Also makes the OIDs of notifications shorter

- I strongly recommend to use the MIB structure as per RFC4181,
  which suggests that you would replace
       ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 6 }
  with
       ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 2 }

- I see OBJECT-GROUP definitions like
   ntpEntObjectsGroup1 OBJECT-GROUP
      OBJECTS {
              ntpEntSoftwareName,
              ntpEntSoftwareVersion,
              ntpEntSoftwareVersionVal,
              ntpEntSoftwareVendor,
              ntpEntSystemType,
              ntpEntStatusEntityUptime,
              ntpEntStatusDateTime,
              ntpAssocName,
              ntpAssocRefId,

              ntpAssocAddressType,
              ntpAssocAddress
     }
     STATUS      current
     DESCRIPTION
         "A collection of objects for the NTP MIB that all NTP
          or SNTP entities should implement."
     ::= { ntpEntGroups 1 }

  Text about "should implement" or "optional" or such is NOT supposed to be
  in an OBJECT-GROUP DESCRIPTION clause. The OBJECT-GROUP is
  only meant to group objects logically together.
  If such a group is mandatory or optional is something that is expressed
  via MANDATORY-GROUPS or via the GROUP clauses in the
  MODULE-COMPLIANCE.

 As another example, you have

     ntpEntObjectsGroup2 OBJECT-GROUP
     ...
     DESCRIPTION
        "A collection of objects for the NTP MIB that are optional
         for NTP or SNTP entities to implement."
     ::= { ntpEntGroups 2 }

  So here you say that the group is optional while you have listed this 
group
  as a MANDATORY-GROUP here:

    ntpEntNTPCompliance MODULE-COMPLIANCE
       STATUS      current
       DESCRIPTION
          "The compliance statement for SNMP entities which use NTP and
           implement the NTP MIB"
      MODULE  -- this module
          MANDATORY-GROUPS {
                           ntpEntObjectsGroup1,
                           ntpEntObjectsGroup2,
                           ntpEntNotifPrefixGroup
         }
         ::= { ntpEntCompliances 1 }

  So that is a conflict in your MIB-Module.

- I see a lot of MIB-level comment lines, aka:

  ntpEntSoftwareVersion OBJECT-TYPE
    SYNTAX      DisplayString
    MAX-ACCESS  read-only
    STATUS      current
    DESCRIPTION
        "The software version of the installed NTP implementation."
    -- full version string, e.g. "[email-masked] ..."
    ::= { ntpEntInfo  2 }

   I think it is much better to include that informatrion in the DESCRIPTION
   clauses. Otherwise it will often be lost when the MIB module is extracted
   or used as input for generating help text etc.

- I see references/citations aka:

NtpDateTime ::= TEXTUAL-CONVENTION
    DISPLAY-HINT "4d:4d:4d.4d"
    STATUS      current
    DESCRIPTION
        "NTP date/time on the device, in 128-bit
         NTP date format. Ref: draft-ietf-ntp-ntpv4-proto-06,
         section 6:
          It includes a 64-bit signed seconds field
          spanning 584 billion years and a 64-bit fraction
          field resolving .05 attosecond (i.e. 0.5e-18).
          For convenience in mapping between formats, the
          seconds field is divided into a 32-bit era field
          and a 32-bit timestamp field.

         If time is not syncronized this field shall be a
         zero-length string.

         This TC is not to be used for objects that are used
         to set the time of the node querying this object.

         NTP should be used for this--or at least SNTP."
    SYNTAX      OCTET STRING (SIZE (0 | 16))

  I suggest that the pointer to Ref: draft-ietf-ntp-ntpv4-proto-06
  is changed to Ref: RFC yyyy (and ask RFC-editor to fill in
  that yyyy value. I see that Ref: draft-ietf-ntp-ntpv4-proto-11 is
  also in Last Call, so that would be fine.
  In genreal though, it is probably better to use the REFERENCE
  clause. So it would be something like

    REFERENCE   "Network Time Protocol Version 4 Protocol
                                 And Algorithms Specification, sect 6, RFC 
yyyy"

- It seems to me that for something like this:
   ntpEntStatusEntityUptime OBJECT-TYPE
      SYNTAX      Unsigned32
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
          "The uptime of the NTP entity in seconds."
      -- time since ntpd was (re-)started (not sysUptime!)
      DEFVAL { 0 }
      ::= { ntpEntStatus 9 }

  I would add a  UNITS clause.
  I would also move the comment line inside the DESCRIPTION clause.
  I am not sure if a DEFVAL of 0 (zero) makes sense.
  Specifically... what would the value be if the snmp agent has started
  later that ntpd? What if the snmp agent restarts and the ntpd does not?
  In fact, I think that a DEFVAL clause makes no sense for this object.

  I in fact wonder if most of the DEFVAL values for the read-only objects
  make sense.

- For
  ntpEntStatPktModeTable OBJECT-TYPE
    SYNTAX           SEQUENCE OF NtpEntStatPktModeEntry
    MAX-ACCESS       not-accessible
    STATUS           current
    DESCRIPTION
        "The number of packets sent and received by packet mode."
    ::= { ntpEntStatus 18 }

  ntpEntStatPktModeEntry OBJECT-TYPE
    SYNTAX      NtpEntStatPktModeEntry
    MAX-ACCESS  not-accessible

    STATUS      current
    DESCRIPTION
        "The number of packets sent and received by packet mode."
    INDEX       { ntpEntStatPktMode }
    ::= { ntpEntStatPktModeTable 1 }

  I am surprised by the SAME DESCRIPTION clause for each of these!!??

- When I see:
ntpAssocAddressType OBJECT-TYPE
    SYNTAX      InetAddressType
    MAX-ACCESS  read-only
    STATUS      current
    DESCRIPTION
        "The type of address of the association."
    -- contains the type of address for uni/multi/broadcast associations
    ::= { ntpAssociationEntry 4 }

ntpAssocAddress OBJECT-TYPE
    SYNTAX      InetAddress
    MAX-ACCESS  read-only
    STATUS      current
    DESCRIPTION
        "The IP address (IPv4 or IPv6) of the association."
    -- contains IP address of uni/multi/broadcast associations
    ::= { ntpAssociationEntry 5 }

  Then I like to point out that RFC4001 states that the use of InetAddress
  SYNTAX, MUST have a DESCRIPTION clause that explain./states
  which object of SYNTAX InetAddressType controls the format of
  the InetAddress object. It is most probably ntpAssocAddressType
  but it would be good to state so.

  Besides, if you are sure that this ALWAYS only applies to IPv4 and IPv6,
  then I would do it as follows:

    ntpAssocAddressType OBJECT-TYPE
       SYNTAX      InetAddressType { ipv4, ipv6 }

   ntpAssocAddress OBJECT-TYPE
       SYNTAX      InetAddress SIZE(4|16)

 You must check if you also want to use Zoned addressed, because then
  that must be added too.

 If, at the other hand, other values than ipv4/ipv6 are possible (maybe
 in the future?) then it might be better to put the "minimum support for
 ipv4/ipv6 requirement in the MODULE-COMPLIANCE via an OBJECT
 and SYNTAX clause.

- I see some 8 notifications. Is there a risk that too many get generated
  at any point in time, such that it would flood the network and/or
  overload a NMS ??
  It would be good to add some text about the expected max notification
  rates and/or add a throthle or an enable/disable switch.

- It seems that RFC4001 is missing as a normative reference.

- I would think that the new draft-ietf-ntp-ntpv4-proto-11 (version 4 of 
NTP)
  should also be a normative reference

- The security section is not according to the guidelines provided at
       http://www.ops.ietf.org/mib-security.html

- I have not checked the new NTP version 4 spec to see if the knobs/sensors
  in that protocol have properly been modeled in this MIB module.
  That is a separate task that may actually need much more time than the
  review I have done for now.

Bert Wijnen 

_______________________________________________
ntpwg mailing list
[email-masked]
https://lists.ntp.org/mailman/listinfo/ntpwg

---

From: Heiko Gerstung <[email-masked]>
Date: Thu, 19 Feb 2009 08:19:57 +0100

Bert,

thank you very much for your detailed and useful review of our MIB 
module. I will try to implement as many points as possible but I am 
unsure what that does to the standardization process (it is my first RFC 
....). It took us all a long time to get into last call and I am a 
little bit scared that the modifications will restart the whole 
procedure again. I asked my wg chair for advise what to do.

See my additional comments below (inline):

>   *** 1 error and 0 warnings in parsing
Strange, I used smilint at level 6 and did not get any errors. This is 
an easy fix, though.
>  about expected persistency behaviour
That means you want to know whether an object stores the changes on that 
object permanently or not? I am not sure if this is not 
implementation-specific.
>  the snmpd restarts (e.g. at other times than reset of sysUpTime)
Not sure what you mean, can you please provide me with an example?
>  that took place during I-D revisions.
OK.
> - I am missing the Copyright statement in the DESCRIPTION clause
I guess this has been overseen but can be added quite easily. Where can 
I find the required copyright statement in its correct form?
>  (xxxx to be filled in by RFC-editor)
OK.
>            Utf8String, or LongUtf8String for such objects.
The DisplayString objects are mainly used for values which require to be 
floating point and we agreed in the working group discussions that it 
would make sense to use a string instead that can be converted into an 
fp value by a machine.
>  Also makes the OIDs of notifications shorter
OK, agreed.
>       ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 2 }
OK.
>  via MANDATORY-GROUPS or via the GROUP clauses in the
OK, I will remove the text from the description and change the group 
clauses accordingly.
>  So that is a conflict in your MIB-Module.
OK.
>   or used as input for generating help text etc.
Good point, noted.
>                                 And Algorithms Specification, sect 6, 
> RFC yyyy"
>
OK, makes perfect sense.
>  In fact, I think that a DEFVAL clause makes no sense for this object.
Agreed, I will remove the DEFAL line. Can you provide me with an example 
on how to use the UNITS clause?

>  I am surprised by the SAME DESCRIPTION clause for each of these!!??
I will change that.
>       SYNTAX      InetAddress SIZE(4|16)
Thanks, I will do it this way because we currently only have NTP over 
IPv4 and IPv6 - to my knowledge.
> You must check if you also want to use Zoned addressed, because then
Any thoughts on that by anybody?
> ipv4/ipv6 requirement in the MODULE-COMPLIANCE via an OBJECT
Again, since this is my first MIB I would appreciate an example.
>  rates and/or add a throthle or an enable/disable switch.
OK, I will look into this.
> - It seems that RFC4001 is missing as a normative reference.
OK.
> - I would think that the new draft-ietf-ntp-ntpv4-proto-11 (version 4 
> of NTP)
Shouldn't we use the RFC yyyy approach instead (yyyy being the 
to-be-assigned RFC for the protocol RFC)?
>       http://www.ops.ietf.org/mib-security.html
I unfortunately currently seem to be unable to access this website (and 
www.ietf.org as well), but I think it would be helpful to know which 
points are missing / wrong in your oppinion.
>  review I have done for now.
Since this has been discussed in the working group I would expect that 
everybody is happy with what is in the MIB right now. But I really 
appreciate your efforts and your very detailed review of this document. 
I only wish we would have somebody sending me such a review earlier in 
the process, before everything went into last call. Anyway, you did a 
great job and I think this was a very useful review. I will go back to 
my drawing desk and start to implement the advised changes.

Best Regards,
  Heiko

---

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Fri, 20 Feb 2009 00:05:01 +0100

inline
----- Original Message ----- 
From: Heiko Gerstung
To: Bert Wijnen (IETF)
Cc: [email-masked]
Sent: Thursday, February 19, 2009 8:19 AM
Subject: Re: My review of: draft-ietf-ntp-ntpv4-mib-05.txt

Bert,

That means you want to know whether an object stores the changes on that
object permanently or not? I am not sure if this is not
implementation-specific.
<bw>
It SHOULD not be implementation specific.
How can a management station manage multiple devices (multiple vendors)
if persistency behaviour of data is unknown? Just trial and error?
</bw>

<bw>
COuld there be discontinuities when the ntp deamon restarts?
If so, the management station wants to have a timer-like object to check
if such things occur. See the definition of Counter32 (RFC2578) and/or check
other MIB examples
</bw>

OK.

<bw>
I am not 100% sure what it is at this point in time... there was/is a lot
of discussion on this on the IETF list at this time. Check with your WG 
chair(s)
</bw>

OK.

<bw>
I suggest you describe this at some place in your document; possibly as a 
comment
in your MIB module. Even then, I am not sure if this is wise. I will check 
with
other MIB doctors (in fact they are copied on this email).
But I also see many non FP objects that use DisplayString (for as far as I 
can tell no
good reason).
</bw>

OK, agreed.
OK.
OK.
Good point, noted.
OK, makes perfect sense.

<bw>
       UNITS           "seconds"

</bw>

I will change that.

<bw>
Look for example at the MODULE-COMPLIANCE for rfc3289; here is an extract:
    OBJECT diffServMultiFieldClfrAddrType
    SYNTAX  InetAddressType { unknown(0), ipv4(1), ipv6(2) }
    DESCRIPTION
       "An implementation is only required to support IPv4 and IPv6
       addresses."

    OBJECT diffServMultiFieldClfrDstAddr
    SYNTAX  InetAddress (SIZE(0|4|16))
    DESCRIPTION
       "An implementation is only required to support IPv4 and globally
       unique IPv6 addresses."

</bw>

OK.

<bw> yes please</bw>

<bw>
basically all of it. Pls try again when you can access the web site
</bw>

Since this has been discussed in the working group I would expect that
everybody is happy with what is in the MIB right now.

<bw>
That may be the case, but given the many basic concerns I raise, that does 
not
give me a warm feeling.
</bw>

But I really
appreciate your efforts and your very detailed review of this document.
I only wish we would have somebody sending me such a review earlier in
the process, before everything went into last call.

<bw>
Basically WG chairs can/should know that they can ask for MIB doctor help
early in the process. And as far as I knopw, Chris is well aware of that 
too.
</bw>

Anyway, you did a

Best Regards,
  Heiko

---

From: Brian Haberman <[email-masked]>
Date: Wed, 25 Feb 2009 10:54:07 -0500

Inline...

Bert Wijnen (IETF) wrote:

<snip>

> if such things occur. See the definition of Counter32 (RFC2578) and/or check

As an example of how the UDP MIB deals with discontinuities, take a look 
at the udpInDatagrams object (and other Counter32 objects) in RFC 4113.

> of discussion on this on the IETF list at this time. Check with your WG 

I believe the following is the correct text to include in the 
DESCRIPTION clause:

    Copyright (c) 2009 IETF Trust and the persons identified as the
    document authors.  All rights reserved.

    This document is subject to BCP 78 and the IETF Trust's Legal
    Provisions Relating to IETF Documents
    (http://trustee.ietf.org/license-info) in effect on the date of
    publication of this document.  Please review these documents
    carefully, as they describe your rights and restrictions with respect
    to this document.

<snip>

> Any thoughts on that by anybody?

I believe we would need scoped (or zoned) addresses *if* there are 
deployment scenarios where an NTP speaker is associated with multiple 
peers in different limited-scoped networks.  For example, can I have one 
NTP association with node1 via a site-local IPv6 multicast address in a 
VPN connection and another association with node2 via the same 
site-local IPv6 multicast address over a different interface?

If the answer is "yes", then we need to support the scoped (zoned) 
addresses.

Regards,
Brian