[PATCH] BGP: Add support for BGP hostname capability
This is an implementation of draft-walton-bgp-hostname-capability-02. It's implemented since quite some time for FRR and in datacenter, this gives a nice output to avoid using IP addresses. Currently, it is always enabled. The hostname is retrieved from uname(2) and not configurable. The domain name is never set nor displayed. I don't think being able to set the hostname is very important. I don't think the domain name is useful. However, maybe the capability should not be enabled by default. Also, the case where the capability is supported but hostname is empty is handled as if the capability was not supported. I think this is not worth handling such an edge case. The RFC doesn't say if hostname is optional or not. The return code of uname(2) is not checked, but I think this is not bound to happen and if it happens, this is handled nicely by disabling the capability. The draft is expired but I hope a second implementation could allow to revive it. Output from BIRD: Local capabilities Multiprotocol AF announced: ipv6 Route refresh Graceful restart 4-octet AS numbers Enhanced refresh Long-lived graceful restart Hostname: bird2 Neighbor capabilities Multiprotocol AF announced: ipv6 Route refresh Graceful restart 4-octet AS numbers ADD-PATH RX: ipv6 TX: Hostname: frr1 Output from FRR: Neighbor V AS MsgRcvd MsgSent TblVer InQ OutQ Up/Down State/PfxRcd PfxSnt bird1(2001:db8::11) 4 65000 548 495 0 0 0 00:03:06 0 0 bird2(2001:db8::12) 4 65000 281 256 0 0 0 00:01:49 0 0 frr2(2001:db8::22) 4 65000 501 501 0 0 0 00:24:56 0 0 Signed-off-by: Vincent Bernat <vincent@bernat.ch> --- proto/bgp/bgp.c | 3 +++ proto/bgp/bgp.h | 2 ++ proto/bgp/config.Y | 1 + proto/bgp/packets.c | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 44 insertions(+) diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c index 302d026c0103..d4c15f0538b1 100644 --- a/proto/bgp/bgp.c +++ b/proto/bgp/bgp.c @@ -2415,6 +2415,9 @@ bgp_show_capabilities(struct bgp_proto *p UNUSED, struct bgp_caps *caps) bgp_show_afis(-1006, " AF supported:", afl1, afn1); bgp_show_afis(-1006, " AF preserved:", afl2, afn2); } + + if (*caps->hostname) + cli_msg(-1006, " Hostname: %s", caps->hostname); } static void diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h index dd7dc28fd03d..3a99bc9667b5 100644 --- a/proto/bgp/bgp.h +++ b/proto/bgp/bgp.h @@ -98,6 +98,7 @@ struct bgp_config { int enable_refresh; /* Enable local support for route refresh [RFC 2918] */ int enable_as4; /* Enable local support for 4B AS numbers [RFC 6793] */ int enable_extended_messages; /* Enable local support for extended messages [draft] */ + int enable_hostname; /* Enable local support for hostname [draft] */ u32 rr_cluster_id; /* Route reflector cluster ID, if different from local ID */ int rr_client; /* Whether neighbor is RR client of me */ int rs_client; /* Whether neighbor is RS client of me */ @@ -225,6 +226,7 @@ struct bgp_caps { u16 gr_time; /* Graceful restart time in seconds */ u8 llgr_aware; /* Long-lived GR capability, RFC draft */ + char hostname[UINT8_MAX + 1]; /* Hostname, RFC draft */ u8 any_ext_next_hop; /* Bitwise OR of per-AF ext_next_hop */ u8 any_add_path; /* Bitwise OR of per-AF add_path */ diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y index 18c3560dfc9b..f8bd4ef03b11 100644 --- a/proto/bgp/config.Y +++ b/proto/bgp/config.Y @@ -62,6 +62,7 @@ bgp_proto_start: proto_start BGP { BGP_CFG->error_delay_time_max = 300; BGP_CFG->enable_refresh = 1; BGP_CFG->enable_as4 = 1; + BGP_CFG->enable_hostname = 1; BGP_CFG->capabilities = 2; BGP_CFG->interpret_communities = 1; BGP_CFG->allow_as_sets = 1; diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index 78fdd1e006a3..71ab1be48b46 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -11,6 +11,7 @@ #undef LOCAL_DEBUG #include <stdlib.h> +#include <sys/utsname.h> #include "nest/bird.h" #include "nest/iface.h" @@ -221,6 +222,8 @@ bgp_prepare_capabilities(struct bgp_conn *conn) struct bgp_channel *c; struct bgp_caps *caps; struct bgp_af_caps *ac; + struct utsname uts = {}; + size_t length; if (!p->cf->capabilities) { @@ -252,6 +255,16 @@ bgp_prepare_capabilities(struct bgp_conn *conn) if (p->cf->llgr_mode) caps->llgr_aware = 1; + if (p->cf->enable_hostname) + { + uname(&uts); + if ((length = strlen(uts.nodename)) > UINT8_MAX) + length = UINT8_MAX; + memcpy(caps->hostname, uts.nodename, length); + caps->hostname[length] = 0; + } + + /* Allocate and fill per-AF fields */ WALK_LIST(c, p->p.channels) { @@ -295,6 +308,7 @@ bgp_write_capabilities(struct bgp_conn *conn, byte *buf) struct bgp_af_caps *ac; byte *buf_head = buf; byte *data; + size_t length; /* Create capability list in buffer */ @@ -412,6 +426,24 @@ bgp_write_capabilities(struct bgp_conn *conn, byte *buf) data[-1] = buf - data; } + if (*caps->hostname) + { + *buf++ = 73; /* Capability 73: Hostname */ + *buf++ = 0; /* Capability data length */ + data = buf; + + /* Hostname */ + length = strlen(caps->hostname); + *buf++ = length; + memcpy(buf, caps->hostname, length); + buf += length; + + /* Domain, not implemented */ + *buf++ = 0; + + data[-1] = buf - data; + } + caps->length = buf - buf_head; return buf; @@ -573,6 +605,12 @@ bgp_read_capabilities(struct bgp_conn *conn, byte *pos, int len) } break; + case 73: /* Hostname, RFC draft */ + if (cl < 2 || cl < 2 + pos[2]) + goto err; + + memcpy(caps->hostname, pos + 3, pos[2]); + /* We can safely ignore all other capabilities */ } -- 2.30.0
On Wed, Feb 03, 2021 at 07:19:50PM +0100, Vincent Bernat wrote:
This is an implementation of draft-walton-bgp-hostname-capability-02. It's implemented since quite some time for FRR and in datacenter, this gives a nice output to avoid using IP addresses.
Currently, it is always enabled. The hostname is retrieved from uname(2) and not configurable. The domain name is never set nor displayed. I don't think being able to set the hostname is very important. I don't think the domain name is useful. However, maybe the capability should not be enabled by default.
Hi Thanks for the patch. I like this feature, it is simple, i would merge it. I have several comments: 1) Hostname has similar role like router id. It seems to me that there should be global hostname config property, like config->router_id, initialized like router_id in global_commit(). 2) Consequently, there should be global option to set it, and it should report in 'show status'. 3) Function calling uname() should be probably somewhere in sysdep/unix/ code 4) The hostname field in bgp_caps should be ptr, allocated based on length, instead of preallocatted full-length buffer. 5) As Job Snijders wrote, the capability should be disabled by default. 6) Received hostname should be treated for unsafe characters like text from RFC 8203 (shutdown communication), see bgp_handle_message().
Also, the case where the capability is supported but hostname is empty is handled as if the capability was not supported. I think this is not worth handling such an edge case. The RFC doesn't say if hostname is optional or not.
-- 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."
This is an implementation of draft-walton-bgp-hostname-capability-02. It's implemented since quite some time for FRR and in datacenter, this gives a nice output to avoid using IP addresses. It is disabled by default. The hostname is retrieved from uname(2) and can be overriden with "hostname". The domain name is never set nor displayed. I don't think the domain name is useful. The return code of uname(2) is not checked, but I think this is not bound to happen and if it happens, this is handled nicely by disabling the capability. The draft is expired but I hope a second implementation could allow to revive it. Output from BIRD: Local capabilities Multiprotocol AF announced: ipv6 Route refresh Graceful restart 4-octet AS numbers Enhanced refresh Long-lived graceful restart Hostname: bird2 Neighbor capabilities Multiprotocol AF announced: ipv6 Route refresh Graceful restart 4-octet AS numbers ADD-PATH RX: ipv6 TX: Hostname: frr1 Output from FRR: Neighbor V AS MsgRcvd MsgSent TblVer InQ OutQ Up/Down State/PfxRcd PfxSnt bird1(2001:db8::11) 4 65000 548 495 0 0 0 00:03:06 0 0 bird2(2001:db8::12) 4 65000 281 256 0 0 0 00:01:49 0 0 frr2(2001:db8::22) 4 65000 501 501 0 0 0 00:24:56 0 0 Signed-off-by: Vincent Bernat <vincent@bernat.ch> --- conf/conf.c | 14 ++++++++++++++ conf/conf.h | 1 + doc/bird.sgml | 6 ++++++ nest/cmds.c | 1 + nest/config.Y | 6 +++++- proto/bgp/bgp.c | 3 +++ proto/bgp/bgp.h | 2 ++ proto/bgp/config.Y | 2 ++ proto/bgp/packets.c | 36 ++++++++++++++++++++++++++++++++++++ 9 files changed, 70 insertions(+), 1 deletion(-) diff --git a/conf/conf.c b/conf/conf.c index 6f64b5416c59..1b699c97361e 100644 --- a/conf/conf.c +++ b/conf/conf.c @@ -42,6 +42,7 @@ #include <setjmp.h> #include <stdarg.h> +#include <sys/utsname.h> #undef LOCAL_DEBUG @@ -217,6 +218,19 @@ config_del_obstacle(struct config *c) static int global_commit(struct config *new, struct config *old) { + if (!new->hostname) + { + struct utsname uts = {}; + if (uname(&uts) == -1) + log(L_WARN "Cannot determine hostname"); + else + { + char *hostname; + hostname = lp_strdup(new->mem, uts.nodename); + new->hostname = hostname; + } + } + if (!old) return 0; diff --git a/conf/conf.h b/conf/conf.h index 3e47c9185469..860d267aa7b9 100644 --- a/conf/conf.h +++ b/conf/conf.h @@ -40,6 +40,7 @@ struct config { struct timeformat tf_log; /* Time format for the logfile */ struct timeformat tf_base; /* Time format for other purposes */ u32 gr_wait; /* Graceful restart wait timeout (sec) */ + const char *hostname; /* Hostname */ int cli_debug; /* Tracing of CLI connections and commands */ int latency_debug; /* I/O loop tracks duration of each event */ diff --git a/doc/bird.sgml b/doc/bird.sgml index 28b0e400b464..592a76a294ea 100644 --- a/doc/bird.sgml +++ b/doc/bird.sgml @@ -585,6 +585,9 @@ include "tablename.conf";; See <ref id="proto-iface" name="interface"> section for detailed description of interface patterns with extended clauses. + <tag><label id="opt-hostname">hostname "<m/name/</tag> + Set hostname. Default: node name as returned by 'uname -n'. + <tag><label id="opt-graceful-restart">graceful restart wait <m/number/</tag> During graceful restart recovery, BIRD waits for convergence of routing protocols. This option allows to specify a timeout for the recovery to @@ -2536,6 +2539,9 @@ using the following configuration parameters: This option is relevant to IPv4 mode with enabled capability advertisement only. Default: on. + <tag><label id="bgp-advertise-hostname">advertise hostname <m/switch/</tag> + Advertise hostname capability along with the hostname. Default: off. + <tag><label id="bgp-disable-after-error">disable after error <m/switch/</tag> When an error is encountered (either locally or by the other side), disable the instance automatically and wait for an administrator to fix diff --git a/nest/cmds.c b/nest/cmds.c index da4015cfba0d..18f39eb56c10 100644 --- a/nest/cmds.c +++ b/nest/cmds.c @@ -27,6 +27,7 @@ cmd_show_status(void) cli_msg(-1000, "BIRD " BIRD_VERSION); tm_format_time(tim, &config->tf_base, current_time()); cli_msg(-1011, "Router ID is %R", config->router_id); + cli_msg(-1011, "Hostname is %s", config->hostname); cli_msg(-1011, "Current server time is %s", tim); tm_format_time(tim, &config->tf_base, boot_time); cli_msg(-1011, "Last reboot on %s", tim); diff --git a/nest/config.Y b/nest/config.Y index 0bb8ca51dba0..39bf61490001 100644 --- a/nest/config.Y +++ b/nest/config.Y @@ -87,7 +87,7 @@ proto_postconfig(void) CF_DECLS -CF_KEYWORDS(ROUTER, ID, PROTOCOL, TEMPLATE, PREFERENCE, DISABLED, DEBUG, ALL, OFF, DIRECT) +CF_KEYWORDS(ROUTER, ID, HOSTNAME, PROTOCOL, TEMPLATE, PREFERENCE, DISABLED, DEBUG, ALL, OFF, DIRECT) CF_KEYWORDS(INTERFACE, IMPORT, EXPORT, FILTER, NONE, VRF, DEFAULT, TABLE, STATES, ROUTES, FILTERS) CF_KEYWORDS(IPV4, IPV6, VPN4, VPN6, ROA4, ROA6, FLOW4, FLOW6, SADR, MPLS) CF_KEYWORDS(RECEIVE, LIMIT, ACTION, WARN, BLOCK, RESTART, DISABLE, KEEP, FILTERED) @@ -151,6 +151,10 @@ idval: } ; +conf: hostname_override ; + +hostname_override: HOSTNAME text ';' { new_config->hostname = $2; } ; + conf: gr_opts ; gr_opts: GRACEFUL RESTART WAIT expr ';' { new_config->gr_wait = $4; } ; diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c index 302d026c0103..a23871f42b9d 100644 --- a/proto/bgp/bgp.c +++ b/proto/bgp/bgp.c @@ -2415,6 +2415,9 @@ bgp_show_capabilities(struct bgp_proto *p UNUSED, struct bgp_caps *caps) bgp_show_afis(-1006, " AF supported:", afl1, afn1); bgp_show_afis(-1006, " AF preserved:", afl2, afn2); } + + if (caps->hostname) + cli_msg(-1006, " Hostname: %s", caps->hostname); } static void diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h index dd7dc28fd03d..d6cbaa5b53b8 100644 --- a/proto/bgp/bgp.h +++ b/proto/bgp/bgp.h @@ -98,6 +98,7 @@ struct bgp_config { int enable_refresh; /* Enable local support for route refresh [RFC 2918] */ int enable_as4; /* Enable local support for 4B AS numbers [RFC 6793] */ int enable_extended_messages; /* Enable local support for extended messages [draft] */ + int enable_hostname; /* Enable local support for hostname [draft] */ u32 rr_cluster_id; /* Route reflector cluster ID, if different from local ID */ int rr_client; /* Whether neighbor is RR client of me */ int rs_client; /* Whether neighbor is RS client of me */ @@ -225,6 +226,7 @@ struct bgp_caps { u16 gr_time; /* Graceful restart time in seconds */ u8 llgr_aware; /* Long-lived GR capability, RFC draft */ + char *hostname; /* Hostname, RFC draft */ u8 any_ext_next_hop; /* Bitwise OR of per-AF ext_next_hop */ u8 any_add_path; /* Bitwise OR of per-AF add_path */ diff --git a/proto/bgp/config.Y b/proto/bgp/config.Y index 18c3560dfc9b..2dfbdca9bc92 100644 --- a/proto/bgp/config.Y +++ b/proto/bgp/config.Y @@ -62,6 +62,7 @@ bgp_proto_start: proto_start BGP { BGP_CFG->error_delay_time_max = 300; BGP_CFG->enable_refresh = 1; BGP_CFG->enable_as4 = 1; + BGP_CFG->enable_hostname = 0; BGP_CFG->capabilities = 2; BGP_CFG->interpret_communities = 1; BGP_CFG->allow_as_sets = 1; @@ -173,6 +174,7 @@ bgp_proto: | bgp_proto ENABLE ROUTE REFRESH bool ';' { BGP_CFG->enable_refresh = $5; } | bgp_proto ENABLE AS4 bool ';' { BGP_CFG->enable_as4 = $4; } | bgp_proto ENABLE EXTENDED MESSAGES bool ';' { BGP_CFG->enable_extended_messages = $5; } + | bgp_proto ADVERTISE HOSTNAME bool ';' { BGP_CFG->enable_hostname = $4; } | bgp_proto CAPABILITIES bool ';' { BGP_CFG->capabilities = $3; } | bgp_proto PASSWORD text ';' { BGP_CFG->password = $3; } | bgp_proto SETKEY bool ';' { BGP_CFG->setkey = $3; } diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index 78fdd1e006a3..0fb24c617e02 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -252,6 +252,13 @@ bgp_prepare_capabilities(struct bgp_conn *conn) if (p->cf->llgr_mode) caps->llgr_aware = 1; + if (p->cf->enable_hostname) + { + size_t length = strlen(p->cf->c.global->hostname); + caps->hostname = mb_allocz(p->p.pool, length+1); + memcpy(caps->hostname, p->cf->c.global->hostname, length); + } + /* Allocate and fill per-AF fields */ WALK_LIST(c, p->p.channels) { @@ -295,6 +302,7 @@ bgp_write_capabilities(struct bgp_conn *conn, byte *buf) struct bgp_af_caps *ac; byte *buf_head = buf; byte *data; + size_t length; /* Create capability list in buffer */ @@ -412,6 +420,24 @@ bgp_write_capabilities(struct bgp_conn *conn, byte *buf) data[-1] = buf - data; } + if (caps->hostname) + { + *buf++ = 73; /* Capability 73: Hostname */ + *buf++ = 0; /* Capability data length */ + data = buf; + + /* Hostname */ + length = strlen(caps->hostname); + *buf++ = length; + memcpy(buf, caps->hostname, length); + buf += length; + + /* Domain, not implemented */ + *buf++ = 0; + + data[-1] = buf - data; + } + caps->length = buf - buf_head; return buf; @@ -573,6 +599,16 @@ bgp_read_capabilities(struct bgp_conn *conn, byte *pos, int len) } break; + case 73: /* Hostname, RFC draft */ + if (cl < 2 || cl < 2 + pos[2]) + goto err; + + caps->hostname = mb_allocz(p->p.pool, pos[2]+1); + memcpy(caps->hostname, pos + 3, pos[2]); + for (i = 0; i < pos[2]; i++) + if (caps->hostname[i] < ' ') + caps->hostname[i] = ' '; + /* We can safely ignore all other capabilities */ } -- 2.30.0
On Fri, Feb 05, 2021 at 03:12:01PM +0100, Vincent Bernat wrote:
This is an implementation of draft-walton-bgp-hostname-capability-02. It's implemented since quite some time for FRR and in datacenter, this gives a nice output to avoid using IP addresses.
Thanks, merged. -- 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."
❦ 3 février 2021 23:37 +01, Ondrej Zajicek:
1) Hostname has similar role like router id. It seems to me that there should be global hostname config property, like config->router_id, initialized like router_id in global_commit().
Done. Use `hostname "bla"`
2) Consequently, there should be global option to set it, and it should report in 'show status'.
Done.
3) Function calling uname() should be probably somewhere in sysdep/unix/ code
Not done. I don't know exactly where I would put it and it would be a trivial wrapper.
4) The hostname field in bgp_caps should be ptr, allocated based on length, instead of preallocatted full-length buffer.
Done. I think I have used the pools correctly.
5) As Job Snijders wrote, the capability should be disabled by default.
Done. Can be enabled with `advertise hostname yes`.
6) Received hostname should be treated for unsafe characters like text from RFC 8203 (shutdown communication), see bgp_handle_message().
Done. Also, reload works, but as I didn't write anything for that, it's a bit a mystery how BIRD knows there is a change.
participants (3)
-
Ondrej Zajicek -
Vincent Bernat -
Vincent Bernat