From 57aa077227d1f2440dc1b2bb6cbbebd418a6b898 Mon Sep 17 00:00:00 2001
From: Ondrej Zajicek <santiago@crfreenet.org>
Date: Thu, 5 Oct 2023 21:29:04 +0200
Subject: [PATCH] BGP: Improve custom BGP attributes

 - Implement EA_GET for custom BGP attributes
 - Forbid EA_SET on existing opaque attributes
 - Forbid redefining existing attributes
 - Document possible compatibility problems
---
 doc/bird.sgml      | 12 ++++++++----
 filter/f-inst.c    | 11 ++++++++++-
 proto/bgp/attrs.c  |  6 ++++++
 proto/bgp/bgp.h    |  1 +
 proto/bgp/config.Y | 11 +++++++----
 5 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/doc/bird.sgml b/doc/bird.sgml
index a023473f..5e795450 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -3561,10 +3561,14 @@ some of them (marked with `<tt/O/') are optional.
 	name="local role"> is configured it set automatically.
 </descrip>
 
-<p>For attributes unknown by BIRD, the user can assign a name (on top level)
-to an attribute by its number. This defined name can be used then to both set
-(by a bytestring literal, transitive) or unset the given attribute even though
-BIRD knows nothing about it:
+<p>For attributes unknown by BIRD, the user can assign a name (on top level) to
+an attribute by its number. This defined name can be used then to get, set (as a
+bytestring, transitive) or unset the given attribute even though BIRD knows
+nothing about it.
+
+<p>Note that it is not possible to define an attribute with the same number
+as one known by BIRD, therefore use of this statement carries a risk of
+incompatibility with future BIRD versions.
 
 <tt><label id="bgp-attribute-custom">attribute bgp <m/number/ bytestring <m/name/;</tt>
 
diff --git a/filter/f-inst.c b/filter/f-inst.c
index 4356a735..9cc46aa0 100644
--- a/filter/f-inst.c
+++ b/filter/f-inst.c
@@ -838,7 +838,10 @@
 	RESULT_(T_QUAD, i, e->u.data);
 	break;
       case EAF_TYPE_OPAQUE:
-	RESULT_(T_ENUM_EMPTY, i, 0);
+	if (da.f_type == T_ENUM_EMPTY)
+	  RESULT_(T_ENUM_EMPTY, i, 0);
+	else
+	  RESULT_(T_BYTESTRING, ad, e->u.ptr);
 	break;
       case EAF_TYPE_IP_ADDRESS:
 	RESULT_(T_IP, ip, *((ip_addr *) e->u.ptr->data));
@@ -870,6 +873,12 @@
     ARG_ANY(1);
     DYNAMIC_ATTR;
     ARG_TYPE(1, da.f_type);
+
+    FID_NEW_BODY;
+      if (da.f_type == T_ENUM_EMPTY)
+	cf_error("Setting opaque attribute is not allowed");
+
+    FID_INTERPRET_BODY;
     {
       struct ea_list *l = lp_alloc(fs->pool, sizeof(struct ea_list) + sizeof(eattr));
 
diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
index 9387ddba..4346cd5d 100644
--- a/proto/bgp/attrs.c
+++ b/proto/bgp/attrs.c
@@ -1152,6 +1152,12 @@ bgp_attr_known(uint code)
   return (code < ARRAY_SIZE(bgp_attr_table)) && bgp_attr_table[code].name;
 }
 
+const char *
+bgp_attr_name(uint code)
+{
+  return (code < ARRAY_SIZE(bgp_attr_table)) ? bgp_attr_table[code].name : NULL;
+}
+
 void bgp_fix_attr_flags(ea_list *attrs)
 {
   for (u8 i = 0; i < attrs->count; i++)
diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h
index 1ba3de5e..2e95037c 100644
--- a/proto/bgp/bgp.h
+++ b/proto/bgp/bgp.h
@@ -611,6 +611,7 @@ bgp_unset_attr(ea_list **to, struct linpool *pool, uint code)
 
 int bgp_encode_mp_reach_mrt(struct bgp_write_state *s, eattr *a, byte *buf, uint size);
 
+const char * bgp_attr_name(uint code);
 int bgp_encode_attrs(struct bgp_write_state *s, ea_list *attrs, byte *buf, byte *end);
 ea_list * bgp_decode_attrs(struct bgp_parse_state *s, byte *data, uint len);
 void bgp_finish_attrs(struct bgp_parse_state *s, rta *a);
diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y
index 4e7d0329..6981a117 100644
--- a/proto/bgp/config.Y
+++ b/proto/bgp/config.Y
@@ -365,11 +365,14 @@ dynamic_attr: BGP_LARGE_COMMUNITY
 dynamic_attr: BGP_OTC
 	{ $$ = f_new_dynamic_attr(EAF_TYPE_INT, T_INT, EA_CODE(PROTOCOL_BGP, BA_ONLY_TO_CUSTOMER)); } ;
 
-custom_attr: ATTRIBUTE BGP NUM type symbol ';' {
-  if($3 > 255 || $3 < 1)
-    cf_error("Invalid attribute number. (Given %i, must be 1-255.)", $3);
-  if($4 != T_BYTESTRING)
+custom_attr: ATTRIBUTE BGP expr type symbol ';' {
+  if ($3 > 255 || $3 < 1)
+    cf_error("Invalid attribute number (Given %i, must be 1-255)", $3);
+  if ($4 != T_BYTESTRING)
     cf_error("Attribute type must be bytestring, not %s", f_type_name($4));
+  if (bgp_attr_name($3))
+    cf_error("Attribute BGP.%d already known as %s", $3, bgp_attr_name($3));
+
   struct f_dynamic_attr *a = cfg_alloc(sizeof(struct f_dynamic_attr));
   *a = f_new_dynamic_attr(f_type_attr($4), T_BYTESTRING, EA_CODE(PROTOCOL_BGP, $3));
   a->flags = BAF_TRANSITIVE | BAF_OPTIONAL;