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

[Adam Roach via Datatracker <[e-mail]>]

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").

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

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

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


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

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

§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. 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")

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

§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."

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

§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?
(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. 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.

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

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

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

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

§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.  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".