Re: openbgpd: support for bgp administrative shutdown communication

[Job Snijders]

Dear all,

The below is based on feedback from Sebastian Benoit, Theo de Raadt,
and Peter Hessler. The patch adds less lines of code, and adheres
better to style(9). Thank you for your time.

Kind regards,

Job


Index: bgpctl/bgpctl.8
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.8,v
retrieving revision 1.71
diff -u -p -r1.71 bgpctl.8
--- bgpctl/bgpctl.8 26 Oct 2016 17:24:13 -  1.71
+++ bgpctl/bgpctl.8 9 Jan 2017 21:52:31 -
@@ -14,7 +14,7 @@
 .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 .\"
-.Dd $Mdocdate: October 26 2016 $
+.Dd $Mdocdate: January 9 2017 $
 .Dt BGPCTL 8
 .Os
 .Sh NAME
@@ -104,8 +104,14 @@ Destroy a previously cloned peer.
 The peer must be down before calling this function.
 .Ar peer
 may be the neighbor's address or description.
-.It Cm neighbor Ar peer Cm down
-Take the BGP session to the specified neighbor down.
+.It Cm neighbor Ar peer Cm down Op Ar reason
+Take the BGP session to the specified neighbor down. If a
+.Ar reason
+is provided, the
+.Ar reason
+is sent as Administrative Shutdown Communication to the neighbor. The
+.Ar reason
+cannot exceed 128 octets.
 .Ar peer
 may be the neighbor's address or description.
 .It Cm neighbor Ar peer Cm refresh
Index: bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.190
diff -u -p -r1.190 bgpctl.c
--- bgpctl/bgpctl.c 14 Oct 2016 16:05:35 -  1.190
+++ bgpctl/bgpctl.c 9 Jan 2017 21:52:31 -
@@ -162,6 +162,7 @@ main(int argc, char *argv[])
 
memcpy(, >peeraddr, sizeof(neighbor.addr));
strlcpy(neighbor.descr, res->peerdesc, sizeof(neighbor.descr));
+   strlcpy(neighbor.shutcomm, res->shutcomm, sizeof(neighbor.shutcomm));
 
if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
err(1, "control_init: socket");
@@ -722,6 +723,13 @@ show_neighbor_msg(struct imsg *imsg, enu
inet_ntoa(ina));
printf("%s\n", print_auth_method(p->auth.method));
printf("  BGP state = %s", statenames[p->state]);
+   if (p->conf.down) {
+   printf(", marked down");
+   if (*(p->conf.shutcomm)) {
+   printf(" with shutdown reason \"%s\"",
+   log_shutcomm(p->conf.shutcomm));
+   }
+   }
if (p->stats.last_updown != 0)
printf(", %s for %s",
p->state == STATE_ESTABLISHED ? "up" : "down",
@@ -756,6 +764,10 @@ show_neighbor_msg(struct imsg *imsg, enu
break;
print_neighbor_msgstats(p);
printf("\n");
+   if (*(p->stats.last_shutcomm)) {
+   printf("  Last received shutdown reason: \"%s\"\n",
+   log_shutcomm(p->stats.last_shutcomm));
+   }
if (p->state == STATE_IDLE) {
static const char   *errstr;
 
Index: bgpctl/parser.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
retrieving revision 1.74
diff -u -p -r1.74 parser.c
--- bgpctl/parser.c 14 Oct 2016 16:05:35 -  1.74
+++ bgpctl/parser.c 9 Jan 2017 21:52:31 -
@@ -45,6 +45,7 @@ enum token_type {
PREFIX,
PEERDESC,
RIBNAME,
+   SHUTDOWN_COMMUNICATION,
COMMUNITY,
LARGE_COMMUNITY,
LOCALPREF,
@@ -245,9 +246,15 @@ static const struct token t_neighbor[] =
{ ENDTOKEN, "", NONE,   NULL}
 };
 
+static const struct token t_nei_mod_shutc[] = {
+   { NOTOKEN,  "", NONE,   NULL},
+   { SHUTDOWN_COMMUNICATION,   "", NONE,   NULL},
+   { ENDTOKEN, "", NONE,   NULL}
+};
+
 static const struct token t_neighbor_modifiers[] = {
{ KEYWORD,  "up",   NEIGHBOR_UP,NULL},
-   { KEYWORD,  "down", NEIGHBOR_DOWN,  NULL},
+   { KEYWORD,  "down", NEIGHBOR_DOWN,  
t_nei_mod_shutc},
{ KEYWORD,  "clear",NEIGHBOR_CLEAR, NULL},
{ KEYWORD,  "refresh",  NEIGHBOR_RREFRESH,  NULL},
{ KEYWORD,  "destroy",  NEIGHBOR_DESTROY,   NULL},
@@ -571,6 +578,16 @@ match_token(int *argc, char **argv[], co
t = [i];
}
break;
+   case SHUTDOWN_COMMUNICATION:
+   if (!match && word != NULL && wordlen > 0) {
+   if (strlcpy(res.shutcomm, word,
+   

Re: openbgpd: support for bgp administrative shutdown communication

[Peter van Dijk]

Hello Sebastian,

On 8 Jan 2017, at 22:10, Sebastian Benoit wrote:


Job Snijders(j...@instituut.net) on 2017.01.08 20:24:19 +0100:

Dear OpenBSD developers,

This patch adds support for the "BGP Administrative Shutdown
Communication" to bgpd(8) and bgpctl(8).


Hi Job and Peter,

thanks, this is nice!


Thank you :)


 .Re
 .Pp
 .Rs
+.%A J. Snijders
+.%A J. Heitz
+.%A K. Patel
+.%A I. Bagdonas
+.%A A. Simpson
+.%D January 2017
+.%R draft-ietf-idr-large-community
+.%T Large BGP Communities Attribute


this duplicates this entry

also newer stuff sould go to the bottom of the list i believe.


While there is some overlap in authors, these are different entries. If 
it should go to the bottom then both of them are in the wrong place - 
maybe we should fix this afterwards in a separate commit?



+.%D January 2017
+.%R draft-ietf-idr-shutdown
+.%T BGP Administrative Shutdown Communication
+   p+=shutcomm_len;


please add spaces around +=


Fixed locally + next three.


+   p = communication;
+   for (q = buf; *p && q < [sizeof(buf) - 1]; p++) {
+   if (*p == '\n')
+   *q++ = ' ';
+   else
+   q = vis(q, *p, 0, 0);
+   }
+   *q = '\0';
+
+   return buf;


while i think this is correct, would it not be easier to encode \n as
control char with VIS_NL?

then you could just use strnvis()


Ah! I just stole this code from syslogd.c without checking if we could 
do better. Have changed it to a single strnvis call locally, with 
VIS_NL, and also added VIS_OCTAL while at it because it will be more 
familiar to admins and because the tcpdump patch for shutcomm also uses 
octal.


Thanks for the review, Sebastian. I’ll wait for some more feedback 
before resubmitting.


Kind regards,
--
Peter van Dijk
PowerDNS.COM BV - https://www.powerdns.com/

Re: openbgpd: support for bgp administrative shutdown communication

[Sebastian Benoit]

Job Snijders(j...@instituut.net) on 2017.01.08 20:24:19 +0100:
> Dear OpenBSD developers,
> 
> This patch adds support for the "BGP Administrative Shutdown
> Communication" to bgpd(8) and bgpctl(8).

Hi Job and Peter,

thanks, this is nice!

some comments below.
 
> The draft-ietf-idr-shutdown 
> (https://tools.ietf.org/html/draft-ietf-idr-shutdown)
> document specifies a mechanism to transmit a short freeform message
> across the wire as part of an Administrative Shutdown Cease NOTIFICATION
> message. This message serves to inform the neighbor why the BGP session
> was shut down. This is useful to communicate for instance a ticket
> reference, contact person or other information to the neighbor.
> 
> Example usage:
> 
> [job@kiera ~]$ bgpctl show
> NeighborASMsgRcvd  MsgSent  OutQ Up/Down   State/PrfRcvd
> 165.254.255.24  15562  70684  60 0   00:21:16  33653
> 
> [job@kiera ~]$ bgpctl neighbor 165.254.255.24 down "goodbye, we are 
> upgrading to openbsd 6.1"
> request processed
> 
> [job@kiera ~]$ bgpctl show
> NeighborASMsgRcvd  MsgSent  OutQ Up/Down  State/PrfRcvd
> 165.254.255.24  15562  70708 62  0   00:00:08 Idle
> 
> [job@kiera ~]$ bgpctl show neighbor 165.254.255.24 | grep reason
>   BGP state = Idle, marked down with shutdown reason "goodbye, we are 
> upgrading to openbsd 6.1", down for 00:00:17
> 
> On the neighbor's side you'll see the following in syslog:
> 
> Jan  8 19:28:54 shutdown bgpd[50719]: neighbor 165.254.255.26: received 
> notification: Cease, administratively down
> Jan  8 19:28:54 shutdown bgpd[50719]: neighbor 165.254.255.26: received 
> shutdown reason: "goodbye, we are upgrading to openbsd 6.1"
> 
> and this is also visible through bgpctl(8):
> 
> [job@shutdown~]$ bgpctl show neighbor 165.254.255.26 | grep reason
>   Last received shutdown reason: "goodbye, we are upgrading to openbsd 
> 6.1"
> 
> This work has been tested against pmacct and exabgp which also
> support draft-ietf-idr-shutdown.
> 
> The BGP Administrative Shutdown Communication feature for OpenBGPD was
> developed by Peter van Dijk  and Job Snijders 
> .
> 
> Kind regards,
> 
> Job
> 
> 
> Index: usr.sbin/bgpctl/bgpctl.8
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.8,v
> retrieving revision 1.69
> diff -u -p -r1.69 bgpctl.8
> --- usr.sbin/bgpctl/bgpctl.8  25 May 2016 14:15:59 -  1.69
> +++ usr.sbin/bgpctl/bgpctl.8  8 Jan 2017 17:45:24 -
> @@ -104,8 +104,14 @@ Destroy a previously cloned peer.
>  The peer must be down before calling this function.
>  .Ar peer
>  may be the neighbor's address or description.
> -.It Cm neighbor Ar peer Cm down
> -Take the BGP session to the specified neighbor down.
> +.It Cm neighbor Ar peer Cm down Op Ar reason
> +Take the BGP session to the specified neighbor down. If a
> +.Ar reason
> +is provided, the 
> +.Ar reason
> +is send as Administrative Shutdown Communication to the neighbor. The
> +.Ar reason
> +cannot exceed 128 octets.
>  .Ar peer
>  may be the neighbor's address or description.
>  .It Cm neighbor Ar peer Cm refresh
> Index: usr.sbin/bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.188
> diff -u -p -r1.188 bgpctl.c
> --- usr.sbin/bgpctl/bgpctl.c  3 Jun 2016 17:36:37 -   1.188
> +++ usr.sbin/bgpctl/bgpctl.c  8 Jan 2017 17:45:24 -
> @@ -159,6 +159,7 @@ main(int argc, char *argv[])
>  
>   memcpy(, >peeraddr, sizeof(neighbor.addr));
>   strlcpy(neighbor.descr, res->peerdesc, sizeof(neighbor.descr));
> + strlcpy(neighbor.shutcomm, res->shutcomm, sizeof(neighbor.shutcomm));
>  
>   if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
>   err(1, "control_init: socket");
> @@ -702,6 +703,13 @@ show_neighbor_msg(struct imsg *imsg, enu
>   inet_ntoa(ina));
>   printf("%s\n", print_auth_method(p->auth.method));
>   printf("  BGP state = %s", statenames[p->state]);
> + if (p->conf.down) {
> + printf(", marked down");
> + if (*(p->conf.shutcomm)) {
> + printf(" with shutdown reason \"%s\"",
> + log_shutcomm(p->conf.shutcomm));
> + }
> + }
>   if (p->stats.last_updown != 0)
>   printf(", %s for %s",
>   p->state == STATE_ESTABLISHED ? "up" : "down",
> @@ -736,6 +744,10 @@ show_neighbor_msg(struct imsg *imsg, enu
>   break;
>   print_neighbor_msgstats(p);
>   printf("\n");
> + if (*(p->stats.last_shutcomm)) {
> + printf("  Last received shutdown reason: \"%s\"\n",
> + 

openbgpd: support for bgp administrative shutdown communication

[Job Snijders]

Dear OpenBSD developers,

This patch adds support for the "BGP Administrative Shutdown
Communication" to bgpd(8) and bgpctl(8).

The draft-ietf-idr-shutdown 
(https://tools.ietf.org/html/draft-ietf-idr-shutdown)
document specifies a mechanism to transmit a short freeform message
across the wire as part of an Administrative Shutdown Cease NOTIFICATION
message. This message serves to inform the neighbor why the BGP session
was shut down. This is useful to communicate for instance a ticket
reference, contact person or other information to the neighbor.

Example usage:

[job@kiera ~]$ bgpctl show
NeighborASMsgRcvd  MsgSent  OutQ Up/Down   State/PrfRcvd
165.254.255.24  15562  70684  60 0   00:21:16  33653

[job@kiera ~]$ bgpctl neighbor 165.254.255.24 down "goodbye, we are 
upgrading to openbsd 6.1"
request processed

[job@kiera ~]$ bgpctl show
NeighborASMsgRcvd  MsgSent  OutQ Up/Down  State/PrfRcvd
165.254.255.24  15562  70708 62  0   00:00:08 Idle

[job@kiera ~]$ bgpctl show neighbor 165.254.255.24 | grep reason
  BGP state = Idle, marked down with shutdown reason "goodbye, we are 
upgrading to openbsd 6.1", down for 00:00:17

On the neighbor's side you'll see the following in syslog:

Jan  8 19:28:54 shutdown bgpd[50719]: neighbor 165.254.255.26: received 
notification: Cease, administratively down
Jan  8 19:28:54 shutdown bgpd[50719]: neighbor 165.254.255.26: received 
shutdown reason: "goodbye, we are upgrading to openbsd 6.1"

and this is also visible through bgpctl(8):

[job@shutdown~]$ bgpctl show neighbor 165.254.255.26 | grep reason
  Last received shutdown reason: "goodbye, we are upgrading to openbsd 6.1"

This work has been tested against pmacct and exabgp which also
support draft-ietf-idr-shutdown.

The BGP Administrative Shutdown Communication feature for OpenBGPD was
developed by Peter van Dijk  and Job Snijders 
.

Kind regards,

Job


Index: usr.sbin/bgpctl/bgpctl.8
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.8,v
retrieving revision 1.69
diff -u -p -r1.69 bgpctl.8
--- usr.sbin/bgpctl/bgpctl.825 May 2016 14:15:59 -  1.69
+++ usr.sbin/bgpctl/bgpctl.88 Jan 2017 17:45:24 -
@@ -104,8 +104,14 @@ Destroy a previously cloned peer.
 The peer must be down before calling this function.
 .Ar peer
 may be the neighbor's address or description.
-.It Cm neighbor Ar peer Cm down
-Take the BGP session to the specified neighbor down.
+.It Cm neighbor Ar peer Cm down Op Ar reason
+Take the BGP session to the specified neighbor down. If a
+.Ar reason
+is provided, the 
+.Ar reason
+is send as Administrative Shutdown Communication to the neighbor. The
+.Ar reason
+cannot exceed 128 octets.
 .Ar peer
 may be the neighbor's address or description.
 .It Cm neighbor Ar peer Cm refresh
Index: usr.sbin/bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.188
diff -u -p -r1.188 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c3 Jun 2016 17:36:37 -   1.188
+++ usr.sbin/bgpctl/bgpctl.c8 Jan 2017 17:45:24 -
@@ -159,6 +159,7 @@ main(int argc, char *argv[])
 
memcpy(, >peeraddr, sizeof(neighbor.addr));
strlcpy(neighbor.descr, res->peerdesc, sizeof(neighbor.descr));
+   strlcpy(neighbor.shutcomm, res->shutcomm, sizeof(neighbor.shutcomm));
 
if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
err(1, "control_init: socket");
@@ -702,6 +703,13 @@ show_neighbor_msg(struct imsg *imsg, enu
inet_ntoa(ina));
printf("%s\n", print_auth_method(p->auth.method));
printf("  BGP state = %s", statenames[p->state]);
+   if (p->conf.down) {
+   printf(", marked down");
+   if (*(p->conf.shutcomm)) {
+   printf(" with shutdown reason \"%s\"",
+   log_shutcomm(p->conf.shutcomm));
+   }
+   }
if (p->stats.last_updown != 0)
printf(", %s for %s",
p->state == STATE_ESTABLISHED ? "up" : "down",
@@ -736,6 +744,10 @@ show_neighbor_msg(struct imsg *imsg, enu
break;
print_neighbor_msgstats(p);
printf("\n");
+   if (*(p->stats.last_shutcomm)) {
+   printf("  Last received shutdown reason: \"%s\"\n",
+   log_shutcomm(p->stats.last_shutcomm));
+   }
if (p->state == STATE_IDLE) {
static const char   *errstr;
 
Index: usr.sbin/bgpctl/parser.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
retrieving