[PATCH] bgp: Implement RFC 9234

[Eugene Bogomazov]

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 at 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/c73b5d2d3d94204d2a81d93efd02c4c115859353
>
> 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 at 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."
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20220712/84b5cb8c/attachment.htm>