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

[Benjamin Kaduk <[e-mail]>]

On Fri, Feb 15, 2019 at 09:36:26AM +0000, [e-mail] wrote:
> Hi Ben, 
> 
> Please see inline. 
> 

Thanks.  The -27 is in good enough shape that I've requested the IETF last
call; we can finish discussing the last few topics here in parallel.

> 
> > -----Message d'origine-----
> > De : Benjamin Kaduk [mailto:[e-mail]]
> > Envoyé : jeudi 14 février 2019 20:17
> > À : BOUCADAIR Mohamed TGI/OLN
> > Cc : [e-mail]; [e-mail]
> > Objet : Re: AD review of draft-ietf-dots-data-channel-25
> > 
> > Hi Med,
> > 
> > On Thu, Feb 14, 2019 at 02:31:26PM +0000, [e-mail] wrote:
> > > 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.
> > 
> > Whoops, my bad for not checking.
> > 
> > > 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.
> > 
> > Thanks.  (To be clear, I'm happy to go to bat for everyone with the IESG
> > for this sort of thing, I just need an argument to present that's not me
> > making things up.)
> > 
> > I don't know of a specific policy for YANG module authorship, so that part
> > should be okay as far as I know.
> > 
> > > >
> > > > 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.
> > 
> > I think it would be a somewhat better fit in the architecture document,
> > just to note that the "domain" is something that can concretely be
> > demarcated by IP addresses/prefixes (as opposed to a more nebulous concept
> > to aid conceptual understanding).
> > 
> 
> [Med] Let's add that text into the architecture I-D.

Okay.  Tiru had a follow-up about how servers can identify DOTS client's
domains, which is okay but not quite what I was aiming for.  In the text
Tiru quoted, we note that

   The DOTS server enforces authorization of DOTS clients' signals for
   mitigation.  The mechanism of enforcement is not in scope for this
   document, but is expected to restrict requested mitigation scope to
   addresses, prefixes, and/or services owned by the DOTS client's
   administrative domain, such that a DOTS client from one domain is not
   able to influence the network path to another domain.

I was hoping to see something like "For a given client (administrative)
domain, the DOTS server needs to be able to determine whether a given
resource is in that domain.  This could take the form of associating a set
of IP Prefixes per domain, for example."  But I'm open to being persuaded
that the existing text conveys the same meaning.

> > > >
> > > > 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]"
> > 
> > I guess I'm mostly just concerned about if there's anything that doesn't
> > translate directly, since the CoAP and HTTP constructs would have to
> > describe the formal validation in somewhat different terms (i.e., for
> > consistency between URL paths and message bodies).  But in general I'm
> > happy to avoid redundancy as you desire.
> > 
> > > 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.
> > 
> > Is this supposed to be RFC 8040 Section 3.5.3?  I am not sure that I'm
> > reading that as covering the property I want (and will double-check if you
> > confirm that's what you have in mind).
> > 
> 
> [Med] In addition to 3.5.3, Section 4.5 specifies the rules for PUT. 
> 
> The main point is that the validation you mentioned is not specific to DOTS but are governed by RESTCONF procedures.

I only see 3.5.3 as talking about how to construct a request URI for a
given data node in the YANG tree.

What I'm wondering about for the data-channel document is if there are
cases where a given path component of the YANG module appears both in the
request URI and in the data in the request body.  If so, it seems that the
server will need to verify that the redundant data agree, and I still don't
see where this is mandated by RFC 8040.  (Sorry if I am being dense.)

> > >   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.
> > 
> > Of course.
> > 
> > > >
> > > > 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.
> > 
> > Ah, thanks for the pointer.
> > 
> > > >
> > > > 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.
> > 
> > Thanks for the explanation.
> > 
> > > > 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.
> > 
> > Correct.
> > 
> > > The module allows to return other capabilities than those listed in table
> > 1.
> > 
> > Yes.  But the figure should not claim to be an example that *only* supports
> > the mandatory parts, when it shows an example that supports additional
> > capabilities than the mandatory ones.
> > 
> 
> [Med] Which "additional capabilities" are you referring to?  

(The text in the -27 addresses my complaint here, even if I was confused
about filtering criteria vs. actions.)

> 
> > > >
> > > > 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 :-)
> > 
> > My personal opinion is that it *is* harmful, since we are not using a
> > floating-point wire encoding; we're using a string encoding.
> 
> [Med] I removed the ref, but added a sentence to justify the unit we are using: mainly to align with traffic-rate in 5575.

OK

> > 
> > > 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.
> > 
> > (RFC 5575 seems to claim to be using the actual binary floating-point
> > encoding.)
> > 
> > >  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.
> > 
> > Okay, we can see if anyone else complains.
> > 
> > >  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.)
> > 
> > On further reflection, would "further manual investigation" be appropriate?
> 
> [Med] No assumption is made how that investigation is made. I prefer to leave the text as it. 

OK

> > 
> > > > 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.
> > 
> > I always have to consider edge cases in my IESG reviews -- what happens if
> > an attack starts after the request is sent but before the response is
> > received?
> 
> [Med] The dots client will know if its request is successfully delivered. When an attack is ongoing, the dots client should not use it data channel because it is likely to be perturbed.   

(There was some further discussion with Med and Tiru but I seem to have
lost track of the resolution.)

> > 
> > > >
> > > >    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.
> > 
> > I guess we'll just have to wait to see what kind of comments we get, then.
> > (Thanks for the pointer to 8071.)
> > 
> > > >
> > > >    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.
> > 
> > In effect a default is a recommendation, though -- a recommendation for
> > what to do if you don't know any better.  Perhaps "it is out of scope of
> > this specification to provide guidance on how conflicting requests may be
> > safely handled, with the default behavior being to reject such requests"?
> 
> [Med] I went for this updated text:  
> 
>    DOTS servers SHOULD support a configuration parameter to indicate the
>    behavior to follow when a conflict is detected (e.g., reject all,
>    reject the new request, notify an administrator for validation).
>    Section 7.2 specifies a default behavior when no instruction is
>    supplied to a DOTS server.

Works for me.

> > 
> > > 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
> > >                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > That helps (maybe the "and" is not even needed?).  Do you want to do a
> > sweep for other places where the text talks about 409 responses?
> 
> [Med] OK.
> 
> > 
> > >    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.
> > 
> > It seems that the default behavior of disallowing conflicts would always be
> > interoperable, but I'm happy to leave this as-is for now.
> > 
> > > >
> > > > 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].
> > 
> > Cool, thanks.
> > 
> > > >
> > > > 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.
> > 
> > Thanks for the pointer.   It sounds like I'll need to ask around about this
> > one.
> 
> [Med] I checked this with Martin Bjorklund. He confirmed that the error list in 6241 is fixed in the protocol version. He recommended to go with app-tags. Our error will look like the following:
> 
> OLD:
>       error-tag:      loop-detected
>       error-type:     transport, application
>       error-severity: error
>       error-info:     <via-header> : A copy of the Via header when
>                       the loop was detected.
>       Description:    An infinite loop has been detected when forwarding
>                       a requests via a proxy.
> 
> NEW:
> 
>        error-app-tag:  loop-detected
>        error-tag:      operation-failed
>        error-type:     transport, application
>        error-severity: error
>        error-info:     <via-header> : A copy of the Via header when
>                        the loop was detected.
>        Description:    An infinite loop has been detected when forwarding
>                        a requests via a proxy. 

Thanks!  I will try to remember to solicit a YANG doctors review during
IETF LC if it does not get scheduled automagically.

> > 
> > > 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.
> > 
> > Indeed.
> > 
> > > >
> > > > 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.
> > 
> > Ah, good to know.
> > 
> > > >
> > > > 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.";
> > 
> > Thanks!
> > 
> > > >
> > > > 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.
> > 
> > For names this makes natural sense to me, but this question is about a
> > typedef.  I have less clear of a picture for how typedefs are or are not
> > namespaced (is it just per-module?).
> > 
> 
> [Med] They are namespaced. For example: 
> 
>       type operator;
> 
> is about the operator defined in the module itself. But, if we use: 
> 
>        type packet-fields:operator;
> 
> this means the operator is the one imported from ietf-packet-fields.

Ah, thanks for the YANG lesson.

> 
> > > >
> > > >     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.
> > 
> > I was looking for a few more words in the text, like "must be set to 0 when
> > it appears in an IPv6 filter".
> 
> [Med] OK.
> 
> > 
> > > > 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.
> > >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Whoops, I clearly missed that part.  Thanks for calling it out.
> > 
> > > >
> > > > 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.
> > 
> > Ok.
> > 
> > > >
> > > > 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.
> > 
> > sounds good.
> > 
> > > >
> > > >    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".
> > 
> > Okay.  (I was literally just wondering if this was a typo.)
> > 
> > > >
> > > >       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.
> > 
> > That's a clever workaround!
> > 
> > > 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.
> > 
> > The wording here is still weird, though -- by calling out the
> > delayed-activation filters it implies that immediate-activation filters are
> > excluded from the statement, but we know that immediate-activation filters
> > also have the same property.   So I'm not entirely sure exactly what
> > information this sentence is intended to call out.
> 
> [Med] When a mitigation is in progress, only the filters bound to the client which triggered the mitigation have to be activated. 'activate-when-mitigating' filters of other clients of the same domain should not be activated: 
> 
> I changed the text as follows:
> 
> OLD:
>       Filters that are activated only when a mitigation is in progress
>       MUST be bound to the DOTS client which created the filtering rule.
> 
> NEW:
>       When a mitigation is in progress, the DOTS server MUST only
>       activate 'activate-when-mitigating' filters that are bound to the
>       DOTS client which triggered the mitigation.

That looks great; thanks.

-Ben

> > 
> > > > 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:
> > 
> > Oh.  I think I was remembering Section 5.2's "resources bound to this DOTS
> > client will be deleted by the DOTS server" and assuming it applies to the
> > filters associated with that client.
> > 
> > >    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.
> > 
> > This is probably enough to ensure safe operation without the above, though,
> > hmm...
> > 
> > > >
> > > >    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.
> > 
> > Okay.
> > 
> > > >
> > > >                                                  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.
> > 
> > Thanks for the pointer.
> > 
> > > >
> > > > 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.
> > 
> > Whoops, my misread.
> > 
> > > >
> > > > 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.
> > 
> > I would suggest adding a new sentence in the "All data nodes defined"
> > paragraph about "This module reuses YANG structures from
> > [I-D.ietf-netmod-acl-model], and the security considerations for those
> > nodes continue to apply for this usage.", since that text is at the other
> > end of the section and it's otherwise hard to make a linkage between them.
> > 
> 
> [Med] OK.
> 
> > Thanks,
> > 
> > Ben