[sidr] Review of draft-ietf-sidr-publication-09

[Peter Yee]

Reviewer: Peter Yee
Review result: Ready with Nits

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

.

Document: draft-ietf-sidr-publication-09
Reviewer: Peter Yee
Review Date: 2017-01-06
IETF LC End Date: 2017-01-06
IESG Telechat date: 2017-01-19

Summary: This specification defines a protocol for handling objects in
an RPKI repository.   The document seems fairly straightforward and
simple to understand.

Major issues:

Minor issues:

Nits/editorial comments: 

Page 5, Section 2.2, last paragraph, last sentence: perhaps change
"are permitted to" to "MAY"?

Page 7, Section 2.6, 1st paragraph: change "RelaxNG" to "RELAX NG".
(Hey, I had to look it up.)

Page 14, Section 4, 1st paragraph after rsync enumation: "safely" is
used but no subsequent mention is made of what is unsafe about the
non-overlapping rsync directories.  Is the reader expected to know
something about rsync's safety?  Nothing in the Security
Considerations deals with this topic.

Page 16, Section 6, 3rd paragraph, 2nd sentence: insert "private"
before "keys".  Insert "to" before "delete".

Page 17, Section 7: it might be good to include references to: XML,
RelaxNG, and maybe rsync (yeah, I know that one is a little tough).

___
sidr mailing list
sidr@ietf.org
https://www.ietf.org/mailman/listinfo/sidr

Re: [sidr] draft-ietf-sidr-bgpsec-protocol

[Randy Bush]

re keyur's point 4.  the issue is that 4271 has semantics of AS_PATH,
bgpsec replaces it with BGPSEC_PATH, but bgpsec does not explicitly
repeat things such as loop detection using BGPSEC_PATH.  instead of this
becoming another text explosion, a simple statement that, in bgpsec, the
following AS_PATH semantics of 4271: , hold analogously for
BGPSEC_PATH.

randy

___
sidr mailing list
sidr@ietf.org
https://www.ietf.org/mailman/listinfo/sidr

Re: [sidr] draft-ietf-sidr-bgpsec-protocol

[Alvaro Retana (aretana)]

On 1/6/17, 12:40 AM, "Sriram, Kotikalapudi (Fed)" 
 wrote:



[Cut the distribution list a little.]



Sriram:



Hi!  Happy New Year!



I have some comments on this, please see below.



Thanks!



Alvaro.





…

| | 1)  Section 4.1 “The BGPsec Path attribute and the AS_PATH attribute are 
mutually

| | exclusive. That is, any update message containing the BGPsec Path attribute 
MUST NOT

| | contain the AS_PATH attribute”.  For any restarting speakers in a GR mode, 
where the bgp

| | capability is not exchanged, the existing stale routes won’t have an 
AS_PATH attribute. We

| | could add some clarifying that helps to indicate that such routes should be 
considered

| | valid in stale mode (till they get refreshed)?

|

| [Sriram]  As you have clarified for me on the phone, what you are saying here 
is that the

| two BGPsec peers lost the BGPsec session and now restarting in GR mode, but 
they have not

| exchanged BGPsec capability this time. Hence, they are now simple BGP 
(non-BGPsec) peers

| in GR mode. RFC4271 considers update message received without a well-known 
AS_PATH

| attribute as an error, and unfortunately in this case the cached BGPsec 
updates do not have

| AS_PATH (albeit they have BGPsec_Path). So you are saying "the router should 
not panic"

| and instead simply treat each cached update as NOT-IN-ERROR even though it is 
missing

| AS_PATH attribute. This way the GR can work properly. Of course, shortly the 
updates will

| have AS_PATH (and not considered in error) when they get refreshed (over the 
new simple

| BGP session). Per your suggestion, I will include new text in Section 7 to 
describe this

| required behavior for the GR mode.



I don’t have an objection for this behavior, but I think we should make the WG 
(and idr!) aware of the change and get their comments (if any) before I approve 
the publication.





…

| | 3)  Section 5 and Section 5.2, 1st paragraph: RFC4271 considers update 
message received

| | without a well-known AS_PATH attribute as an error.  We need some text to 
clarify the

| | (error handling if any) behavior when an update message is received without 
a bgpsec and

| | an aspath attribute. The current draft text seems unclear about generation 
of bgpsec

| | attribute as well (in a ibgp scenario). Is it a requirement to generate an 
empty bgpsec

| | attribute?

|

| [Sriram]  As you have clarified for me over the phone, RFC 4271 (page 26) 
says the

| following :

|

|  "When a BGP speaker originates a route then:

|   b) the originating speaker includes an empty AS_PATH attribute in

|all UPDATE messages sent to internal peers.  (An empty AS_PATH

| attribute is one whose length field contains the value zero)."

|

|

| [Sriram]  So what needs to be said in the BGPsec document is the following:  
The

| BGPsec_Path attribute is not attached in updates originated inside an AS and 
propagated to

| BGPsec capable internal peers. However, when a route is originated inside an 
AS and

| propagated to non-BGPsec internal peers, an empty AS_PATH attribute is 
included in the

| update (see [RFC 4271], page 26).



The Route Selection Section (9.1.2) in RFC4271 is not explicit about performing 
loop detection only on eBGP sessions – the criteria is generic to any route, so 
there is a possibility that a BGPsec-capable router may want to perform loop 
detection on an iBGP-received Update.  Given this text from Section 5 in the 
BGPsec spec:



   Whenever the use of AS path information is called for

   (e.g., loop detection, or use of AS path length in best path

   selection) the externally visible behavior of the implementation

   shall be the same as if the implementation had run the algorithm in

   Section 4.4 and used the resulting AS_PATH attribute as it would for

   a non-BGPsec update message.



…how should an iBGP speaker perform loop detection if there’s no BGPsec_Path 
attribute?  In other words, there is no defined mechanism to run the algorithm 
in 4.4 without it.



I’m not suggesting that you include an empty attribute, but that you indicate 
in 4.4 that no BGPsec_Path attribute is equivalent to an empty AS_PATH.



Thanks!



Alvaro.
___
sidr mailing list
sidr@ietf.org
https://www.ietf.org/mailman/listinfo/sidr

Re: [sidr] AD Review of draft-ietf-sidr-delta-protocol-04

[Alvaro Retana (aretana)]

On 12/28/16, 11:43 AM, "Tim Bruijnzeels"  wrote:



Tim:



Happy New Year!!





| Thank you for your in-depth review :) It really helps to clarify the 
document. Replies in-line.

|

| I attached an updated document, but.. I did not discuss this with co-authors 
yet. So, I invite any of them to

| disagree or suggest changes to the things below.



Thanks for that.  I have some comments below – I want us to discuss the Update 
issue (M16) so we can start the IETF Last Call.



Thanks!



Alvaro.





| On 20 Dec 2016, at 14:15, Alvaro Retana (aretana)  wrote:

…

| | M2. Section 3.2. (Certificate Authority Use): “Relying Parties that do not 
support this delta protocol MUST

| | NOT reject a CA certificate merely because it has an SIA extension 
containing this new kind of

| | AccessDescription.”  By definition, an RP that has never even considered 
this document will not support

| | the delta protocol – IOW, trying to specify the behavior of RPs that may 
have never even seen this

| | document makes no sense to me, and can’t be enforced.  What is the current 
behavior when an RP receives

| | the extra SIA extension?

|

| The point of documenting this is to give RP software the option of 
recognising that a new protocol exists,

| even if they do not fully support it yet. In practice all current RPs do this 
(see below), and future RPs should

| consider this document. We have in fact been publishing these additional SIAs 
in the RIPE NCC production

| RPKI for more than a year without problems. I changed it slightly to this:

|

| "Relying Parties that choose not to support this delta protocol yet, MUST NOT 
reject a CA certificate merely

| because it has an SIA extension containing this new kind of 
AccessDescription."

|

| But, I can also live with taking this sentence out altogether.



I would prefer it if we do that.





…

| | M4. From 3.3.2. (Publishing Updates): “The update notification file SHOULD 
be kept small…older delta files

| | that…will result in total size of deltas exceeding the size of the 
snapshot, MUST be excluded.”  Here the

| | “SHOULD” and the “MUST” are in contradiction: you either do it always 
(MUST) or there may be cases when

| | you don’t (SHOULD).  s/SHOULD/should

|

| Ok, I moved the file size concern to the next bullet point instead, so that 
we have:

|

| o Any older delta files that, when combined with all more recent delta files, 
will result

|   in a total size of deltas exceeding the size of the snapshot, MUST be 
excluded to avoid

|   that RPs download more data than necessary.

|

| o The server MAY also exclude more recent delta files if it finds that their 
usage by a

|   small number of RPs that would be forced to perform a full synchronisation 
is outweighed

|   performance benefits of having a smaller update notification file. However, 
the repository

|   server MUST include all deltas that it has available for the last two hours.

|

| I hope that explains it better. I changed the SHOULD in the second bullet 
point to a MUST. The unspoken

| reason for the SHOULD was that a publication server may not have deltas.



The problem that I think can still exist is time dependent:  “older delta 
files…MUST be excluded…MUST include all deltas that is has available for the 
last two hours”.  What happens if all “older delta files” are less than two 
hours old, but the size would be too big?  Is there the possibility of a case 
where there are a lot of changes that happen close together (but more than a 
minute apart) that would then result in many delta files?  Besides not 
postponing publication for more than a minute, I don’t remember other 
mechanisms that would avoid a large number of changes…  If this case is not 
possible, then we should be ok – I would still like to understand why.





…

| | M7. 3.4.1. (Processing the Update Notification File): “MUST issue an 
operator error”  What is an “operator

| | error” and how do you issue one?  I didn’t see an error definition in the 
schema.

|

| I see your point, but.. afaik none of the other sidr RFCs and I-Ds define 
this, and just avoid talking about this

| altogether. This was undefined in over five years of deployment with rsync, 
and yet operational issues with

| rsync also happen and get reported and resolved somehow. In short I suggest 
that I just leave it out here as

| well.



No.  Sorry, but “Others didn’t do it” is not a valid reason for an incomplete 
specification.



| But to continue: All RPs do have the common sense to inform their users about 
issues (e.g. also things like:

| hey, this certificate is invalid because..), but there is no standard 
defined, so they use various formats and

| ways. I do see some benefit in defining at least a common standard because it 
might help (1) monitoring, (2)

| normative documenting of error conditions and responses, and (3) interop 
testing between RPs (Rob, David

| and I have done this a few times 

Re: [sidr] draft-ietf-sidr-bgpsec-protocol

[Sriram, Kotikalapudi (Fed)]

My previous post of this message seems to have line wrap issue.

Resending ... hopefully this avoids that issue.   - Sriram

---


Keyur,

Thank you for taking the time to read and offer comments.

I find the comments very insightful and helpful.

My comments are marked with [Sriram] below.

>From: Keyur Patel ke...@arrcus.com

>Sent: Wednesday, January 4, 2017 5:53 PM

>The document is well written, easy to read and follow.

>Some minor comments are listed below:

>1)  Section 4.1 “The BGPsec Path attribute and the AS_PATH attribute are 
>mutually exclusive. That is, any update message containing the BGPsec Path 
>attribute MUST NOT contain the AS_PATH attribute”.  For any restarting 
>speakers in a GR mode, where the bgp capability is not exchanged, the existing 
>stale routes won’t have an AS_PATH attribute. We could add some clarifying 
>that helps to indicate that such routes should be considered valid in stale 
>mode (till they get refreshed)?

[Sriram]  As you have clarified for me on the phone, what you are saying here 
is that the two BGPsec peers lost the BGPsec session and now restarting in GR 
mode, but they have not exchanged BGPsec capability this time. Hence, they are 
now simple BGP (non-BGPsec) peers in GR mode. RFC4271 considers update message 
received without a well-known AS_PATH attribute as an error, and unfortunately 
in this case the cached BGPsec updates do not have AS_PATH (albeit they have 
BGPsec_Path). So you are saying "the router should not panic" and instead 
simply treat each cached update as NOT-IN-ERROR even though it is missing 
AS_PATH attribute. This way the GR can work properly. Of course, shortly the 
updates will have AS_PATH (and not considered in error) when they get refreshed 
(over the new simple BGP session). Per your suggestion, I will include new text 
in Section 7 to describe this required behavior for the GR mode.

>2)   4.1 4th paragraph: "Note also that new signatures are only added to a 
>BGPsec update message when a BGPsec speaker is generating an update message to 
>send to an external peer (i.e., when the AS number of the peer is not equal to 
>the BGPsec speaker's own AS number).  Therefore, a BGPsec speaker who only 
>sends BGPsec update messages to peers within its own AS does not need to 
>possess any private signature keys." This text doesn't seem to apply to confed 
>peers? If so, it would be nice to clarify that this text doesn't apply to any 
>confed peers.

[Sriram] You have clarified in our phone conversation that you consider the 
inter-AS-member sessions as "iBGP" since they are all within a confederation AS 
domain. The BGPsec document considers the inter-AS-member sessions as "eBGP" 
(not "iBGP") and intra-Member-AS sessions as "iBGP".  You also clarified that 
you may call inter-AS-member sessions as "confederation-eBGP" sessions. 
Obviously, private key is required to sign over such 
"confederation-eBGP/BGPsec" sessions. I understand your point. I will put in 
new text (notes) to clarify this in the document.

>3)  Section 5 and Section 5.2, 1st paragraph: RFC4271 considers update message 
>received without a well-known AS_PATH attribute as an error.  We need some 
>text to clarify the (error handling if any) behavior when an update message is 
>received without a bgpsec and an aspath attribute. The current draft text 
>seems unclear about generation of bgpsec attribute as well (in a ibgp 
>scenario). Is it a requirement to generate an empty bgpsec attribute?

[Sriram]  As you have clarified for me over the phone, RFC 4271 (page 26) says 
the following :

   "When a BGP speaker originates a route then:

   b) the originating speaker includes an empty AS_PATH attribute in

 all UPDATE messages sent to internal peers.  (An empty AS_PATH

 attribute is one whose length field contains the value zero)."

[Sriram]  So what needs to be said in the BGPsec document is the following:  
The BGPsec_Path attribute is not attached in updates originated inside an AS 
and propagated to BGPsec capable internal peers. However, when a route is 
originated inside an AS and propagated to non-BGPsec internal peers, an empty 
AS_PATH attribute is included in the update (see [RFC 4271], page 26).

>4)   With an AS_PATH attribute in 4271 there was loop detection in place.  
>With BGPSec I don’t see that being called explicitly other than a passing 
>remark in section 5. Section 5.2 should have a check that allows a BGPsec 
>speaker to bail out of a validation procedure when a aspath loop is detected.

[Sriram]  I agree. I will include loop detection in the list of error checks in 
Section 5.2.

Sriram
___
sidr mailing list
sidr@ietf.org
https://www.ietf.org/mailman/listinfo/sidr