Rework parse_notification() to use the ibuf API for everything.

While there fix the RFC5492 handling of ERR_OPEN_CAPA (the current code
has the logic inversed). ERR_OPEN_CAPA is there to signal that a needed
capability is missing in our OPEN message. Just add the handling of
ERR_OPEN_CAPA to log_notification().

Also rework the handling of the shutdown reason and move the printing
into log_notification().

OK tb@

diff --git a/usr.sbin/bgpd/bgpd.h b/usr.sbin/bgpd/bgpd.h
index 6e72746..212b10f 100644 (file)
--- a/usr.sbin/bgpd/bgpd.h
+++ b/usr.sbin/bgpd/bgpd.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: bgpd.h,v 1.487 2024/03/18 10:49:24 claudio Exp $ */
+/*     $OpenBSD: bgpd.h,v 1.488 2024/03/22 07:19:28 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -408,6 +408,17 @@ struct capabilities {
        int8_t  policy;                 /* Open Policy, RFC 9234, 2 = enforce */
 };
 
+enum capa_codes {
+       CAPA_NONE = 0,
+       CAPA_MP = 1,
+       CAPA_REFRESH = 2,
+       CAPA_ROLE = 9,
+       CAPA_RESTART = 64,
+       CAPA_AS4BYTE = 65,
+       CAPA_ADD_PATH = 69,
+       CAPA_ENHANCED_RR = 70,
+};
+
 /* flags for RFC 4724 - graceful restart */
 #define        CAPA_GR_PRESENT         0x01
 #define        CAPA_GR_RESTART         0x02
@@ -1546,6 +1557,7 @@ const char        *log_roa(struct roa *);
 const char     *log_aspa(struct aspa_set *);
 const char     *log_rtr_error(enum rtr_error);
 const char     *log_policy(enum role);
+const char     *log_capability(uint8_t);
 int             aspath_asprint(char **, struct ibuf *);
 uint32_t        aspath_extract(const void *, int);
 int             aspath_verify(struct ibuf *, int, int);

diff --git a/usr.sbin/bgpd/logmsg.c b/usr.sbin/bgpd/logmsg.c
index dfa89f2..359f94e 100644 (file)
--- a/usr.sbin/bgpd/logmsg.c
+++ b/usr.sbin/bgpd/logmsg.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: logmsg.c,v 1.11 2024/01/16 13:15:31 claudio Exp $ */
+/*     $OpenBSD: logmsg.c,v 1.12 2024/03/22 07:19:28 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -133,12 +133,18 @@ log_statechange(struct peer *peer, enum session_state nstate,
 
 void
 log_notification(const struct peer *peer, uint8_t errcode, uint8_t subcode,
-    struct ibuf *data, const char *dir)
+    const struct ibuf *data, const char *dir)
 {
+       struct ibuf      ibuf;
        char            *p;
        const char      *suberrname = NULL;
        int              uk = 0;
 
+       if (data != NULL)
+               ibuf_from_ibuf(&ibuf, data);
+       else
+               ibuf_from_buffer(&ibuf, NULL, 0);
+
        p = log_fmt_peer(&peer->conf);
        switch (errcode) {
        case ERR_HEADER:
@@ -154,6 +160,18 @@ log_notification(const struct peer *peer, uint8_t errcode, uint8_t subcode,
                        uk = 1;
                else
                        suberrname = suberr_open_names[subcode];
+               if (errcode == ERR_OPEN && subcode == ERR_OPEN_CAPA) {
+                       uint8_t capa_code;
+
+                       if (ibuf_get_n8(&ibuf, &capa_code) == -1)
+                               break;
+                       
+                       logit(LOG_ERR, "%s: %s notification: %s, %s: %s",
+                           p, dir, errnames[errcode], suberrname,
+                           log_capability(capa_code));
+                       free(p);
+                       return;
+               }
                break;
        case ERR_UPDATE:
                if (subcode >= sizeof(suberr_update_names) / sizeof(char *) ||
@@ -168,6 +186,18 @@ log_notification(const struct peer *peer, uint8_t errcode, uint8_t subcode,
                        uk = 1;
                else
                        suberrname = suberr_cease_names[subcode];
+
+               if (subcode == ERR_CEASE_ADMIN_DOWN ||
+                   subcode == ERR_CEASE_ADMIN_RESET) {
+                       if (peer->stats.last_reason[0] != '\0') {
+                               logit(LOG_ERR, "%s: %s notification: %s, %s: "
+                                   "reason \"%s\"", p, dir,
+                                   errnames[errcode], suberrname,
+                                   log_reason(peer->stats.last_reason));
+                               free(p);
+                               return;
+                       }
+               }
                break;
        case ERR_HOLDTIMEREXPIRED:
                if (subcode != 0)

diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c
index 3aeec77..0d8ac4e 100644 (file)
--- a/usr.sbin/bgpd/session.c
+++ b/usr.sbin/bgpd/session.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: session.c,v 1.464 2024/03/20 09:35:46 claudio Exp $ */
+/*     $OpenBSD: session.c,v 1.465 2024/03/22 07:19:28 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004, 2005 Henning Brauer <henning@openbsd.org>
@@ -2479,11 +2479,8 @@ parse_notification(struct peer *peer)
        struct ibuf      ibuf;
        u_char          *p;
        uint16_t         datalen;
-       uint8_t          errcode;
-       uint8_t          subcode;
-       uint8_t          capa_code;
-       uint8_t          capa_len;
-       size_t           reason_len;
+       uint8_t          errcode, subcode;
+       uint8_t          reason_len;
 
        /* just log */
        p = peer->rbuf->rptr;
@@ -2495,142 +2492,40 @@ parse_notification(struct peer *peer)
        p += MSGSIZE_HEADER;    /* header is already checked */
        datalen -= MSGSIZE_HEADER;
 
-       memcpy(&errcode, p, sizeof(errcode));
-       p += sizeof(errcode);
-       datalen -= sizeof(errcode);
-
-       memcpy(&subcode, p, sizeof(subcode));
-       p += sizeof(subcode);
-       datalen -= sizeof(subcode);
-
        /* XXX */
        ibuf_from_buffer(&ibuf, p, datalen);
-       log_notification(peer, errcode, subcode, &ibuf, "received");
+       
+       if (ibuf_get_n8(&ibuf, &errcode) == -1 ||
+           ibuf_get_n8(&ibuf, &subcode) == -1) {
+               log_peer_warnx(&peer->conf, "received bad notification");
+               return (-1);
+       }
 
        peer->errcnt++;
        peer->stats.last_rcvd_errcode = errcode;
        peer->stats.last_rcvd_suberr = subcode;
 
-       if (errcode == ERR_OPEN && subcode == ERR_OPEN_CAPA) {
-               if (datalen == 0) {     /* zebra likes to send those.. humbug */
-                       log_peer_warnx(&peer->conf, "received \"unsupported "
-                           "capability\" notification without data part, "
-                           "disabling capability announcements altogether");
-                       session_capa_ann_none(peer);
-               }
-
-               while (datalen > 0) {
-                       if (datalen < 2) {
-                               log_peer_warnx(&peer->conf,
-                                   "parse_notification: "
-                                   "expect len >= 2, len is %u", datalen);
-                               return (-1);
-                       }
-                       memcpy(&capa_code, p, sizeof(capa_code));
-                       p += sizeof(capa_code);
-                       datalen -= sizeof(capa_code);
-                       memcpy(&capa_len, p, sizeof(capa_len));
-                       p += sizeof(capa_len);
-                       datalen -= sizeof(capa_len);
-                       if (datalen < capa_len) {
-                               log_peer_warnx(&peer->conf,
-                                   "parse_notification: capa_len %u exceeds "
-                                   "remaining msg length %u", capa_len,
-                                   datalen);
-                               return (-1);
-                       }
-                       p += capa_len;
-                       datalen -= capa_len;
-                       switch (capa_code) {
-                       case CAPA_MP:
-                               memset(peer->capa.ann.mp, 0,
-                                   sizeof(peer->capa.ann.mp));
-                               log_peer_warnx(&peer->conf,
-                                   "disabling multiprotocol capability");
-                               break;
-                       case CAPA_REFRESH:
-                               peer->capa.ann.refresh = 0;
-                               log_peer_warnx(&peer->conf,
-                                   "disabling route refresh capability");
-                               break;
-                       case CAPA_ROLE:
-                               if (peer->capa.ann.policy == 1) {
-                                       peer->capa.ann.policy = 0;
-                                       log_peer_warnx(&peer->conf,
-                                           "disabling role capability");
-                               } else {
-                                       log_peer_warnx(&peer->conf,
-                                           "role capability enforced, "
-                                           "not disabling");
-                               }
-                               break;
-                       case CAPA_RESTART:
-                               peer->capa.ann.grestart.restart = 0;
-                               log_peer_warnx(&peer->conf,
-                                   "disabling restart capability");
-                               break;
-                       case CAPA_AS4BYTE:
-                               peer->capa.ann.as4byte = 0;
-                               log_peer_warnx(&peer->conf,
-                                   "disabling 4-byte AS num capability");
-                               break;
-                       case CAPA_ADD_PATH:
-                               memset(peer->capa.ann.add_path, 0,
-                                   sizeof(peer->capa.ann.add_path));
-                               log_peer_warnx(&peer->conf,
-                                   "disabling ADD-PATH capability");
-                               break;
-                       case CAPA_ENHANCED_RR:
-                               peer->capa.ann.enhanced_rr = 0;
+       CTASSERT(sizeof(peer->stats.last_reason) > UINT8_MAX);
+       memset(peer->stats.last_reason, 0, sizeof(peer->stats.last_reason));
+       if (errcode == ERR_CEASE &&
+           (subcode == ERR_CEASE_ADMIN_DOWN ||
+            subcode == ERR_CEASE_ADMIN_RESET)) {
+               /* check if shutdown reason is included */
+               if (ibuf_get_n8(&ibuf, &reason_len) != -1 && reason_len != 0) {
+                       if (ibuf_get(&ibuf, peer->stats.last_reason,
+                           reason_len) == -1)
                                log_peer_warnx(&peer->conf,
-                                   "disabling enhanced route refresh "
-                                   "capability");
-                               break;
-                       default:        /* should not happen... */
-                               log_peer_warnx(&peer->conf, "received "
-                                   "\"unsupported capability\" notification "
-                                   "for unknown capability %u, disabling "
-                                   "capability announcements altogether",
-                                   capa_code);
-                               session_capa_ann_none(peer);
-                               break;
-                       }
+                                   "received truncated shutdown reason");
                }
-
-               return (1);
        }
 
+       log_notification(peer, errcode, subcode, &ibuf, "received");
+
        if (errcode == ERR_OPEN && subcode == ERR_OPEN_OPT) {
                session_capa_ann_none(peer);
                return (1);
        }
 
-       if (errcode == ERR_CEASE &&
-           (subcode == ERR_CEASE_ADMIN_DOWN ||
-            subcode == ERR_CEASE_ADMIN_RESET)) {
-               if (datalen > 1) {
-                       reason_len = *p++;
-                       datalen--;
-                       if (datalen < reason_len) {
-                               log_peer_warnx(&peer->conf,
-                                   "received truncated shutdown reason");
-                               return (0);
-                       }
-                       if (reason_len > REASON_LEN - 1) {
-                               log_peer_warnx(&peer->conf,
-                                   "received overly long shutdown reason");
-                               return (0);
-                       }
-                       memcpy(peer->stats.last_reason, p, reason_len);
-                       peer->stats.last_reason[reason_len] = '\0';
-                       log_peer_warnx(&peer->conf,
-                           "received shutdown reason: \"%s\"",
-                           log_reason(peer->stats.last_reason));
-                       p += reason_len;
-                       datalen -= reason_len;
-               }
-       }
-
        return (0);
 }
 

diff --git a/usr.sbin/bgpd/session.h b/usr.sbin/bgpd/session.h
index 13cbd92..1f459b7 100644 (file)
--- a/usr.sbin/bgpd/session.h
+++ b/usr.sbin/bgpd/session.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: session.h,v 1.167 2024/01/16 13:15:31 claudio Exp $ */
+/*     $OpenBSD: session.h,v 1.168 2024/03/22 07:19:28 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -106,17 +106,6 @@ enum opt_params {
        OPT_PARAM_EXT_LEN=255,
 };
 
-enum capa_codes {
-       CAPA_NONE = 0,
-       CAPA_MP = 1,
-       CAPA_REFRESH = 2,
-       CAPA_ROLE = 9,
-       CAPA_RESTART = 64,
-       CAPA_AS4BYTE = 65,
-       CAPA_ADD_PATH = 69,
-       CAPA_ENHANCED_RR = 70,
-};
-
 struct bgp_msg {
        struct ibuf     *buf;
        enum msg_type    type;
@@ -273,7 +262,7 @@ char        *log_fmt_peer(const struct peer_config *);
 void    log_statechange(struct peer *, enum session_state,
            enum session_events);
 void    log_notification(const struct peer *, uint8_t, uint8_t,
-           struct ibuf *, const char *);
+           const struct ibuf *, const char *);
 void    log_conn_attempt(const struct peer *, struct sockaddr *,
            socklen_t);
 

diff --git a/usr.sbin/bgpd/util.c b/usr.sbin/bgpd/util.c
index db4f5e7..cdbcc57 100644 (file)
--- a/usr.sbin/bgpd/util.c
+++ b/usr.sbin/bgpd/util.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: util.c,v 1.83 2024/03/20 09:35:46 claudio Exp $ */
+/*     $OpenBSD: util.c,v 1.84 2024/03/22 07:19:28 claudio Exp $ */
 
 /*
  * Copyright (c) 2006 Claudio Jeker <claudio@openbsd.org>
@@ -309,6 +309,32 @@ log_policy(enum role role)
        }
 }
 
+const char *
+log_capability(uint8_t capa)
+{
+       static char buf[20];
+
+       switch (capa) {
+       case CAPA_MP:
+               return "Multiprotocol Extensions";
+       case CAPA_REFRESH:
+               return "Route Refresh";
+       case CAPA_ROLE:
+               return "BGP Role";
+       case CAPA_RESTART:
+               return "Graceful Restart";
+       case CAPA_AS4BYTE:
+               return "4-octet AS number";
+       case CAPA_ADD_PATH:
+               return "ADD-PATH";
+       case CAPA_ENHANCED_RR:
+               return "Enhanced Route Refresh";
+       default:
+               snprintf(buf, sizeof(buf), "unknown %u", capa);
+               return buf;
+       }
+}
+
 static const char *
 aspath_delim(uint8_t seg_type, int closing)
 {