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).
_______________________________________________
sidr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/sidr
