FRR: Initial support for TCP Authentication Option #9442
Conversation
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/d08076df4d413456718c7b2d4a977852/raw/eefda0619f69277590db97c061b62083a85ebb2b/cr_9442_1629317311.diff | git apply
diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c
index e2e075110..2e0ee2bdd 100644
--- a/bgpd/bgp_network.c
+++ b/bgpd/bgp_network.c
@@ -237,13 +237,14 @@ static int bgp_tcp_authopt_connect(struct peer *peer)
if (!peer->tcp_authopt_keychain_name)
return BGP_SUCCESS;
- zlog_info("peer %p %s tcp_authopt_keychain %s fd %d",
- peer, peer->host,
- peer->tcp_authopt_keychain_name, peer->fd);
- ret = tcp_authopt_user_init(&peer->tcp_authopt_user, peer->fd, &peer->su);
+ zlog_info("peer %p %s tcp_authopt_keychain %s fd %d", peer, peer->host,
+ peer->tcp_authopt_keychain_name, peer->fd);
+ ret = tcp_authopt_user_init(&peer->tcp_authopt_user, peer->fd,
+ &peer->su);
if (ret)
return BGP_ERR_TCP_AUTHOPT_FAILED;
- ret = tcp_authopt_user_set(&peer->tcp_authopt_user, peer->tcp_authopt_keychain_name);
+ ret = tcp_authopt_user_set(&peer->tcp_authopt_user,
+ peer->tcp_authopt_keychain_name);
if (ret)
return BGP_ERR_TCP_AUTHOPT_FAILED;
@@ -254,16 +255,13 @@ static int bgp_tcp_authopt_accept(struct peer *peer)
{
int ret;
- zlog_info("peer %p %s fd=%d keychain_name=%s",
- peer, peer->host,
- peer->fd, peer->tcp_authopt_keychain_name);
- ret = tcp_authopt_user_init_accept(
- &peer->tcp_authopt_user,
- peer->fd,
- &peer->su,
- peer->tcp_authopt_keychain_name);
+ zlog_info("peer %p %s fd=%d keychain_name=%s", peer, peer->host,
+ peer->fd, peer->tcp_authopt_keychain_name);
+ ret = tcp_authopt_user_init_accept(&peer->tcp_authopt_user, peer->fd,
+ &peer->su,
+ peer->tcp_authopt_keychain_name);
- return ret ? BGP_ERR_TCP_AUTHOPT_FAILED: BGP_SUCCESS;
+ return ret ? BGP_ERR_TCP_AUTHOPT_FAILED : BGP_SUCCESS;
}
int bgp_tcp_authopt_close(struct peer *peer)
@@ -274,25 +272,21 @@ int bgp_tcp_authopt_close(struct peer *peer)
int bgp_tcp_authopt_transfer(struct peer *newpeer, struct peer *oldpeer)
{
- zlog_info("oldpeer %p %s keychain_name=%s",
- oldpeer, oldpeer->host,
- oldpeer->tcp_authopt_keychain_name);
- zlog_info("newpeer %p %s fd=%d keychain_name=%s",
- newpeer, newpeer->host,
- newpeer->fd, newpeer->tcp_authopt_keychain_name);
+ zlog_info("oldpeer %p %s keychain_name=%s", oldpeer, oldpeer->host,
+ oldpeer->tcp_authopt_keychain_name);
+ zlog_info("newpeer %p %s fd=%d keychain_name=%s", newpeer,
+ newpeer->host, newpeer->fd,
+ newpeer->tcp_authopt_keychain_name);
tcp_authopt_user_reset(&oldpeer->tcp_authopt_user);
- tcp_authopt_user_init_accept(
- &newpeer->tcp_authopt_user,
- newpeer->fd,
- &newpeer->su,
- newpeer->tcp_authopt_keychain_name);
+ tcp_authopt_user_init_accept(&newpeer->tcp_authopt_user, newpeer->fd,
+ &newpeer->su,
+ newpeer->tcp_authopt_keychain_name);
return 0;
}
-int
-bgp_tcp_authopt_set(struct peer *peer)
+int bgp_tcp_authopt_set(struct peer *peer)
{
struct listnode *node;
struct bgp_listener *listener;
@@ -304,14 +298,16 @@ bgp_tcp_authopt_set(struct peer *peer)
for (ALL_LIST_ELEMENTS_RO(bm->listen_sockets, node, listener)) {
if (listener->su.sa.sa_family != peer->su.sa.sa_family)
continue;
- zlog_info("peer %p %s tcp_authopt_keychain %s listener %p fd %d",
- peer, peer->host,
- peer->tcp_authopt_keychain_name,
- listener, listener->fd);
- ret = tcp_authopt_user_init(&peer->tcp_authopt_user_listen, listener->fd, &peer->su);
+ zlog_info(
+ "peer %p %s tcp_authopt_keychain %s listener %p fd %d",
+ peer, peer->host, peer->tcp_authopt_keychain_name,
+ listener, listener->fd);
+ ret = tcp_authopt_user_init(&peer->tcp_authopt_user_listen,
+ listener->fd, &peer->su);
if (ret)
return BGP_ERR_TCP_AUTHOPT_FAILED;
- ret = tcp_authopt_user_set(&peer->tcp_authopt_user_listen, peer->tcp_authopt_keychain_name);
+ ret = tcp_authopt_user_set(&peer->tcp_authopt_user_listen,
+ peer->tcp_authopt_keychain_name);
if (ret)
return BGP_ERR_TCP_AUTHOPT_FAILED;
}
diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index e65e23814..c7dcd960b 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -4692,13 +4692,11 @@ DEFUN (no_neighbor_password,
return bgp_vty_return(vty, ret);
}
-DEFUN (neighbor_tcp_authopt,
- neighbor_tcp_authopt_cmd,
- "neighbor <A.B.C.D|X:X::X:X|WORD> tcp-authopt [KEYCHAIN]",
- NEIGHBOR_STR
- NEIGHBOR_ADDR_STR2
- "Enable the TCP Authentication Option\n"
- "The name of the KEYCHAIN to use\n")
+DEFUN(neighbor_tcp_authopt, neighbor_tcp_authopt_cmd,
+ "neighbor <A.B.C.D|X:X::X:X|WORD> tcp-authopt [KEYCHAIN]",
+ NEIGHBOR_STR NEIGHBOR_ADDR_STR2
+ "Enable the TCP Authentication Option\n"
+ "The name of the KEYCHAIN to use\n")
{
struct peer *peer;
int ret;
@@ -4711,13 +4709,10 @@ DEFUN (neighbor_tcp_authopt,
return bgp_vty_return(vty, ret);
}
-DEFUN (no_neighbor_tcp_authopt,
- no_neighbor_tcp_authopt_cmd,
- "no neighbor <A.B.C.D|X:X::X:X|WORD> tcp-authopt",
- NO_STR
- NEIGHBOR_STR
- NEIGHBOR_ADDR_STR2
- "Disable the TCP Authentication Option\n")
+DEFUN(no_neighbor_tcp_authopt, no_neighbor_tcp_authopt_cmd,
+ "no neighbor <A.B.C.D|X:X::X:X|WORD> tcp-authopt",
+ NO_STR NEIGHBOR_STR NEIGHBOR_ADDR_STR2
+ "Disable the TCP Authentication Option\n")
{
struct peer *peer;
int ret;
diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c
index a63cdc64f..85d97ceb4 100644
--- a/bgpd/bgpd.c
+++ b/bgpd/bgpd.c
@@ -5960,17 +5960,19 @@ int peer_local_as_unset(struct peer *peer)
int peer_tcp_authopt_keychain_set(struct peer *peer, const char *keychain_name)
{
- zlog_info("peer %p %s tcp_authopt keychain_name %s", peer, peer->host, keychain_name);
+ zlog_info("peer %p %s tcp_authopt keychain_name %s", peer, peer->host,
+ keychain_name);
/* Set flag and configuration on peer. */
peer_flag_set(peer, PEER_FLAG_TCP_AUTHOPT_KEYCHAIN);
- if (peer->tcp_authopt_keychain_name && keychain_name &&
- strcmp(peer->tcp_authopt_keychain_name, keychain_name) == 0)
+ if (peer->tcp_authopt_keychain_name && keychain_name
+ && strcmp(peer->tcp_authopt_keychain_name, keychain_name) == 0)
return 0;
if (peer->tcp_authopt_keychain_name == NULL && keychain_name == NULL)
return 0;
XFREE(MTYPE_PEER_TCP_AUTHOPT_KEYCHAIN, peer->tcp_authopt_keychain_name);
- peer->tcp_authopt_keychain_name = XSTRDUP(MTYPE_PEER_TCP_AUTHOPT_KEYCHAIN, keychain_name);
+ peer->tcp_authopt_keychain_name =
+ XSTRDUP(MTYPE_PEER_TCP_AUTHOPT_KEYCHAIN, keychain_name);
if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->status))
@@ -5981,7 +5983,8 @@ int peer_tcp_authopt_keychain_set(struct peer *peer, const char *keychain_name)
return bgp_tcp_authopt_set(peer);
} else {
- zlog_err("tcp authopt not supported for group or dynamic range\n");
+ zlog_err(
+ "tcp authopt not supported for group or dynamic range\n");
return BGP_ERR_TCP_AUTHOPT_FAILED;
}
}
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 3cbf0cacc..83c8c3aab 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -1276,7 +1276,8 @@ struct peer {
#define PEER_FLAG_PASSWORD (1U << 20) /* password */
#define PEER_FLAG_LOCAL_AS (1U << 21) /* local-as */
#define PEER_FLAG_UPDATE_SOURCE (1U << 22) /* update-source */
-#define PEER_FLAG_TCP_AUTHOPT_KEYCHAIN (1U << 23) /* tcp authentication option keychain */
+#define PEER_FLAG_TCP_AUTHOPT_KEYCHAIN \
+ (1U << 23) /* tcp authentication option keychain */
/* BGP-GR Peer related flags */
#define PEER_FLAG_GRACEFUL_RESTART_HELPER (1U << 23) /* Helper */
@@ -1933,7 +1934,7 @@ enum bgp_clear_type {
#define BGP_ERR_GR_OPERATION_FAILED -37
#define BGP_GR_NO_OPERATION -38
-#define BGP_ERR_TCP_AUTHOPT_FAILED -39
+#define BGP_ERR_TCP_AUTHOPT_FAILED -39
/*
* Enumeration of different policy kinds a peer can be configured with.
@@ -2170,7 +2171,8 @@ extern int peer_advertise_map_set(struct peer *peer, afi_t afi, safi_t safi,
extern int peer_password_set(struct peer *, const char *);
extern int peer_password_unset(struct peer *);
-extern int peer_tcp_authopt_keychain_set(struct peer *peer, const char *keychain_name);
+extern int peer_tcp_authopt_keychain_set(struct peer *peer,
+ const char *keychain_name);
extern int peer_unsuppress_map_unset(struct peer *, afi_t, safi_t);
diff --git a/lib/keychain.c b/lib/keychain.c
index da798fcaa..4aea4e66f 100644
--- a/lib/keychain.c
+++ b/lib/keychain.c
@@ -141,7 +141,8 @@ bool key_valid_for_accept(struct key *key, time_t now)
if (key->accept.start == 0)
return true;
- if (key->accept.start <= now && (key->accept.end >= now || key->accept.end == -1))
+ if (key->accept.start <= now
+ && (key->accept.end >= now || key->accept.end == -1))
return true;
return false;
@@ -152,7 +153,8 @@ bool key_valid_for_send(struct key *key, time_t now)
if (key->send.start == 0)
return true;
- if (key->send.start <= now && (key->send.end >= now || key->send.end == -1))
+ if (key->send.start <= now
+ && (key->send.end >= now || key->send.end == -1))
return true;
return false;
@@ -989,8 +991,7 @@ static const char *tcp_authopt_alg_id_to_name(enum zebra_tcp_authopt_alg id)
return NULL;
}
-DEFUN(tcp_authopt_enable,
- tcp_authopt_enable_cmd,
+DEFUN(tcp_authopt_enable, tcp_authopt_enable_cmd,
"tcp-authopt <enabled|disabled>",
"TCP Authentication Option\n"
"Enable using this key for TCP Authentication Option\n"
@@ -1001,8 +1002,7 @@ DEFUN(tcp_authopt_enable,
return CMD_SUCCESS;
}
-DEFUN(tcp_authopt_algorithm,
- tcp_authopt_algorithm_cmd,
+DEFUN(tcp_authopt_algorithm, tcp_authopt_algorithm_cmd,
"tcp-authopt algorithm <hmac-sha-1-96|aes-128-cmac-96>",
"TCP Authentication Option\n"
"TCP Authentication Option algorithm\n"
@@ -1023,8 +1023,7 @@ DEFUN(tcp_authopt_algorithm,
}
}
-DEFUN(tcp_authopt_send_id,
- tcp_authopt_send_id_cmd,
+DEFUN(tcp_authopt_send_id, tcp_authopt_send_id_cmd,
"tcp-authopt send-id (0-255)",
"TCP Authentication Option\n"
"SendID\n"
@@ -1035,8 +1034,7 @@ DEFUN(tcp_authopt_send_id,
return CMD_SUCCESS;
}
-DEFUN(tcp_authopt_recv_id,
- tcp_authopt_recv_id_cmd,
+DEFUN(tcp_authopt_recv_id, tcp_authopt_recv_id_cmd,
"tcp-authopt recv-id (0-255)",
"TCP Authentication Option\n"
"RecvID\n"
@@ -1132,11 +1130,15 @@ static int keychain_config_write(struct vty *vty)
if (key->tcp_authopt_enabled)
vty_out(vty, " tcp-authopt enabled\n");
if (key->tcp_authopt_alg)
- vty_out(vty, " tcp-authopt algorithm %s\n", tcp_authopt_alg_id_to_name(key->tcp_authopt_alg));
+ vty_out(vty, " tcp-authopt algorithm %s\n",
+ tcp_authopt_alg_id_to_name(
+ key->tcp_authopt_alg));
if (key->tcp_authopt_send_id)
- vty_out(vty, " tcp-authopt send-id %hhu\n", key->tcp_authopt_send_id);
+ vty_out(vty, " tcp-authopt send-id %hhu\n",
+ key->tcp_authopt_send_id);
if (key->tcp_authopt_recv_id)
- vty_out(vty, " tcp-authopt recv-id %hhu\n", key->tcp_authopt_recv_id);
+ vty_out(vty, " tcp-authopt recv-id %hhu\n",
+ key->tcp_authopt_recv_id);
vty_out(vty, " exit\n");
}
vty_out(vty, "!\n");
diff --git a/vtysh/vtysh.h b/vtysh/vtysh.h
index 234a5e311..a08135cf3 100644
--- a/vtysh/vtysh.h
+++ b/vtysh/vtysh.h
@@ -57,7 +57,7 @@ DECLARE_MGROUP(MVTYSH);
#define VTYSH_RMAP VTYSH_ZEBRA|VTYSH_RIPD|VTYSH_RIPNGD|VTYSH_OSPFD|VTYSH_OSPF6D|VTYSH_BGPD|VTYSH_ISISD|VTYSH_PIMD|VTYSH_EIGRPD|VTYSH_FABRICD
#define VTYSH_INTERFACE VTYSH_ZEBRA|VTYSH_RIPD|VTYSH_RIPNGD|VTYSH_OSPFD|VTYSH_OSPF6D|VTYSH_ISISD|VTYSH_PIMD|VTYSH_NHRPD|VTYSH_EIGRPD|VTYSH_BABELD|VTYSH_PBRD|VTYSH_FABRICD|VTYSH_VRRPD
#define VTYSH_VRF VTYSH_INTERFACE|VTYSH_STATICD
-#define VTYSH_KEYS VTYSH_RIPD|VTYSH_EIGRPD|VTYSH_BGPD
+#define VTYSH_KEYS VTYSH_RIPD | VTYSH_EIGRPD | VTYSH_BGPD
/* Daemons who can process nexthop-group configs */
#define VTYSH_NH_GROUP VTYSH_PBRD|VTYSH_SHARPD
#define VTYSH_SR VTYSH_ZEBRA|VTYSH_PATHD
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 0 tests failed, has VALGRIND issues
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedNetBSD 8 amd64 build: Failed (click for details)NetBSD 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI012BUILD/config.log/config.log.gzMake failed for NetBSD 8 amd64 build: NetBSD 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI012BUILD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build: FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI009BUILD/config.status/config.status OpenBSD 6 amd64 build: Failed (click for details)Make failed for OpenBSD 6 amd64 build: OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI011BUILD/config.status/config.status FreeBSD 12 amd64 build: Failed (click for details)Make failed for FreeBSD 12 amd64 build: FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/FBSD12AMD64/config.status/config.status Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsNetBSD 8 amd64 build: Failed (click for details)NetBSD 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI012BUILD/config.log/config.log.gzMake failed for NetBSD 8 amd64 build: NetBSD 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI012BUILD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build: FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI009BUILD/config.status/config.status OpenBSD 6 amd64 build: Failed (click for details)Make failed for OpenBSD 6 amd64 build: OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/CI011BUILD/config.status/config.status FreeBSD 12 amd64 build: Failed (click for details)Make failed for FreeBSD 12 amd64 build: FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21196/artifact/FBSD12AMD64/config.status/config.status |
|
Attempted to fix failure on non-linux systems. |
Outdated results 🛑Basic BGPD CI results: FAILURE
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedCentOS 7 amd64 build: Failed (click for details)CentOS 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/ErrorLog/ CentOS 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/config.status/config.status CentOS 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/config.log/config.log.gz CentOS 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/frr.xref.xz/frr.xref.xz CentOS 7 amd64 build: No useful log foundOpenBSD 6 amd64 build: Failed (click for details)Make failed for OpenBSD 6 amd64 build: OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI011BUILD/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Fedora 29 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/ErrorLog/ Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/config.status/config.status Fedora 29 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/config.log/config.log.gz Fedora 29 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/frr.xref.xz/frr.xref.xz Fedora 29 amd64 build: No useful log foundCentOS 8 amd64 build: Failed (click for details)CentOS 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/config.status/config.status CentOS 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/ErrorLog/ CentOS 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/config.log/config.log.gz CentOS 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/frr.xref.xz/frr.xref.xz CentOS 8 amd64 build: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsCentOS 7 amd64 build: Failed (click for details)CentOS 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/ErrorLog/ CentOS 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/config.status/config.status CentOS 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/config.log/config.log.gz CentOS 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI005BUILD/frr.xref.xz/frr.xref.xz CentOS 7 amd64 build: No useful log foundOpenBSD 6 amd64 build: Failed (click for details)Make failed for OpenBSD 6 amd64 build: OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CI011BUILD/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Fedora 29 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/ErrorLog/ Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/config.status/config.status Fedora 29 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/config.log/config.log.gz Fedora 29 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/F29BUILD/frr.xref.xz/frr.xref.xz Fedora 29 amd64 build: No useful log foundCentOS 8 amd64 build: Failed (click for details)CentOS 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/config.status/config.status CentOS 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/ErrorLog/ CentOS 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/config.log/config.log.gz CentOS 8 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-21225/artifact/CENTOS8BUILD/frr.xref.xz/frr.xref.xz CentOS 8 amd64 build: No useful log found |
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 0 tests failed, has VALGRIND issues
For details, please contact louberger |
|
Any chance for this to use the XFRM infrastructure, i.e. |
|
This ABI mirrors TCP_MD5SIG because the standard is very directly intended to replace TCP-MD5. TCP sockopts are very easy to use and not at all scarce so why not use them? I'm not very familiar with XFRM so I'd have to investigate how that could be used here. As far as I understand xfrm is meant for "global" config which matches and transforms all packets matching certain criteria, this would be very different from my approach where TCP-AO configuration only exists at the socket level. My approach seems to match the intent behind RFC5925; they describe user interface recommendations which are all per-connection. Are you expecting TCP-AO to "match" packets based on global config? Another related issue is that on netdev people asked to use generic algorithm definitions similar to xfrm instead of an enum based on just what RFC5926 describes. I am very reluctant to add so much general functionality. Link: https://lore.kernel.org/netdev/CAJwJo6aicw_KGQSM5U1=0X11QfuNf2dMATErSymytmpf75W=tA@mail.gmail.com/ |
|
Posted v3 of kernel changes to netdev: https://lore.kernel.org/netdev/cover.1629840814.git.cdleonard@gmail.com/ You can send ABI feedback there as well. |
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Style checking failed; check logs
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
|
Fixed tcp-authopt property handling in peer groups (using test_peer_attr). Also fix overlapping flag with graceful-restart. |
|
Style warnings apparently here: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-CHECKOUT-21406/log Lots of "lines over 80 characters", maybe you should follow the kernel and invest in 100-column terminals? :) |
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 0 tests failed, has VALGRIND issues
For details, please contact louberger |
For the TCP Authentication option we need to enumerate and add all valid keys to the socket so the current "lookup" interface is insufficient. This also makes the code slightly cleaner. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
Add new members to `struct key` which are specific to the tcp authentication option. Only keys with the "tcp-authopt enabled" property will be used for TCP authentication. Existing users will ignore these new properties. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
A struct tcp_authopt_user is meant to ensure that a certain keychain is applied to a certain socket and towards a certain address. Eventually this will also handle keychain updates but they are curretly ignored. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
Make keychain commands reach bgpd. They currently only reach ripd and eigrpd. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
Add a "neighbor 1.2.3.4 tcp-authopt KEYCHAIN" command in order to make BGP require TCP authentication towards that neighbor. Actual handling of the property is in a following commit. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
This should validate that the new tcp-autopt attribute behaves nicely with peer groups. Execute all the tests used for the "password" property for TCP-MD5. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
Basic test which configure a tcp authentication keychain and verifies that the expected keys are being used. Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
|
Attempting for fix checkpatch |
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 0 tests failed, has VALGRIND issues
For details, please contact louberger |
I would've preferred that
You can set a socket policy with My thought was simply that it would be nice to have the same interface for IPsec AH (for OSPF & co.) and TCP-AO (for BGP & LDP) [and maybe even TCP-MD5.] But I don't know if it's really the "right thing", I just want to know if you considered it (and maybe what was the reason against it.) |
|
I have not considered XFRM in detail because I'm not familiar with the xfrm API. I read the ipsec RFCs some years ago and I remember them poorly. You're right that This is more complex to implement but in theory it would work even for unmodified applications. I don't think there are notable interactions between TCP and XFRM in the kernel, XFRM is mostly at the IP level. Attaching all keys to sockets instead of making them global objects made my implementation a lot easier! I'm not sure how IP_XFRM_POLICY would apply here. There would still be a need to make multiple TCP-AO keychains apply to a listen socket. A sockopt would still be used in order to get/set the keyids currently being used on the connection: this is a requirement of RFC5925. Maybe some sort of netlink interface could also be provided (like tcp_info) but that would be complimentary and mostly used by administrative tools. The application handling the connect would still use sockopts on the FD. |
|
Looking through failures reported by bamboo a lot of the issues are crashes related to my code in tests that should not be affected. Need to run locally and fix. |
|
This PR is stale because it has been open 180 days with no activity. Comment or remove the |
Please don't close this github. It's stale but I might restore it. |
|
I'm removing the autoclose as that I want to keep this around as well |
|
On a side note how is getting the tcp-A0 into the kernel going? I have not been paying attention but it should be on my radar |
|
Last upstream post was v6 last week: https://lore.kernel.org/netdev/cover.1658815925.git.cdleonard@gmail.com/ |
|
A different unrelated kernel implementation was posted upstream: https://lore.kernel.org/netdev/20220818170005.747015-1-dima@arista.com/ ABI feedback might be useful. |
|
Posted v8 upstream: https://lore.kernel.org/netdev/cover.1662361354.git.cdleonard@gmail.com/ Major difference is that key rollover can now be handled entirely inside the kernel so this would greatly simplify userspace (no more timers required). |
Outdated results 🚧Basic BGPD CI results: Partial FAILURE, 0 tests failed, has VALGRIND issues
For details, please contact louberger |
🚧 Basic BGPD CI results: Partial FAILURE, 0 tests failed, has VALGRIND issuesResults table
For details, please contact louberger |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
@cdleonard -> looks like tcp-ao got into the kernel. Any possibility you can pick this up again? |
Sorry but no (notice how I closed the PR). Somebody else should probably rewrite this from scratch. |
This contains the minimum required to show that TCP authentication can work between BGP peers.
The kernel part is still a RFC, this was posted here early mainly to get ABI feedback. This version was tested with https://github.com/cdleonard/linux/commits/tcp_authopt%4020210813T195209, ABI might change in the future. Since kernel changes are required CI is expected to fail.
Last netdev post is here: https://lore.kernel.org/netdev/cover.1628544649.git.cdleonard@gmail.com/
Some ABI issues:
For implementing key rollover there is already support in the kernel but not in these FRR patches. My best guess is that I should add a timer for "next key validity change event" and a frr hook for any keychain changes? The lib/tcp_authopt.[ch] layer was created in the idea that BGP and LDP and maybe others would share it.
RFC5925 warns against changing keys while in use but it's not clear the extent to which this should enforced at the vtysh level.
There are also a bunch of issues related to bgp code itself: