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

[Benjamin Kaduk via Datatracker <[e-mail]>]

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.


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