Subject: [Last-Call] Genart last call review of draft-ietf-dots-telemetry-19

From: Robert Sparks via Datatracker <[email-masked]>
Date: Fri, 21 Jan 2022 15:01:27 -0800

Reviewer: Robert Sparks
Review result: Ready with Issues

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-dots-telemetry-19
Reviewer: Robert Sparks
Review Date: 2022-01-21
IETF LC End Date: 2022-01-24
IESG Telechat date: Not scheduled for a telechat

Summary: Ready for publication as a Proposed Standard RFC, but with issues and
nits to consider before publication

Issues:
-------

This document relies heavily on the requirements in the DOTS signal and data
channel specifications (RFC9132 and RFC8783) saying authenticated encryption
MUST be used. Explicitly calling that out in this document's security section
would be worthwhile.

I am not a YANG Doctor. This document seems to make a reasonable but unusual
use of YANG. I would really like to see YANG Doctor review of -19. I wonder if
it would be possible to factor some of the more generic enums out into
something that could be reused elsewhere. Why do you stop at discouraging
rather than forbidding server deviations? The point of a specification like
this is interoperability.

I'm a little uncomfortable with the passively stated requirement to use
ietf-dots-rehoming (at "clients are assumed to follow") while that document is
an Informative reference.

In 6.1.2, I'm confused by the requirement that "'tsid' values MUST increase
monotonically when a new PUT is generated" combined with "If the dot server
finds the 'tsid' parameter value conveyed in the PUT request in its
configuration data and (it) has accepted the updated configuration parameters,
a 2.04 (Changed) MUST be returned". What am I missing that would allow a PUT
with a tsid that's already in the server's configuration data? Is there perhaps
leftover tension here from earlier designs that were changed?

At the end of 7.1, there is a MUST NOT level requirement against repeating
attack-descriptions that have been previously sent to a peer. Is it always the
case that when these descriptions are sent, the message they are contained in
is acknowledged? If not, consider adding discussion of how/whether a peer can
ask for a description when it gets an attack-id it doesn't have a description
for.

In 7.2, you require a freshly rebooted client to send a GET request to retrieve
active 'tmid' values. Why? Why not let it just send a DELETE with no 'tmid'
right away if that's what it would do after receiving the response to the GET
anyhow?

In 7.3 at "The PUT request MUST be maintained in an active state", you mean the
configuration that the PUT established must be kept. The PUT request itself has
no state beyond its response. Consider saying something like "The
pre-or-ongoing mitigation requests MUST be considered active" instead.

In the yang module, there's a reference at page 79 that points to "Section
4.4.2 of RFC UUUU." This is the only occurrence of UUUU in the document, and if
it's meant to be replaced with 'this RFC', this document has no section 4.4.2.

Nits:
---------------

Last paragraph of introduction at "Nevertheless, when..". That sentence is
particularly hard to parse, please simplify it.

Curious that you called out TCP port 80 in the discussion in 3.3. Why not 443?

At 4.1, where you say "that truly require reliable delivery", I think you mean
something more like "that are important to deliver". Nothing here is trying to
guarantee reliable delivery and the statement misleads the reader to look for
reliable delivery mechanisms.

In 4.5 at "The reason for not including these keys...". That sentence does not
parse. Please simplify.

In 4.6, the second paragraph is about validating messages, not examples. It's
not clear the paragraph is useful as a roadmap through the document. Consider
deleting it.

The outline structure of section 4 is odd. There are several Considerations
subsections including "Generic Considerations" (which is jarring), and several
subsections that are not considerations sections. Having them under a section
titled "Design Overview" doesn't help pull them together. Perhaps a mild
restructure/reorder would bring more clarity?

At 6.1, consider replacing "(with suitable default)." with ". Default values
are defined later in this section."

In 6.1 you claim clients can negotiate configuration parameters with server. I
don't find a negotiation protocol specified here. I think you mean clients can
configure parameters at a servers, and servers can configure parameters at a
client. There's not really a mechanism for a peer to say "no, I don't the
percentile config you're trying to set and I won't do that".

The introduction of "a third-party DOTS server" in the narrative after Figure
13 is surprising. For the paragraph to make sense, the messages in the
discussion before Figure 13 would also have to have gone to that third-part
DOTS server. Consider calling out that the DOTS server may not be hosted at any
of the ISPs at the beginning of 6.2.1 instead.

In 6.2.1 where you require servers to reject PUT requests with 0 capacity for
all included links, consider saying "Use DELETE instead." Maybe say _why_ you
require that PUT to be rejected.

In the sentence after Figure 19 (at least in the text version), you mean to
point to Figure 20.

At 7.1 "DOTS telemetry attributes are optionally signaled and therefore MUST
NOT be treated as mandatory fields in the DOTS signal channel protocol" is an
odd thing to call out this way. I think this was mentioned in the earlier yang
doctors review as well. Why is it here?

In 7.1.1 consider replacing "the same attributes" with simply "the attributes".

In 7.1.6, please replace "reiterates" with "repeats".

In 7.1.6, please replace the phrase "Concretely, " with "A".

---

From: <[email-masked]>
Date: Mon, 24 Jan 2022 12:08:32 +0000

Hi Robert, 

Many thanks for the careful review. 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Robert Sparks via Datatracker <[email-masked]>
> Envoyé : samedi 22 janvier 2022 00:01
> À : [email-masked]
> Cc : [email-masked]; [email-masked]; last-
> [email-masked]
> Objet : Genart last call review of draft-ietf-dots-telemetry-19
> 
> Reviewer: Robert Sparks
> Review Team (Gen-ART) reviews all IETF documents being processed by the
> IESG for the IETF Chair.  Please treat these comments just like any
> other last call comments.
> 
> Summary: Ready for publication as a Proposed Standard RFC, but with
> issues and nits to consider before publication
> 
> Issues:
> -------
> 
> This document relies heavily on the requirements in the DOTS signal and
> data channel specifications (RFC9132 and RFC8783) saying authenticated
> encryption MUST be used. Explicitly calling that out in this document's
> security section would be worthwhile.

[Med] We do have pointers to the relevant sections from 9132/8783 in Section 13. We avoided to extract specific requirements to avoid the confusion that we are creating new requirements or to misguide readers that only the "extracted" parts are worth to be considered. That’s said, added this sentence to design overview to remind that secure communications channels are required for DOTS. 

> 
> I am not a YANG Doctor. This document seems to make a reasonable but
> unusual use of YANG. I would really like to see YANG Doctor review of -
> 19.

[Med] The design we have in -19 was already reviewed by yangdoctors. FWIW, the design in this extension follows the one in 9132.

 I wonder if it would be possible to factor some of the more generic
> enums out into something that could be reused elsewhere.

[Med] We don't have that much types + we don't know if these will be reused. FWIW, rfc8407#section-4.12 says the following: 

   If a significant number of derived types are defined, and it is
   anticipated that these data types will be reused by multiple modules,
   then these derived types SHOULD be contained in a separate module or
   submodule, to allow easier reuse without unnecessary coupling.

 Why do you stop
> at discouraging rather than forbidding server deviations? The point of a
> specification like this is interoperability.

[Med] This is a point we discussed with the yangdoctors in the past. Here is the reasoning: 

===
> 
> #6) deviations
> 
> Deviations are never mentioned. Are implementors allowed to deviate
> from the protocol in any way, if declared using deviation
> statements? If so, how would a client know if a deviated model is in
> use?

[Med] Some comments here: 
* A client in general does not know what model is used by a peer as what it sees is CBOR.
* A server can use distinct default values (e.g., percentile values), but this can be retrieved by a client during the telemetry-setup. 

Other than that, deviations should be avoided as this will impact message validation.
===

> ietf-dots-rehoming (at "clients are assumed to follow") while that
> document is an Informative reference.

[Med] Changed the wording to: "Considerations for multi-homed DOTS clients to select which ....are discussed in [ietf-dots-multihoming]." 

> 
> In 6.1.2, I'm confused by the requirement that "'tsid' values MUST
> increase monotonically when a new PUT is generated"

[Med] The full context is: 

      " (when a new PUT is generated
        by a DOTS client to convey new configuration parameters for the
        telemetry). "

 combined with "If
> the dot server finds the 'tsid' parameter value conveyed in the PUT
> request in its configuration data and (it) has accepted the updated
> configuration parameters, a 2.04 (Changed) MUST be returned".

[Med] this one is to cover updating the change of a parameter that was already configured:

  "DOTS clients update their
   telemetry setup configuration upon change of a parameter that may
   impact attack mitigation."

 What am I
> missing that would allow a PUT with a tsid that's already in the
> server's configuration data? Is there perhaps leftover tension here from
> earlier designs that were changed?
> 
> At the end of 7.1, there is a MUST NOT level requirement against
> repeating attack-descriptions that have been previously sent to a peer.
> Is it always the case that when these descriptions are sent, the message
> they are contained in is acknowledged?

[Med] The exchange happens over a reliable transport.

 If not, consider adding
> discussion of how/whether a peer can ask for a description when it gets
> an attack-id it doesn't have a description for.
> 
> In 7.2, you require a freshly rebooted client to send a GET request to
> retrieve active 'tmid' values. Why?

[Med] This allows (amnesic) clients to recover state. Whether they maintain all or a subset of recovered entries is then local to the client (policy, sanity checks, etc.). 

 Why not let it just send a DELETE
> with no 'tmid'
> right away if that's what it would do after receiving the response to
> the GET anyhow?
> 
> In 7.3 at "The PUT request MUST be maintained in an active state", you
> mean the configuration that the PUT established must be kept. The PUT
> request itself has no state beyond its response. Consider saying
> something like "The pre-or-ongoing mitigation requests MUST be
> considered active" instead.

[Med] This is better. Updated the text. Thank you.

> 4.4.2 of RFC UUUU." This is the only occurrence of UUUU in the document,
> and if it's meant to be replaced with 'this RFC', this document has no
> section 4.4.2.

[Med] This refers to RFC9132. Fixed. Thank you. 

> 
> Nits:
> ---------------
> 
> Last paragraph of introduction at "Nevertheless, when..". That sentence
> is particularly hard to parse, please simplify it.

[Med] Changed to: 

"When no limitation policy is provided to a DOTS agent, it can signal available telemetry attributes to it peers in order to optimize the overall mitigation service provisioned using DOTS. "

> Curious that you called out TCP port 80 in the discussion in 3.3. Why

[Med] That's just an example. We could use 443 as we did in 9132. 

> 
> At 4.1, where you say "that truly require reliable delivery", I think
> you mean something more like "that are important to deliver". Nothing
> here is trying to guarantee reliable delivery and the statement misleads
> the reader to look for reliable delivery mechanisms.

[Med] Yes, fixed. 

> 
> In 4.5 at "The reason for not including these keys...". That sentence
> does not parse. Please simplify.

[Med] Changed to:

"The reason for not including these keys is because they are not included in the message body of DOTS requests; such keys are included as mandatory Uri-Paths .."

> 
> In 4.6, the second paragraph is about validating messages, not examples.
> It's not clear the paragraph is useful as a roadmap through the
> document. Consider deleting it.

[Med] Agree. Moved to a new subsection under 4.2.

> 
> The outline structure of section 4 is odd. There are several
> Considerations subsections including "Generic Considerations" (which is
> jarring), and several subsections that are not considerations sections.
> Having them under a section titled "Design Overview" doesn't help pull
> them together. Perhaps a mild restructure/reorder would bring more
> clarity?

[Med] Will see if there is a better approach to structure that part. 

> 
> At 6.1, consider replacing "(with suitable default)." with ". Default
> values are defined later in this section."

[Med] Went for "Default values are defined in Section 6.1.2."

> 
> In 6.1 you claim clients can negotiate configuration parameters with
> server. I don't find a negotiation protocol specified here. I think you
> mean clients can configure parameters at a servers, and servers can
> configure parameters at a client. There's not really a mechanism for a
> peer to say "no, I don't the percentile config you're trying to set and
> I won't do that".

[Med] There is text such as: 

   *  If any of the enclosed configurable attribute values are not
      acceptable to the DOTS server (Section 6.1.1), 4.22 (Unprocessable
      Entity) MUST be returned in the response.

      The DOTS client may retry and send the PUT request with updated
      attribute values acceptable to the DOTS server.

> discussion before Figure 13 would also have to have gone to that third-
> part DOTS server. Consider calling out that the DOTS server may not be
> hosted at any of the ISPs at the beginning of 6.2.1 instead.

[Med] OK.

> 
> In 6.2.1 where you require servers to reject PUT requests with 0
> capacity for all included links, consider saying "Use DELETE instead."
> Maybe say _why_ you require that PUT to be rejected.

[Med] OK

> 
> In the sentence after Figure 19 (at least in the text version), you mean
> to point to Figure 20.

[Med] Good catch. Fixed. Thanks. 

> 
> At 7.1 "DOTS telemetry attributes are optionally signaled and therefore
> MUST NOT be treated as mandatory fields in the DOTS signal channel
> protocol" is an odd thing to call out this way. I think this was
> mentioned in the earlier yang doctors review as well. Why is it here?

[Med] Fair comment. That text is redundant with this one "comprehension-optional parameters" in the IANA section. Deleted the sentence you quoted. 

> In 7.1.1 consider replacing "the same attributes" with simply "the

[Med] OK

> In 7.1.6, please replace "reiterates" with "repeats".

[Med] ACK.

> In 7.1.6, please replace the phrase "Concretely, " with "A".

[Med] ACK.

_________________________________________________________________________________________________________________________

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.

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 25 Jan 2022 16:58:43 -0800

Hi Med, Robert,

I echo the thanks to Robert for the review.
A few notes inline (trimming as needed)...

> > Objet : Genart last call review of draft-ietf-dots-telemetry-19
[...]
>         by a DOTS client to convey new configuration parameters for the

I think I see how this causes Robert to be confused.
In particular, a PUT is just a single CoAP request, but what we really mean
here is the persistent configuration object on the server that's identified
by the 'tsid'.  So, we should impose the requirement not on a "new PUT"
being generated, but rather that when we allocate a new tsid value for a
new configuration object, we must make that allocation in the monotonic
manner.

> > earlier designs that were changed?
[...]
> [Med] This allows (amnesic) clients to recover state. Whether they maintain all or a subset of recovered entries is then local to the client (policy, sanity checks, etc.). 

To attempt to expound on this a bit more, the state that the client would
be recovering applies (in some cases) to the entire client domain and not
just the client itself.  There is assumed to be some internal (e.g.,
monitoring or state synchronization) protocol within the domain to identify
the resources needing protection and the corresponding type of state
expected at the DOTS layer.  Deleting all state at the DOTS layer is very
unlikely to be a useful response to a client reboot.  (Which does not
definitively force a strict prohibition on going straight to DELETE without
GET, but it's why I didn't have a problem with the specified behavior.)

-Ben

---

From: <[email-masked]>
Date: Wed, 26 Jan 2022 14:26:46 +0000

Hi Ben, all, 

Thank you for zooming on these two points. 

Please see below. 

Cheers,
Med

> De : Dots <[email-masked]> De la part de Benjamin Kaduk
> Envoyé : mercredi 26 janvier 2022 01:59
> À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>
> Cc : [email-masked]; [email-masked]; draft-ietf-dots-
> [email-masked]; [email-masked]; Robert Sparks
> <[email-masked]>
> Objet : Re: [Dots] Genart last call review of draft-ietf-dots-telemetry-
> 19
> 
> Hi Med, Robert,
> 
> > > De : Robert Sparks via Datatracker <[email-masked]> Envoyé :
> > > samedi 22 janvier 2022 00:01 À : [email-masked] Cc :
> > > [email-masked]; [email-masked]; last-
> > > [email-masked] Objet : Genart last call review of
> > > draft-ietf-dots-telemetry-19
> > >
> [...]
> In particular, a PUT is just a single CoAP request, but what we really
> mean here is the persistent configuration object on the server that's
> identified by the 'tsid'.  So, we should impose the requirement not on a
> "new PUT"

[Med] We made this change:

OLD:
   tsid:  Telemetry Setup Identifier is an identifier for the DOTS
        telemetry setup configuration data represented as an integer.
        This identifier MUST be generated by DOTS clients.  'tsid'
        values MUST increase monotonically (when a new PUT is generated
        by a DOTS client to convey new configuration parameters for the
        telemetry).

NEW:
   tsid:  Telemetry Setup Identifier is an identifier for the DOTS
        telemetry setup configuration data represented as an integer.
        This identifier MUST be generated by DOTS clients.  'tsid'
        values MUST increase monotonically whenever new configuration
        parameters need to be conveyed by the DOTS client.

(also tweaked a similar text for tmid)

I hope this removes the confusion.