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

[[e-mail]]

Hi Benjamin,

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

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

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

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


            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.

—————————




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.

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


    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)

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


> 
> ----------------------------------------------------------------------
> 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".
> 
> 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"?
> 
> 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)?
> 
>     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).
> 
>     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.
> 
>       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.)
> 
>     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).
> 
>     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?)
> 
>             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.
> 
> 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?
> 
>     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.)
> 
> 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.
> 
>     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.
> 
>           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.)
> 
>               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.)
> 
>               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.)
> 
>                 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?
> 
>     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?
> 
>     notification non-success-code-sent {
>       description
>         "Notification sent when the server responded
>         to a client with non-success status code.";
> 
> (similarly here)
> 
> 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.
> 
> 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?
> 
>     grouping message-statistics {
> 
> The relay module has a counter for discarded messages; should we?
> 
>     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?
> 
>     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)
> 
>       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.
> 
>     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?
> 
>       [...]
>       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.
> 
>     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.
> 
>     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?
> 
> Section 5
> 
> It's probably worth mentioning the risk of notifications turning into a
> DoS vector (absent rate-limiting) here.
> 
> 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?
> 
>   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).
> 
>   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).
> 
>   [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.
> 
> 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.
> 
> 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.
> 
> NITS
> 
> We might check the spelling of "grouping message-stats" (vs "grouping
> message-statistics") across modules.
> 
> 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.
> 
>     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.
> 
>     grouping inf-max-rt-option-group {
>       [...]
>         leaf inf-max-rt-value {
>           type dhc6:timer-seconds32;
>           description
>             "inf max rt value";
> 
> (ditto)
> 
>               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?
> 
> 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.
> 
> 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.
> 
>   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/
> 
>   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!
> 
> 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.
> 
> 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/
> 
>             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.)
> 
>               leaf mysql-port {
>                 type inet:port-number;
>                 default 5432;
> 
> mysql's registered port seems to be 3306 (5432 is assigned for
> postgres).
> 
> 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/
> 
>     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".
> 
>           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.
> 
>             leaf vendor-class-data {
>               type string;
>               mandatory true;
>               description
>                 "Vendor class data to match.";
> 
> (likewise)
> 
>             leaf remote-id {
>               type string;
>               mandatory true;
>               description
>                 "Remote-ID data to match.";
> 
> (and here)
> 
> 
> 
> _______________________________________________
> dhcwg mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/dhcwg