Definitions of Managed Objects for Network Time Protocol Version 4 (NTPv4)
RFC 5907

[Ballot comment] 1. Most DisplayString objects have been changed into Utf8String objects, but there are still a few objects where they use DisplayString.I understand ...

[Ballot comment]
1. Most DisplayString objects have been changed into Utf8String objects, but there are still a few objects where they use DisplayString.I understand that in these cases there will (most probably) never be international text, so we can live with this. On the other hand, I do not see what the problem would be to also make those utf8 based. If the content is ASCII, then basically that is exactly the same, no matter if it is repreented in UTF8 or ASCII.

2. Now that they use Utf8String (from RFC 2287), I think that RFC2287 should be added as a normative reference.

3. It would be nice to acknowledge the contribution of the MIB Doctor Bert Wijnen.

[Ballot discuss] Thank you for addressing the bulk of my Discuss issues. One remains. Maybe you want to discuss this in email? Discuss 4 In ...

[Ballot discuss]
Thank you for addressing the bulk of my Discuss issues.

One remains. Maybe you want to discuss this in email?

Discuss 4

In general, it seems odd to have DisplayString and Integer variants
of many objects. It causes the implementer to have to handle the case
when the objects do not agree.

In some cases, this is particluarly bad. For example, why is
ntpEntStatusCurrentMode a DisplayString with a well-known set of
legitimate values? Surely this gives rise to potential discrepencies.
Doesn't ntpEntStatusCurrentModeVal give you everything you need?

[Ballot discuss] As noted, by several others, the guidelines in http://www.ops.ietf.org/mib-security.html were not used to develop the security considerations ...

[Ballot discuss]
As noted, by several others, the guidelines in http://www.ops.ietf.org/mib-security.html
were not used to develop the security considerations section.  I believe there are plausible
attacks based on changes to the notification values, and some of the read-only information
may be useful in designing a successful attack (e.g., product name and software version).
(It is less clear to me if the objects in sections 2 and 3, "Current NTP status" and "The status
of all currently mobilized associations" are of utility to an attacker.)

These issues should be documented consistent with the guidelines referenced above.

[Ballot discuss] The DISCUSS below is based in part on the MIB Doctor review performed by Bert Wijnen. This review was sent to the NTP ...

[Ballot discuss]
The DISCUSS below is based in part on the MIB Doctor review performed by Bert Wijnen. This review was sent to the NTP WG list, but was never answered and the document was not revised.

1. The MIB module does not compile clean.

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 }
which will fix it.


2. There are two writable objects for which there is no text in the DESCRIPTION clauses about expected persistency behaviour

3. There are Counter32 objects with no test in the DESCRIPTION clauses about possible discontinuities and which timer object would indicate such.
Specifically, the fact that 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)

4. There is a lot of commented text in the MIB module which would rather be included in the DESCRIPTION clauses of the objects (the information that is contained seems usefeul)

5. The design of duplicate MIB objects with similar semantics in both text and numerical formats is broken. I suggest to eliminate ntpEndTimeResolution and ntpEntTimePrecision and rather define UNITS clauses for the numerical objects, or units objects if this is not possible. Similarly I would give ntpEntStatusCurrentMode up and keep just ntpEntStatusCurrentModeVal. The text objects are not too useful, are not interoperable, and they may be derived easily by a management station if needed. Duplication also can lead to inconsistency (what would a manager do if the two values are not consistent?)

6. ntpEntSoftName, ntpEntSoftwareVersion duplicate functionality from the Entity MIB

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

8. The Copyright statement in the DESCRIPTION clause of the MODULE_IDENTITY is missing

9. The statement that this revision of the document is published as RFC xxxx
  (xxxx to be filled in by RFC-editor)
is missing

10. There are severa 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.


11. 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  the current practice in MIB modules. Also makes the OIDs of notifications shorter

12. 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 }

13. There are 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

    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 the text says that the group is optional while it listed 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 the MIB-Module.

14. Several references/citations look like:

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.  In genreal though, it is 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"

15. 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 }

It's better to use UNITS clauses. 

Same for all Counter32 objects - defining UNITS clauses would be very helpful.

16. DEFVAL does not make sense for read-only objects.

17.
  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 }

How comes that the same DESCRIPTION clause is defined for each of these?

18.

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 }

RFC4001 states that the use of InetAddress SYNTAX, MUST have a DESCRIPTION clause that explains/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 it is better to do 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.

19. There are 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 throtle or an enable/disable switch.

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

21. draft-ietf-ntp-ntpv4-proto-11 (version 4 of NTP)should also be a normative reference

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

[Ballot discuss] Discuss 1 The document does not state which track this is on. I can find nothing in the write-up either. Discuss 2 http ...

[Ballot discuss]
Discuss 1

The document does not state which track this is on.
I can find nothing in the write-up either.

Discuss 2

http://www.ops.ietf.org/mib-security.html describes the security text
that should be present. The security section seems particularly light.

Discuss 3
                                                                             
The copyright appears to be missing from within the MIB module.

Discuss 4

In general, it seems odd to have DisplayString and Integer variants
of many objects. It causes the implementer to have to handle the case
when the objects do not agree.

In some cases, this is particluarly bad. For example, why is
ntpEntStatusCurrentMode a DisplayString with a well-known set of
legitimate values? Surely this gives rise to potential discrepencies.
Doesn't ntpEntStatusCurrentModeVal give you everything you need?

Discuss 5

Several read-only objects have DEFVAL clauses. This has no meaning!

Discuss 6

Shouldn't ntpEntStatusEntityUptime use a TimeStamp syntax?

[Ballot comment] Comment 1 The MIB module guidelines say that you should include boilerplate for RFC 2119. However, there is no such language used (which ...

[Ballot comment]
Comment 1

The MIB module guidelines say that you should include boilerplate
for RFC 2119. However, there is no such language used (which is a
bit surprising) so perhaps the boilerplate is not needed.

Comment 2

It would be nice if the modules included by the IMPORTS clause
were identified by RFC name (in comments) and included as
normative references.

Comment 3

I think the longish change log comment should be removed before
publication as an RFC.

Comment 4

You should update the REVISION clause so that the published RFC
is the first revision of this MIB module.

Comment 5 - Nits

Abstract
s/independant/independent/
s/Internet draft/document/

Comment 6

Instead of stating "Ref: draft-ietf-ntp-ntpv4-proto-06" within
a DESCRIPTION clause, you should include a REFERENCE clause.

It might be good if you included a request to the RFC Editor
so that REFERENCE clauses citing draft-ietf-ntp-ntpv4-proto-06
will be updated to indicate the RFC number of that work which
we assume will go forward in lock-step.

[Ballot discuss] The following comment was raised by Ben Campbell in his Gen-ART Review, and no response has been received. Please respond to the comment ...

[Ballot discuss]
The following comment was raised by Ben Campbell in his Gen-ART
  Review, and no response has been received.  Please respond to the
  comment.  Ben said:

  The comment says "shows the resolution based on 1 second, e.g. "1ms"
  translates to 1000". That confused me at first, since 1ms != 1000 s.
  I suspect you are talking about the number of divisions in a second, 
  in which 1000 would make sense--but I didn't immediately get that 
  from the text.