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

[<[e-mail]>]

Hi Ben, 

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk [mailto:[e-mail]]
> Envoyé : mercredi 27 février 2019 05:10
> À : BOUCADAIR Mohamed TGI/OLN
> Cc : [e-mail]; [e-mail]
> Objet : Re: AD review of draft-ietf-dots-data-channel-25
> 
> 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.
> 

[Med] Sure. 

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

[Med] Your proposed wording can be added to the architecture I-D, IMHO.

FWIW, we do have this text in the protocol spec: 

   DOTS servers MUST verify that requesting DOTS clients are entitled to
   enforce filtering rules on a given IP prefix.  That is, only
   filtering rules on IP resources that belong to the DOTS client's
   domain can be authorized by a DOTS server.  The exact mechanism for
   the DOTS servers to validate that the target prefixes are within the
   scope of the DOTS client's domain is deployment-specific.

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

[Med] I hear you. As mentioned in my previous answer, this is covered in Section 4.5 of RFC8040:

   If the target resource represents a YANG leaf-list, then the PUT
   method MUST NOT change the value of the leaf-list instance.

   If the target resource represents a YANG list instance, then the key
   leaf values, in message-body representation, MUST be the same as the
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   key leaf values in the request URI.  The PUT method MUST NOT be used
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   to change the key leaf values for a data resource instance.


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

[Med] The conclusion is that how the client syncs with the server is implementation-specific. An implementation note may be added. 

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