Re: [Dots] Adam Roach's Discuss on draft-ietf-dots-signal-channel-31: (with DISCUSS and COMMENT)

[<[e-mail]>]

Re-,

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Adam Roach via Datatracker [mailto:[e-mail]]
> Envoyé : jeudi 2 mai 2019 07:01
> À : The IESG
> Cc : [e-mail]; Liang Xia; dots-
> [e-mail]; [e-mail]; [e-mail]
> Objet : Adam Roach's Discuss on draft-ietf-dots-signal-channel-31: (with
> DISCUSS and COMMENT)
> 
> Adam Roach has entered the following ballot position for
> draft-ietf-dots-signal-channel-31: 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-dots-signal-channel/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Thanks for all the work everyone put into this document. I think it's great
> to
> have a solution defined for automating these kinds of operations, and look
> forward to widespread deployment of this technology. I do have a small number
> of comments that I think we need to close on prior to publication, and a
> handful of other suggestions of varying (but lesser) importance.
> 
> ---------------------------------------------------------------------------
> 
> This is a discuss because the current document attempts to override normative
> language in an external document.
> 
> §4.3:
> 
> >  In reference to Figure 4, the DOTS client sends two TCP SYNs and two
> >  DTLS ClientHello messages at the same time over IPv6 and IPv4.
> 
> This is problematic for the reason described in RFC 8305 §5 ("In order to
> avoid unreasonable network load, connection attempts SHOULD NOT be made
> simultaneously").
> 

[Med] I would agree with you if the text in 8305 is using MUST NOT. 

It is completely fine to send requests sequentially when not attack is ongoing, but if a connection is to be established during attack time (e.g., redirect), sending the requests simultaneously would be appropriate so that an attack mitigation is placed as ASAP.     


> To be clear, my discuss is only on the fact that this document violates a
> normative statement in RFC 8305. The following comments are merely my
> thoughts
> on the best way to resolve this issue.
> 
> It's also worth noting that RFC 8305 is geared towards getting users the
> fastest
> possible response to a user action, while the text in DOTS implies that the
> selection of the "most preferred" connection is significantly more important

[Med] Yes. 

> (e.g., it talks about migrating from TCP to UDP, and performing periodic
> checks
> to enable such a migration). This factor, combined with the fact that this is
> not a transaction that involves user interactivity requirements, would seem
> to
> increase, rather than decrease, the desire to space out checks across the
> various transport/address-family pairs.
> 
> My strong recommendation would be remove the specific description of

[Med] This is not that straightforward because of specifics to the DOTS case, e.g., probing, caching, no need to cancel other connections if a first one wins, etc. 
 
> happy-eyeballs-like behavior from this document, and to instead defer all
> such
> specification to the text in RFC 8305. I would further recommend
> specification
> of a "Connection Attempt Delay" (as that term is defined in RFC 8305) that is
> substantially larger than those used for interactive connections: something
> on
> the order of 2 to 5 seconds would be my suggestion.
> 
> Of course, be sure to adjust the example to match the specified handling.
> 
> 
> ---------------------------------------------------------------------------
> 
> This is a discuss because it impacts interoperability.
> 
> §4.4.1:
> 
> >  target-uri:   A list of Uniform Resource Identifiers (URIs) [RFC3986]
> >     identifying resources under attack.
> 
> This definition needs to be clearer about what parts of the URI are used for
> what purposes.

[Med] The text does not define any restriction. All parts may be included. 

The text focuses on the host part as this is sensitive: 

      The same validation checks used for 'target-fqdn' MUST be followed
      by DOTS servers to validate a target URI.

How other parts are used is trivial, IMO. 

 For example:
> 
>  - The URI scheme can be taken to specify a 'target-protocol'
>  - The URI host can be taken to specify a 'target-fqdn' or 'target-prefix'
>  - The URI port (or scheme, if absent) can be taken to specify a
>    'target-port-range'
> 
> It is unclear whether this specification intends the URI to impact one, two,
> or
> all three of these. This can result in a client asking for one thing and the
> server doing something else.
> 
> ---------------------------------------------------------------------------
> 
> This is a discuss because it impacts interoperability.
> 
> §6:
> 
> The handling of 64-bit values in Table 4 seems problematic. Section 3
> specifies:
> 
> >  Representing these data
> >  as CBOR data can either follow the rules in [I-D.ietf-core-yang-cbor]
> >  or those in [RFC7951] combined with JSON/CBOR conversion rules in
> >  [RFC7049]; both approaches produce a valid encoding.
> 
> However, if we consider, say, mitigation-start:
> 
> >  +----------------------+-------------+-----+---------------+--------+
> >  | Parameter Name       | YANG        | CBOR| CBOR Major    | JSON   |
> >  |                      | Type        | Key |    Type &     | Type   |
> >  |                      |             |     | Information   |        |
> >  +----------------------+-------------+-----+---------------+--------+
> > ...
> >  | mitigation-start     | uint64      |  15 | 0 unsigned    | String |
> 
> 
> If an implementation follows the first path (draft-ietf-core-yang-cbor), then
> this value is sent on the wire as a CBOR 64-bit unsigned integer type. If an
> implementation instead uses RFC 7951 followed by RFC 7049 §4, the resulting
> value is encoded on the wire as a CBOR string.

[Med] Both are valid. 

> 
> If this is the intention, then it represents a huge gotcha for implementors
> of
> both clients and of servers, as all implementations must be ready to accept
> both strings and 64-bit data types for these fields. 

[Med] No, an implementer does not need to do the mapping. An implementer only needs to rely upon the mapping table provided in Section 6.

If this is the
> intention, please add strongly-worded text warning implementors of this
> particular gotcha, since it's pretty non-obvious.
> 

[Med] We do already have the following: 

   All parameters in the payload of the DOTS signal channel MUST be
   mapped to CBOR types as shown in Table 4 ...

> It's worth noting that, while some implementations may set limits on the
> precision of JSON Numbers, and that such limits may be smaller than 64 bits,
> nothing in the format defined by RFC 8259 has any such inherent limitations.
> 
> There is a similar (but subtly different) problem with the handling of
> enumerations that may cause them to be encoded as either strings or as
> integers:
> 
> >  | status               | enumeration |  16 | 0 unsigned    | String |
> 
> If you choose to maintain the situation currently described in the document,
> then the table in section 9.6.1.2 needs to be updated to allow both formats;
> e.g.:
> 
> >  +----------------------+-------+-------+------------+---------------+
> >  | Parameter Name       | CBOR  | CBOR  | Change     | Specification |
> >  |                      | Key   | Major | Controller | Document(s)   |
> >  |                      | Value | Type  |            |               |
> >  +----------------------+-------+-------+------------+---------------+
> ...
> >  | mitigation-start     |   15  | 0 or 3|    IESG    |   [RFCXXXX]   |
> 

[Med] This will lead to interoperability problems. We just need to pick one and record it in the spec.  

> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> §3:
> 
> >  The other method
> >  DOTS agents use to indicate that a CBOR data structure is a DOTS
> >  signal channel object is the use of the "application/dots+cbor"
> >  content type (Section 9.3).
> 
> Nit: "...structure as a..."
> 
> ---------------------------------------------------------------------------
> 
> §4.4.1:
> 
> >     Figure 6: PUT to Convey DOTS Mitigation Requests (Message Body
> >                                 Schema)
> 
> This seems to be a very informal sketch rather than a formal schema.
> It's probably worth calling forward to Section 6 as the formal definition of
> the schema, and being clearer that this figure (and other similar figures)
> are
> non-normative sketches of the rough structure.
> 

[Med] Good point. Done. 

> ---------------------------------------------------------------------------
> 
> §4.4.1:
> 
> >     Implementations SHOULD set 'cuid' to the output of a cryptographic
> >     hash algorithm whose input is the Distinguished Encoding Rules
> >     (DER)-encoded Abstract Syntax Notation One (ASN.1) representation
> >     of the Subject Public Key Info (SPKI) of the DOTS client X.509
> >     certificate [RFC5280], the DOTS client raw public key [RFC7250],
> >     or the "Pre-Shared Key (PSK) identity" used by the DOTS client in
> >     the TLS ClientKeyExchange message.  In this version of the
> >     specification, the cryptographic hash algorithm used is SHA-256
> >     [RFC6234].  The output of the cryptographic hash algorithm is
> >     truncated to 16 bytes; truncation is done by stripping off the
> >     final 16 bytes.  The truncated output is base64url encoded.
> 
> It's not clear how much of this is a recommendation and how much of it is
> mandatory.

[Med] This is not mandatory, but a recommended approach for the reasons detailed in a new appendix we added to the draft: 

==
Appendix A.  CUID Generation

   The document recommends the use of SPKI to generate the 'cuid'.  This
   design choice is motivated by the following reasons:

   o  SPKI is globally unique.

   o  It is deterministic.

   o  It allows to avoid extra cycles that may be induced by 'cuid'
      collision.

   o  DOTS clients do not need to store the 'cuid' in a persistent
      storage.

   o  It allows to detect compromised DOTS clients that do not adhere to
the 'cuid' generation algorithm.
==

 For example: if I'm a server, would it be reasonable for me to
> expect
> the CUID to be BASE-64 (e.g., to minimize the size of an index structure) and
> error out if it contains characters other than those allowed in base64url?
> 
> It's also not clear that "SHOULD" is entirely called for here, especially
> since
> the following paragraph's statement that 'cuid' can vary based on remote host
> (which requires a variation from the normative algorithm). Consider the
> definition of SHOULD:
> 
>   3. SHOULD   This word, or the adjective "RECOMMENDED", mean that there
>      may exist valid reasons in particular circumstances to ignore a
>      particular item, but the full implications must be understood and
>      carefully weighed before choosing a different course.
> 
> This seems potentially stronger than what is called for here. Consider
> specifying this behavior non-normatively, and reserve the normative language
> for
> the actual requirement (e.g., "MUST be globally unique")
> 

[Med] That is an option, but we wanted a recommendation for the reasons listed in the NEW appendix. 

> ---------------------------------------------------------------------------
> 
> §4.4.1:
> 
> >     As a reminder, the prefix length must be less than or equal to 32
> >     (resp. 128) for IPv4 (resp.  IPv6).
> 
> This is a notation that I haven't seen before, and it may serve to confuse
> implementors. Consider rephrasing as: "...less than or equal to 32 for IPv4,
> and
> less than or equal to 128 for IPv6."

[Med] OK. Changed to "to 32 (or 128) for IPv4 (or IPv6)."

> 
> ---------------------------------------------------------------------------
> 
> §4.4.1:
> 
> >  This PUT request MUST use the
> >  same 'mid' value, and MUST repeat all the other parameters as sent in
> >  the original mitigation request apart from a possible change to the
> >  lifetime parameter value.
> 
> Is the server intended to detect violations of this? How would it react?

[Med] The spec leaves it to the server to decide how to handle the refresh (policy-based):

   If the DOTS server finds the 'mid' parameter value conveyed in the
   PUT request in its configuration data bound to that DOTS client, it
   MAY update the mitigation request, and a 2.04 (Changed) response is
   returned to indicate a successful update of the mitigation request.


> (Section 4.4.3 says to send a 4.00 if the request was an Efficacy Update;
> this
> is probably the correct general answer, and should be specified here).
> 
> ---------------------------------------------------------------------------
> 
> §4.4.2:
> 
> >  {
> >    "ietf-dots-signal-channel:mitigation-scope": {
> >      "scope": [
> >        {
> >          "mid": 12332,
> >          "mitigation-start": "1507818434",
> >          "target-prefix": [
> >               "2001:db8:6401::1/128",
> >               "2001:db8:6401::2/128"
> >          ],
> >          "target-protocol": [
> >            17
> >          ],
> >          "lifetime": 1756,
> >          "status": "attack-successfully-mitigated",
> >          "bytes-dropped": "134334555",
> >          "bps-dropped": "43344",
> >          "pkts-dropped": "333334444",
> >          "pps-dropped": "432432"
> >        },
> >        {
> >          "mid": 12333,
> >          "mitigation-start": "1507818393",
> >          "target-prefix": [
> >               "2001:db8:6401::1/128",
> >               "2001:db8:6401::2/128"
> >          ],
> >          "target-protocol": [
> >            6
> >          ],
> >          "lifetime": 1755,
> >          "status": "attack-stopped",
> >          "bytes-dropped": "0",
> >          "bps-dropped": "0",
> >          "pkts-dropped": "0",
> >          "pps-dropped": "0"
> >        }
> >      ]
> >    }
> >  }
> 
> I get that this is intended to be an informal example, but the use of quotes
> around values that are formally encoded as integers -- such as
> "mitigation-start" -- may trip up users.

[Med] We are using JSON encoding similar to RFC7951. We will double check how to make this clear in the document.


 I would strongly suggest removing
> quotes from anything encoded as an integer.
> 
> This applies especially to "status". Since these figures have explicitly been
> called out as JSON diagnostic notation (and *not* the JSON structure produced
> by RFC 7951), the values need to be the CBOR values sent on the wire. (This
> relies on an assumption on my part that may be incorrect, depending on the
> resolution of the points I raise about section 6 in my DISCUSS).
> 
> This comment applies to several other examples, such as Figure 16.
> 
> For Figure 20 (and several subsequent figures), it also applies to those
> values encoded as decimals, such as "max-value-decimal".
> 
> ---------------------------------------------------------------------------
> 
> §4.4.2:
> 
> >  bps-dropped:  The average number of dropped bytes per second for the
> >     mitigation request since the attack mitigation is triggered.  This
> >     average SHOULD be over five-minute intervals.
> 
> The first sentence conflicts with the second:
>  - The first sentence says this is the average since mitigation was
> triggered.
>  - The second sentence says this is a five-minute (rolling?) average.
> 
> Please make them agree with each other.
> 
> If something more subtle is meant -- like measuring bytes into five-minute
> buckets and then averaging these buckets over the time since the mitigation
> was triggered -- then the text needs more detail to convey it.
> 

[Med] We meant something similar to https://tools.ietf.org/html/rfc5835#section-5.1. 

> This same comment applies to "pps-dropped," as well as the descriptions of
> both
> properties in §5.3.
> 
> ---------------------------------------------------------------------------
> 
> §5.1:
> 
> >  A DOTS signal
> >  message can either be a mitigation or a configuration message.
> 
> This is at odds with both the tree and the text in the YANG module in §5.3,
> both of which specify three message types rather than two (mitigation,
> configuration, and redirect).

[Med] OK.

> 
> ---------------------------------------------------------------------------
> 
> §5.1:
> 
> >          |     +--ro bps-dropped?            yang:zero-based-counter64
> ...
> >          |     +--ro pps-dropped?            yang:zero-based-counter64
> 
> These aren't counters in the way that RFC 6021 defines counters. Both need to
> be
> of type "yang:gauge64." This comment, of course, also bears on §5.3 as well
> as
> Table 4.

[Med] Hmm. We are using counter64 similar to the usage in the interface YANG module, for example.

> 
> ---------------------------------------------------------------------------
> 
> §6:
> 
> >  | trigger-mitigation   | boolean     |  45 | 7 bits 20     | False  |
> >  |                      |             |     | 7 bits 21     | True   |
> 
> You might consider cleaning up the JSON column here to just say "Boolean," as
> "True" and "False" are values rather than any of the six types defined in RFC
> 8259. 

[Med] I hear you, but we wanted to indicate the extact value corresponding the CBOR type. We prefer to leave it. Thanks.

 Similarly, to be consistent with the rest of the table, perhaps the
> CBOR column here should simply say "7 simple value". Failing that, consider
> at
> least labeling the CBOR values as "False" and "True" rather than "bits 20"
> and
> "bits 21".
>