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

ianfarrer@gmx.com Mon, 17 January 2022 17:40 UTC

Return-Path: <ianfarrer@gmx.com>
X-Original-To: dhcwg@ietfa.amsl.com
Delivered-To: dhcwg@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 156C43A094B; Mon, 17 Jan 2022 09:40:09 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.896
X-Spam-Level:
X-Spam-Status: No, score=-1.896 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=gmx.net
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id htIdkFo2q5cC; Mon, 17 Jan 2022 09:40:03 -0800 (PST)
Received: from mout.gmx.net (mout.gmx.net [212.227.15.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 10F7F3A0948; Mon, 17 Jan 2022 09:40:02 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1642441198; bh=NtG1BJNSxAf633ZX2JzPsVm8lk83vQVGEGoHYISw0MM=; h=X-UI-Sender-Class:Subject:From:In-Reply-To:Date:Cc:References:To; b=iWVQ4vk8hrYIcg0Q9pnDR82MRKQqGzf2rHf3m+BkbKkj7noyGzqFnfIgU60xikCNI c48RrYoxZa1JWMscWM5m4w22dCNFNntPJSitfWcyVRRMm3f6vlm6Awg2MHM6g4q7Nw 056cGHk3Ih2gNx2EtWUZl78rkDukfmsbS8btW8/Q=
X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c
Received: from smtpclient.apple ([87.78.108.189]) by mail.gmx.net (mrgmx004 [212.227.17.184]) with ESMTPSA (Nemesis) id 1MgvvT-1mfPpT0LPx-00hM0f; Mon, 17 Jan 2022 18:39:58 +0100
Content-Type: text/plain; charset="utf-8"
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.40.0.1.81\))
From: ianfarrer@gmx.com
In-Reply-To: <163952685584.6395.6879611267419166159@ietfa.amsl.com>
Date: Mon, 17 Jan 2022 18:39:56 +0100
Cc: The IESG <iesg@ietf.org>, draft-ietf-dhc-dhcpv6-yang@ietf.org, dhc-chairs@ietf.org, dhcwg <dhcwg@ietf.org>, Timothy Winters <tim@qacafe.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <F7517675-B8E0-4777-AB7D-A7A8A6283D75@gmx.com>
References: <163952685584.6395.6879611267419166159@ietfa.amsl.com>
To: Benjamin Kaduk <kaduk@mit.edu>
X-Mailer: Apple Mail (2.3693.40.0.1.81)
X-Provags-ID: V03:K1:Bu63pQZJAomKiHVQ8hgdRg1EVha82E+o9T+xjpwq4iinCADNjyn 6Pn1wkbYOzqC7GiqZDZq8epWo2hSc5cHAvl5b8iVIsNNpq1EefAdydF9JSm0C0IS1jb8BGj xyF2RG3Ar+38GyX4Lf9t6xoh/q7qJUByDZdziClqQBRDZWmCZgcRrFHEnP3ftSpQJtfM48z cNpJj1mYJL7Cap/HpGDqA==
X-UI-Out-Filterresults: notjunk:1;V03:K0:8I/zgAEYVRw=:cHsz4nq/C3qu5TmNTuZ7T3 HT+a8ZE1BX4oVdUYE/j+aBg/xPfY4O+EiMdckW4e0CZHm2lLhT1TUZnna69Wi38FV3+Gj+ZO9 n/Hfk4vFf6MG5WvLyMn1/jogOpHRAZCnLKr2Mi53JdYqicEJl+dJspAPmj3PGiNxiUYwCZMQ/ QaSgqPcG337/sCcPqzeXWKhk2r6XbcPr3JzU1TbiNYCHpVC2YoQ+fYCbxIn2wWtQCrp0YoP5i XIGOnsZk43iWgq/Vbal6ewz/0oLilV0vXdfRHk+joM0tzYZ5+CigvnG7yY1zl+Ox52qqsfWiH zhZmVcLFFciBotfJc7y/18bw8/ijWWxD6z8slPfH86YYjnJRuxZzHlj3tMyRTVpDJwtx23srA ZXyxgs3a8nuXygTo8XwwCU1GykoCOhZUS0hW2x2GTpdZDKKG+G00+OB0+wOgOM9jN/IXY2IXx lUrfk4PNs2uv0BXF/3vxgVAUEWQYedyARr2xHrIiysjpROHM+2Hp7iHNzwn2hUMgM/GgwsFZo ae9uM9OecYXfv9LNTQg4hIcUwVI+d9Eh6fTA4xSEg78sQSsJ2SleCGcgyyMhlsi91p6IS/hJc cYZmIMs8iXj/Gs1HBRIct43AkF9uAp9ULUrafon/d4cKiuiQhne4egtvZFDLzBJGV7Oereaph mtc2S39WUkJ0WkGVO+JdACIXiuuXGLaPyw8gCcr6KXByyfN+Geuc09fm7nUPaGgsoEH7N5y75 kAnxsxn9A9vR51Hgzjv7iVrWrKvr0vmSVcgtU+XIVMnzDEKoEPaO8aYWZELFgPqnd126wTvIA 4/C2fZr9Zl19AD++WoPx3fNf4r2kpvMgsqXqWXk9zYLl6aN3kfivt2Kw+tCviEJsT+0D7ETci iSBnZK+zmIAi/zmSqtrh+0iYmhjsuD2A2H54b17awE45OKlDWh9hPNdJmg00q2zzfJA2iP09C M0JZxFegWEyflDr606ZzfCXxlz/DOCs3jxAyvGEv/qxHVU96fqDGBYUTx/yeAYnu93dPHXL47 M/Wb+DA7Dj+hyl4c2c/uu060dmb0jcObVARK0OQWgVAkk/AHqWZVeaXLadeZGp+xrTcRiDxFn cy6VSUtQv+Tc5w=
Archived-At: <https://mailarchive.ietf.org/arch/msg/dhcwg/riMGn6_2rZMJwrZRZtH1ABPM-yM>
Subject: Re: [dhcwg] Benjamin Kaduk's Discuss on draft-ietf-dhc-dhcpv6-yang-24: (with DISCUSS and COMMENT)
X-BeenThere: dhcwg@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Dynamic Host Configuration <dhcwg.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/dhcwg>, <mailto:dhcwg-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/dhcwg/>
List-Post: <mailto:dhcwg@ietf.org>
List-Help: <mailto:dhcwg-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/dhcwg>, <mailto:dhcwg-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 17 Jan 2022 17:40:09 -0000

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 <noreply@ietf.org> 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?



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

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

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

> 
> 
>