[Dots] AD review of draft-ietf-dots-data-channel-25

[Benjamin Kaduk <[e-mail]>]

This is my AD review of the -25

I see that Med made a commit on github that preemptively addressed at least
one of these comments, but will trust the authors to do to the right thing
with anything in here that's stale.

A handful of generic and/or high-level comments before the
section-by-section nitty-gritty stuff.

The author count is large (7); RFC 7322 notes that the stream approver
(i.e., the IESG) will ask questions if the count is more than five.  What
should I tell people when they ask?  (The authors are listed also in the
YANG module itself, if changes are made.)

Can someone (the shepherd?) confirm that an automated syntax checker has
run over the JSON in examples?

The concept of "DOTS client domain" is being used for actual protocol
purposes here (most notably as a bound on the prefix(es) that a client can
request mitigation for, but I don't remember seeing a formal definition for
how any DOTS actor would know the specific bounds of such a client domain.
Is there text somewhere that I missed that we can point to, or will we need
to add some?

We don't say much about what validation the server does on input data, and
we probably should.  For example, does the server need to validate 'cuid'
and/or 'cdid' in dots-client-creation requests?  What about validating that
the (e.g.) ACL name in the PUT URL matching the name in the body of the
request?  There are probably others as well.

The examples all use "Host: {host}:{port}" which is not really an example
but rather a template.  Since they are supposed to be examples, we should
pick a concrete hostname (and port) to use.

There is, shall we say, a "high degree of overlap" between Sections 5/6/7
and the YANG model field descriptions.  I mostly assume that the WG
considered letting the YANG model (and the core RESTCONF spec) stand alone
without the additional examples/specification of the use of RESTCONF to
manage clients, aliases, and filter rules, so I won't suggest that we
revisit the question.  But I do think that it would be good to have some
explicit text acknowledging the overlap and stating which one is
authoritative.

There seems to be some mismatch between whether the IPv6 ACL subtree uses
"ttl" or "hoplimit" -- Figure 7 has "ttl" but the rest of the document
seems to (properly) use "hoplimit".  Someone else should double-check the
relevant places, though, as I'm not sure I was looking at all of them with
this issue in mind.

I'm also a bit curious about the use of an explicit "capabilities" tree for
fine-grained feature detection, as opposed to native YANG "feature"s.  The
latter would allow the relevant nodes to just not exist when unsupported,
as opposed to the explicit-capabilities formulation where they exist but
are (presumably) ignored.  (I don't remember us explictily saying that
they're ignored in this case, either; might be worth adding some text.)

In a similar vein, we include 'capabilities' nodes for a few matching
features that are listed as "mandatory fields" for ACL matching in Table 1,
and thus whose value would always be "true".  Why do we need the capability
leaves in such a case?

idnits notes a few references that can be updated along with the other
changes; it also flags a "reference in abstract" for the RFC Editor note
which we could probably ignore (but could probably also just remove the
brackets around the text in question).

I also looked at the references (especially the normative/informative
split) and have a few suggestions:

- the IEEE.754.1985 reference is not needed; we don't use the binary
  floating-point encoding but rather a string one for YANG decimal64
- I think that RFC 8499 (dnsop-terminology-bis) can wholly supersede our
  usage of RFC 1983, so the 1983 cite can be dropped as well
- draft-ietf-dots-requirements (for terminology), RFC 7950, and RFC 8259
  should probably all move to the normative section

Section 1

The sub-bullet point about "If a network resource ... informs its servicing
DOTS gateway of all suspect IP addresses that need to be drop- or
accept-listed ..." made me wonder if that was more of a signal-channel
activity or a data-channel one.  Perhaps this is just a matter of the text
not being as clear as it could be.  (I also wonder if we should say
"further investigation" since we don't really have an automated way to
indicate that yet.)

Section 2

When we talk about tree diagrams, should we also say something like "Note
that the full module's tree has been split across several figures to aid
the exposition of the various sub-trees"?

Section 3.1

When we talk about using GET to retrieve running configuration, do we want
to note that since the data channel can fail during attack time, it's
expected to be common to perform such a GET before attempting to make
modifications to configuration?

   A DOTS client registers itself to its DOTS server(s) in order to set
   up DOTS data channel-related configuration data and receive state
   data (i.e., non-configuration data) from the DOTS server(s)
   (Section 5).  Mutual authentication and coupling of signal and data
   channels are specified in [I-D.ietf-dots-signal-channel].

I think we should split the "mutual authentication" and "coupling of signal
and data channels" into their own sentence, and flesh them out slightly
more.  So, section references to Sections 8 and 4.4.1, respectively, the
usage of TLS client certificates, the coupling of channels via the client's
identity ('cuid' and 'cdid'), etc.

                                  A TLS heartbeat [RFC6520] verifies
   that the DOTS server still has TLS state by returning a TLS message.

I expect this will get some pointed comments during IETF LC, given the
recent-ish IETF-wide discussions about heartbeats/keepalives in general.
Is there perhaps a RESTCONF-level probe message that could be used to check
liveliness; a capabilities query perhaps?

   A DOTS server may detect conflicting filtering requests from distinct
   DOTS clients which belong to the same domain.  For example, a DOTS
   client could request to drop-list a prefix by specifying the source
   prefix, while another DOTS client could request to accept-list that
   same source prefix, but both having the same destination prefix.  It
   is out of scope of this specification to recommend the behavior to
   follow for handling conflicting requests (e.g., reject all, reject
   the new request, notify an administrator for validation).  DOTS
   servers SHOULD support a configuration parameter to indicate the
   behavior to follow when a conflict is detected.  Section 7.2
   specifies the behavior when no instruction is supplied to a DOTS
   server.

I'm a bit confused by the "out of scope of this specification to recommend
the behavior to follow for handling conflicting requests", since not only
does the last sentence of the paragraph give a pointer to a specified
behavior in case of conflicts, but we also mention it in a couple other
places (e.g., the bottom of page 43).

Also in that paragraph, it's unclear that a 2119 SHOULD is appropriate for
"support a configuration parameter"; there's no interoperability
considerations for having or not having such a configuration knob.

Section 3.3 NAT Considerations have "high overlap" with the
text at the end of the signal-channel's "Design Overview".  At a minimum we
should diff them and enforce convergence, but do we want to consider just
having one refer to the other?

Section 3.5

I had to read up on RESTCONF and NETCONF while reviewing this, but from
what I've seen so far, the "error-severity" field is only present in
NETCONF and not RESTCONF.  If so, then we shouldn't need to talk about it
here, since we use RESTCONF exclusively.  I also couldn't find whether
there's a registry that we should add the "loop-detected" error-tag to.
Can anyone here help me out?

Section 4.2

Is there any plan/expectation for filtering/match rules for QUIC?  It is
probably premature to put anything in this document specifically, but still
worth thinking about.

The order in which the leaves appear in the "capabilities/ipv6" and
"capabilities/tcp" subtrees do not match the order they appear in the ACL
subtree itself; it would be good to keep the order consistent.

We don't really say much about what the semantis of the "port-range"
capabilities are; is that supposed to indicate any port-matching ability at
all, or specifically the low-to-high range matching, or also the "operator"
options?

Section 4.3

We define an "operator" typedef that is rather different from that from
netmod-acl-model.  Do we want to use a different name to aid
disambiguation?  ("bitmask-operator" comes to mind as an option.)

    typedef fragment-type {
      type bits {
        bit df {
          position 0;
          description
            "Don't fragment bit for IPv4.
             This bit must be set to 0 for IPv6.";

Set to zero by whom?  What should the receiver do if it is set otherwise?

What are the semantics if (either or both of target-fqdn and target-uri)
and target-prefix are specified?  (I suppose maybe this could be covered in
Section 6.1 when we say that at least one is required in ACL-creation
requests.)

The units for the "rate-limit" node should be specified in the YANG module
and not in the description of how to install filtering rules.

      list dots-client {
        key "cuid";
        description
          "List of DOTS clients.";
        leaf cuid {
          type string;
          description
            "A unique identifier that is randomly generated by
             a DOTS client to prevent request collisions.";

I don't think "randomly generated" is really correct here.

The "capabilities/icmp/rest-of-header" description should be more clear
that (per draft-ietf-netmod-acl-model) it is supposed to match both the
ICMP four-byte field and the ICMPv6 message body.

Section 5.1

It may be worth reiterating that (per the signal-channel doc) gateways may
rewrite the 'cuid'.

When POST is used to create a dots-client resource, how does the client
know the path of the created resource (to be used for subsequent RESTCONF
requests)?  (I assume it is supposed to just use the submitted 'cuid', but
we need to explicitly say this.  This also seems to render much of the
distinction between POST and PUT moot, for our usage, though I do not
propose any change to the text.)

   The DOTS gateway, that inserted a 'cdid' in a PUT request, MUST strip
   the 'cdid' parameter in the corresponding response before forwarding
   the response to the DOTS client.

Why does this apply only to PUT and not POST?

Section 6.1

   DOTS clients within the same domain can create different aliases for
   the same resource.

My reading of this text is that client A can create alias "foo" for IP
prefix 128.0.1.5/31 and clinet B can create alias "bar" for the same IP
prefix, and that DOTS supports that process.  (Just to confirm that the
text is saying what it's intended to.)  I do wonder if we want to say
something about whether alias names need only be unique per 'cuid' or in
some more global fashion.  (Having a global uniqueness property is perhaps
convenient in order to interface with non-DOTS mechanisms for querying
aliases, or for the DOTS server to interact with network filtering
appliances.)

Figure 16 puts double-quotes around "string" in the pseudo-schema, which
feels weird to me.

   target-fqdn:   A list of Fully Qualified Domain Names (FQDNs).  An
      FQDN is the full name of a resource, rather than just its
      hostname.  For example, "venera" is a hostname, and
      "venera.isi.edu" is an FQDN [RFC1983].  Refer to
      [I-D.ietf-dnsop-terminology-bis] for more details.

I don't think this text is particularly well-aligned with RFC 8499 and
would suggest trimming it substantially and just pointing to that RFC.

   If the request is missing a mandatory attribute or its contains an
   invalid or unknown parameter, "400 Bad Request" status-line MUST be
   returned by the DOTS server.  The error-tag is set to "missing-
   attribute", "invalid-value", or "unknown-element" as a function of
   the encountered error.

This text seems to preclude any future extension that adds new error tags;
is this intended?

Section 7.1

   A DOTS client which issued a GET request to retrieve the filtering
   capabilities supported by its DOTS server, SHOULD NOT request for
   filtering actions that are not supported by that DOTS server.

What is the server behavior if the client ignores this SHOULD NOT?

   Figure 23 shows an example of a response received from a DOTS server
   which only supports the mandatory filtering criteria listed in
   Section 4.1.

This seems inaccurate, as the mandatory filtering criteria exlude the
rate-limit among others.

Section 7.2

   activation-type:  Indicates whether an ACL has to be activated
      (immediately or during mitigation time) or instantiated without
      being activated (deactivated).  Deactivated ACLs can be activated
      using a variety of means such as manual configuration on a DOTS
      server or using the DOTS data channel.

Is this done by the data channel or the signal channel?

      If this attribute is not provided, the DOTS server MUST use
      'activate-when-mitigating' as default value.

Why do we specify default values here instead of in the YANG module?

      Filters that are activated only when a mitigation is in progress
      MUST be bound to the DOTS client which created the filtering rule.

Other filters do not need to be bound to the DOTS client that created them?
Wouldn't we still want to remove them when the dots-client resource in
question is deleted?

   destination-ipv4-network:  The destination IPv4 prefix.  DOTS servers
      [...]
      This is a mandatory attribute in requests with an 'activation-
      type' set to 'immediate'.

I somehow thought there were YANG attributes that could indicate this
conditional requirement in the module itself, though I am hardly a YANG
expert.

                                                 The error-tag is set to
   "missing-attribute", "invalid-value", or "unknown-element" as a
   function of the encountered error.

Same comment as above about (non-)extensibility.

Section 7.3

I see that the "test-acl-*" and "test-ace-*" acl and ace objects in these
examples do in fact use different names for the semantically different
objects, but perhaps we could help the reader's eye and use strings with a
larger Hamming distance?  (I thought they were all the same on my first
read.)

(I also am a little confused at why the "ace" "name" field is considered a
non-config field, in Figure 31, but this seems more likely to be my
confusion than an error in the document.)

Section 8

   o  DOTS server MUST NOT enable both DOTS data channel and direct
      configuration to avoid race conditions and inconsistent
      configurations arising from simultaneous updates from multiple
      sources.

   o  DOTS agents SHOULD enable DOTS data channel to configure aliases
      and ACLs, and only use direct configuration as a stop-gap
      mechanism to test DOTS signal channel with aliases and ACLs.
      Further, direct configuration SHOULD only be used when the on-path
      DOTS agents are within the same domain.

Doesn't all this discussion of "direct configuration" conflict with the
"MUST NOT" in the first bullet point?

Section 10

Generally with the security considerations template for YANG modules, we
need to list out all the nodes considered sensitive and the consequences of
writing(/reading) each one in turn.