Re: [ippm] Benjamin Kaduk's No Objection on draft-ietf-ippm-twamp-yang-11: (with COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Wed, Jun 20, 2018 at 06:55:23PM -0700, Mahesh Jethanandani wrote:
> Hi Ben,
> 
> > On Jun 18, 2018, at 9:18 AM, Benjamin Kaduk <[e-mail]> wrote:
> > 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-ippm-twamp-yang-11: No Objection
> > 
> > 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/
> > 
> > 
> > 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> > 
> > Perhaps I am confused and/or misreading things, but the descriptions
> > of the control-client and session-reflector include discussion of
> > 'sid' session identifiers as if they were always used, but the mode
> > bitmap includes a separate bit for negotiation of
> > 'individual-session-control' for session identifier usage.  Is there
> > some conflict between this mandatory/negotiable distinction, or are
> > they actually talking about different things?
> 
> I believe Al has responded on this issue already.
> 
> > 
> > Comments below in document order, but please pay special note to the
> > (potential) need for global uniqueness of key-ids, the PBKDF2
> > iteration count, and the list of sensitive nodes to call out in the
> > security considerations.
> > 
> > 
> > Section 3.1
> > 
> >   o  Authentication and encryption attributes such as KeyID, Token and
> >      the Client Initialization Vector (Client-IV); see also the last
> >      paragraph of Section 6 in OWAMP [RFC4656] and Randomness
> >      Requirements for Security [RFC4086].
> > 
> > I'm confused about what the RFC4656 reference is intended to call
> > out -- the reliance on AES to be resistant to chosen plaintext, or
> > the randomly generated challenge from the server, or the existential
> > forgeries?
> 
> The reference should have been to Section 3.1 of RFC4656.

To be clear, to all of Section 3.1, not just the last paragraph?

> > 
> >   o  Information pertaining to the test packet stream, such as the test
> >      starting time, which performance metric is to be used Registry for
> >      Performance Metrics [I-D.ietf-ippm-metric-registry], or whether
> >      the test should be repeated.
> > 
> > Is there something missing before or around "Registry for
> > Performance Metrics"?  The current text is hard to read.
> 
> Agree. How about this:
> 
> OLD
>       Information pertaining to the test packet stream, such as the test
>       starting time, which performance metric is to be used Registry for
>       Performance Metrics [I-D.ietf-ippm-metric-registry <https://tools.ietf.org/html/draft-ietf-ippm-twamp-yang-11#ref-I-D.ietf-ippm-metric-registry>], or whether
>       the test should be repeated.
> 
> 
> NEW
>       Information pertaining to the test packet stream, such as the test
>       starting time, which performance metric is to be used, as defined in Registry for
>       Performance Metrics [I-D.ietf-ippm-metric-registry <https://tools.ietf.org/html/draft-ietf-ippm-twamp-yang-11#ref-I-D.ietf-ippm-metric-registry>], or whether
>       the test should be repeated.

Much better!

> 
> > 
> > Section 3.4
> > 
> >   Each Session-Reflector is associated with zero or more TWAMP-Test
> >   sessions.  For each test session, the REFWAIT timeout parameter which
> >   determines whether to discontinue the session if no packets have been
> >   received (TWAMP [RFC5357], Section 4.2) can be configured.
> > 
> > nit: I think this would be easier to read if "which
> > determines...received" was offset by commas or parentheses.
> 
> How about this:
> 
> OLD
>    For each test session, the REFWAIT timeout parameter which
>    determines whether to discontinue the session if no packets have been
>    received (TWAMP [RFC5357], Section 4.2 <https://tools.ietf.org/html/rfc5357#section-4.2>) can be configured.
> 
> 
> NEW
>    For each test session, the REFWAIT timeout parameter can be configured, which
>    determines whether to discontinue the session, if no packets have been
>    received (TWAMP [RFC5357], Section 4.2 <https://tools.ietf.org/html/rfc5357#section-4.2>).

I'm not sure this is quite right -- the clause offset by commas starting
with "which" is supposed to be a "parenthetical", a clause adding some
additional information that is not necessary to understand the sentence.
That is, the sentence is still supposed to read fine if the entire clause
(and commas) are removed.  But "the REFWAIT timeout parameter can be
configured if no packets have been received" seems like a different meaning
than is intended.

> 
> 
> > 
> >   Read-only access to other data model parameters, such as the Sender
> >   IP address is foreseen.  Each test session can be uniquely identified
> >   by the 4-tuple mentioned in Section 3.2.
> > 
> > Nit: comma after "Sender IP address”.
> 
> Ok.
> 
> > 
> > Section 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 thought I remembered some discussion about this text being unclear
> > and removing "and subsequent integers increase by one" being
> > proposed.  But I don't see that discussion in an obvious place, so
> > maybe it was on a different document.
> 
> You do have a good memory. It was for this document. See the discussion on the other thread with Pete, where this has come up and edits proposed.

I think I remember those going by and looking good.

> > 
> >   Note that the list of preferred Modes may set bit position
> >   combinations when necessary, such as when referring to the extended
> >   [...]
> > 
> > Maybe "may set multiple bits independently" would be more clear?
> > But it seems that some bit combinations don't make any sense, like
> > unauthenticated+authenticated -- is there need for more expository
> > text here?
> 
> How about:
> 
> OLD:
>    Note that the list of preferred Modes may set bit position
>    combinations when necessary, such as when referring to the extended
>    TWAMP features in [RFC5618 <https://tools.ietf.org/html/rfc5618>], [RFC5938 <https://tools.ietf.org/html/rfc5938>], [RFC6038 <https://tools.ietf.org/html/rfc6038>], and [RFC7717 <https://tools.ietf.org/html/rfc7717>].  If
>    the Control-Client cannot determine an acceptable Mode, it MUST
>    respond with zero Mode bits set in the Set-up Response message,
>    indicating it will not continue with the control connection.
> 
> 
> 
> NEW:
>    Note that the list of preferred Modes may set multiple bit position
>    independently, such as when referring to the extended
>    TWAMP features in [RFC5618 <https://tools.ietf.org/html/rfc5618>], [RFC5938 <https://tools.ietf.org/html/rfc5938>], [RFC6038 <https://tools.ietf.org/html/rfc6038>], and [RFC7717 <https://tools.ietf.org/html/rfc7717>].  If
>    the Control-Client cannot determine an acceptable Mode, or when the 
>    bit combinations do not make sense, e.g. both authenticated and 
>    unauthenticated bit set, it MUST respond with zero Mode bits set 
>    in the Set-up Response message, indicating it will not continue with 
>    the control connection.

That looks good.  (But put a comma after "e.g." as well as before.)

> > 
> >   [...] The secret-key is the shared secret, a sequence
> >   of octets of arbitrary length whose interpretation is unspecified.
> >   The key-id and secret-key encoding SHOULD follow Section 9.4 of YANG
> >   [RFC7950]. [...]
> > 
> > Section 9.4 of YANG is for (printable) strings, but the secret-key
> > is binary -- should this get a Section 9.8 reference as well?
> > I'm also not sure that leaving it as "arbitrary length"
> > is great -- if we're using it to derive 16-byte AES keys and 32-byte
> > HMAC-SHA1 keys, we could at least say "SHOULD contain at least 128
> > bits of entropy”.
> 
> This was my bad. I changed the text for the secret-key from string to binary in the YANG model, at the behest of Tom Petch, but forgot to update this text. I can update the reference to Section 9.8 of RFC7950. Will add text around the 128 bits of entropy.

Ah, okay.  (I had originally put in a lot more text about this before I
noticed that he rest of the document had the key as binary, and it was why
I was looking so closely at the base64 encoding later on.)

> > 
> > Section 4.2
> > 
> >   [...] The Server, being prepared to conduct
> >   sessions with more than one Control-Client, uses KeyIDs to choose the
> >   appropriate secret-key; a Control-Client would typically have
> >   different secret keys for different Servers. key-id tells the Server
> >   which shared-secret the Control-Client wishes to use for
> >   authentication or encryption.
> > 
> > Does this imply a global uniqueness requirement for key-ids?  If so,
> > that should be called out more clearly.
> 
> Section 4.1 says. Do you think this is clear enough?
> 
>    Both the Server and
>    the Control-Client use the same mappings from KeyIDs to shared
>    secrets (key-id and secret-key in Figure 3, respectively)

I'd prefer to have it also say explicitly that they must be unique, as in
something like

NEW:

    Both the Server and
    the Control-Client use the same mappings from KeyIDs to shared
    secrets (key-id and secret-key in Figure 3, respectively); in order
    for this to work properly, KeyIds must be unique across all systems in
    the administrative domain.


> > 
> > Section 4.3
> > 
> >                            | name                      |
> >                            | ctrl-connection-name {ro} |
> >                            | fill-mode                 |
> >                            | number-of-packets         |
> >                            | state {ro}                |
> >                            | sent-packets         {ro} |
> >                            | rcv-packets          {ro} |
> >                            | last-sent-seq        {ro} |
> >                            | last-rcv-seq         {ro} |
> >                            +---------------------------+
> > 
> > nit: should the "{ro}" on "state" be right-aligned with the others?
> 
> Ok. 
> 
> > 
> > Is there any privacy concern about exposing the parent-connection
> > 4-tuple?
> 
> As discussed in the other thread, the test is run between the four logical entities within the same provider network. That combined with the security of the session, and use of access control (NACM) can limit how much is exposed and what they can do with that information.
> 
> > 
> > Section 5.2
> > 
> > In the 'count' leaf, a default value of 10 (corresponding to an
> > iteration count of 2^10 == 1024 for PBKDF2) is described.  This
> > seems quite low for a PBKDF2 iteration count, by modern standards.
> > In "normal" cryptographic protocols we would generally be using a
> > default closer to 32768 == 2^15 (which I see is the default *max*
> > count value, and there is additional discussion of the issue in the
> > leaf description for that leaf).  Perhaps one could make an argument
> > that this is just for test setups and the keys and data exchanged
> > are "not very valuable", but there is always risk of key sharing
> > across protocols, and my preference is to present the strong
> > defaults and give users the option to reduce where appropriate.
> > What are the authors' thoughts here?
> 
> We can bump the default for ‘count to be 15, and the default for ‘max-count-exponent’ to 20.

Thanks!

> > 
> > Section 7
> > 
> > There are probably more nodes that can get called out as
> > particularly vulnerable, such as the count and max-count nodes that
> > can cause a long time to be spent on PBKDF2 iterations, the dscp
> > markings, the mode bitmask, etc.
> 
> The question of more nodes being identified from a vulnerability perspective has been called out on the other thread. We can use the above discussion, and the point you make below to call out ‘count’ and ‘max-count-exponent’. What do you see as issue with different DSCP markings or with mode bit mask?

If I understand correctly, different DCSP markings could affect the
processing of the test traffic on the network (skewing the results), and
changing the mode mask could result in big changes, like using
unauthenticated (and MITM-able) test traffic instead of authenticated
traffic.

> > 
> > Appendix A
> > 
> > The <secret-key> elements appear to be using base64-encoded values.
> > Where is it specified that such encoding is used for the binary
> > values?  (I assume this is just my ignorance of a generic standard,
> > so please enlighten me!)
> 
> RFC 7950 says in section 9.8.2 that:
> 
>    Binary values are encoded with the base64 encoding scheme (see
>    Section 4 in [RFC4648] <https://tools.ietf.org/html/rfc4648#section-4>).
> 
> 
> > 
> > Am I reading it right that the <count>30</count> means 2^30 (one
> > billion) PBKDF2 iterations?  Has this actually been run in practice?
> > It seems like it would be painfully slow.
> 
> It was setup as an example to show how a test session could be configured. We can reduce the value of count to 15.

Thanks!

-Benjamin