Re: [dhcwg] Benjamin Kaduk's Discuss on draft-ietf-dhc-dhcpv6-yang-24: (with DISCUSS and COMMENT)

[Timothy Winters <[e-mail]>]

Hi Ian,



On Tue, Feb 1, 2022 at 10:07 AM <[e-mail]> wrote:

> Hi Tim,
>
> Thanks for your reply. There’s one clarification question in line below.
> I’ve changed the other 2 types to binary per your suggestion.
>
> Thanks,
> Ian
>
> On 21. Jan 2022, at 14:00, Timothy Winters <[e-mail]> wrote:
>
> Hi Ian,
>
> I've responded below anywhere @Bernie/Tim.  Thanks for you all your work
> on this.
>
> ~Tim
>
> On Mon, Jan 17, 2022 at 12:39 PM <[e-mail]> wrote:
>
>> Hi Benjamin,
>>
>> First of all, many thanks for the detailed review and my apologies that
>> it’s taken so long for me to
>> reply.
>>
>> Please see inline below.
>>
>> @Bernie/@Tim, there are a couple of points below that I would appreciate
>> your input on (specifically, restructuring of the OPTION_AUTH modelling and
>> the best type for  interface-id-option-group and user-class-option-group to
>> use).
>>
>> Thanks,
>> Ian
>>
>>
>> > On 15. Dec 2021, at 01:07, Benjamin Kaduk via Datatracker <
>> [e-mail]> wrote:
>> >
>> > Benjamin Kaduk has entered the following ballot position for
>> > draft-ietf-dhc-dhcpv6-yang-24: Discuss
>> >
>> > When responding, please keep the subject line intact and reply to all
>> > email addresses included in the To and CC lines. (Feel free to cut this
>> > introductory paragraph, however.)
>> >
>> >
>> > Please refer to
>> https://www.ietf.org/blog/handling-iesg-ballot-positions/
>> > for more information about how to handle DISCUSS and COMMENT positions.
>> >
>> >
>> > The document, along with other ballot positions, can be found here:
>> > https://datatracker.ietf.org/doc/draft-ietf-dhc-dhcpv6-yang/
>> >
>> >
>> >
>> > ----------------------------------------------------------------------
>> > DISCUSS:
>> > ----------------------------------------------------------------------
>> >
>> > Roman's comment touched on a related point, but I'd like to (hopefully
>> > briefly) discuss the way we encode certain opaque protocol fields.
>> > There are some places where we clearly intend a hex representation (a
>> > string with a pattern that's marked out as pairs of hex digits), but
>> > there are others where we just say "type string" with no indication of
>> > encoding, and even a "type binary".  If we want the specification to
>> > admit interoperable implementation we need to be more clear about what
>> > encoding we expect for all of these nodes.  I have noted most (possibly
>> > all, but please double check) in the COMMENT section, with a preference
>> > for "type binary" where we don't need to apply regex restrictions.  But
>> > that's just a personal preference, and choosing to use hex (or base64,
>> > or any other well-defined) encoding will suffice to resolve the discuss
>> > point.
>>
>> [if - I’ve changed from type string to binary where-ever possible
>> throughout according
>> To the specific comments below. The one exception is for the auth-option,
>> which
>> Allows for different formats depending on the auth protocol in use. I’ve
>> reworked the
>> Format of the option to make this explicit.]
>>
>> >
>> >
>> > ----------------------------------------------------------------------
>> > COMMENT:
>> > ----------------------------------------------------------------------
>> >
>> > Thanks to the shepherd for answering the second part of question (1) in
>> > the template, as well as the first; this part of the question is often
>> > missed.  (There is, however, a third part as well...)
>> >
>> > Section 1.2.1
>> >
>> > Table 1 is described as showing "the DHCPv6 options that are modeled,
>> > the element(s) they are sent by, and the relevant YANG module name".  In
>> > my interpretation that means that:
>> >
>> > - the only options that are in the table are ones that are modeled
>> > - the contents of the table shows which elements send those options
>> >
>> > But the contents of the table don't quite seem to match that -- it seems
>> > that they only indicate which nodes have YANG modeling for how to send
>> > the option, and not indicate nodes that send the option but do not need
>> > YANG configuration to control how to populate the option.  (E.g.,
>> > OPTION_AUTH and OPTION_USER_CLASS.)  If this is the intent, then perhaps
>> > it's clearer to say "which YANG modules they are modeled for" or
>> > something similar, rather than "the element(s) they are sent by".
>>
>> [if - Changed]
>>
>>
>> >
>> > Section 3.1
>> >
>> >   *  address/prefix-pool-utilization-threshold-exceeded: Raised when
>> >      the number of leased addresses or prefixes exceeds the configured
>> >      usage threshold.
>> >
>> > The node that seems to be where the "configured usage threshold" is
>> > configured is scoped to an address pool or prefix pool.  Should this
>> > description say "number of leased [...] in an address pool exceeds the
>> > configured usage threshold"?
>>
>> [if - I’ve changed this to read:
>> "address/prefix-pool-utilization-threshold-exceeded: Raised when the
>> number
>> of leased addresses or prefixes in a pool exceeds the configured usage
>> threshold."
>>
>> >
>> > Section 4.1
>> >
>> >         +------+------+----------+--------------+
>> >         | 0001 | 0006 | 28490058 | 00005E005300 |
>> >         +------+------+----------+--------------+
>> >
>> >         This example includes the 2-octet DUID type of 1 (0x01), the
>> >         hardware type is 0x06 (IEEE Hardware Types) the creation
>> >         time is 0x028490058 (constructed as described above). Finally,
>> >         the link-layer address is 0x5E005300 (EUI-48 address
>> >         00-00-5E-00-53-00)";
>> >
>> > Should there be some more zeros in here (e.g., in the artwork before
>> > 28490058)?
>>
>> [if - The creation time is a 4-octet field, so the artwork is correct.
>> I’ve
>> changed the value in the description to 0x28490058]
>>
>> >
>> >     typedef duid-en {
>> >       type duid-base {
>> >         pattern '0002'
>> >           + '[0-9a-fA-F]{4,}';
>> >       }
>> >       description
>> >         "DUID type 2, assigned by vendor based on Enterprise
>> >         Number (DUID-EN). This DUID consists of the 4-octet vendor's
>> >         registered Private Enterprise Number as maintained by IANA
>> >         followed by a unique identifier assigned by the vendor. For
>> >         example:
>> >
>> > A 4-octet PEN would be 8 hex digits (the YANG allows as few as 4).
>>
>> [if - Changed to 8]
>>
>> >
>> >     typedef duid-unstructured {
>> >       type duid-base {
>> >         pattern '(000[1-4].*|.*[^0-9a-fA-F].*)' {
>> >           modifier invert-match;
>> >         }
>> >       }
>> >
>> > My understanding is that the "pattern" modifiers from the base type and
>> > the derived type are "and"-ed together, so the "|.*[^0-9a-fA-F].*"
>> > clause is redundant with the pattern in the base type and thus not
>> > needed.
>>
>> [if - Changed to
>> pattern '(000[1-4].*)']
>>
>> >
>> >       container status {
>> >       [...]
>> >         leaf message {
>> >           type string;
>> >           description
>> >             "A UTF-8 encoded text string suitable for display to an
>> >             end user. It MUST NOT be null-terminated.";
>> >
>> > (Related to Roman's comment)
>> > This is probably more of a comment on RFC 8415 than this document, but
>> > BCP 18 seems pretty clear that a string destined for display to a human
>> > should be accompanied by a language indicator.
>> > (This comment probably applies to every "leaf message" but I will just
>> > mention it this once.)
>>
>> [if - I’m not sure how we can handle this as the leaf(s) is only used for
>> Holding RO state data which it obtains from the client/relay/server
>> implementation. As
>> You mention, RFC8415 does not place any requirements on the message to
>> contain
>> a language identifier.]
>>
>> >
>> >     grouping auth-option-group {
>> >     [...]
>> >       container auth-option {
>> >       [...]
>> >         leaf auth-information {
>> >           type string;
>> >           description
>> >             "The authentication information, as specified by the
>> >             protocol and algorithm used in this Authentication
>> >             option.";
>> >
>> > This should probably be binary, not a string.  Or, if it needs to be a
>> > string, it should specify an encoding (e.g., hex).
>>
>> [if - The authentication information format is dependant on the value
>> Of the protocol field. I’ve re-modelled this option to take this into
>> account.
>>
>> The configuration-token (RFC3118) does allow for plain text and
>> specifically
>> Gives a clear text password as an example use case.
>>
>> @Bernie, @Tim, please can you review this an make sure that I’ve
>> Understood DHCPv6 auth properly?
>>
> [tim] - I think it can be any data based on the protocol field.  So maybe
> this should be a binary?
>
>
> [if - To be clear, in the updated model that I proposed below, there are
> two fields that contain auth information, one for conf-token
> (which was typed as string) and one for rkap (which is binary).
>
> I’ve changed both to type binary. Was this your intention?]
>
[tw] - Yes that was my intention.

>
>
>>
>>
>>   grouping auth-option-group {
>>     description
>>       "OPTION_AUTH (11) Authentication Option.";
>>     reference "RFC 8415: Dynamic Host Configuration Protocol
>>       for IPv6 (DHCPv6), Section 21.11
>>
>>       RFC 3118: Authentication for DHCP Messages
>>       IANA 'Dynamic Host Configuration Protocol (DHCP) Authentication
>>       Option Name Spaces' registry.
>>       <https://www.iana.org/assignments/auth-namespaces>";
>>     container auth-option {
>>       description
>>         "OPTION_AUTH (11) Authentication Option container.";
>>       leaf algorithm {
>>         type uint8;
>>         description
>>           "The algorithm used in the authentication protocol.";
>>       }
>>       leaf rdm {
>>         type uint8;
>>         description
>>           "The Replay Detection Method (RDM) used in this
>>           Authentication option.";
>>       }
>>       leaf replay-detection {
>>         type uint64;
>>         description
>>           "The replay detection information for the RDM.";
>>       }
>>       choice protocol {
>>
>>         description
>>           "The authentication protocol used in the option. Namespace
>>           values 1 (delayed authentication) and 2 (Delayed
>>           Authentication (Obsolete) are not applicable and so are
>>           not modelled.";
>>         case conf-token {
>>           leaf token-auth-information {
>>           type string;
>>           description
>>
>>             "Protocol Namespace Value 0. The authentication
>>
>>             information, as specified by the protocol and
>>             algorithm used in this Authentication option.";
>>           }
>>         }
>>       case rkap {
>>
>>         description
>>           "Protocol Namespace Value 3. RKAP provides protection
>>           against misconfiguration of a client caused by a Reconfigure
>>           message sent by a malicious DHCP server.";
>>           leaf datatype {
>>             type uint8 {
>>               range "1 .. 2";
>>             }
>>             description
>>               "Type of data in the Value field carried in this
>>               option.
>>                 1  Reconfigure key value (used in the Reply
>>                    message).
>>                 2  HMAC-MD5 digest of the message (used in
>>                    the Reconfigure message).";
>>           }
>>           leaf auth-info-value {
>>
>>             type binary {
>>
>>               length 16;
>>
>>             }
>>           description "Data as defined by the Type field.  A 16-octet
>>             field.";
>>           }
>>         }
>>       }
>>     }
>>   }
>>
>> ]
>>
>> >
>> >     grouping status-code-option-group {
>> >
>> > FWIW, this grouping seems unused by the rest of the document.  (Perhaps
>> > a remnant of a decision to move the status information out of the
>> > options section and into a higher level of the relevant YANG trees?)
>>
>> [if - Yes, this was an unused remanent. Removed]
>>
>> >
>> >             leaf sub-option-data {
>> >               type string {
>> >                 pattern '([0-9a-fA-F]{2}){0,}';
>> >               }
>> >               description
>> >                 "The data area for the sub-option.";
>> >
>> > Why does this need to be hex-encoded (vs. YANG binary)?
>> > And, if it is hex-encoded, we should probably say so explicitly in the
>> > description.
>>
>> [if - Changed to binary]
>>
>> >
>> > Section 4.2
>> >
>> >       leaf preferred-lifetime {
>> >         type dhc6:timer-seconds32;
>> >         description
>> >           "Preferred lifetime for the Identity Association (IA).";
>> >         reference "RFC 8415: Dynamic Host Configuration Protocol for
>> >           IPv6 (DHCPv6), Section 6";
>> >
>> > Is Section 6 really the best section reference for this concept?
>>
>> [if - Changed to Section 12.1 (also changed for the valid-lifetime leaf)]
>>
>> >
>> >     grouping message-stats {
>> >
>> > The relay module has a counter for discarded messages; would a similar
>> > counter make sense here?  (Also for the server module, I think.)
>>
>> [if - Added - also for the client module.]
>>
>> >
>> > Also, YANG does have dedicated "counter" types to more precisely
>> > indicate semantics than just "uint32".  They are by definition
>> > monotonically increasing, though, and are their use is typically
>> > accompanied by a "last discontinuity time" node to indicate when they
>> > have been reset.
>>
>> [if - Changed to yang:counter32 throughout and added a discontinuity-time
>> node to server/relay/client modules]
>>
>> >
>> >     grouping info-refresh-time-option-group {
>> >       [...]
>> >         leaf info-refresh-time {
>> >           type dhc6:timer-seconds32;
>> >           description
>> >             "Time duration relative to the current time, expressed
>> >             in units of seconds.";
>> >
>> > Is this really "relative to the current time" for purposes of the YANG
>> > module?  This is the description that RFC 8415 uses, yes, but that
>> > refers to the current time when the protocol message is received, and
>> > YANG interactions may be asynchronous to that.
>>
>> [if - Changed to read:
>>           "Time duration specifying an upper bound for how long a
>>           client should wait before refreshing information retrieved
>>           from a DHCP server.”;  ]
>>
>> >
>> >           container address-pools {
>> >           [...]
>> >             list address-pool {
>> >               key pool-id;
>> >               [...]
>> >               leaf pool-id {
>> >                 type string;
>> >
>> > What's the motivation for making the pool-id a string vs the option-set
>> > and allocation-range lists that have an integer identifier?  (Also
>> > applies to prefix pools.)
>>
>> [If - Initially all of the identifiers were integers. The change to string
>>
>> Specifically for Pool-id was made by Éric during the AD review.
>>
>> Agree that it makes sense to use the same format, so the option-set-id
>> And allocation-range id are now type string.]
>>
>> >
>> >               leaf pool-prefix {
>> >                 type inet:ipv6-prefix;
>> >                 mandatory true;
>> >                 description
>> >                   "IPv6 prefix for the pool.";
>> >
>> > Does this prefix need to be contained within the overall
>> > "network-prefix"?  (Same question for prefix-pools' pool-prefix.)
>>
>> [if - With the DHCP server implementations that I am familiar with, yes
>> it would
>> need to be contained, but I guess other implementations may not. I’m not
>> aware
>> of any way of enforcing this in the YANG.]
>>
>> >
>> >               leaf max-address-utilization {
>> >                 type dhc6:threshold;
>> >                 description
>> >                   "Maximum amount of the addresses in the
>> >                   pool which can be simultaneously allocated,
>> >                   calculated as a percentage of the available
>> >                   addresses (end-address minus start-address plus
>> >                   one).";
>> >
>> > Do we want to mention the relationship between this node and the
>> > "address-pool-utiliziation-threshold-exceeded" notification?  I see the
>> > pointer the other direction, but since we don't use the word "threshold"
>> > in the description here I wouldn't mind a more explicit linkage.
>> > (Likewise for the prefix-allocation case.)
>>
>> [if - I’ve updated the description with:
>> " Used to set the value for the
>> address-pool-utilization-threshold-exceeded
>>  notification”;
>>
>> And the equivalent change for prefix-allocation.]
>>
>> >
>> >                 list active-lease {
>> >                   key leased-prefix;
>> >                   description
>> >                     "List of active prefix leases.";
>> >                   leaf leased-prefix {
>> >                     type inet:ipv6-prefix;
>> >                     description
>> >                       "Active leased prefix entry.";
>> >
>> > Is the prefix length available from some other information in the tree?
>> > If not, should it be listed here?
>>
>> [if I’m not sure I understand the comment. The prefix pool has the
>> ‘Client-prefix-length’ leaf which defines the length of prefixes that
>> are offered for the pd-pool.
>>
>> However, client’s may hint that they want a different prefix length in
>> TOheir pd request and depending on implementation / policy the server
>> May honour this (for a longer prefix than the defined
>> client-prefix-length).
>> RFC8168 discusses this.]
>>
>> >
>> >     notification decline-received {
>> >       if-feature na-assignment;
>> >       description
>> >         "Notification sent when the server has received a
>> >         Decline (9) message from a client.";
>> >
>> > Is this a potential DoS vector to the notification recipient (if a
>> > malicious client just starts declining everything)?  Should we say
>> > anything about rate-limiting?
>>
>> [if - From the perspective of the YANG module and its implementation,
>> I don’t think so. The reason is that state/counter data is accessed via
>> the YANG
>> Operational data store and this is only retrieved from the process /
>> function
>> Being managed when a NETCONF request Is made for the information,
>> so that it is as current as possible.
>>
>> This is for every YANG module implementation that we\ve done, but I think
>> this is general best practice.]
>>
>> >
>> >     notification non-success-code-sent {
>> >       description
>> >         "Notification sent when the server responded
>> >         to a client with non-success status code.";
>> >
>> > (similarly here)
>>
>> [if - See above]
>>
>> >
>> > Section 4.3
>> >
>> >     grouping interface-id-option-group {
>> >       [...]
>> >        leaf interface-id {
>> >           type string;
>> >           description
>> >             "An opaque value of arbitrary length generated by the
>> >             relay agent to identify one of the relay agent's
>> >             interfaces.";
>> >
>> > I think this is better as a YANG "binary" type than a string.  If not,
>> > we should say something about the encoding.
>>
>> [if - I’m familiar with the use of this option in Cisco and ALU relay
>> Implementations and both allow clear text configuration of the
>> value that is carried in this field.
>>
>> RFC8415 doesn’t give any specific guidance on whether this
>> Should be ASCII or can be UTF-8.
>>
>> RFC7227 Section 5.8 does state that future options should use UTF-8,
>> But this option would have been defined before RFC7227 was published.
>>
>> @Bernie/@Tim, do you have any guidance on this?]
>>
> [tim] -  According to RFC 8415 this is an opaque value, therefore it can
> be anything so I think binary makes sense.
>
>
> [if - Changed]
>
>
>> >
>> > Section 4.4
>> >
>> > Why do we use "prefix-del" for the feature name in this module but the
>> > full "prefix-delegation" in the other modules?
>>
>> [if - now using prefix-delegation throughout]
>>
>> >
>> >     grouping message-statistics {
>> >
>> > The relay module has a counter for discarded messages; should we?
>>
>> [if - Added]
>>
>> >
>> >     grouping option-request-option-group {
>> >       description
>> >         "OPTION_ORO (6) Option Request Option. A client MUST include
>> >         an Option Request option in a Solicit, Request, Renew,
>> >            Rebind, or Information-request message to inform the server
>> >              about options the client wants the server to send to the
>> >              client.";
>> >
>> > If I understand correctly, there are further MUST-level requirements for
>> > what options must be present in the ORO.  E.g., INFORMATION_REFRESH_TIME
>> > is required in Information-Request.  Do we want to mention that here?
>> > Are there any considerations for how that requirement interacts with the
>> > values configured by YANG?  E.g., do we expect implementations to
>> > forcibly add those required options on top of the configured ones, in
>> > scenarios where they are required?
>>
>> [if - Description now updated to:
>>           "List of options that the client is requesting,
>>           identified by option code. This list MUST include the
>>           code for option SOL_MAX_RT (82) when included in a
>>
>>           Solicit-message. If this option is being
>>           sent in an Infoformation-request message, then the code
>>           for option OPTION_INFORMATION_REFRESH_TIME (32) and
>>
>>           INF_MAX_RT (83) MUST be included.”;
>>
>> The references also updated for the sections of RFC8415 defining the
>> requirements.]
>>
>> >
>> >     grouping user-class-option-group {
>> >       [...]
>> >           leaf user-class-data {
>> >             type string;
>> >             description
>> >               "Opaque field representing a User Class
>> >               of which the client is a member.";
>> >
>> > I think this would be better as YANG binary than string.  If we keep it
>> > as string, we need to say what encoding is used.
>>
>> >
>> >       container vendor-class-option {
>> >         [...]
>> >         list vendor-class-option-instances {
>> >         [...]
>> >           list vendor-class-data-element {
>> >            [...]
>> >             leaf vendor-class-data {
>> >               type string;
>> >               description
>> >                 "Opaque field representing a vendor class of which
>> >                 the client is a member.";
>> >
>> > (likewise here)
>>
>> [if - IME these are usually a user configurable text string on client /
>> server implementations.
>>
>> I think the same comment for the format as for Section 4.3
>> interface-id-option group
>> Applies here.
>>
>> @Bernie/@Tim, any thoughts?]
>>
> [tim] - Since anything can be put in this field as a opaque value, I think
> this a binary for yang model.
>
>
> [if - Changed.]
>
>
>> ]
>> >
>> >       leaf client-duid {
>> >         if-feature "non-temp-addr or prefix-del " +
>> >           "or temp-addr and not anon-profile";
>> >
>> > Does the default YANG operator precedence do what we want with respect
>> > to ((non-temp-addr or prefix-del or temp-addr) and not anon-profile) vs
>> > (non-temp-addr or prefix-del or (temp-addr and not anon-profile))?
>> >> From the context in the rest of the document, it's clear that we want
>> > the former, but I'm not familiar enough with YANG minutiae to say if
>> > that's what we actually are specifying.
>>
>> [if - Changed to "(non-temp-addr or prefix-delegation or temp-addr) and
>> not anon-profile”.
>>
>> Per RFC7950, groupings in parentheses are evaluated first, then the ‘not'
>> statement.]
>>
>> >
>> >     notification invalid-ia-address-detected {
>> >       if-feature "non-temp-addr or temp-addr";
>> >       [...]
>> >       leaf ia-id {
>> >         type uint32;
>> >         mandatory true;
>> >         description
>> >           "IA-ID";
>> >
>> > If I understand correctly, in DHCP the IA-ID is scoped per client DUID.
>> > Does this notification convey enough information to disambiguate what
>> > scope this IA-ID value belongs to?
>>
>> [if - The IA-ID is a 4-octet field which is present in options requesting
>> an address or prefix (OPTION_IA_NA/IA_TA/IA_PD).
>> The client will normally have a single DUID for all of it’s requests on
>> all DHCP
>> Enabled interfaces, but could make requests for multiple
>> addresses/prefixes in
>> each DHCP request. In this case each IA_xA option would have the same
>> DUID, and a unique IA-ID field so the
>> Client can associate the allocated address/prefix from the server to the
>> specific request option.]
>>
>> >
>> >       [...]
>> >       leaf ia-na-t1-timer {
>> >         type uint32;
>> >         description
>> >           "The value of the T1 time field for non-temporary address
>> >           allocations (OPTION_IA_NA).";
>> >       }
>> >       leaf ia-na-t2-timer {
>> >         type uint32;
>> >         description
>> >           "The value of the preferred-lifetime field for non-temporary
>> >           address allocations (OPTION_IA_NA).";
>> >
>> > Should these two be here without a feature conditional?  They seem to be
>> > NA-specific but we could send this notification if NA is not
>> > implemented.
>>
>> [if - notification invalid-ia-address-detected has if-feature
>> non-temp-addr or temp-addr which
>> Is applicable to all of the nodes defined in the notification.
>>
>> The notification can only be triggered if an address is requested and
>> allocated then found to
>> Be invalid, e.g. it fails DAD, so this couldn’t happen if the client
>> doesn’t implement IA_NA/IA_TA
>> (And has the relevant feature enabled).]
>>
>> >
>> >     notification transmission-failed {
>> >         [...]
>> >       leaf failure-type {
>> >         type enumeration {
>> >
>> > (side note?) I get antsy about not leaving an extension point for other
>> > types of (re)transmission failure, but I also don't have any specific
>> > scenario in mind that's not already covered, so.
>>
>> [if - I think that all of the client message transmission failures can be
>> boiled down to
>> ‘I tried/retried sending this type of message and didn’t get an answer’
>> which is what’s
>> Covered in the notification. We could add an other/unspecified to the
>> enum, but I also
>> Can’t think of what would trigger this.]
>>
>> >
>> >     notification unsuccessful-status-code {
>> >       description
>> >         "Notification sent when the client receives a message that
>> >         includes an unsuccessful Status Code option.";
>> >
>> > As for the other notifications on failure, is this a potential DoS
>> > vector?
>>
>> [if - I don’t think so. For Netconf notifications, the network management
>> system
>> Creates a session and subscribes to the notifications from the DHCP
>> client.
>> If one (or many) DHCP clients were generating too many notifications,
>> then the NMS would
>> Close the session and stop receiving the DHCP client’s notifications.]
>>
>> >
>> > Section 5
>> >
>> > It's probably worth mentioning the risk of notifications turning into a
>> > DoS vector (absent rate-limiting) here.
>>
>> [if - This is surely a problem with Netconf notifications overall, rather
>> than specifically
>> For the DHCP modules? I’ve checked through RFC5277 (Netconf event
>> notifications) and
>> There is nothing on rate-limiting messages or in the Security
>> Considerations section
>> on this subject.]
>>
>> >
>> > Can a misconfigured client cause problems for a (honest) server, e.g.,
>> > by sending a lot of requests for a lot of things, very quickly?
>>
>> [if - I’m sure that it would be possible. RFC8415 only places requirements
>> On clients to limit the rate for messages that they send, but not on a
>> server
>> To limit the rate of received messages it will try and process.
>>
>> >From a little research, I see that some DHCP server implementations have
>>
>> configuration for this, but others do not. It’s feasible that this could
>> also be
>> Done through e.g. ip6tables rules on a host.
>>
>> I would see this as being a feature that should be configured (if
>> available) through the
>> Implementation specific YANG module or possibly the ACL YANG (RFC8519)]
>>
>> >
>> >   All data nodes defined in the YANG modules which can be created,
>> >   modified, and deleted (i.e., config true, which is the default) are
>> >   considered sensitive.  Write operations (e.g., edit-config) to these
>> >   data nodes without proper protection can have a negative effect on
>> >   network operations.
>> >
>> > Do we want to go further and give some broad treatment of how the
>> > negative effects would come about (e.g., by disrupting address
>> > allocation and the routability of addresses/prefixes that clients
>> > attempt to use)?
>> >
>> >   An attacker with read/write access the DHCPv6 relay can undertake
>> >   various attacks, such as:
>> >
>> >   *  Modifying the relay's "destination-address" to send messages to a
>> >      rogue DHCPv6 server.
>> >
>> >   *  Deleting information about a client's delegated prefix, causing a
>> >      denial of service attack as traffic will no longer be routed to
>> >      the client.
>> >
>> > I'd considering reiterating the "full denial of service" capability
>> > (which is currently implicit in "send messages to a rouge DHCPv6 server"
>> > combined with the list of attacks that a compromised server can
>> > undertake).
>>
>> [if - I’ve updated the examples as follows:
>>
>> Server:
>>
>>         Denial of service attacks, such as disabling the DHCP
>>           server sevice, or removing address/prefix pool
>>
>>           configuration.
>>
>>         Various attacks based on re-configuring the contents
>>           of DHCPv6 options, leading to several types of security or
>>           privacy threats.  For example, changing the address of a
>>
>>           DNS server supplied in a DHCP option to point
>>           to a rogue server.
>>
>>
>> And for the relay
>>
>>         Denial of service attacks, based on disabling the
>>           DHCP relay function, or modifying the relay's
>>           "destination-address" to a non-existant address.
>>
>>
>>         Modifying the relay's "destination-address" to send
>>           messages to a rogue DHCPv6 server.
>>
>>         Deleting information about a client's delegated
>>           prefix, causing a denial of service attack as traffic
>>           will no longer be routed to the client.
>>
>> ]
>>
>>
>> >
>> >   Some of the readable data nodes in this YANG module may be considered
>> >   sensitive or vulnerable in some network environments.  Therefore, it
>> >   is important to control read access (e.g., only permitting get, get-
>> >   config, or notifications) to these data nodes.  These subtrees and
>> >   data nodes can be misused to track the activity of a host:
>> >
>> > First off, thank you for stating this consideration so clearly.
>> >
>> > Second, there may be an additional consideration for read-only access --
>> > knowledge of what pools of addresses are available for allocation might
>> > facilitate network reconaissance (viz. RFC 7707).
>>
>> [if - I’ve added the following text:
>>
>>    Information about a server's configured address and prefix pools may
>>    be used by an attacker for network reconnaissance [RFC7707].  The
>>    following subtrees and data nodes could be used for this purpose:
>>
>>    *  Information about client address allocation ranges: (dhc6-srv/
>>       allocation-ranges/allocation-range/address-pools/ address-pool/
>>       pool-prefix)
>>
>>    *  Information about client prefix allocation ranges: (dhc6-srv/
>>
>>       allocation-ranges/allocation-range/prefix-pools/ prefix-pool/pool-
>>       prefix)
>> ]
>>
>> >
>> >   [RFC7824] covers privacy considerations for DHCPv6 and is applicable
>> >   here.
>> >
>> > I'd suggest mentioning the RFC 7844 privacy profile as a partial
>> > mitigation that is sometimes applicable.
>>
>> [if - Do you mean that RFC7844 can provide partial mitigation of the
>> server
>> Privacy issues in RFC7824?]
>>
>> >
>> > Section 9.2
>> >
>> > RFC 8987 is used as in a few YANG reference stanzas, but is not
>> > mentioned in the preface before the containing YANG module.  It could
>> > arguably be classified as normative based on that usage.
>>
>> [if - I’ve added
>> "The RPCs in the module are taken from requirements defined [RFC8987].”
>>
>> To the preface before the relay tree diagram. RFC8987 is now normative.]
>>
>> >
>> > Appendix D
>> >
>> >         case user-class-option-id {
>> >           description
>> >             "Client class selection based on the value of the
>> >             OPTION_USER_CLASS(15) and its user-class-data field.";
>> >           leaf user-class-data {
>> >             type string;
>> >             mandatory true;
>> >             description
>> >               "Value of the enterprise-number field.";
>> >
>> > This description doesn't look right.
>>
>> [if - Changed to “User Class value to match”]
>>
>> >
>> > NITS
>> >
>> > We might check the spelling of "grouping message-stats" (vs "grouping
>> > message-statistics") across modules.
>>
>> [if - now uses ‘message-statistics’ throughout]
>>
>> >
>> > Section 4.2
>> >
>> >       leaf rapid-commit {
>> >         type boolean;
>> >         description
>> >           "When set to 'true', Specifies that the pool supports
>> >           client-server exchanges involving two messages.";
>> >
>> > This is in the "grouping resource-config" that may or may not be used
>> > within a "pool" container.  A more generic phrasing might be in order.
>>
>> [if - Changed to
>> "When set to 'true', Specifies that client-server exchanges involving
>> two messages is supported.”;]
>>
>> >
>> >     grouping sol-max-rt-option-group {
>> >       [...]
>> >         leaf sol-max-rt-value {
>> >           type dhc6:timer-seconds32;
>> >           description
>> >             "sol max rt value";
>> >
>> > That's a pretty sparse description.
>>
>> [if - Changed to:
>> "Maximum Solicit timeout value.”]
>>
>> >
>> >     grouping inf-max-rt-option-group {
>> >       [...]
>> >         leaf inf-max-rt-value {
>> >           type dhc6:timer-seconds32;
>> >           description
>> >             "inf max rt value";
>> >
>> > (ditto)
>>
>> [if - Changed to:
>> "Maximum Information-request timeout value.”]
>>
>> >
>> >               leaf max-address-utilization {
>> >                 type dhc6:threshold;
>> >                 description
>> >                   "Maximum amount of the addresses in the
>> >                   pool which can be simultaneously allocated,
>> >                   calculated as a percentage of the available
>> >                   addresses (end-address minus start-address plus
>> >                   one).";
>> >
>> > The analogous prefix-allocation node has "rounded up" stated
>> > explicitly.  Should we do so here as well?
>>
>> [if - added]
>>
>> >
>> > Section 4.3
>> >
>> >       leaf reply-sent-count {
>> >         type uint32;
>> >         config "false";
>> >         description
>> >           "Number of Reply (7) messages received.";
>> >       }
>> >       leaf release-received-count {
>> >         type uint32;
>> >         config "false";
>> >         description
>> >           "Number of Release (8) messages sent.";
>> >       }
>> >       leaf decline-received-count {
>> >         type uint32;
>> >         config "false";
>> >         description
>> >           "Number of Decline (9) messages sent.";
>> >
>> > The descriptions seem out of sync about sent vs received, here.
>>
>> [if - fixed]
>>
>> >
>> > Section 5
>> >
>> >   As the RPCs for deleting/clearing active address and prefix entries
>> >   in the server and relay modules are particularly sensitive, these use
>> >   'nacm:default-deny-all'.
>> >
>> > This looks like a comma splice.
>>
>> [if - Reworded to:
>>
>> The RPCs for deleting/clearing active address and prefix
>>         entries in the server and relay modules are particularly
>>         sensitive. These RPCs use 'nacm:default-deny-all'.    ]
>>
>> >
>> >   An attacker with read/write access the DHCPv6 server can undertake
>> >   various attacks, such as:
>> >   [...]
>> >   An attacker with read/write access the DHCPv6 relay can undertake
>> >   various attacks, such as:
>> >
>> > s/access/access to/
>>
>> [if - fixed]
>>
>> >
>> >   Some of the readable data nodes in this YANG module may be considered
>> >   sensitive or vulnerable in some network environments.  Therefore, it
>> >   is important to control read access (e.g., only permitting get, get-
>> >   config, or notifications) to these data nodes.  These subtrees and
>> >
>> > This doesn't match the template at
>> > https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines , where
>> the
>> > parenthetical is "(e.g., via get, get-config, or notification)" -- the
>> > meaning is different!
>>
>> [if - Changed to ‘via’]
>>
>> >
>> > Appendix A.1
>> >
>> >   The 'max-pd-space-utilization' is set to 80 so that a 'prefix-pool-
>> >   utilization-threshold-exceeded' notification will be raised if the
>> >   number of prefix allocations exceeds this.
>> >
>> > the max-pd-space-utilitization is a percentage, so the "number of prefix
>> > allocations" needs a unit conversion to be comparable to it.
>>
>> [if - added 'percent' to the description]
>>
>> >
>> > Appendix C
>> >
>> >         container lease-storage {
>> >           description
>> >             "Configures how the server will stores leases.";
>> >           choice storage-type {
>> >             description
>> >               "The type storage that will be used for lease
>> >               information.";
>> >
>> > s/type storage/type of storage/
>>
>> [if - fixed]
>>
>> >
>> >             case mysql {
>> >               leaf mysql-name {
>> >                 type string;
>> >                 description
>> >                   "Name of the database.";
>> >               }
>> >               [...]
>> >
>> > This seems to not have provision for configuring mandatory TLS usage for
>> > connection to the mysql server.
>> > (Same for the postgres case.)
>>
>> [if - I’ve just found draft-ietf-netconf-tls-client-server-26 which
>> covers this and
>> Would seem to be the right way to do this.
>>
>> However, importing this module and its dependencies and using the
>> tis-client-grouping for
>> Mysql and Postgres results in the tree diagram growing from 49 lines to
>> 401 with the TLS configuration, which somewhat detracts from the point of
>> the example.
>>
>> Given that this is provided as an example module for a theoretical
>> implementation,
>> Would it make more sense to remove the mysql-host choice and add text in
>> The mysql-host configuration description to say that the database must be
>> running
>> On the localhost? This could be noted in the Appendix descriptive text as
>> well.]
>>
>> >
>> >               leaf mysql-port {
>> >                 type inet:port-number;
>> >                 default 5432;
>> >
>> > mysql's registered port seems to be 3306 (5432 is assigned for
>> > postgres).
>>
>> [if - changed]
>>
>> >
>> > Appendix D
>> >
>> >   classifying clients.  So this standard does not try to provide full
>> >   specification for class selection, it only shows an example how it
>> >   could be defined.
>> >
>> > s/example/example of/
>>
>> [if - changed]
>>
>> >
>> >     grouping client-class-id {
>> >       description
>> >         "Definitions of client message classification for
>> >         authorization and assignment purposes.";
>> >       leaf client-class-name {
>> >         type string;
>> >         description
>> >           "Unique Identifier for client class identification list
>> >           entries.";
>> >
>> > Shouldn't this be mandatory if it's going to be the only way we refer to
>> > class IDs?  I guess the grouping is currently only used in one place,
>> > and it's a list key there (so implicitly mandatory), but I still wonder
>> > if the grouping is more reusable with "mandatory true".
>>
>> [if - Added mandatory true]
>>
>> >
>> >           leaf relay-interface {
>> >             type string;
>> >             description
>> >               "Reference to the interface entry for the incoming
>> >               DHCPv6 message.";
>> >
>> > Whatever we do in the main module for the encoding of the interface
>> > entry should be reflected here as well.
>>
>> [if - Changed the descriptions to align with the interface-id option text
>> in the relay module:
>>
>>       case relay-interface-id {
>>         description
>>           "Client class selection based on a received instance of
>>           OPTION_INTERFACE_ID (18).";
>>         leaf relay-interface {
>>           type string;
>>           description
>>             "An opaque value of arbitrary length generated by the
>>
>>             relay agent to identify one of the relay agent's
>>             interfaces.”;
>> ]
>>
>>
>> >
>> >             leaf vendor-class-data {
>> >               type string;
>> >               mandatory true;
>> >               description
>> >                 "Vendor class data to match.";
>> >
>> > (likewise)
>>
>> [if - Changed to:
>>
>>         container vendor-class-option-data {
>>           description
>>             "Vendor class option data container.";
>>           leaf enterprise-number {
>>             type uint32;
>>             description
>>               "The vendor's registered Enterprise Number as
>>               maintained by IANA.";
>>           }
>>           leaf vendor-class-data-id {
>>             type uint8;
>>             description
>>               "Vendor class data ID";
>>           }
>>           leaf vendor-class-data {
>>             type string;
>>             description
>>               "Opaque field for matching the client's vendor class
>>               data.";
>>           }
>>
>> ]
>>
>> >
>> >             leaf remote-id {
>> >               type string;
>> >               mandatory true;
>> >               description
>> >                 "Remote-ID data to match.";
>> >
>> > (and here)
>>
>> [if - The remote-id option case in the class selction example module was
>> left over from a previous version of the draft.
>> As remote-id is not defined in RFC8415 it’s no longer modelled in this
>> doc, so I’ve removed it from the example.]
>>
>> >
>> >
>> >
>
>
>