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

From: Timothy Winters <[email-masked]>
Date: Tue, 1 Feb 2022 11:08:06 -0500

Hi Ian,

On Tue, Feb 1, 2022 at 10:07 AM <[email-masked]> 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 <[email-masked]> 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 <[email-masked]> 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 <
>> [email-masked]> 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.]
>>
>> >
>> >
>> >
>
>
>

---

From: [email-masked]
Date: Tue, 1 Feb 2022 20:30:13 +0100

Hi Benjamin,

Please see inline below.

Thanks,
Ian

> On 15. Dec 2021, at 01:07, Benjamin Kaduk via Datatracker <[email-masked]> wrote:
> 
> Please refer to https://www.ietf.org/blog/handling-iesg-ballot-positions/

[if - In each of the cases that you identified (5 in total), the types have been changed to ‘binary’. I’ve checked through the models and you have identified all of the relevant instances in your review.

Here is a summary of the changes that have been made:

Ietf-dhcpv6-common.yang
----------------------------------
Auth-option-group
This is now modelled with a choice for the protocol type (conf-token or rkap). In both cases the relevant leaves (token-auth-information and auth-info-value respectively) are type binary.

Vendor-specific-information-option-group/
sub-option-data -> binary

Ietf-dhcpv6-relay.yang
———————————————
Interface-id-option-group
interface-id -> binary

ietf-dhcpv6-client.yang
——————————
User-class-option-group
-> user-class-data

Vendor-class-option-group
-> vendor-class-data

For reference, here are the associated review comments:

------------------

            leaf sub-option-data {
              type string {

—————————

Section 4.3

--------------------

-------------------

(likewise here)

-----------------

---

From: [email-masked]
Date: Wed, 16 Feb 2022 17:01:09 +0100

Hi Benjamin,

Please see below regarding the changes that have been made to address your DISCUSS.

Thanks,
Ian

Ietf-dhcpv6-common.yang

Ietf-dhcpv6-relay.yang
———————————————

ietf-dhcpv6-client.yang
——————————
User-class-option-group
-> user-class-data

------------------

            leaf sub-option-data {
              type string {

—————————

Section 4.3

--------------------

-------------------

(likewise here)

-----------------

> https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines , where the
> _______________________________________________
> dhcwg mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/dhcwg

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 25 Feb 2022 13:53:37 -0800

Hi Ian,

My apologies for the rather slow response.

The changes below all look good to me -- thank you for the careful
attention and confirming with Tim/Bernie that they properly reflect the
protocol fields in question.

I will also go and reply to a couple topics in the COMMENT section from
your original reply, in a separate message.

Please go ahead and upload a new revision when you get a chance, and I can
update my ballot position accordingly.

Thanks again,

Ben

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 25 Feb 2022 14:18:11 -0800

Hi Ian,

As promised in my reply on the DISCUSS thread, a few remaining comments
interspersed here.  I'll trim heavily since the information density would
be quite low otherwise.

On Mon, Jan 17, 2022 at 06:39:56PM +0100, [email-masked] wrote:
> [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 

I'm not aware of any way to enforce it via YANG either, but perhaps we
could put a few more words in the description string that covers the
expected relationship in prose.

> are offered for the pd-pool.

I think the "client-prefix-length" leaf is what I was looking for, thanks.

> [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.]

It would be nice if 5277 had said more on this topic, sure.
I think what's interesting to me here, specifically, is that there is a
layer of separation between where the (hypothetical) attacker sits and the
DoS-via-notifications channel.  That is, because this module exists,
there's potential for an attacker that can only send DHCPv6 traffic to
induce new traffic to be generated (as YANG notifications) fromm the DHCPv6
server to the recipient of the notifications.  In the scenario in question,
the frequency of notification traffic is controlled by the attacker's
DHCPv6 traffic rather than by the DHCPv6 server itself.  The new security
consideration is the leverage for turning DHCPv6 traffic into a different
kind of traffic.  Does that make sense?  I don't really expect this to be a
useful DoS vector in practice, but I've been wrong before, and the cost of
documenting the risk seems pretty low.

> [if - Do you mean that RFC7844 can provide partial mitigation of the server

On closer look, it doesn't look to actually be the case that RFC7844
provides any means for mitigating server privacy issues, just client
privacy issues.  But since we provide YANG models for DHCPv6 client
behavior here, wouldn't even that limited functionality still be relevant?

[now in the "NITS" portion]
> [if - I’ve just found draft-ietf-netconf-tls-client-server-26 which covers this and
> However, importing this module and its dependencies and using the tis-client-grouping for 
> 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,
> 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.]

That seems like a reasonable way to keep the example focused (please note
that as far as I know, the situation for postgres and mysql is equivalent
in this regard).
In the general case, even running only on localhost is not always
sufficient protection such that TLS is not needed, so I'd advise covering
that in the Appendix descriptive text as well.  Brainstorming (please do
adjust as appropriate), that might resemble:

This exmaple aims to illustrate the extension of the server YANG module
with configuration such as a lease storage database.  For simplicity and to
maintain focus on that goal, it assumes that the actual database server is
colocated and can serve traffic securely on localhost without additional
cryptographic protection.  A production service would likely not be
colocated and thus use TLS to secure the database connection between DHCPv6
server and database server; the TLS configuration would require additional
configuration, as might be modeled using
[draft-ietf-netconf-tls-client-server].

Many thanks for all the updates that I trimmed -- there was a lot of good
stuff in there and it's great to see that.

-Ben