Reviewer: Mohamed Boucadair Review result: Has Issues Hi all, I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir. Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft. Document: draft-ietf-sidrops-8210bis-06 Reviewer: Mohamed Boucadair Review Date: 25/04/2022 IETF LC End Date: 2022-04-29 Intended Status: Standards Track Summary: This document describes version 2 of the RPKI-Router protocol. The main addition compared to version 1 of the protocol is the definition of a new PDU type to support I-D.ietf-sidrops-aspa-profile. However, the document does not illustrate the use of the new PDU. Even if not recorded in the "changes list", the document updates the meaning of some flags. This update introduces many inconsistencies in the document. The document also updates the version negotiation procedure, but the specification has several issues that are called out in the detailed comments below. # Major Issues: o Sections 5.6 and 5.10: The following conflicts with the new definition of the flags as per Section 5.1 "The lowest-order bit of the Flags field is 1 for an announcement and 0 for a withdrawal." o Section 7: (1) "A router MUST start each transport connection by issuing either a Reset Query or a Serial Query." Shouldn't the text indicate, e.g., that the router has to use the highest version it supports? (2) “This query will tell the cache which version of this protocol the router implements.” What if the router supports more than one version? This text seems to assume that only one version can be supported. (3) “If a cache which supports version N receives a query from a router which specifies version Q < N, the cache MUST downgrade to protocol version Q [RFC6810] or [RFC8210] or send a version 1 Error Report PDU with Error Code 4 ("Unsupported Protocol Version") and terminate the connection.” * I suggest "s/supports version/supports a highest version" to cover the case of multiple versions. * Shouldn't the Error Report PDU be the version 2, not version 1. * Shouldn't the error PDU be designed to return the supported versions to avoid cycles in the version negotiation? (4) “1. The cache may terminate the connection, perhaps with a version 0 Error Report PDU. In this case, the router MAY retry the connection using protocol version C.” Why version 1 error is not considered? o Section 8: ASPA PDUs are not illustrated in this section. Minor Issues: o Section 1 says that "This document updates [RFC8210]." while this should say that RFC8210 is obsoleted. o Section 1.2: The changes of the meaning or some flags are not listed in this section. o Section 2: Consider adding NEW entries for ASPA, CAS, and SPAS. o Section 5.1: "Customer Autonomous System Number" and "Provider Autonomous System Number" are not listed here. o Section 5.1: Shouldn't version be set to 2 in the following text: "Protocol Version: An 8-bit unsigned integer, currently 1 , denoting the version of this protocol." o Section 5.1: Isn't this behavior redundant for IPv4/IPv6 PDU types? Which one takes precedence if there is a conflict between the flag and the IPvX PDU Type? "Flags: The lowest-order bit of the Flags field is 0 for IPv4 and 1 for IPv6." o Section 5.1: Consider adding a pointer to the section where the new PDU is defined. OLD: For the ASPA PDU, the announce/withdraw Flag ... NEW: For the ASPA PDU (Section 5.12), the announce/withdraw Flag ... o Section 5.1: Isn't the following redundant with this text in the preamble of Section 5? "In protocol version 2, they MUST be zero on transmission and MUST be ignored on receipt." I'm referring to this preamble: "Reserved fields (marked "zero" in PDU diagrams) MUST be zero on transmission and MUST be ignored on receipt." o Section 5.8: I have several comments about this part: “Note that the End of Data PDU changed significantly between versions 0 and 1. For version 0 compatibility , the following is the version 0 End of Data PDU.” * It seems this text wasn't updated to reflect the version change. At least, the text should indicate that the same format is preserved for version 2. * Not sure to understand the intent of "For version 0 compatibility" as the version negotiation will lead to the use of a common version (if supported). * Isn't the last part of the text already covered in rfc6810#section-5.8? o Section 5.12: "An ASPA PDU represents one single customer AS and its provider ASs for a particular Address Family." Did the WG considered whether it useful to refer to all AFs rather than sending PDUs, each per AF? o Section 5.12: "The router should see at most one ASPA from a cache for a particular Customer Autonomous System Number active at any time. As a number of conditions in the global RPKI may present multiple valid ASPA objects for a single customer to a particular RP cache , this places a burden on the cache to form the union of multiple ASPA records it has received from the global RPKI into one ASPA PDU." * Is "ASPA object" the same as "ASPA record"? * What is meant by "RP cache"? o Section 5.12: "the cache SHOULD set the Provider AS Count to zero, and have a null Provider AS Numbers list." You may explain why "MUST" is not used here. o Section 6: * Please explain why the recommended Refresh Interval was changed from 3600 (v1). * Idem for the recommended Expire Interval that was changed from from 7200 (v1). o Section 13: "There is an IANA registry where valid error codes are listed; see Section 15." The IANA registry is authoritative here. I suggest to add a pointer to that registry instead of Section 15. o Section 13: The flag values are not aligned with this text "The lowest-order bit of the Flags field is 0 for IPv4 and 1 for IPv6". Unless I'm mistaken, the check should be against next lowest flag. If my understanding is correct, then the following should be updated: OLD: 6: Withdrawal of Unknown Record (fatal): The received PDU has Flag=0, but a matching record ({Prefix, Len, Max-Len, ASN} tuple for an IPvX PDU or {SKI, ASN, Subject Public Key} tuple for a Router Key PDU) does not exist in the receiver's database. 7: Duplicate Announcement Received (fatal): The received PDU has Flag=1, but a matching record ({Prefix, Len, Max-Len, ASN} tuple for an IPvX PDU or {SKI, ASN, Subject Public Key} tuple for a Router Key PDU) is already active in the router. o Section 15: I suggest to update the following text to reflect only v2 changes OLD: All existing entries in the IANA "rpki-rtr-pdu" registry remain valid for protocol version 0. All of the PDU types allowed in protocol version 0 are also allowed in protocol version 1, with the addition of the new Router Key PDU. To reduce the likelihood of confusion, the PDU number used by the Router Key PDU in protocol version 1 is hereby registered as reserved (and unused) in protocol version 0. o Section 15: The following should be updated because code 8 does not apply for all versions. OLD: All previous entries in the IANA "rpki-rtr-error" registry remain valid for all protocol versions. # Nits: o Section 1.2: Please expand ASPA in the following text: "* A new ASPA PDU type (Section 5.12) has added to support [I-D.ietf-sidrops-aspa-profile]." o Section 1.2: Please expand ROA in the following text: "* A small section, Section 11, has been added to handle two ROA PDU race conditions, Break Before Make and Shorter Prefix First." o Section 1.2: Consider the following change OLD: * The protocol version number incremented from 1 (one) to 2 (two) and the Section 7 section has been updated accordingly. NEW: * The protocol version number incremented from 1 (one) to 2 (two) and Section 7 has been updated accordingly. o Section 5.1: Please fix this nit OLD: AS Count as well as the Provider AS Numbers list MUST BE zero. NEW: AS Count as well as the Provider AS Numbers list MUST be zero. o Section 5.5: Consider making this change for consistency with the definition of flags in Section 5.1: OLD: When replying to a Reset Query (Section 5.4), the cache sends the set of all data records it has; in this case, the withdraw/announce field in the payload PDUs MUST have the value 1 (announce). NEW: When replying to a Reset Query (Section 5.4), the cache sends the set of all data records it has; in this case, the announce/withdraw field in the payload PDUs MUST have the value 1 (announce). o Section 5.12: Consider these changes (1) OLD: The ASPA PDU is to support [I-D.ietf-sidrops-aspa-profile]. NEW: The ASPA PDU is meant to support [I-D.ietf-sidrops-aspa-profile]. (2) OLD: withdrawn PDU and a new announced PDU. NEW: withdrawn PDU and a new announced ASPA PDU. (3) This text is redundant with what is already provided in 5.2. Consider deleting it: The Flags field is defined as follows: Bit Bit Name ---- ------------------- 0 AFI (IPv4 == 0, IPv6 == 1) 1 Announce == 1, Delete == 0 2-7 Reserved, must be zero (4) OLD: MUST BE ignored by the router when the Flags field indicates a NEW: MUST be ignored by the router when the Flags field indicates a o Section 5.12: "Receipt of an ASPA PDU announcement (Flag.Announce == 1 ) when the router already has an ASPA PDU with the same Customer Autonomous System Number and the same Address Family (see Flags field), replaces the previous one." I would refer to announce/withdraw flag instead of "Flag.Announce" for consistency. o Section 11: "When a cache is sending ROA PDUs to the router, especially during an initial full load, two undesirable race conditions are possible:" * Not sure what is meant by "initial full load" here. * Consider s/ROA PDUs to the router/ROA PDUs to a router Also, fix this nit: OLD: This is is a case of "make before NEW: This is a case of "make before o Section 15: Consider the following change OLD: This section only discusses updates required in the existing IANA protocol registries to accommodate version 1 of this protocol. See [RFC8210] for IANA considerations from the original (version 0) protocol. NEW: This section only discusses updates required in the existing IANA protocol registries to accommodate version 2 of this protocol. See [RFC8210] for IANA considerations from the original (version 1) protocol. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
