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

[<[e-mail]>]

Hi Ben, 

Thank you for the review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk [mailto:[e-mail]]
> Envoyé : mercredi 13 février 2019 17:46
> À : [e-mail]
> Cc : [e-mail]
> Objet : AD review of draft-ietf-dots-data-channel-25
> 
> 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.)

[Med] Actually, the set of co-authors of the YANG module is not exactly the same.

Anyway, in order to comply with the rules and avoid spending extra cycles on this, we added a new section called "Contributing Authors". Nevertheless, the set of authors is not modified in the YANG module.  

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

[Med] Both "DOTS client domain" and "DOTS server domain" are used in the architecture draft. "DOTS client's domain" and "DOTS server's domain" are also used in the architecture and requirements I-D.

If a formal definition is needed, I prefer this to be done in the architecture or the requirements I-D.

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

[Med] We avoided to be redundant here. This is covered by this text: 

"This attribute has the same
      meaning, syntax, and processing rules as the 'cuid' attribute
      defined in [I-D.ietf-dots-signal-channel]."

> and/or 'cdid' in dots-client-creation requests?

[Med] This is covered by this text: 

"This attribute has the same meaning, syntax, and processing
      rules as the 'cdid' attribute defined in
      [I-D.ietf-dots-signal-channel]"

And other parts in the text such as:

   If the request is received via a server-domain DOTS gateway, but the
   DOTS server does not maintain a 'cdid' for this 'cuid' while a 'cdid'
   is expected to be supplied, the DOTS server MUST reply with "403
   Forbidden" status-line and the error-tag "access-denied".  Upon
   receipt of this message, the DOTS client MUST register (Section 5).


  What about validating that
> the (e.g.) ACL name in the PUT URL matching the name in the body of the
> request?

[Med] Those are supposed to be covered following RESTCONF base spec.  

  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.

[Med] I will change some figures to "example.com" but will leave it for schema-like figures. 

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

[Med] Fixed. 

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

[Med] Both are correct. 

Figure 7 reuses draft-ietf-netmod-acl-model which defines TTL as : 

    leaf ttl {
      type uint8;
      description
        "This field indicates the maximum time the datagram is allowed
         to remain in the internet system.  If this field contains the
         value zero, then the datagram must be dropped.

         In IPv6, this field is known as the Hop Limit.";
      reference
        "RFC 791: Internet Protocol,
         RFC 8200: Internet Protocol, Version 6 (IPv6) Specification.";
    }

For the other figures, the fields are defined in the data-channel draft.  

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

[Med] These are not serving the same purpose. Features are about parts of a module which are conditional, if you will. Our "capabilities" branch is meant to retrieve the filtering match capabilities are supported/enabled by a DOTS server. That information is used by a client to tweak its requests.

  The
> latter would allow the relevant nodes to just not exist when unsupported,

[Med] A filtering capability may be supported but not enabled. The server is free to include an explicit field with the appropriate status or not. This is implementation-specific. 

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

[Med] I guess you are referring to Figure 23 which says the following:

   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.

The module allows to return other capabilities than those listed in table 1. 

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

[Med] idnits confuses the note to the RFC editor with the abstract. Fixed, anyway.

> 
> I also looked at the references (especially the normative/informative
> split) and have a few suggestions:
> 
> - the IEEE.754.1985 reference is not needed;

[Med] Agree, but it is not harmful to cite it either :-) 

The reference is quoted to justify why we went with decimal64, not uint64 for example + why that unit is chosen. This allows to ease grafting DOTS with BGP Flowspec for instance, which cites IEEE.754.1985 too. 

 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

[Med] Done. 

> - draft-ietf-dots-requirements (for terminology),

[Med] For this one, we are following the same approach as for other terminology documents (e.g., RFC8340) that are listed as informative. I prefer to leave it as informative for now.  

 RFC 7950, and RFC 8259

[Med] OK for these two. 

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

[Med] The point is that a client is not sure that an attack is active and targeting the client domain but it wants to enforce some preventive actions during an investigation period. For example, this can be triggered by an administrator who is informed that an attack is currently targeting other networks, but its network is likely to be subject to that attack too. Other preventive actions which require further investigation may be considered.  

I changed the text as follows: 

OLD:
  detects a potential DDoS attack from

NEW
   is informed about a potential DDoS attack from 

 (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"?

[Med] Done. Thanks. 

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

[Med] The data channel is supposed to be invoked when no attack is ongoing. Normal RESTCONF operations are therefore followed. I don't see the need to add a note. 

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

[Med] Done. 

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

[Med] There is no such mechanism. The approach in the data channel draft is aligned with RFC8071 for keepalives.

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

[Med] The specified behavior is a default behavior, not a recommended one.

I update this text: 

   If the request is conflicting with an existing filtering installed by
   another DOTS client of the domain, and absent any local policy, the
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  
   DOTS server returns "409 Conflict" status-line to the requesting DOTS
   client.  The error-tag "resource-denied" is used in this case.

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

[Med] This is important for interoperability (or at least for ensuring a deterministic behavior). E.g., during service subscription a technical clause may be negotiated out of band how to deal with conflicts from clients of the same domain. 

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

[Med] Makes sense. Section 3.3 is now deleted and replaced with this NEW text:

   NAT considerations for the DOTS data channel are similar to those
   discussed in Section 3 of [I-D.ietf-dots-signal-channel].

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

[Med] We used the template in https://tools.ietf.org/html/rfc6241#appendix-A because the errors in 8040 are the ones imported from there. 

There is no registry for the errors; only a YANG module which is not maintained by IANA. This is why no action is included in the IANA section. 

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

[Med] Some of the existing fields can be already reused for QUIC (UDP, port number). There are few fields in the QUIC public header that are available (public flags, CID, version). A match on the first N bytes of the UDP payload can be considered but I do think this is not mature enough. 

The key point is that our module is extensible. 

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

[Med] Fixed. 

FWIW, the order in capabilities/* follows the order of the fields as they appear in the header. We don't have a control on the ACL order because we are reusing an external module.  

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

[Med] Updated as follows: 

OLD:
              "Support of filtering based on a port range.";

NEW:

             "Support of filtering based on a port range.

              This includes filtering based on a source port range,
              destination port range, or both. All operators
              (i.e, less than or equal, greater than or equal, equal to,
              and not equal to) are supported.";

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

[Med] It is not recommended in yang to use prefixes to disambiguate nodes. The disambiguation is ensured by the context/position in the tree. For example, this is why we are using name and not acl-name or ace-name, etc. I will leave the text as it is. 

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

[Med] In IPv6, fragmentation is only done by the source. Hence, this value for IPv6. A fragment-type with the first bit set will be discarded by the server.  

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

[Med] The resulting IP prefixes/addresses will be bound to the alias. Added the following in Section 6.1:

   If more target-* clauses (e.g., 'target-prefix' and 'target-fqdn')
   are included in a POST or PUT request, the DOTS server binds all
   resulting IP addresses/prefixes to the same resource.

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

[Med] Added "units" to the YANG module.

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

[Med] Changed to: 

         "A unique identifier that is generated by a DOTS client
          to prevent request collisions.";

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

[Med] OK.

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

[Med] OK: 

   As a reminder, DOTS gateways may rewrite the 'cuid' used by peer DOTS
   clients (Section 4.4.1 of [I-D.ietf-dots-signal-channel]).

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

[Med] The procedure to determine the root path is recalled in Section 3.1, then normal RESTCONF operation is followed.

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

[Med] Because RFC8040 says the following: 

   If the POST method succeeds, a "201 Created" status-line is returned
   and there is no response message-body.
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 

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

[Med] Yes.

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

[Med] The specification does require uniqueness per cuid: 

        |  +--rw aliases
          |  |  +--rw alias* [name]
          |  |     +--rw name                 string

We don't have a requirement for uniqueness per domain or globally. 

If such requirement arise, the semantic/logic for creating those aliases can be handled out of band. 

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

[Med] This is also what we have done for other figures , e.g., 11. The intent is to use a scheme-like message body while trying to preserve JSON compliance.   

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

[Med] Done. 

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

[Med] Those errors are only about the tree failure cases called in the first sentence. 

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

[Med] This is covered by this text: 

   If the request is missing a mandatory attribute or
   contains an invalid or unknown parameter (e.g., a match field not
   supported by the DOTS server), "400 Bad Request" status-line MUST be
   returned by the DOTS server in the response. 

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

[Med] rate-limit is an action, not a filtering criteria. 

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

[Med] Yes, but this is not supported in the base signal-channel spec. This is the done using the filtering control feature (draft-nishizuka-dots-signal-control-filtering). This is why signal channel is not listed after "such as". 

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

[Med] There is no default statement for enumeration. To solve this, a new type is defined in the module (activation-type). This type is then used for the activation-type leaf with a default value set to activate-when-mitigating. 

The change in tree diagram is (no need to insert the code here):

OLD:
       |        +--rw activation-type?    Enumeration

NEW:
     |        +--rw activation-type?    activation-type


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

[Med] They are...but those filters are already enforced because they are immediate. 

> Wouldn't we still want to remove them when the dots-client resource in
> question is deleted?

[Med] This is supposed to be done by the client itself. For amnesiac clients, we do have the following: 

   In order to avoid stale entries, a lifetime is associated with alias
   and filtering entries created by DOTS clients.  Also, DOTS servers
   may track the inactivity timeout of DOTS clients to detect stale
   entries.

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

[Med] We are reusing fields from ietf-netmod-acl, it is not easy to manipulate the fields as we would want.

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

[Med] Idem as above.

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

[Med] Fixed. 

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

[Med] This is because the name is the key of the ace list. 

RFC8040 says the following: 

   To retrieve only the non-configuration child resources, the "content"
   parameter is set to "nonconfig".  Note that configuration ancestors
   (if any) and list key leafs (if any) are also returned.

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

[Med] The second bullet is about controlled testing of the *signal channel* with aliases/acls communicated by the data channel.

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

[Med] The text does already call out those that are specific to this document. For other nodes, we do have this text: 

   "YANG ACL-specific security
   considerations are discussed in [I-D.ietf-netmod-acl-model]."

I think we are OK.