Subject: [ippm] Adam Roach's Discuss on draft-ietf-ippm-twamp-yang-11: (with DISCUSS and COMMENT)

From: Adam Roach <[email-masked]>
Date: Wed, 20 Jun 2018 21:41:41 -0700

Adam Roach has entered the following ballot position for
draft-ietf-ippm-twamp-yang-11: 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/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-ippm-twamp-yang/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Thanks for the work and thought that everyone involved in this document spent. I
find the model well described and easy to understand.

I agree with Ben's comments about including more information about the privacy
and security properties of specific entities in the module. See
https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines for specific
guidance.

Since this conflicts with normative language in RFC 6087 §3.4 (and 6087bis
§3.7), it is a blocking defect that needs to be remedied prior to publication.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I applaud the use of UML in this document. I believe you need to include a
reference to ISO/IEC 19501:2005 (or the OMG equivalent); and, since the
diagrams appear to be normative, this reference probably needs to be
normative. It may be helpful (but less necessary) to also include a brief key
to the symbols used by this document, for those readers who may not be
familiar with UML notation.

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

§4.1:

>  Specifically, mode-preference-chain lists the
>  mode and its corresponding priority, expressed as a 16-bit unsigned
>  integer, where zero is the highest priority and subsequent integers
>  increase by one.

I think I know what this is trying to say, but it doesn't really say it.
Subsequent integers increase by one, by the definition of "integer," so I
don't think we need to reiterate that behavior here. Perhaps: "with zero as
the highest priority, and 65535 as the lowest."

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

Figure 6:

>  +----=--------------+
>  | session-reflector |
>  +-------------------+
>  | admin-state       |
>  | refwait           |
>  +-------------------+

Nit: the top line appears to have had an "=" character sneak in.

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

Page 81:

>             +--ro reflector-udp-port                   inet:port-numbe
>r
>             +--ro parent-connection-client-ip?         inet:ip-address
>             +--ro parent-connection-client-tcp-port?   inet:port-numbe
>r
>             +--ro parent-connection-server-ip?         inet:ip-address
>             +--ro parent-connection-server-tcp-port?   inet:port-numbe
>r

Wrapping the final character of the word "number" is probably not what you want
to happen here.

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

Page 34:

                  The configured (or auto-allocated) value is
                      advertized in the Sender Port field of the

Nit: "advertised"

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

Page 35:

>            leaf timeout {
>              type uint64;
>              units seconds;
>              default 2;
>              description
>                "The length of time (in seconds) that the
>                 Session-Reflector should continue to respond to
>                 packets belonging to this TWAMP-Test session after
>                 a Stop-Sessions TWAMP-Control message has been
>                 received.
>
>                 This value will be placed in the Timeout field of
>                 the Request-TW-Session message.";
>                                reference
>                "RFC 5357: TWAMP, Section 3.8";
>            }

This reference (Section 3.8 "Stop-Sessions") doesn't seem right. Perhaps
section 3.5 was intended? Possibly even section 3.5 of RFC 4656 would be more
appropriate.

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

Page 36:

>              type union {
>                type uint32 {
>                  range 0..4294967294;
>                }
>                type enumeration {
>                  enum forever {
>                    description
>                      "Indicates that the test session SHALL be
>                       repeated *forever* using the information in
>                       repeat-interval parameter, and SHALL NOT
>                       decrement the value.";
>                  }
>                }

If I had to guess here, the intention is to reserve the value 4294967295 for
"forever," but the model doesn't appear to do that. The preceding definition
defines "forever" as "0", which would appear to conflict with a repeat value
of 0. See RFC 7950 §9.6.4.2:

>  If a value is not specified, then one will be automatically assigned.
>  If the "enum" substatement is the first one defined, the assigned
>  value is zero (0); otherwise, the assigned value is one greater than
>  the current highest enum value (i.e., the highest enum value,
>  implicit or explicit, prior to the current "enum" substatement in the
>  parent "type" statement).

I think this should be:

                 type enumeration {
                   enum forever {
                     value 4294967295;
                     description
                       "Indicates that the test session SHALL be
                        repeated *forever* using the information in
                        repeat-interval parameter, and SHALL NOT
                        decrement the value.";
                   }

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

The examples include the address "192.68.0.2" in several places. Please change
this to an address in one of the RFC 5737 ranges (e.g., 192.0.2.2).

---

From: Mahesh Jethanandani <[email-masked]>
Date: Fri, 22 Jun 2018 12:14:10 -0700

Hi Adam,

> On Jun 20, 2018, at 9:41 PM, Adam Roach <[email-masked]> wrote:
> 

I have added more nodes in the Security Considerations section.

Have added a reference to ISO/IEC 19501:2005.

> the highest priority, and 65535 as the lowest.”

This particular sentence has been commented on by several folks and several suggestions and counter suggestions made. Please see other threads. At this point, someone please just tell me what they want to see here, and I will add it.

> Nit: the top line appears to have had an "=" character sneak in.

Fixed.

This diagram is generated using a tool, using a max-length parameter. If it has to be fixed, it will have to be done manually, preferably in the final version of the draft (just before publication). 

> Nit: “advertised"

Fixed.

Ok. Will change reference to Section 3.5. But since we say that the timeout in TWAMP is interpreted differently from the definition in RFC 4656, it would be confusing to add reference to RFC 4656.

Done.

Fixed. 

Thanks.

Mahesh Jethanandani
[email-masked]

---

From: Spencer Dawkins at IETF <[email-masked]>
Date: Fri, 29 Jun 2018 10:08:16 -0500

Hi, Manesh,

On Fri, Jun 22, 2018 at 2:14 PM Mahesh Jethanandani <[email-masked]>
wrote:

> > Please refer to
> https://www.ietf.org/iesg/statement/discuss-criteria.html

I'm not the Adam Interpreter, but I'm seeing considerable new discussion
about the MTI security for NETCONF and RESTCONF, and about what happens if
writable nodes are modified by an attacker. Thank you for that.

Adam said in his Discuss that he was agreeing with Ben's comments on this
topic, and Ben's comments included this question:

 Are there no nodes that are privacy (or otherwise) sensitive when just
readable?

I didn't see any new text about this. Are there privacy-sensitive read-only
nodes?

Thanks,

Spencer

---

From: Mahesh Jethanandani <[email-masked]>
Date: Fri, 29 Jun 2018 12:19:53 -0700

Hi Spencer,

> On Jun 29, 2018, at 8:08 AM, Spencer Dawkins at IETF <[email-masked]> wrote:
> 
> Hi, Manesh, 
> 
> On Fri, Jun 22, 2018 at 2:14 PM Mahesh Jethanandani <[email-masked] <mailto:[email-masked]>> wrote:
> Hi Adam,
> 
> > On Jun 20, 2018, at 9:41 PM, Adam Roach <[email-masked] <mailto:[email-masked]>> wrote:
> > 
> > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html <https://www.ietf.org/iesg/statement/discuss-criteria.html>
> > https://datatracker.ietf.org/doc/draft-ietf-ippm-twamp-yang/ <https://datatracker.ietf.org/doc/draft-ietf-ippm-twamp-yang/>
> > 
> > 
> > 
> > https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines <https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines> for specific
> > guidance.
> > 
> I'm not the Adam Interpreter, but I'm seeing considerable new discussion about the MTI security for NETCONF and RESTCONF, and about what happens if writable nodes are modified by an attacker. Thank you for that. 
> 
> Adam said in his Discuss that he was agreeing with Ben's comments on this topic, and Ben's comments included this question: 
> 
>  Are there no nodes that are privacy (or otherwise) sensitive when just readable?
> 
> I didn't see any new text about this. Are there privacy-sensitive read-only nodes?

I took another look at nodes that are marked read-only in the YANG module. These nodes include session state, sent and receive packet count, last sequence number received or transmitted, start time, repeat count, tcp/ip address and port numbers etc. Nothing jumped out as particularly privacy sensitive.

One field that did catch my attention was the ‘token’ field, which is defined as "This parameter holds the 64 octets containing the concatenation of a 16-octet Challenge, a 16-octet AES Session-key used for encryption, and a 32-octet HMAC-SHA1 Session-key used for authentication; see also the last paragraph of Section 6 in RFC 4656.”

I am not a security expert, and do not know if being able to read this or other read-only information is sufficient in itself to launch an attack or disrupt an existing or new session. Maybe somebody from SEC-DIR can comment on it. If it is, I would be happy to add some text around it. 

Mahesh Jethanandani
[email-masked]