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

Perhaps s/bgp_path.last_nonaggregated/bgp_path.last/g #56

Closed
wants to merge 1 commit into from

Conversation

job
Copy link
Contributor

@job job commented Apr 20, 2020

bgp_path.last is consistent with RFC 6907 7.1.9-11 according to BIRD
developers.

I think I misunderstood what last_nonaggregated does when I recommended it, what do you think?

bgp_path.last is consistent with RFC 6907 7.1.9-11 according to BIRD
developers.
@pierky pierky closed this in 350bc38 May 3, 2020
@pierky
Copy link
Owner

pierky commented May 3, 2020

Thanks @job, merged.

@s1sfa
Copy link

s1sfa commented May 28, 2020

I don't think this RFC is is for the same thing that has changed here. This seems to discus RPKI roa validation. This is changing the checks if origin ASN is in IRRDB ASSET. This bgp_path.last attribute returns 0 for routes with atomic aggregate set. Unless behaviour has changed in BIRD we will have to tell people that using atomic aggregate on BGP isn't supported. This should at least be a configurable option. I suggested last_nonaggregated initially because a bunch of my participant routes were being dropped on VANIX routeserver(not in allowed as-sets - REJECTING).

@job
Copy link
Contributor Author

job commented May 28, 2020 via email

@pierky
Copy link
Owner

pierky commented May 28, 2020

Hi @s1sfa,

could you share, even privately, some output from your BIRD instance?

show route x.y.z.0/L all

show route x.y.z.0/L all where bgp_path.last = <expected ASN>
show route x.y.z.0/L all where bgp_path.last_nonaggregated = <expected ASN>

show route x.y.z.0/L all where bgp_path.last ~ AS_SET_xxxx
show route x.y.z.0/L all where bgp_path.last_nonaggregated ~ AS_SET_xxxx

show route x.y.z.0/L where roa_check(RPKI, net, bgp_path.last) = ROA_VALID
show route x.y.z.0/L where roa_check(RPKI, net, bgp_path.last_nonaggregated) = ROA_VALID

show route x.y.z.0/L where roa_check(RPKI, net, bgp_path.last) = ROA_INVALID
show route x.y.z.0/L where roa_check(RPKI, net, bgp_path.last_nonaggregated) = ROA_INVALID

show route x.y.z.0/L where roa_check(RPKI, net, bgp_path.last) = ROA_UNKNOWN
show route x.y.z.0/L where roa_check(RPKI, net, bgp_path.last_nonaggregated) = ROA_UNKNOWN

Here x.y.z.0/L should be the prefix of one of those routes that you mentioned were dropped, and AS_SET_xxxx should be the name of the BIRD AS set generated by ARouteServer for your participant.

@pierky pierky reopened this May 28, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 72.934% when pulling b85e19d on job:master into b0b3436 on pierky:master.

@s1sfa
Copy link

s1sfa commented May 29, 2020

Here is a specific example of a route that is accepted now that would be rejected by this update. bgp_path.last == 0 for all aggregated routes. If bgp_path.last is the only variable used to check ASN in ASSETs it will always fail on these aggregated routes.

`bird> show route 199.59.185.0/24 all
199.59.185.0/24 via 206.41.104.36 on eno2 [AS6391_1 2020-04-15] * (100) [?]
Type: BGP unicast univ
BGP.origin: Incomplete
BGP.as_path: 6391 53661 53661 53661 { 53661 53661 }
BGP.next_hop: 206.41.104.36
BGP.med: 0
BGP.local_pref: 100
BGP.aggregator: 199.59.186.129 AS53661
BGP.community: (0,27232) (0,54643) (6391,3000) (6391,53661)
BGP.ext_community: (generic, 0x43000000, 0x1)
BGP.large_community: (64234, 11, 3) (64234, 11, 1)
via 206.41.104.27 on eno2 [AS6939_1 2020-05-08] (100) [i]
Type: BGP unicast univ
BGP.origin: IGP
BGP.as_path: 6939 16696 53661 53661 53661 { 53661 53661 }
BGP.next_hop: 206.41.104.27
BGP.med: 0
BGP.local_pref: 100
BGP.aggregator: 199.59.186.129 AS53661
BGP.community: (0,6939) (0,16509) (0,15169) (0,13335) (0,15133) (0,22822) (0,12989) (0,2906) (0,20940) (0,16265) (0,16276) (0,12876) (0,48641)
BGP.ext_community: (generic, 0x43000000, 0x1)
BGP.large_community: (64234, 11, 3) (64234, 11, 1)
bird> show route 199.59.185.0/24 where bgp_path.last = 53661
bird> show route 199.59.185.0/24 where bgp_path.last_nonaggregated = 53661
199.59.185.0/24 via 206.41.104.36 on eno2 [AS6391_1 2020-04-15] * (100) [?]
via 206.41.104.27 on eno2 [AS6939_1 2020-05-08] (100) [i]
bird> show route 199.59.185.0/24 where bgp_path.last ~ AS_SET_AS_URBAN_asns
bird> show route 199.59.185.0/24 where bgp_path.last_nonaggregated ~ AS_SET_AS_URBAN_asns
199.59.185.0/24 via 206.41.104.36 on eno2 [AS6391_1 2020-04-15] * (100) [?]
via 206.41.104.27 on eno2 [AS6939_1 2020-05-08] (100) [i]

bird> show route 199.59.185.0/24 where roa_check(RPKI, net, bgp_path.last) = ROA_INVALID
bird> show route 199.59.185.0/24 where roa_check(RPKI, net, bgp_path.last_nonaggregated) = ROA_INVALID
bird> show route 199.59.185.0/24 where roa_check(RPKI, net, bgp_path.last) = ROA_VALID
bird> show route 199.59.185.0/24 where roa_check(RPKI, net, bgp_path.last_nonaggregated) = ROA_VALID
bird> show route 199.59.185.0/24 where roa_check(RPKI, net, bgp_path.last_nonaggregated) = ROA_UNKNOWN
199.59.185.0/24 via 206.41.104.36 on eno2 [AS6391_1 2020-04-15] * (100) [?]
via 206.41.104.27 on eno2 [AS6939_1 2020-05-08] (100) [i]
bird> show route 199.59.185.0/24 where roa_check(RPKI, net, bgp_path.last) = ROA_UNKNOWN
199.59.185.0/24 via 206.41.104.36 on eno2 [AS6391_1 2020-04-15] * (100) [?]
via 206.41.104.27 on eno2 [AS6939_1 2020-05-08] (100) [i]
`

@pierky
Copy link
Owner

pierky commented May 29, 2020

It seems that bgp_path.last_nonaggregated must be used for comparison with AS sets, but not for BOV (accordingly to the devs https://www.mail-archive.com/bird-users@network.cz/msg05152.html).

I will try to setup a test scenario to validate this. Does anyone know if there's an easy way to forge and announce an atomic aggregate route using ExaBGP? I believe (not sure though) that's not possible doing it in BIRD. Worst case I'll try to play a bit with some BGP replay tool (bgpsimple maybe?), in order to read it from a bgpdump file and announce it to BIRD.

@s1sfa
Copy link

s1sfa commented May 30, 2020

I haven't done this in EXABGP before but google shows the options.
https://manpages.debian.org/testing/exabgp/exabgp.conf.5.en.html
static { has 2 options: atomic-aggregate; and aggregator :; . I think that is what we are dealing with in the BIRD routing table that ends up with bgp_path.last with 0 as the value.
I looked at how juniper handles this. It looks like it ignores the { } braces and just treats it like a standard ASPATH for ASPATH regex matching anyway.

@job
Copy link
Contributor Author

job commented May 30, 2020

Hey Chris, Thank you so much for sharing more details! I think you are right that this change broke something.

I did a blanket replace on bgp_path.last_nonaggregated in the entire codebase, but it seems (like @pierky already observed!) that my change proposal should've been to only update the parts of the code that use BIRD's roa_checkfunction. And not touched the IRR stuff.

From a code development perspective, @pierky do you want me to --force push into this PR a rebase on top of what you have in master to see what I think we want? Or how do we proceed?

@pierky
Copy link
Owner

pierky commented May 30, 2020

Thanls @s1sfa, I'll try to use ExaBGP to inject a similar route into the integration testing environment and build a test case for this scenario.

From a code development perspective, @pierky do you want me to --force push into this PR a rebase on top of what you have in master to see what I think we want? Or how do we proceed?

@job my plan is to just push new commits into the "dev" branch (I'll recreate it on top of master), so that it will eventually trigger the test pipeline to validate that everything works as expected. I'll probably work on it during this weekend.

@pierky
Copy link
Owner

pierky commented May 30, 2020

@s1sfa, I've looked a bit into BOV and AS_SETs in the AS_PATH attributes, and my conclusion is that using bgp_path.last is consistent with RFC 6907 7.1.9.

Specifically, RFC6907 says

Route has {10.1.0.0/24, AS_SET [AS 64496] appears in the rightmost
position in the AS_PATH}

ROA: {10.1.0.0/22, maxLength = 24, AS 64496}

Recommended RPKI prefix-origin validation interpretation: Route is
Invalid.

This matches the case of 199.59.185.0/24 with AS_PATH 6391 53661 53661 53661 { 53661 53661 }.
And this is from that RFC again:

In the spirit of [RFC6472], any route with an AS_SET in it
should not be considered valid (by ROA-based validation). If the
route contains an AS_SET and a covering ROA prefix exists for the
route prefix, then the route should get an Invalid status.
(Note: AS match or mismatch consideration does not apply.)

So basically any route with an AS_SET in the AS_PATH for which a covering ROA exists should be treated as an INVALID.

BCP172/RFC6472 also covers the topic of AS_SETs when it's up to origin validation:

By performing aggregation, a router is, in essence, combining
multiple existing routes into a single new route. This type of
aggregation blurs the semantics of what it means to originate a
route.

I believe that in terms of origin validation (which is also the goal of the IRR-based check) using bgp_path.last or bgp_path.last_nonaggregated would equally lead to a misleading result, since what's being compared against the AS set generated from the IRR objects is either an AS_SET (thus, not comparable with an ASN) or an ASN which is not guaranteed to be the origin of the route (as 111 in the case of an AS_PATH like [111, {222, 222}]). In the former case the route would be rejected (as in the case you mentioned), in the latter the route would be accepted, maybe mistakenly (as in the case above).

Along the line of BCP172/RFC6472, I'm inclined towards not changing the behaviour of the tool: of course, I'd love to hear more from you and @job.

If you want to forcedly accept those routes, you can configure a client-level white list as mentioned here.
Entries like the following should be enough to bypass the IRR level check for a specific prefix:

white_list_route:
  - prefix: 199.59.185.0
    length: 24

Please note the absence of the asn attribute, that is optional; if it's not set, AS-based origin validation is not performed at all and that prefix is accepted.

pierky added a commit that referenced this pull request May 30, 2020
pierky added a commit that referenced this pull request May 30, 2020
pierky added a commit that referenced this pull request May 30, 2020
pierky added a commit that referenced this pull request May 30, 2020
@pierky
Copy link
Owner

pierky commented May 30, 2020

Accordingly to further tests, it seems that OpenBGPD implements different behaviours for RPKI BOV and "regular" origin validation against an AS set.

A route with an AS_PATH like 222 333 { 444 555 } fails the BOV validation against a ROA whose AS is 333, as wanted by RFC 6907 7.1.9, but then a match like the following one succeeds:

as-set "AS_SET_AS_AS222_asns" {
    333
}
match ... source-as as-set AS_SET_AS_AS222_asns

The OpenBGPD implementation of vanilla origin AS validation in presence of an AS_SET looks like the same that could be obtained in BIRD using bgp_path.last_nonaggregated.

On the basis of my findings, it seems that we can define the following cases:

BGP speaker Feature Behaviour
BIRD RPKI BOV reject, as per RFC6907
OpenBGPD RPKI BOV reject, as per RFC6907
BIRD, bgp_path.last IRR OV reject
BIRD, bgp_path.last_nonaggregated IRR OV accept
OpenBGPD IRR OV accept

The goal of both the mechanisms (RPKI BOV and IRR-based filtering) is to someway validate the origin of a route, and I see the implementation of how that is done in RPKI more oriented towards a possible "future" (worth mentioning draft-ietf-idr-deprecate-as-set-confed-set-03 on this topic besides BCP172/RFC6472 already mentioned in my previous comment) and more consistent with the idea of "origin".
Also, even if a route would pass the IRR filter, it would be dropped by the RPKI BOV check (assuming it's covered with a ROA).

That said, I'm still inclined towards not reverting the implementation of BIRD back to the use of bgp_path.last_nonaggregated for IRR filters, keeping it consistent with the RPKI origin validation mechanism, but I'd rather mention this different implementation in the doc.

Any thoughts from the field?

@job
Copy link
Contributor Author

job commented May 30, 2020 via email

@pierky
Copy link
Owner

pierky commented May 31, 2020

Small question of clarification: when you say “RPKI BOV” do you mean “RPKI ROV” (as in RFC 6811 Route Origin Validation)?

Yes, that.

@s1sfa
Copy link

s1sfa commented May 31, 2020

Ok, thanks for the detailed look into this @pierky and @job . It looks like the answer is that AS_SET isn't intended to be supported on the internet and it's use is discouraged as per RFC6472. https://tools.ietf.org/html/rfc6472. I think a tool like this used by many organizations has an obligation to abide by RFC's.

@job
Copy link
Contributor Author

job commented May 31, 2020

What’s really confusing in all of this is that there are AS-SET objects in the IRR, and AS_SET constructs in BGP, and the as-set config construct in OpenBGPD - and in this discussion we are addressing all three and now they interact with each other.

pierky added a commit that referenced this pull request Jun 7, 2020
This commit adds test cases related to the way AS_PATHs whose origin
is an AS_SET are treated with regards to RPKI origin validation and
IRR checks.

RFC6907:

> Route has {10.1.0.0/24, AS_SET [AS 64496] appears in the rightmost
> position in the AS_PATH}
>
> ROA: {10.1.0.0/22, maxLength = 24, AS 64496}
>
> Recommended RPKI prefix-origin validation interpretation: Route is
> Invalid.

> In the spirit of [RFC6472], any route with an AS_SET in it
> should not be considered valid (by ROA-based validation). If the
> route contains an AS_SET and a covering ROA prefix exists for the
> route prefix, then the route should get an Invalid status.
> (Note: AS match or mismatch consideration does not apply.)

So basically any route with an AS_SET in the AS_PATH for which a
covering ROA exists should be treated as an INVALID.
With this commit a test case to validate this is introduced.
Even though such test case is not strictly related to verify the
behaviour of ARouteServer but rather the implementation of the
BGP speakers, it was useful to deal with PR #56.

Two more test cases cover the way IRR checks are performed when the
AS_PATH ends with an AS_SET.

BIRD and OpenBGPD behave differently under that circumstance.

In BIRD, comparing a list of valid origin ASNs (generated from info
fetched from IRRs) against `bgp_path.last` leads to a mismatch,
regardless of the content of the AS_SET

In OpenBGPD, the vanilla origin AS validation in presence of an
AS_SET looks like the same that could be obtained in BIRD using
`bgp_path.last_nonaggregated`. A route with an AS_PATH like
222 333 { 444 555 } fails the BOV validation against a ROA whose
AS is 333, as wanted by RFC6907 7.1.9, but then a match like the
following one succeeds:

as-set "AS_SET_AS_AS222_asns" {
    333
}
match ... source-as as-set AS_SET_AS_AS222_asns

The goal of both the mechanisms (RPKI OV and IRR-based filtering)
is to someway validate the origin of a route, and I see the
implementation of how that is done in RPKI more oriented towards a
possible "future" (worth mentioning
draft-ietf-idr-deprecate-as-set-confed-set-03 on this topic, and
BCP172/RFC6472 too) and more consistent with the idea of "origin".

The BIRD implementation of IRR-based checks using `bgp_path.last`
is consistent with the RPKI OV behaviour, and the one I also
personally prefer and see more future-oriented. The OpenBGPD one
is more "relaxed". This will lead to an inconsistency of how
route-servers configured with the same ARouteServer configuration
but running different BGP speakers will behave when it's up to
IRR-based checks of AS_PATH ending with AS_SET.
pierky added a commit that referenced this pull request Jun 7, 2020
This commit adds test cases related to the way AS_PATHs whose origin
is an AS_SET are treated with regards to RPKI origin validation and
IRR checks.

RFC6907:

> Route has {10.1.0.0/24, AS_SET [AS 64496] appears in the rightmost
> position in the AS_PATH}
>
> ROA: {10.1.0.0/22, maxLength = 24, AS 64496}
>
> Recommended RPKI prefix-origin validation interpretation: Route is
> Invalid.

> In the spirit of [RFC6472], any route with an AS_SET in it
> should not be considered valid (by ROA-based validation). If the
> route contains an AS_SET and a covering ROA prefix exists for the
> route prefix, then the route should get an Invalid status.
> (Note: AS match or mismatch consideration does not apply.)

So basically any route with an AS_SET in the AS_PATH for which a
covering ROA exists should be treated as an INVALID.
With this commit a test case to validate this is introduced.
Even though such test case is not strictly related to verify the
behaviour of ARouteServer but rather the implementation of the
BGP speakers, it was useful to deal with PR #56.

Two more test cases cover the way IRR checks are performed when the
AS_PATH ends with an AS_SET.

BIRD and OpenBGPD behave differently under that circumstance.

In BIRD, comparing a list of valid origin ASNs (generated from info
fetched from IRRs) against `bgp_path.last` leads to a mismatch,
regardless of the content of the AS_SET

In OpenBGPD, the vanilla origin AS validation in presence of an
AS_SET looks like the same that could be obtained in BIRD using
`bgp_path.last_nonaggregated`. A route with an AS_PATH like
222 333 { 444 555 } fails the BOV validation against a ROA whose
AS is 333, as wanted by RFC6907 7.1.9, but then a match like the
following one succeeds:

as-set "AS_SET_AS_AS222_asns" {
    333
}
match ... source-as as-set AS_SET_AS_AS222_asns

The goal of both the mechanisms (RPKI OV and IRR-based filtering)
is to someway validate the origin of a route, and I see the
implementation of how that is done in RPKI more oriented towards a
possible "future" (worth mentioning
draft-ietf-idr-deprecate-as-set-confed-set-03 on this topic, and
BCP172/RFC6472 too) and more consistent with the idea of "origin".

The BIRD implementation of IRR-based checks using `bgp_path.last`
is consistent with the RPKI OV behaviour, and the one I also
personally prefer and see more future-oriented. The OpenBGPD one
is more "relaxed". This will lead to an inconsistency of how
route-servers configured with the same ARouteServer configuration
but running different BGP speakers will behave when it's up to
IRR-based checks of AS_PATH ending with AS_SET.
@pierky
Copy link
Owner

pierky commented Jun 7, 2020

I've added to the doc a mention about the different way in which BIRD and OpenBGPD handle IRR checks for routes whose AS_PATH ends with an AS_SET.

Going to close this for now.

@pierky pierky closed this Jun 7, 2020
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.

4 participants