Hi Robert, Many thanks for the careful review. Please see inline. Cheers, Med > -----Message d'origine----- > De : Robert Sparks via Datatracker <noreply@xxxxxxxx> > Envoyé : samedi 22 janvier 2022 00:01 > À : gen-art@xxxxxxxx > Cc : dots@xxxxxxxx; draft-ietf-dots-telemetry.all@xxxxxxxx; last- > call@xxxxxxxx > Objet : Genart last call review of draft-ietf-dots-telemetry-19 > > 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. [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. === > > 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. [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. > > 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. [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 > not 443? [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. > > 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. [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 > attributes". [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. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
