Hello, RFC 9234 provides a new mechanism for route leak detection and prevention. In the attachment you can find a patch which introduces this functionality for BIRD 2.0.9.
On Tue, May 31, 2022 at 02:38:07PM +0300, Eugene Bogomazov wrote:
Hello,
RFC 9234 provides a new mechanism for route leak detection and prevention.
In the attachment you can find a patch which introduces this functionality for BIRD 2.0.9.
Hello Thanks for the patch, i will check it soon. One issue i just noticed, could you also add patch for documentation (doc/bird.sgml), documenting these options? -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
I will add a documentation patch in attachment. I will also add a patch that removes BGP ROLE MAX from consideration, as was discussed before in a private conversation. This will make the previous patch with Roles more reliable and secure in case new roles suddenly appear in the future. However, after creating a documentation patch and consulting with the main RFC author, I ran into trouble. TL;DR: What are the best ways to include AFI/SAFI channel check during attribute creation? RFC 9234 specifically clarifies that it can only be applied on IPv4/IPv6 unicast sessions. On all other sessions the OTC attribute should be decoded from and also transferred to such sessions without any change. The problem is how to include AFI/SAFI checks in the code: 1) If we have information about a channel (as in *bgp_update_attrs*), we can simply run the following check proto/bgp/attrs.c if (bgp_channel_is_role_applicable(c)) { ... } proto/bpg/bgpd.h static inline int bgp_channel_is_role_applicable(struct bgp_channel *c) { return (c->afi == BGP_AF_IPV4 || c->afi == BGP_AF_IPV6); } static inline int bgp_cc_is_role_applicable(struct bgp_channel_config *c) { return (c->afi == BGP_AF_IPV4 || c->afi == BGP_AF_IPV6); } 2) However, in *bgp_decode_attrs *the AFI/SAFI information is only available after NLRI decoding. There is also a side approach with using *bgp_find_update_afi,* a static function from proto/bgp/packets.c to extract AFI/SAFI from a packet payload. The question is - what is the best way to extract AFI/SAFI information: apply OTC rules after NLRI is decoded (but where?) or change and reuse static bgp_find_update_afi (if possible) to get AFI/SAFI information before processing the OTC attribute? 3) But the biggest problem is with *bgp_preexport*. It uses bgp_proto, which doesn't use channel information when filtering routes with BGP specific rules. And I don't find a way to add this support to this filter. The question is - how to apply AFI/SAFI check for OTC attribute rules during bgp export and where is the best place to do so? Of course, if we want it to be like a predefined route-map. On Thu, Jun 2, 2022 at 6:31 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, May 31, 2022 at 02:38:07PM +0300, Eugene Bogomazov wrote:
Hello,
RFC 9234 provides a new mechanism for route leak detection and prevention.
In the attachment you can find a patch which introduces this functionality for BIRD 2.0.9.
Hello
Thanks for the patch, i will check it soon. One issue i just noticed, could you also add patch for documentation (doc/bird.sgml), documenting these options?
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Tue, Jun 14, 2022 at 01:35:36PM +0300, Eugene Bogomazov wrote:
TL;DR: What are the best ways to include AFI/SAFI channel check during attribute creation?
Hi
RFC 9234 specifically clarifies that it can only be applied on IPv4/IPv6 unicast sessions. On all other sessions the OTC attribute should be decoded from and also transferred to such sessions without any change.
The problem is how to include AFI/SAFI checks in the code: 1) If we have information about a channel (as in *bgp_update_attrs*), we can simply run the following check proto/bgp/attrs.c if (bgp_channel_is_role_applicable(c)) { ... } proto/bpg/bgpd.h static inline int bgp_channel_is_role_applicable(struct bgp_channel *c) { return (c->afi == BGP_AF_IPV4 || c->afi == BGP_AF_IPV6); }
static inline int bgp_cc_is_role_applicable(struct bgp_channel_config *c) { return (c->afi == BGP_AF_IPV4 || c->afi == BGP_AF_IPV6); }
Yes. in bgp_update_attrs() it would make sense to add the check directly in such way.
2) However, in *bgp_decode_attrs *the AFI/SAFI information is only available after NLRI decoding. There is also a side approach with using *bgp_find_update_afi,* a static function from proto/bgp/packets.c to extract AFI/SAFI from a packet payload. The question is - what is the best way to extract AFI/SAFI information: apply OTC rules after NLRI is decoded (but where?) or change and reuse static bgp_find_update_afi (if possible) to get AFI/SAFI information before processing the OTC attribute?
Technically, one can receive an update message that has IPv4-unicast directly in ip_reach_nlri, and something different (e.g. VPNv4) in mp_reach_nlri. Although such messages are deprecated, they should still be accepted as valid. Therefore, one has to apply OTC rules to some NLRI and not to others. That is a similar issue like with bgp_next_hop, which may have different value for these two sets of NLRI. So perhaps you could handled that in bgp_finish_attrs() ?
3) But the biggest problem is with *bgp_preexport*. It uses bgp_proto, which doesn't use channel information when filtering routes with BGP specific rules. And I don't find a way to add this support to this filter. The question is - how to apply AFI/SAFI check for OTC attribute rules during bgp export and where is the best place to do so? Of course, if we want it to be like a predefined route-map.
We have a change in 3.0 branch to have channel argument in bgp_preexport(), we will rebase this change to master branch soon. So you can assume it will be there. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Hi, I checked BIRD v2.0.10 and didn't find the channel argument in bgp_preexport. So AFI/SAFI checks still can't be added there. When will it appear? And how can I help the patch get accepted as soon as possible? P.S. A pull request with roles for FRR was accepted today. So, the adoption of our RFC has begun. And BIRD could be next.
Dear all, On Mon, Jun 27, 2022 at 02:21:11PM +0300, Eugene Bogomazov wrote:
P.S. A pull request with roles for FRR was accepted today. So, the adoption of our RFC has begun. And BIRD could be next.
I'd like to chime in on this party line... :-) Today OpenBSD developers added rudimentary support for RFC 9234 in OpenBGPD: https://github.com/openbsd/src/commit/1114d9c29bf857e7ee24290cd067ad5afc3af0... The OpenBSD team is very interested in interoperability testing with BIRD (and ofcourse FRR). Kind regards from a fellow BGP enthusiast! Job ps. I added decoding support to OpenBSD's tcpdump [0], perhaps it would be good if some volunteers also extend WireShark and TCPDump.org? Eugene, are you volunteering? :-) [0] https://github.com/openbsd/src/commit/6f4278525629a4c05a97373a19fd289b1083d7...
Hello! On 6/27/22 1:21 PM, Eugene Bogomazov wrote:
Hi,
I checked BIRD v2.0.10 and didn't find the channel argument in bgp_preexport. So AFI/SAFI checks still can't be added there. When will it appear? And how can I help the patch get accepted as soon as possible?
The "next-preexport" branch should be good for you to rebase your changes onto: https://gitlab.nic.cz/labs/bird/-/commits/next-preexport Let me know if there is something more needed from us. Thank you for contributing. Maria
On Mon, Jun 27, 2022 at 07:07:53PM +0200, Maria Matejka wrote:
Hello!
On 6/27/22 1:21 PM, Eugene Bogomazov wrote:
Hi,
I checked BIRD v2.0.10 and didn't find the channel argument in bgp_preexport. So AFI/SAFI checks still can't be added there. When will it appear? And how can I help the patch get accepted as soon as possible?
The "next-preexport" branch should be good for you to rebase your changes onto:
Merged to master together with rebased "next" branch. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Hello everyone, The current changes were sufficient to add the described AFI/SAFI check. Role/OTC rules now only apply to the unicast session. I've checked this on a stand with a mixture of multicast and unicast sessions and it seems to work well in all cases. As a result, all the necessary basic functionality for roles has been achieved. I decided to collect all my patches into one to make patching easier and I'll attach it to this post. This can be applied to the latest master version. If anything else is needed, please let me know. On Mon, Jun 27, 2022 at 10:24 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 27, 2022 at 07:07:53PM +0200, Maria Matejka wrote:
Hello!
On 6/27/22 1:21 PM, Eugene Bogomazov wrote:
Hi,
I checked BIRD v2.0.10 and didn't find the channel argument in bgp_preexport. So AFI/SAFI checks still can't be added there. When will it appear? And how can I help the patch get accepted as soon as possible?
The "next-preexport" branch should be good for you to rebase your changes onto:
Merged to master together with rebased "next" branch.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Tue, Jun 28, 2022 at 12:37:08PM +0300, Eugene Bogomazov wrote:
Hello everyone,
The current changes were sufficient to add the described AFI/SAFI check. Role/OTC rules now only apply to the unicast session. I've checked this on a stand with a mixture of multicast and unicast sessions and it seems to work well in all cases.
As a result, all the necessary basic functionality for roles has been achieved.
I decided to collect all my patches into one to make patching easier and I'll attach it to this post. This can be applied to the latest master version.
Hello Finally did some thorough review and prepared it for merging. Found some bugs: bgp_update_attrs(): bgp_set_attr_u32(&attrs, pool, BA_ONLY_TO_CUSTOMER, 0, p->local_as); Here should be p->public_as, to handle case of AS confederation boundary, where local_as is internal member-AS, while public_as is a confederation ID. bgp_read_capabilities(): caps->role = BGP_ROLE_UNDEFINED; This should be set when caps is allocated (caps = mb_allocz()), as it is possible to call bgp_read_capabilities() multiple times (if capabilities are split to multiple OPEN option). There should also be a check for received capability value equal to BGP_ROLE_UNDEFINED, in which case it should fail. bgp_format_role_name(): if (role <= 5) Here should be (role < 5)? Also did some minor changes: - renamed 'strict mode' to 'require roles' - removed conn->neighbor_role, just used caps->role - simplified error handling in bgp_read_capabilities() and bgp_finish_attrs() - improve error messages in case of route leaks The final patch is here: https://gitlab.nic.cz/labs/bird/-/commit/c73b5d2d3d94204d2a81d93efd02c4c1158... Are you OK with these changes? If so, i will merge it to the master branch. I am also working on some test setup for our CI. There are still two remaining issues related to BGP roles (which could be targetted in following patches): 1) As confederation handling It does not really work properly. RFC says: Also, on egress from the AS Confederation, an UPDATE MUST NOT contain an OTC Attribute with a value corresponding to any Member-AS Number other than the AS Confederation Identifier. That is not done and cannot be done based on local information. That is why AS_PATH has separate AS_SEQUENCE and AS_CONFED_SEQUENCE, so internal ASes can be stripped on confederation boundary without knowledge of all internal ASes. This is something that is underestimated in the RFC 9234. We could either just disallow roles on intra-confederation EBGP links, or just put big fat warning when configured. 2) Filter support for bgp_otc attribute I think there should be filter support, although it could be controversial considering RFC says: Once the OTC Attribute has been set, it MUST be preserved unchanged (this also applies to an AS Confederation). The operator MUST NOT have the ability to modify the procedures defined in this section. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Hello, I've reviewed all the changes and am happy with them. Had some misgivings about renaming 'strict mode ' to 'required roles' , but since this option can be easily found by searching for a phrase in the documentation, it looks fine too. Therefore, I have no additional concerns and agree with the merging of the current version of the patch. Two words about the mentioned problems. 1) In the RFC, we wanted to address a situation where roles could be used in a complex confederation setup without leaking information outside the confederation. However, since this action is NOT RECOMMENDED, the ISP should apply route filtering or attribute removal at the edge of an AS Confederation with a member ASN as an OTC value at its own risk. So, warning seems to be the most appropriate solution for this case. 2) Including the bgp_otc attribute in the route map can be a great option for complex bgp relationships between two ISPs. And they can be used in some weird scenarios (like the confederation scenario mentioned above, for example). They were not included in the core patch only because they are not part of the core functionality of the roles that we want to provide as soon as possible. On Mon, Jul 11, 2022 at 6:59 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Jun 28, 2022 at 12:37:08PM +0300, Eugene Bogomazov wrote:
Hello everyone,
The current changes were sufficient to add the described AFI/SAFI check. Role/OTC rules now only apply to the unicast session. I've checked this on a stand with a mixture of multicast and unicast sessions and it seems to work well in all cases.
As a result, all the necessary basic functionality for roles has been achieved.
I decided to collect all my patches into one to make patching easier and I'll attach it to this post. This can be applied to the latest master version.
Hello
Finally did some thorough review and prepared it for merging.
Found some bugs:
bgp_update_attrs(): bgp_set_attr_u32(&attrs, pool, BA_ONLY_TO_CUSTOMER, 0, p->local_as);
Here should be p->public_as, to handle case of AS confederation boundary, where local_as is internal member-AS, while public_as is a confederation ID.
bgp_read_capabilities(): caps->role = BGP_ROLE_UNDEFINED;
This should be set when caps is allocated (caps = mb_allocz()), as it is possible to call bgp_read_capabilities() multiple times (if capabilities are split to multiple OPEN option).
There should also be a check for received capability value equal to BGP_ROLE_UNDEFINED, in which case it should fail.
bgp_format_role_name(): if (role <= 5)
Here should be (role < 5)?
Also did some minor changes:
- renamed 'strict mode' to 'require roles' - removed conn->neighbor_role, just used caps->role - simplified error handling in bgp_read_capabilities() and bgp_finish_attrs() - improve error messages in case of route leaks
The final patch is here:
https://gitlab.nic.cz/labs/bird/-/commit/c73b5d2d3d94204d2a81d93efd02c4c1158...
Are you OK with these changes? If so, i will merge it to the master branch.
I am also working on some test setup for our CI.
There are still two remaining issues related to BGP roles (which could be targetted in following patches):
1) As confederation handling
It does not really work properly. RFC says:
Also, on egress from the AS Confederation, an UPDATE MUST NOT contain an OTC Attribute with a value corresponding to any Member-AS Number other than the AS Confederation Identifier.
That is not done and cannot be done based on local information. That is why AS_PATH has separate AS_SEQUENCE and AS_CONFED_SEQUENCE, so internal ASes can be stripped on confederation boundary without knowledge of all internal ASes. This is something that is underestimated in the RFC 9234.
We could either just disallow roles on intra-confederation EBGP links, or just put big fat warning when configured.
2) Filter support for bgp_otc attribute
I think there should be filter support, although it could be controversial considering RFC says:
Once the OTC Attribute has been set, it MUST be preserved unchanged (this also applies to an AS Confederation).
The operator MUST NOT have the ability to modify the procedures defined in this section.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Tue, Jul 12, 2022 at 01:54:15PM +0300, Eugene Bogomazov wrote:
Hello,
I've reviewed all the changes and am happy with them. Had some misgivings about renaming 'strict mode ' to 'required roles' , but since this option can be easily found by searching for a phrase in the documentation, it looks fine too.
Therefore, I have no additional concerns and agree with the merging of the current version of the patch.
Merged to master. The name 'strict mode' makes sense in the context of RFC 9234, but it is fairly nondescriptive in the general context of whole BGP protocol, that is why i changed that. I thought about several alternatives like 'strict role mode' or 'neighbor role strict', but after some discussion with others i settled on 'require roles'.
Two words about the mentioned problems. 1) In the RFC, we wanted to address a situation where roles could be used in a complex confederation setup without leaking information outside the confederation. However, since this action is NOT RECOMMENDED, the ISP should apply route filtering or attribute removal at the edge of an AS Confederation with a member ASN as an OTC value at its own risk. So, warning seems to be the most appropriate solution for this case.
Added warning. In general, i think that the proper way to support OTC values inside confederations would require to have 8-byte OTC attribute in confederations with one slot for stored external ASN (e.g. ASN as received on confederation boundary) and one slot for active internal ASN.
2) Including the bgp_otc attribute in the route map can be a great option for complex bgp relationships between two ISPs. And they can be used in some weird scenarios (like the confederation scenario mentioned above, for example). They were not included in the core patch only because they are not part of the core functionality of the roles that we want to provide as soon as possible.
Added filter support for bgp_otc. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
participants (4)
-
Eugene Bogomazov -
Job Snijders -
Maria Matejka -
Ondrej Zajicek