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

[Timothy Winters <[e-mail]>]

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?

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

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

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