[sidr] AD Review of draft-ietf-sidr-bgpsec-protocol-18

["Alvaro Retana (aretana)" <[e-mail]>]

Dear authors:

Hi!  First of all, thank you for taking on the duties of editing this document.

I have several comments (see below).  For the most part, I think they should be easy to solve as many are related to clarifications.  Most of the comments I classified as Major are due to the use of Normative language.

The biggest concern I have with this document is the lack of an Operations and Management Considerations Section – please take a look at RFC5706 (Guidelines for Considering Operations and Management of New Protocols and Protocol Extensions).  Some of the information suggested there is present in draft-ietf-sidr-bgpsec-ops, and (ironically) in the “Design and Deployment Considerations” section of draft-ietf-sidr-bgpsec-overview.  However, important items such as migration or management are missing.  I would like to see a well thought out Operations and Management Section in this document before moving it forward.  Note that I’m not suggesting that a YANG model (for example) is required to move forward, but I would like to see considerations about migration, and the impact on network operations, to mention two items, all in one place in the document.  I would like the authors/Chairs/Shepherd/WG to consider even merging in draft-ietf-sidr-bgpsec-ops as the base for this new section (or at least reference it prominently).

Thanks!

Alvaro.



Major:

M1. Registry Definitions

M1.1. Section 2.1. (The BGPsec Capability) includes a Version field and some Reserved bits, but there are no IANA registries defined for how to manage these spaces.  Please define the registries and the corresponding registration procedures.

M1.2. Section 3.1. (Secure_Path) defines the Flags field and assigns the first bit (BTW, is that the MSB or the LSB, please clarify), but doesn't set up the registry or registration procedures.



M2. Error Handling — Several sections don't have proper error handling procedures specified. (This is a management issue that I think is underspecified.)

M2.1. Section 2.2. (Negotiating BGPsec Support) doesn't fully specify the error handling behavior of the Capability, and it fails open.

M2.1.1. What should the action be if the Version is not 0?

M2.1.2. "…a BGP speaker MUST NOT advertise the capability of BGPsec support for a particular AFI unless it has also advertised the multiprotocol extension capability for the same AFI [RFC4760]."  What should happen if it does advertise an AFI that is not covered by the multiprotocol extension capability?   Or if the multi-protocol capability is not advertised at all?  To clarify: if the multi-protocol capability is not advertised then support for BGPsec can’t be advertised either – does that mean that a BGP speaker configured to use BGPsec must not use it if not negotiated?  I know the answer is “yes”, but I’m trying to get to the point of provisioning and expectations – why configure BGPsec if no one is expected to support it?

M2.1.3. Missing the four-byte AS capability results in BGPsec not working ("BGPsec has not been successfully negotiated"), but the ability of exchanging routes is still there, leaving the system in a fail open state and potentially breaking the chain of ASNs.  Personally, it doesn't seem like a good result — please at least include some text about this in the Security Considerations section.


M2.2. The definition of the BGPsec_Path Attribute (and its details) don't have clear error handling procedures defined (RFC7606).  Section 4.3. (Processing Instructions for Confederation Members) does say this: "(As discussed in Section 5.2, any error in the BGPsec_Path attribute MUST be handled using the "treat-as-withdraw", approach as defined in RFC 7606 [RFC7606].)" (including the parentheses).  However, 5,2 only says: "BGPsec speakers MUST handle these errors using the "treat-as-withdraw" approach as defined in RFC 7606 [RFC7606]."   Note: "these errors" is not the same as "any error".  Please discuss error handling in a more prominent place. (Hint: it may fit in a fault management discussion in the Operations and Management Section.)

M2.2.1. There are multiple places in the BGPsec_Path Attribute that could end up in an error, everything from setting bits in the Flags field to wrong Length fields.  Should all errors result in the same behavior?



M3. Section 4.1. (General Guidance): "When propagating a received route advertisement to an internal peer, the BGPsec speaker typically populates the BGPsec_Path attribute by copying the BGPsec_Path attribute from the received update message.  That is, the BGPsec_Path attribute is copied verbatim…. Note that when a BGPsec speaker chooses to forward a BGPsec update message to an iBGP peer, the BGPsec attribute SHOULD NOT be removed, unless the peer doesn't support BGPsec."  The first part of the guidance says that a new BGPsec_Path attribute is created by copying the received attribute (which is then presumably removed), but the second part says that the received attribute SHOULD NOT be removed.  Please clarify so that there is consistency -- I understand that the result is the same, but the description is not and we should try to avoid confusion.  Parts of Section 4.2. (Constructing the BGPsec_Path Attribute) also talk about actions like "…and there is an existing BGPsec_Path attribute, then the BGPsec speaker prepends its new Secure_Path Segment (places in first position) onto the existing Secure_Path", which hint at propagating a received BGPsec_Path attribute (and not creating a new one).



M4. Section 4.2. (Constructing the BGPsec_Path Attribute) says that "The AS number in this Secure_Path segment MUST match the AS number in the AS number resource extension field of the Resource PKI router certificate(s) that will be used to verify the digital signature(s) constructed by this BGPsec speaker [I-D.ietf-sidr-bgpsec-pki-profiles]."  However, there is no extension field or certificate in I-D.ietf-sidr-bgpsec-pki-profiles with that name.  Please be precise with the names.



M5. In 4.2: “BGPsec speakers SHOULD drop incoming update messages with pCount set to zero in cases where the BGPsec speaker does not expect its peer to set pCount to zero.”  This text seems to assume some kind of configuration/provisioning.  Note that Section 5.2. (Validation Algorithm) also has similar text about receiving an UPDATE “from a peer that is not expected to set pCount equal to zero”.



M6. In 4.2: “If the received BGPsec update message contains two Signature_Blocks and the BGPsec speaker supports both of the corresponding algorithm suites, then the new update message generated by the BGPsec speaker SHOULD include both of the Signature_Blocks.”  Why is this “SHOULD” not a “MUST”?   When/why would a speaker remove one or the 2?  If one is removed, should there be a requirement that the one that was used to successfully validate the update be kept?  Note that Section 7.2 later talks about the problems of removing signatures…

M6.1. Note that later in this section the text says that “a 'Valid' BGPsec update message may contain a Signature_Block which is not deemed 'Valid' (e.g., contains signatures that BGPsec does not successfully verify).  Nonetheless, such Signature_Blocks MUST NOT be removed.”   Taking this “MUST NOT” along with the “SHOULD” above, the door is open to remove the Signature_Block used to verify the validity and just forward the one not used (which may itself not be valid).

M6.2. Section 5.2. (Validation Algorithm) opens this door even more when saying that “If at least one Signature_Block is marked as 'Valid', then the validation algorithm terminates and the BGPsec update message is deemed to be 'Valid'.”  The text here doesn’t require that both Signature_Blocks be verified, but implies that as long as the first one is valid then the second one doesn’t really need to be verified.  Is that the intent?



M7. Section 5. (Processing a Received BGPsec Update) talks about “duplicate update messages” (one where “it differs from the first update message only in the Signature fields (within the BGPsec_Path attribute)”).

M7.1. “With regards to the processing of duplicate update messages, if the first update message is valid, then an implementation SHOULD NOT run the validation procedure on the second, duplicate update message (even if the bits of the signature field are different).”  Even though a discussion about non-deterministic signature algorithms precedes this text, the validation is still not run.  How can the validity of the Path be guaranteed in this case?  Should this be the action for all algorithms or only ones known to be non-deterministic?

M7.2. rfc4271 (in Section 9. (UPDATE Message Handling) talks about the implicit withdraw of a route “if the NLRI of the new route is identical to the one the route currently has stored…”.  The same NLRI case seems to be a particular condition of the “duplicate update” described here.  It might be a good idea to mention that a “duplicate update” results in the implicit withdraw of the original update.  What happens if a third duplicate route is received (the first one was valid, the second one was not validated), should it be validated?



M8. Section 5.1. (Overview of BGPsec Validation) says that "BGPsec specifies no changes to the BGP decision process."  However, Section 5. (Processing a Received BGPsec Update) also says that "a BGPsec speaker MUST utilize the AS path information in the BGPsec_Path attribute in all cases where it would otherwise use the AS path information in the AS_PATH attribute."  I'm assuming that the Decision Process is included in "all cases".  Even though 5.1 refers to the use the validation state in the decision process, please make sure that it is clear that the decision process is modified by the use of the different attribute.

M8.1. Please specifically include a section (or text somewhere) about how the information in the BGPsec_Path attribute is used in the Decision Process.  For example, how should the "number of AS numbers" in the path be calculated for 9.1.2.2. (Breaking Ties (Phase 2)) in rfc4271?  The text talks about the "effective length" being the sum of the pCount values, but the "effective length" (at least with that name) is not what is used in rfc4172 — please be clear and consistent.

M8.2. [minor] Section 3. (The BGPsec_Path Attribute) reads: "The information in Secure_Path is used by BGPsec speakers in the same way that information from the AS_PATH is used by non-BGPsec speakers."  This is pretty much the same information that is in Section 5, but with more specificity.  It would help if the more specific case was the one normatively called out.



M9. Section 5.2. (Validation Algorithm).  RFC4271 also specifies a series of validity checks when an UPDATE is received (Section 6.3) – should that check be run before or after the algorithm specified here?  The algorithm focuses on verifying the validity of the BGPsec_Path attribute (and not the whole UPDATE), so I’m assuming it should be executed instead of checking the AS_PATH.  Please include some text about the interaction/changes.

M9.1.  Section 5.2. (Validation Algorithm): “…then the BGPsec speaker MUST treat the update message in the same manner that the BGPsec speaker would treat an (unsigned) update message that arrived without a BGPsec_Path attribute.”   What exactly does this mean?  If the BGPsec_Path attribute is not received, then the AS_PATH should be – does the text imply that the AS_PATH should be reconstructed?  I guess it should be if the update is to be propagated – but the question is while the update is being processed, which AS path information is used, the one in a reconstructed AS_PATH or the one in the BGPsec_Path while assuming that all the signatures are correct?



M10.  In Section 7.2. (On the Removal of BGPsec Signatures): “…the protocol specifies that a BGPsec speaker choosing to propagate the route advertisement in such an update message SHOULD add its signature to each of the Signature_Blocks.”   I believe the reference is Section 4.2 (please add it).  However, that Section doesn’t use normative language (“For each Signature_Block corresponding to an algorithm suite that the BGPsec speaker does support, the BGPsec speaker adds a new Signature Segment to the Signature_Block.”)   In any case, the “SHOULD” in 7.2 is out of place because it is referencing a fact (pointing at the other section) and not being used normatively – if you want the “SHOULD” to be normative, then it should be back in 4.2.


M11. The Figures (which are not numbered) in 4.2. (Constructing the BGPsec_Path Attribute) and 5.2. (Validation Algorithm) present the sequence of octets to be hashed.  I’m guessing that the order of the Signature and Secure_Path Segments may be important, is it?  The order in the Figures is not clear to me, if the order is important, please be clear about it; if not, please also say so.



M12. The mandatory addition of Secure_Path and Signature segments in a Confederation results in the inconsistent authorization chain mentioned in Section 4.3. (Processing Instructions for Confederation Members): “For a signature produced by a peer BGPsec speaker outside of a confederation, the Target AS will always be the AS Confederation Identifier (the public AS number of the confederation) as opposed to the Member-AS Number.”  It seems to me that this discontinuity in the ASN list breaks the “cryptographic assurance that…Every AS on the path of ASes listed in the update message has explicitly authorized the advertisement of the route to the subsequent AS in the path” because there is no way to verify that the Member-AS in the first Secure_Path segment is in fact the one peering with the external neighbor.  I realize that the risk may be minimized by an “internal trust” factor, but I would like to see a discussion about this issue in the Security Considerations.



M13. What about replay attacks?  There is no mention of the risk or potential mitigation anywhere.  Please include in the Security Considerations section.



Minor:

m1. The use of SKI, Subject Key Identifier without a qualifier (field, extension) is confusing at times.   Please expand SKI on first use.  An example: (from 3.2) “The Subject Key Identifier contains the value in the Subject Key Identifier extension of the RPKI…”  The first mention should include “field”, like similar text in 4.2.


m2. Section 4. (BGPsec Update Messages) says: "A BGPsec speaker that is not a member of such a confederation MUST set the Flags field of the Secure_Path Segment to zero in all BGPsec update messages it sends."  While only one flag is defined, the correct statement is "…set the Confed_Segment flag…".


m3. Section 4.1. (General Guidance): s/"A BGPsec update message MUST advertise a route to only a single NLRI."/"A BGPsec update message MUST advertise a route to only a single prefix."   This section contains other places where the NLRI term is not used correctly.  In short, the NLRI in a BGP Update contains one or more prefixes — so the text should talk about a single prefix, not a single NLRI.


m4. In between the text above, the following is written: "However, in the case that the BGPsec speaker is performing an AS Migration, the BGPsec speaker may add an additional signature on ingress before copying the BGPsec_Path attribute (see [I-D.ietf-sidr-as-migration] for more details)."  Because I-D.ietf-sidr-as-migration is marked as Updating this document, I suggest you remove this text (and the one in 4.2) -- note that the statements made in this document are not normative anyway and I-D.ietf-sidr-as-migration can stand on its own by clearly specifying what is needed for AS Migration.


m5. In 4.2: “To prevent unnecessary processing load…a BGPsec speaker SHOULD NOT produce multiple consecutive
Secure_Path Segments with the same AS number.  This means that to achieve the semantics of prepending the same AS number k times, a BGPsec speaker SHOULD produce a single Secure_Path Segment – with pCount of k...”  Given pCount, I’m wondering why these SHOULDs are not MUSTs, especially given the expected additional load.


m6. Section 4.3. (Processing Instructions for Confederation Members) explains the process of adding Secure_Path and Signature segments that may or may not be used at all (given that the verification is optional), only to remove them later.  Why isn’t the process of adding Secure_Path and Signature segments optional itself (instead of just the validation)?


m7. In 4.4. (Reconstructing the AS_PATH Attribute), what should happen if the Confed_Segment flag is set to zero and the most-recently added segment in the AS_PATH is of type AS_CONFED_SEQUENCE?  Theoretically this can’t occur because it means that someone accepted an update that it shouldn’t have, but please include some text about this case being an error.


m8. Section 5.1. (Overview of BGPsec Validation): “It is expected that BGP peers will generally prefer routes received via 'Valid' BGPsec update messages over both routes received via 'Not Valid' BGPsec update messages and routes received via update messages that do not contain the BGPsec_Path attribute…(See [I-D.ietf-sidr-bgpsec-ops]…”  I read this piece of text as saying that routes in Not Valid updates are expected to be used (even though they are not valid).  Besides the fact that I-D.ietf-sidr-bgpsec-ops does recommend using Not Valid announcements (in Section 7), are there other reasons for this document to expect their use?


m9. Section 5.1 contains these references: “…the trusted cache could deliver the necessary validity information to the BGPsec speaker using the router key PDU [I-D.ietf-sidr-rtr-keying] for the RPKI-to-Router protocol [I-D.ietf-sidr-rpki-rtr-rfc6810-bis].”   The reference to I-D.ietf-sidr-rtr-keying seems to be related to the “router key PDU”, but that is defined in I-D.ietf-sidr-rpki-rtr-rfc6810-bis, so it looks like the first reference is not needed.


m10. In 5.1: “As discussed in Section 4, when a BGPsec speaker chooses to forward…, it SHOULD be forwarded with its BGPsec_Path attribute…”   That “SHOULD” is pointing at a fact, not acting normatively in this sentence so please change it to “should”.


m11. Section 5.2. (Validation Algorithm) mentions this check: ‘update..from a peer that is not a member of the BGPsec speaker's AS confederation, check to ensure that none of the Secure_Path segments contain a Flags field with the Confed_Sequence flag set to one.”  I’m sure that the check for the flag set if the peer is a Confederation peer is also needed, but not mentioned in this section (where the normative MUST for the validation algorithm) is present.  Section 4.3. (Processing Instructions for Confederation Members) does say this: “…when a confederation member runs the algorithm in Section 5.2, the confederation member, during processing of a Signature Segment, first checks whether the Confed_Sequence flag in the corresponding Secure_Path segment is set to one.”   I would like to see the full algorithm specified in one place (even if, like in Section 4.3, pieces of it are explained elsewhere).  Also, the text in 4.3 says that the check is performed “during processing of a Signature Segment”, which is fine, but probably late in the process (compared to the text in 5.2 that seems to require the check when the updates are received).


m12. In Section 7.4. (Additional Security Considerations), please add a reference to point at “appropriate transport security mechanisms.”, and/or point at the Security Considerations from rfc4271.



Nits:

n1. In several places the text uses “we” (for example: “we expect…”).  It’s just a matter of style, but using the third person may be more appropriate (for example: “it is expected…”).

n2. The use of “Target AS Number” is inconsistent: sometimes the “number” is capitalized, others it isn’t, and sometimes it is not even mentioned.

n3. I don’t think we need Section 6.2. (Extensibility Considerations).