Re: [Dots] AD review of draft-ietf-dots-telemetry-16 (Sections 8-10)

[[e-mail]]

Re-,

You can track the changes at: https://tinyurl.com/dots-telemetry-latest.

Please see inline for more context for Sections 8 to 10. 

Cheers,
Med

> -----Message d'origine-----
> De : Dots <[e-mail]> De la part de Benjamin Kaduk
> Envoyé : mercredi 24 novembre 2021 23:42
> À : [e-mail]
> Cc : [e-mail]
> Objet : [Dots] AD review of draft-ietf-dots-telemetry-16
> 
> Hi all,
...
> 
> Section 8.2
> 
>    As defined in [RFC8612], the actual mitigation activities can include
>    several countermeasure mechanisms.  The DOTS server signals the
>    current operational status of relevant countermeasures.  A list of
>    attacks detected by each countermeasure MAY also be included.  The
> 
> I see how in stock DOTS signal channel, the server signals whether an
> attack is fully/partially/not mitigated, but I don't see how the server
> indicates the specific relevant countermeasures that were used.  Am I just
> missing something there?

[Med] The text does not say that we signal the measures themselves. I think you were confused by "... each countermeasure ..."

  (Similarly, the list of attacks we returned may
> or may not be at the granularity of "detected by each countermeasure",
> depending on the ansewr to the previous part.)

[Med] s/each countermeasure/these countermeasures

> 
> Also (editorial), I think we should be more clear about when we switch
> from describing RFC 9132 behavior to to describing the new functionality
> provided by this document.
> 
>    Figure 46 shows an example of an asynchronous notification of attack
>    mitigation status from the DOTS server.  This notification signals
> 
> Is the "new stuff" in Figure 46 just the subtree under :(server-to-client-
> only)? 

[Med] What is new is what is indicated right after the text you quoted: 

" This notification signals
   both the mid-percentile value of processed attack traffic and the
   peak count of unique sources involved in the attack. "


this can be identified in the figure by looking at items that are prefixes with "ietf-dots-telemetry:".


 The 'total-attack-traffic' leaf seems to be the same content as in
> Figure 45, and maybe could be omitted?

[Med] Please note this is not for the same mid. Putting that aside, the one in Figure 45 is what is seen by the client, while figure 46 as about the server's view. Changed the total attack traffic value to avoid inducing readers to assume a correlation between the two.  

> 
>            "mitigation-start": "1507818434",
> 
> (This date is from 2017; as above, we could use a more current one if we
> want.)

[Med] This was on purpose so easily identify the telemetry addition vs base spec as depicted in Figure 14 of RFC9134. 

> 
>                "source-count": {
>                  "peak-g": "10000"
> 
> (Suspiciously round numbers like this are indications of fabricated data
> ... which is totally reasonable for an example like this!  But if we
> wanted to be "more realistic" we could use a less-round number.)

[Med] Agree. Updated. 

> 
>    DOTS clients can filter out the asynchronous notifications from the
>    DOTS server by indicating one or more Uri-Query options in its GET
>    request.  A Uri-Query option can include the following parameters:
>    'target-prefix', 'target-port', 'target-protocol', 'target-fqdn',
>    'target-uri', 'alias-name', and 'c' (content) (Section 4.2.4).  The
> 
> Up in §7.3 the analogous list also included 'mid'.  Is 'mid' appropriate
> here (or is it implicitly already included by the base signal channel
> mechanisms)?

[Med] Unlike 7.3, the filtering is happing within a request that includes mid as an uri path.

  Hmm, but maybe 'c' is also already allowed by the base
> signal channel mechanism, so that theory is not great...
> 
>    If the target query does not match the target of the enclosed 'mid'
>    as maintained by the DOTS server, the latter MUST respond with a 4.04
>    (Not Found) error Response Code.  The DOTS server MUST NOT add a new
>    observe entry if this query overlaps with an existing one. 
> 
> How should the server respond if this query overlaps with an existing one?
> 

[Med] We don't mention this as this is already handled by the base spec:

   4.09 (Conflict)  is returned by the DOTS server if the DOTS server
      detects that a request conflicts with a previous request.  
 

Updated the text to make this explicit: 

"In such a case, the DOTS server replies with 4.09 (Conflict)."


> Section 10.1
> 
>    This module uses types defined in [RFC6991] and [RFC8345].
> 
> Should we mention the data structure extension from RFC 8791 here, as
> well?

[Med] It is not cited here because we don't use any type from 8791. We do have a statement in the document that we rely on 8791.  

> 
>   typedef attack-severity {
>     type enumeration {
>       enum none {
>         value 1;
>         description
>           "No effect on the DOTS client domain.";
> 
> It seems like the closest analogue to our attack-serverity typedef in RFC
> 7970 is in §3.12.2, "BusinessImpact Class".

[Med] Yes. We do have the following: 

   attack-severity:  Attack severity level.  This attribute takes one of
      the values defined in Section 3.12.2 of [RFC7970].

 But the phrasing used in RFC
> 7970 is slightly different than the descriptions we have here.

[Med] It was adjusted for the DOTS context, but I think they are aligned. 

> Do we want to tweak our phrasings and/or clarify in the typedef
> description that we use a slightly modified formulation?

[Med] I don't think this is needed. 

  (Indeed, we do
> not have an extension point, either, though 7970 does have an extension
> mechanism.)  Also, a section reference within RFC 7970 might be helpful.
> 

[Med] Added Section 3.12.2  

>       enum kilopacket-ps {
>         value 4;
>         description
>           "Kilo packets per second (kpps).";
> 
> We may get someone who asks if we are using 1000 or 1024 ("kibi"), but I'm
> inclined to leave it alone for now.
> 
>       leaf unit-status {
>         type boolean;
>         default true;
>         description
>           "Enable/disable the use of the measurement unit class.";
> 
> Does the "default true" imply that even unit classes not present in "list
> unit-config" are assumed to be supported by default?

[Med] This is a bug. This should be "mandatory true". 

> 
>   grouping connection {
>     description
>       "A set of data nodes which represent the attack
>        characteristics.";
>     [...]
>     leaf embryonic {
>       type yang:gauge64;
>       description
>         "The number of simultaneous embryonic connections to
>          the target server.";
> 
> It's a little interesting that this is the only leaf in the grouping that
> we can't attach the word "attack" to the description of, but as far as I
> can tell we shouldn't make any change here.
> (I originally wrote a comment that we shouldn't use "attack" for any of
> them, since we have analogous nodes in the total-connection-capacity
> config entries, but those seem to not use this grouping.)
> 
>     list talker {
>       key "source-prefix";
>       description
>         "IPv4 or IPv6 prefix identifying the attacker(s).";
> 
> My read of the YANG is that this is the description for "list talker", but
> the content matches the description given for the "source-prefix" leaf
> within the list.  Should the list itself have a different description?

[Med] Yes. This was a copy/past issue.

> 
>   grouping top-talker-aggregate {
>   [...]
>   grouping top-talker {
> 
> The diff between these two is pretty small -- just the top-level
> description and connection-all vs connection-protocol-all.  Is there any
> value in introducing another grouping to hold the common elements?
> 

[Med] This allows to shorten the doc by 1/2 page. Made the change.

>             container max-config-values {
>               description
>                 "Maximum acceptable configuration values.";
>               uses telemetry-parameters;
> 
> I guess the convenience of having the reusable grouping probably outweighs
> the value of having YANG-level constraints that the 'max'
> values are >= the 'min' values (which we can set for the "telemetry-
> notify-interval" that is not part of "telemetry-parameters", but can't set
> here because we use the grouping).

[Med] I confirm. Please note that we have that guard in the core document. 

> 
>               leaf telemetry-notify-interval {
>                 type uint32 {
>                   range "1 .. 3600";
> 
> (Pedantically, this range would fit in a uint16, though maybe there is
> some other reason to prefer the 32-bit type.)

[Med] I lost the context why we had it like this. From a yang perspective, the current one is still correct, but it is better to adjust. Will change this but will check my archives as well. 

> 
>             case baseline {
>               [...]
>               list baseline {
>                 [...]
>                 leaf id {
>                   type uint32;
>                   must '. >= 1';
>                   description
>                     "An identifier that uniquely identifies a baseline
>                      entry communicated by a DOTS client.";
> 
> I a little bit wonder if we should have a little bit of prose up in §6.3
> discussing the need for "id" and how it is used.

[Med] Added this NEW text to 6.3:

   A DOTS client can share one or multiple normal traffic baselines
   (e.g., aggregate or per-prefix baselines), each are uniquely within
   the DOTS client domain with an identifier 'id'.  This identifier can
   be used to update a baseline entry, delete a specific entry, etc.

> 
>               leaf tmid {
>                 type uint32;
>                 description
>                   "An identifier to uniquely demux telemetry data sent
>                    using the same message.";
> 
> The description for "tsid" in the analogous setup was just "an identifier
> for the DOTS telemetry setup data".  As far as I can tell, the usage of
> the two is analogous, so shouldn't the descriptions be analogous as well?

[Med] Fair. Adjusted the text accordingly. 

> In particular, I am not sure that "uniquely demux" is the only usage of
> this leaf, since in server-initiated telemetry messages, this leaf might
> be needed to indicate which telemetry entry is being described (right?).

[Med] Yes. 

> 
>             leaf-list mid-list {
>               type uint32;
>               description
>                 "Reference a list of associated mitigation requests.";
> 
> Should we reference RFC 9132 somehow to indicate that the base signal
> channel is how these "mid" values are assigned (and assigned meaning)?
> 

[Med] Good suggestion. Added: 

              reference
                "RFC 9132: Distributed Denial-of-Service Open Threat 
                           Signaling (DOTS) Signal Channel 
                           Specification, Section 4.4.1";

> Section 10.2
> 
>          description
>            "Vendor ID is a security vendor's Enterprise Number.";
> 
> Should we say something about "IANA-assigned" and/or link to the
> corresponding registry?

[Med] Added a reference statement to the IANA registry.  

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.