Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incorporates comments from CJ, AA, Warren, Russ #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ksriram25
Copy link
Contributor

@ksriram25 ksriram25 commented Mar 11, 2024

@job @mitradir
Please DO NOT merge anything else bypassing this PR.
Let us discuss this PR for a bit and then merge. This is rebased to the latest Master.

@mitradir @job @cjeker
I closed pull request #23 ( #23 ). That is where we had a lot of good discussions/comments.

This PR incorporates comments received from Claudio and Alexander on Github and in email since my previous PR#23. It also includes new text based on suggestions from Warren and Russ regarding "Updates RFC 9234" in the abstract and in Secs. 9.3 and 9.4.

Alexander: Please consider reading Section 7 carefully before making changes/commits. It should be in very good shape because it is very diligent/aligned with your and Claudio's comments/suggestions. The rest of the draft also preserved your changes with some wording refinements or proper positioning of text where I saw the need. Thanks.

@mitradir mitradir self-assigned this Mar 12, 2024
Copy link
Collaborator

@mitradir mitradir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of comments from the previous review were not properly addressed.
Some stylish changes are affecting semantics of the document.

Resource Public Key Infrastructure (RPKI) to verify the Border Gateway Protocol (BGP) AS_PATH attribute of advertised routes.
This type of AS_PATH verification provides detection and mitigation of route leaks and improbable AS paths.
This document describes procedures that make use of Autonomous System Provider Authorization (ASPA) objects in the Resource Public Key Infrastructure (RPKI) to verify the Border Gateway Protocol (BGP) AS_PATH attribute of advertised routes.
This type of AS_PATH verification provides detection and mitigation of route leaks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though we agreed that we are targeting not only route leaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
I've restored the sentence to:
"This type of AS_PATH verification provides detection and mitigation of route leaks and some forms of AS path manipulation."

submissionType="IETF"
consensus="true"
updates="9234"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where RFC9234 is updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir @cjeker
I think you are right, especially, now that we are basically referring to BGP sessions in this draft in the context of ASPA rather than Roles. I have removed the "Updates RFC 9234" flag.

ASPA-based AS_PATH verification provides detection and mitigation of route leaks and improbable AS paths.
It also provides protection, to some degree, against prefix hijacks with forged-origin or forged-path-segment (<xref target="use-cases"/>).
These new ASPA-based procedures automatically detect such anomalous AS_PATHs in announcements that are advertised between AS.
ASPA-based AS_PATH verification provides detection and mitigation of route leaks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how previous review was addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
Same response as above...
I've restored the sentence to:
"This type of AS_PATH verification provides detection and mitigation of route leaks and some forms of AS path manipulation."

These new ASPA-based procedures automatically detect such anomalous AS_PATHs in announcements that are advertised between AS.
ASPA-based AS_PATH verification provides detection and mitigation of route leaks.
It also provides protection, to some degree, against prefix hijacks with forged-origin or forged-path-segment (<xref target="property"/>).
These new ASPA-based procedures automatically detect such anomalous AS_PATHs in BGP routes that are advertised between ASes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to be precise is hould BGP Updates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
Yes, correct. Done.

The level of risk, however, depends significantly on the extent of propagation of the anomalies.
For example, a route leak or hijack that is propagated only to customers may cause bottlenecking within a particular ISP's customer cone, but if the anomaly propagates through lateral (i.e., non-transit) peers and transit providers, or reaches global distribution through transit-free networks, then the ill effects will likely be amplified and experienced across continents.
For example, a route leak that is propagated only to customers may cause bottlenecking within a particular ISP's customer cone, but if the anomaly propagates through lateral (i.e., non-transit) peers and transit providers at the top-tier, then the ill effects will likely be amplified and experienced worldwide.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why dropping 'or hijack'?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is top-tier? the old definition is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir

Why dropping 'or hijack'?
Yes, no need to drop it. I have put 'hijack' back in.

@@ -310,26 +303,102 @@ authorized(AS x, AS y) = / Else, "Provider+" if the VAP-SPAS entry
If an attacker creates a route leak intentionally, they may try to strip their AS from the AS_PATH.
To partly guard against that, a check is necessary to match the most recently added AS in the AS_PATH to the BGP neighbor's ASN.
This check MUST be performed as specified in Section 6.3 of <xref target="RFC4271"/>.
If the check fails, then the AS_PATH is considered a Malformed AS_PATH and the UPDATE is considered to be in error (Section 6.3 of <xref target="RFC4271"/>).
If the check fails, then the AS_PATH is considered a Malformed AS_PATH and the UPDATE is in error (Section 6.3 of <xref target="RFC4271"/>).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYPO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
I have restored the sentence back to how it was:
"If the check fails, then the AS_PATH is
considered a Malformed AS_PATH and the UPDATE is considered to be in
error (Section 6.3 of [RFC4271])."
Let me know if you meant something else by "TYPO". This sentence is borrowed from RFC 4271.

The case of transparent RS MUST also be appropriately taken care of (e.g., by suspending the neighbor ASN check).
The check fails also when the AS_PATH is empty (zero length) and such UPDATEs will also be considered to be in error.
The AS_PATH check fails also when it is empty (zero length) since such an UPDATE in eBGP is considered to be an error.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ambigious, check is empty?
And we are discussing only specified scenarios for inter-AS connectivity, there is no reason to add eBGP in this abstract.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
Good catch. Fixed it as follows:
"If the AS_PATH is empty (zero length), then also the
UPDATE is considered to be an error."

<section title="Semantics of Valid, Invalid, and Unknown" anchor="semantics">
<t>
The ASPA-based path verification algorithms (<xref target="Upflow"/> and <xref target="Downflow"/>) evaluate an AS_PATH as Valid, Invalid, or Unknown.
'Invalid' outcome means that the AS_PATH contains a route leak (i.e., valley-free violation <xref target="RFC7908"/>) based on the available ASPAs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just abnormal path, we don't know if it is a route leak a malformed path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
Good observation. Section 7.1 is redundant and perhaps misleading because the next section "Basic Principles of Route Leak Detection" defines the terms properly anyway. I have removed Section 7.1 (what used to be "Semantics of Valid, Invalid, and Unknown").

<t>
The ASPA-based path verification algorithms (<xref target="Upflow"/> and <xref target="Downflow"/>) evaluate an AS_PATH as Valid, Invalid, or Unknown.
'Invalid' outcome means that the AS_PATH contains a route leak (i.e., valley-free violation <xref target="RFC7908"/>) based on the available ASPAs.
'Valid' outcome means that sufficient ASPA information is available to determine that the AS_PATH attribute is route-leak free (i.e., valley free).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also incorrect. 'Valid' means that we have enough corresponding ASPA objects, but complex relations may create route leaks, that can't be detected with ASPA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
Same comment as above. I have removed Section 7.1 "Semantics of Valid, Invalid, and Unknown".

</t>
<t>
If N = 1, then the procedure halts with the outcome "Valid".
Else, continue.
</t>
<t>
At this step, N &ge; 2. If max_up_ramp is less than N the procedure halts with the outcome 'Invalid'.
At this step, N &ge; 2.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are again ignoring my comments from the previous review. max and min have exact semntics. It is important to have variables prooperly named.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir
I have said this before. I think we should first describe route leak and the principle of how "Invalid (route leak) is found using ASPA (see new Sec. 7.1.1). (Rather than jump too soon to the inverted V picture and the ramps.)

"... for the AS path to be Invalid (route leak), there must be two hops -- one on the
left and another on the right -- facing each other and each one is not C2P
(i.e., each is P2C or P2P)." (new Section 7.1.1)

In finding this condition using ASPA, NP_left and NP_right is more suitable/intuitive terminology (which map to your max_down_ramp and max_up_ramp). NP = Not Provider. We can do a zoom call to discuss and clarify this and more about Sec. 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitradir @cjeker
I will make a Commit of the changes tomorrow.

@ksriram25
Copy link
Contributor Author

@mitradir @cjeker @job @ksriram25
I have made a new commit. It is here (in PR#24):
5a91feb

Please see email sent May 12 with Commit comments and attachments (.txt and diff files). Comments repeated below:

The following comments/changes have been incorporated:

  1. Comments/convergence (AA, CJ, and I) related to mutual-transit and complex relations/sessions.
  2. All other discussions on Github (AA, CJ, and I).
  3. Input from SIDROPS/GROW mailing list about mutual-transit and complex: https://mailarchive.ietf.org/arch/msg/sidrops/5RpaU4Rzirq96OFb0rxFQGBtmJ8/
  4. Several text refinements scattered throughout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants