Subject: [ippm] Comments on draft-cmzrjp-ippm-twamp-yang

From: Robert Shearman <[email-masked]>
Date: Tue, 14 Apr 2015 14:07:43 +0100

Hi,

Thank you for your great work on defining this model.

I have some comments/questions on this draft. I've organised them by the 
section they best apply to, although in some cases they may have effects 
on other sections as well.

Section 4.1:

    keys for different Servers. keyId is a UTF-8 string, up to 80 octets
    in length (if the string is shorter, it is padded with zero octets),
    that tells the Server which shared secret the Control-Client wishes
    to use to authenticate or encrypt.  The secretKey is the shared
    secret, an octet string of arbitrary length whose interpretation as a
    text string is unspecified.  In the interest of interoperability,
    however, the UTF-8 text encoding MUST be used for secretKey.

There's no need to specify the encoding of keyId or secretKey because 
section 9.4 of RFC6020 (YANG) specifies the encoding of the string 
(presumably along with the top-level XML encoding attribute). There's 
also no need to specifying how the padding works because YANG strings 
are variable-sized.

Section 4.1:

    dkLen   Intended length in octets of the derived key, a positive
            integer, at most (2^32 - 1) * hLen.

Changing the derived key length from value of 128 bits inferred from 
section 6.10 of RFC4656 will result in a client that cannot authenticate 
with a server unless the derived key length in use by the server matches 
and the value is known to the user/client. Neither RFC4656 nor RFC5357 
suggest that this is a parameter that should be configurable. Is there 
an RFC/draft that suggests this and extends the server greeting to 
advertise the derived key length?

Section 4.1:

    pmIndex Numerical index value of a Registered Metric in the
            Performance Metric Registry [I-D.ietf-ippm-metric-registry].
            Output Statistics will be specified in the Registry entry.

Perhaps I'm missing something fundamental, or some background 
information, or this is work in progress, but the purpose of 
I-D.ietf-ippm-metric-registry is stated to be:

    This document defines the IANA Registry for Performance Metrics.
    This document also gives a set of guidelines for Registered
    Performance Metric requesters and reviewers.

Whilst I can see the usefulness of abstracting out the definition of a 
performance metric, how does the index of the registered metric help in 
retrieval of the output statistics?

Section 4.1:

What triggers the Control-Client to send the Start-Sessions message?

Section 4.1:

Should twampSessionRequest expose a property to allow the DSCP to be 
specified via the Type-P field of the Request-TW-Session message?

Section 4.1:

Should the behaviour be specified if any of the settable properties of 
twampSessionRequest aren't set?

E.g.:
- senderIp
- senderUdpPort
- reflectorIp
- reflectorUdpPort
- timeout
- paddingLength
- startTime
- repeat
- pmIndex

Section 4.2:

    maxCount
            If an attacking system sets the maximum value in Count
            (2**32), then the system under attack would stall for a
            significant period of time while it attempts to generate
            keys.  Therefore, TWAMP-compliant systems SHOULD have a
            configuration control to limit the maximum Count value.  The
            default maximum Count value SHOULD be 32768.

Was this intended to be a server property? Section 6 of RFC5357 
continues to say:

    If a
    Control-Client receives a Server-Greeting message with Count greater
    that its maximum configured value, it SHOULD close the control
    connection.

I.e. this applies to clients. The client never suggests a Count to the 
server; instead it's the client that needs to validate the count the 
server has provided.

Section 4.2:

    dscp
            The DSCP value used in the header of the TCP control packets
            sent by the Server for this control connection.  This will
            usually be the same value as is configured for
            twampServer:dscp under the twampServer.  However, in the
            event that the user re-configures twampServer:dscp after this
            control connection is already in progress, this read-only
            value will show the actual dscp value in use by this control
            connection.

I support the inclusion of this property despite section 3.1 of RFC5357 
saying:

    The Server SHOULD use the DSCP of the Control-
    Client's TCP SYN in ALL subsequent packets on that connection
    (avoiding any ambiguity in case of re-marking).

The reasoning being that for practical purposes TWAMP will typically be 
implemented by using a general purpose TCP stack provided by the 
underlying operating system. This statement imposes a requirement on the 
behaviour of the TCP stack which AFAICS isn't specified in standards 
that directly relate to TCP or DSCP. This means that it's not practical 
to implement this behaviour in a OS-portable implementation of TWAMP.

In addition, there are DoS concerns with allowing a client to control 
DSCP values on the return path from a server.

Configuring the server use a DSCP for control packets it sends mitigates 
both issues.

Section 4.2:

    dkLen
            The dkLen value that is in use by this control connection.
            This will usually be the same value as is configured under
            twampServer.  However, in the event that the user re-
            configured twampServer:dkLen after this control connection is
            already in progress, this read-only value will show the
            actual dkLen that is in use for this control connection.

The description here refers to twampServer:dkLen but there is no item 
named dkLen under twampServer. Such an item under twampServer is 
required if the client is to have such an option (see above).

Section 4.2:

    The twampServer container items are as follows:

...
    The following parameters are read-only:

    salt    A parameter used in deriving a key from a shared secret as
            described in Section 3.1 of [RFC4656].  Salt MUST be
            generated pseudo-randomly (independently of anything else in
            the RFC) and is communicated to the Control-Client as part of
            the Server greeting message.

    serverIv
            The Server Initialization Vector (IV) is generated randomly
            by the server.

    challenge
            Challenge is a random sequence of octets generated by the
            server.  As described in Section 4.1 challenge is used by the
            Control-Client to prove possession of a shared secret.

These are all per-connection properties and so should be properties of 
the twampServerCtrlConnection container.

Section 4.3:

    dscp    The DSCP value to be placed in the UDP header of TWAMP-
            Control packets generated by the Session-Sender.

This should say "... UDP header of TWAMP-Test packets ...".

Section 4.3:

Allowing properties to be changed at will whilst test sessions are in 
progress may present implementation challenges (i.e. synchronisation 
with the sending of packets). Do we think this is a valid use case? If 
not, could text be added to guide implementors as to the expectations?

Section 4.4:

    twampServerCtrlConnection object.  This four4-tuple allows the user

The "four" should be removed.

Section 5.2:

          leaf serverIp {
            config "true";
            type inet:ip-address;
          }

Nit: the parent list defaults to config being true, so the config 
statement is unnecessary and leads to questions in the readers' mind as 
to why other apparently configurable properties in the same list don't 
have this statement.

Section 5.2, list twampClientCtrlConnection:

          leaf token {
            config "false";
            type string {
              length "1..64";
            }
            description "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";
          }
          leaf clientIv{
            config "false";
            type string {
              length "1..16";
            }
            description "16 octets, Client-IV is generated randomly
              by the Control-Client.";
          }

Should these be binary types of fixed length instead?

Section 5.2, list twampClientCtrlConnection:

          leaf serverStartTime {
            config "false";
            type uint64;
          }

Should this be exposed in a form more convenient for the operator, such 
as ietf-yang-types:date-and-time?

Section 5.2, container twampServer:

        leaf salt{
          config "false";
          type string {
            length "1..16";
          }
            description "Salt MUST be generated pseudo-randomly";
        }
        leaf serverIv {
          config "false";
          type string {
            length "1..16";
          }
          description "16 octets, Server-IV is generated randomly
            by the Control-Client.";
        }
        leaf challenge {
          config "false";
          type string {
            length "1..16";
          }
          description "Challenge is a random sequence of octets
            generated by the Server";
        }

Should these be binary types of fixed length instead?

Section 7:

Is there a use case in mind behind the exposing of values that can be 
used to derive the session key on both clients and servers?

The reason for this question is that such information will need to be 
restricted to privileged operators and if the implementation of the 
management interface doesn't support a fine enough grained access 
control mechanism then it could prevent much of the data in this model 
from being accessible by ordinary operators.

Thanks,
Rob