[PATCH 0/4] tls-handshake: server-side support

[PATCH 0/4] tls-handshake: server-side support

[Hannes Reinecke]

Hi all,

here are my patches to get server-side for PSK up and
(well, not exactly) running.
Pretty trivial, really; just adding two more netlink
attributes (showing the power of netlink; I really like
this interface) and split server_hello() into two functions.

Based on v4 of the tls handshake netlink patches.

Hannes Reinecke (4):
  tls-handshake: add 'timeout' netlink attribute
  tls-handshake: add 'keyring' netlink attribute
  net/tls_handshake: split tls_server_hello()
  tls_handshake: add 'keyring' argument to server hello

 include/net/tls.h              | 18 +++++--
 include/uapi/linux/handshake.h |  2 +
 net/tls/tls_handshake.c        | 97 ++++++++++++++++++++++++++++++----
 3 files changed, 103 insertions(+), 14 deletions(-)

-- 
2.35.3

[PATCH 1/4] tls-handshake: add 'timeout' netlink attribute

[Hannes Reinecke]

Add a 'timeout' netlink attribute to the 'request' netlink
message to allow the kernel to communicate an internal timeout
to userspace.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 include/net/tls.h              | 11 +++++++----
 include/uapi/linux/handshake.h |  1 +
 net/tls/tls_handshake.c        | 30 ++++++++++++++++++++++++++----
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index f613feb64da8..51bf5a083cce 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -523,20 +523,23 @@ enum {
 	TLS_NO_PEERID = 0,
 	TLS_NO_CERT = 0,
 	TLS_NO_PRIVKEY = 0,
+	TLS_NO_TIMEOUT = 0,
 };
 
 typedef void	(*tls_done_func_t)(void *data, int status,
 				   key_serial_t peerid);
 
 int tls_client_hello_anon(struct socket *sock, tls_done_func_t done,
-			  void *data, const char *priorities);
+			  void *data, const char *priorities,
+			  unsigned int timeout);
 int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
 			  void *data, const char *priorities,
-			  key_serial_t cert, key_serial_t privkey);
+			  key_serial_t cert, key_serial_t privkey,
+			  unsigned int timeout);
 int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
-			 key_serial_t peerid);
+			 key_serial_t peerid, unsigned int timeout);
 int tls_server_hello(struct socket *sock, tls_done_func_t done,
-		     void *data, const char *priorities);
+		     void *data, const char *priorities, unsigned int timeout);
 
 #endif /* _TLS_OFFLOAD_H */
diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
index 33c417cadfcb..b007e346cfc8 100644
--- a/include/uapi/linux/handshake.h
+++ b/include/uapi/linux/handshake.h
@@ -64,6 +64,7 @@ enum handshake_tls_accept_attrs {
 	HANDSHAKE_GENL_ATTR_TLS_X509_CERT,
 	HANDSHAKE_GENL_ATTR_TLS_X509_PRIVKEY,
 	HANDSHAKE_GENL_ATTR_TLS_PSK,
+	HANDSHAKE_GENL_ATTR_TLS_TIMEOUT,
 
 	__HANDSHAKE_GENL_ATTR_TLS_ACCEPT_MAX
 };
diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
index 007308727395..66adac8d660a 100644
--- a/net/tls/tls_handshake.c
+++ b/net/tls/tls_handshake.c
@@ -42,6 +42,7 @@ struct tls_handshake_req {
 	const char		*th_priorities;
 	int			th_type;
 	int			th_auth_type;
+	unsigned int		th_timeout;
 	key_serial_t		th_peerid;
 	key_serial_t		th_certificate;
 	key_serial_t		th_privkey;
@@ -72,6 +73,7 @@ tls_handshake_req_init(struct handshake_req *req, tls_done_func_t done,
 	treq->th_peerid = TLS_NO_PEERID;
 	treq->th_certificate = TLS_NO_CERT;
 	treq->th_privkey = TLS_NO_PRIVKEY;
+	treq->th_timeout = TLS_NO_TIMEOUT;
 	return treq;
 }
 
@@ -162,6 +164,12 @@ static int tls_handshake_put_accept_resp(struct sk_buff *msg,
 		if (ret < 0)
 			goto out;
 	}
+	if (treq->th_timeout != TLS_NO_TIMEOUT) {
+		ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_TLS_TIMEOUT,
+				  treq->th_timeout);
+		if (ret < 0)
+			goto out;
+	}
 
 	ret = nla_put_string(msg, HANDSHAKE_GENL_ATTR_TLS_PRIORITIES,
 			     treq->th_priorities);
@@ -233,6 +241,7 @@ static const struct handshake_proto tls_handshake_proto = {
  * @done: function to call when the handshake has completed
  * @data: token to pass back to @done
  * @priorities: GnuTLS TLS priorities string, or NULL
+ * @timeout: TLS handshake timeout (in seconds)
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -240,7 +249,8 @@ static const struct handshake_proto tls_handshake_proto = {
  *   %-ENOMEM: Memory allocation failed
  */
 int tls_client_hello_anon(struct socket *sock, tls_done_func_t done,
-			  void *data, const char *priorities)
+			  void *data, const char *priorities,
+			  unsigned int timeout)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -260,6 +270,8 @@ int tls_client_hello_anon(struct socket *sock, tls_done_func_t done,
 	treq = tls_handshake_req_init(req, done, data, tp);
 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_CLIENTHELLO;
 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_UNAUTH;
+	if (timeout)
+		treq->th_timeout = timeout;
 
 	return handshake_req_submit(req, flags);
 }
@@ -273,6 +285,7 @@ EXPORT_SYMBOL(tls_client_hello_anon);
  * @priorities: GnuTLS TLS priorities string
  * @cert: serial number of key containing client's x.509 certificate
  * @privkey: serial number of key containing client's private key
+ * @timeout: TLS handshake timeout (in seconds)
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -281,7 +294,8 @@ EXPORT_SYMBOL(tls_client_hello_anon);
  */
 int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
 			  void *data, const char *priorities,
-			  key_serial_t cert, key_serial_t privkey)
+			  key_serial_t cert, key_serial_t privkey,
+			  unsigned int timeout)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -303,6 +317,8 @@ int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_X509;
 	treq->th_certificate = cert;
 	treq->th_privkey = privkey;
+	if (timeout)
+		treq->th_timeout = timeout;
 
 	return handshake_req_submit(req, flags);
 }
@@ -315,6 +331,7 @@ EXPORT_SYMBOL(tls_client_hello_x509);
  * @data: token to pass back to @done
  * @priorities: GnuTLS TLS priorities string
  * @peerid: serial number of key containing TLS identity
+ * @timeout: TLS handshake timeout (in seconds)
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -323,7 +340,7 @@ EXPORT_SYMBOL(tls_client_hello_x509);
  */
 int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
-			 key_serial_t peerid)
+			 key_serial_t peerid, unsigned int timeout)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -344,6 +361,8 @@ int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_CLIENTHELLO;
 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_PSK;
 	treq->th_peerid = peerid;
+	if (timeout)
+		treq->th_timeout = timeout;
 
 	return handshake_req_submit(req, flags);
 }
@@ -355,6 +374,7 @@ EXPORT_SYMBOL(tls_client_hello_psk);
  * @done: function to call when the handshake has completed
  * @data: token to pass back to @done
  * @priorities: GnuTLS TLS priorities string
+ * @timeout: TLS handshake timeout (in seconds)
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -362,7 +382,7 @@ EXPORT_SYMBOL(tls_client_hello_psk);
  *   %-ENOMEM: Memory allocation failed
  */
 int tls_server_hello(struct socket *sock, tls_done_func_t done,
-		     void *data, const char *priorities)
+		     void *data, const char *priorities, unsigned int timeout)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -382,6 +402,8 @@ int tls_server_hello(struct socket *sock, tls_done_func_t done,
 	treq = tls_handshake_req_init(req, done, data, tp);
 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_SERVERHELLO;
 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_UNSPEC;
+	if (timeout)
+		treq->th_timeout = timeout;
 
 	return handshake_req_submit(req, flags);
 }
-- 
2.35.3

[PATCH 2/4] tls-handshake: add 'keyring' netlink attribute

[Hannes Reinecke]

Add a 'keyring' netlink attribute to the 'request' netlink
message to allow the kernel to communicate the keyring to use
to userspace.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 include/net/tls.h              | 1 +
 include/uapi/linux/handshake.h | 1 +
 net/tls/tls_handshake.c        | 8 ++++++++
 3 files changed, 10 insertions(+)

diff --git a/include/net/tls.h b/include/net/tls.h
index 51bf5a083cce..f4baf3b4b179 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -524,6 +524,7 @@ enum {
 	TLS_NO_CERT = 0,
 	TLS_NO_PRIVKEY = 0,
 	TLS_NO_TIMEOUT = 0,
+	TLS_NO_KEYRING = 0,
 };
 
 typedef void	(*tls_done_func_t)(void *data, int status,
diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
index b007e346cfc8..83705b1d1e9a 100644
--- a/include/uapi/linux/handshake.h
+++ b/include/uapi/linux/handshake.h
@@ -65,6 +65,7 @@ enum handshake_tls_accept_attrs {
 	HANDSHAKE_GENL_ATTR_TLS_X509_PRIVKEY,
 	HANDSHAKE_GENL_ATTR_TLS_PSK,
 	HANDSHAKE_GENL_ATTR_TLS_TIMEOUT,
+	HANDSHAKE_GENL_ATTR_TLS_KEYRING,
 
 	__HANDSHAKE_GENL_ATTR_TLS_ACCEPT_MAX
 };
diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
index 66adac8d660a..8171b3c8f3a5 100644
--- a/net/tls/tls_handshake.c
+++ b/net/tls/tls_handshake.c
@@ -43,6 +43,7 @@ struct tls_handshake_req {
 	int			th_type;
 	int			th_auth_type;
 	unsigned int		th_timeout;
+	key_serial_t		th_keyring;
 	key_serial_t		th_peerid;
 	key_serial_t		th_certificate;
 	key_serial_t		th_privkey;
@@ -74,6 +75,7 @@ tls_handshake_req_init(struct handshake_req *req, tls_done_func_t done,
 	treq->th_certificate = TLS_NO_CERT;
 	treq->th_privkey = TLS_NO_PRIVKEY;
 	treq->th_timeout = TLS_NO_TIMEOUT;
+	treq->th_keyring = TLS_NO_KEYRING;
 	return treq;
 }
 
@@ -170,6 +172,12 @@ static int tls_handshake_put_accept_resp(struct sk_buff *msg,
 		if (ret < 0)
 			goto out;
 	}
+	if (treq->th_keyring != TLS_NO_KEYRING) {
+		ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_TLS_KEYRING,
+				  treq->th_keyring);
+		if (ret < 0)
+			goto out;
+	}
 
 	ret = nla_put_string(msg, HANDSHAKE_GENL_ATTR_TLS_PRIORITIES,
 			     treq->th_priorities);
-- 
2.35.3

[PATCH 3/4] net/tls_handshake: split tls_server_hello()

[Hannes Reinecke]

Split tls_server_hello() into 'tls_server_hello_x509()' and 'tls_server_hello_psk()'
to handle X.509 and PSK serverhandshakes.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 include/net/tls.h       |  8 ++++--
 net/tls/tls_handshake.c | 55 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index f4baf3b4b179..be65cbf72f8a 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -540,7 +540,11 @@ int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
 int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
 			 key_serial_t peerid, unsigned int timeout);
-int tls_server_hello(struct socket *sock, tls_done_func_t done,
-		     void *data, const char *priorities, unsigned int timeout);
+int tls_server_hello_x509(struct socket *sock, tls_done_func_t done,
+			  void *data, const char *priorities,
+			  unsigned int timeout);
+int tls_server_hello_psk(struct socket *sock, tls_done_func_t done,
+			 void *data, const char *priorities,
+			 unsigned int timeout);
 
 #endif /* _TLS_OFFLOAD_H */
diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
index 8171b3c8f3a5..93bb3deaf2fb 100644
--- a/net/tls/tls_handshake.c
+++ b/net/tls/tls_handshake.c
@@ -141,7 +141,7 @@ static int tls_handshake_put_accept_resp(struct sk_buff *msg,
 		goto out;
 
 	ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_TLS_TYPE,
-			  HANDSHAKE_GENL_TLS_TYPE_CLIENTHELLO);
+			  treq->th_type);
 	if (ret < 0)
 		goto out;
 	ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_TLS_AUTH,
@@ -377,7 +377,7 @@ int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 EXPORT_SYMBOL(tls_client_hello_psk);
 
 /**
- * tls_server_hello - request a server TLS handshake on a socket
+ * tls_server_hello_x509 - request a X.509 server TLS handshake on a socket
  * @sock: connected socket on which to perform the handshake
  * @done: function to call when the handshake has completed
  * @data: token to pass back to @done
@@ -389,8 +389,9 @@ EXPORT_SYMBOL(tls_client_hello_psk);
  *   %-ENOENT: No user agent is available
  *   %-ENOMEM: Memory allocation failed
  */
-int tls_server_hello(struct socket *sock, tls_done_func_t done,
-		     void *data, const char *priorities, unsigned int timeout)
+int tls_server_hello_x509(struct socket *sock, tls_done_func_t done,
+			  void *data, const char *priorities,
+			  unsigned int timeout)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -409,10 +410,52 @@ int tls_server_hello(struct socket *sock, tls_done_func_t done,
 
 	treq = tls_handshake_req_init(req, done, data, tp);
 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_SERVERHELLO;
-	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_UNSPEC;
+	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_X509;
+	if (timeout)
+		treq->th_timeout = timeout;
+
+	return handshake_req_submit(req, flags);
+}
+EXPORT_SYMBOL(tls_server_hello_x509);
+
+/**
+ * tls_server_hello_psk - request a PSK server TLS handshake on a socket
+ * @sock: connected socket on which to perform the handshake
+ * @done: function to call when the handshake has completed
+ * @data: token to pass back to @done
+ * @priorities: GnuTLS TLS priorities string
+ * @timeout: TLS handshake timeout (in seconds)
+ *
+ * Return values:
+ *   %0: Handshake request enqueue; ->done will be called when complete
+ *   %-ENOENT: No user agent is available
+ *   %-ENOMEM: Memory allocation failed
+ */
+int tls_server_hello_psk(struct socket *sock, tls_done_func_t done,
+			 void *data, const char *priorities,
+			 unsigned int timeout)
+{
+	struct tls_handshake_req *treq;
+	struct handshake_req *req;
+	gfp_t flags = GFP_KERNEL;
+	const char *tp;
+
+	tp = tls_handshake_dup_priorities(priorities, flags);
+	if (!tp)
+		return -ENOMEM;
+
+	req = handshake_req_alloc(sock, &tls_handshake_proto, flags);
+	if (!req) {
+		kfree(tp);
+		return -ENOMEM;
+	}
+
+	treq = tls_handshake_req_init(req, done, data, tp);
+	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_SERVERHELLO;
+	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_PSK;
 	if (timeout)
 		treq->th_timeout = timeout;
 
 	return handshake_req_submit(req, flags);
 }
-EXPORT_SYMBOL(tls_server_hello);
+EXPORT_SYMBOL(tls_server_hello_psk);
-- 
2.35.3

[PATCH 4/4] tls_handshake: add 'keyring' argument to server hello

[Hannes Reinecke]

For ServerHello the TLS handshake has to validate the username/identity
with the keys stored on the server, but these might be stored on a
separate keyring. So add an argument 'keyring' to the server hello
functions to transport this information to the userspace daemon.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 include/net/tls.h       |  4 ++--
 net/tls/tls_handshake.c | 10 ++++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index be65cbf72f8a..2d33e883f7d0 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -542,9 +542,9 @@ int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
 			 key_serial_t peerid, unsigned int timeout);
 int tls_server_hello_x509(struct socket *sock, tls_done_func_t done,
 			  void *data, const char *priorities,
-			  unsigned int timeout);
+			  unsigned int timeout, key_serial_t keyring);
 int tls_server_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
-			 unsigned int timeout);
+			 unsigned int timeout, key_serial_t keyring);
 
 #endif /* _TLS_OFFLOAD_H */
diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
index 93bb3deaf2fb..7fa2baa323e4 100644
--- a/net/tls/tls_handshake.c
+++ b/net/tls/tls_handshake.c
@@ -383,6 +383,7 @@ EXPORT_SYMBOL(tls_client_hello_psk);
  * @data: token to pass back to @done
  * @priorities: GnuTLS TLS priorities string
  * @timeout: TLS handshake timeout (in seconds)
+ * @keyring: keyring with the keys to be accepted
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -391,7 +392,7 @@ EXPORT_SYMBOL(tls_client_hello_psk);
  */
 int tls_server_hello_x509(struct socket *sock, tls_done_func_t done,
 			  void *data, const char *priorities,
-			  unsigned int timeout)
+			  unsigned int timeout, key_serial_t keyring)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -413,6 +414,8 @@ int tls_server_hello_x509(struct socket *sock, tls_done_func_t done,
 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_X509;
 	if (timeout)
 		treq->th_timeout = timeout;
+	if (keyring)
+		treq->th_keyring = keyring;
 
 	return handshake_req_submit(req, flags);
 }
@@ -425,6 +428,7 @@ EXPORT_SYMBOL(tls_server_hello_x509);
  * @data: token to pass back to @done
  * @priorities: GnuTLS TLS priorities string
  * @timeout: TLS handshake timeout (in seconds)
+ * @keyring: keyring with the keys to be accepted
  *
  * Return values:
  *   %0: Handshake request enqueue; ->done will be called when complete
@@ -433,7 +437,7 @@ EXPORT_SYMBOL(tls_server_hello_x509);
  */
 int tls_server_hello_psk(struct socket *sock, tls_done_func_t done,
 			 void *data, const char *priorities,
-			 unsigned int timeout)
+			 unsigned int timeout, key_serial_t keyring)
 {
 	struct tls_handshake_req *treq;
 	struct handshake_req *req;
@@ -455,6 +459,8 @@ int tls_server_hello_psk(struct socket *sock, tls_done_func_t done,
 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_PSK;
 	if (timeout)
 		treq->th_timeout = timeout;
+	if (keyring)
+		treq->th_keyring = keyring;
 
 	return handshake_req_submit(req, flags);
 }
-- 
2.35.3

Re: [PATCH 0/4] tls-handshake: server-side support

[Chuck Lever III]

> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> Hi all,
> 
> here are my patches to get server-side for PSK up and
> (well, not exactly) running.
> Pretty trivial, really; just adding two more netlink
> attributes (showing the power of netlink; I really like
> this interface) and split server_hello() into two functions.
> 
> Based on v4 of the tls handshake netlink patches.

I still need you to sign the OCA before I can accept
these.


> Hannes Reinecke (4):
>  tls-handshake: add 'timeout' netlink attribute
>  tls-handshake: add 'keyring' netlink attribute
>  net/tls_handshake: split tls_server_hello()
>  tls_handshake: add 'keyring' argument to server hello
> 
> include/net/tls.h              | 18 +++++--
> include/uapi/linux/handshake.h |  2 +
> net/tls/tls_handshake.c        | 97 ++++++++++++++++++++++++++++++----
> 3 files changed, 103 insertions(+), 14 deletions(-)
> 
> -- 
> 2.35.3
> 

--
Chuck Lever

Re: [PATCH 1/4] tls-handshake: add 'timeout' netlink attribute

[Chuck Lever III]

> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> Add a 'timeout' netlink attribute to the 'request' netlink
> message to allow the kernel to communicate an internal timeout
> to userspace.

Can you describe the use caae? 

I'm not clear on why we need to parametrize the handshake
timeout. Why would it need to be changed? Why would, say,
a PSK handshake need a different timeout than x.509? And,
why doesn't a timeout wait in the API consumer work just
as well?

Last we left this, you were using the timeout to work
around some socket reference counting issues. I think we
need to address those first.


> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> include/net/tls.h              | 11 +++++++----
> include/uapi/linux/handshake.h |  1 +
> net/tls/tls_handshake.c        | 30 ++++++++++++++++++++++++++----
> 3 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/tls.h b/include/net/tls.h
> index f613feb64da8..51bf5a083cce 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -523,20 +523,23 @@ enum {
> 	TLS_NO_PEERID = 0,
> 	TLS_NO_CERT = 0,
> 	TLS_NO_PRIVKEY = 0,
> +	TLS_NO_TIMEOUT = 0,
> };
> 
> typedef void	(*tls_done_func_t)(void *data, int status,
> 				   key_serial_t peerid);
> 
> int tls_client_hello_anon(struct socket *sock, tls_done_func_t done,
> -			  void *data, const char *priorities);
> +			  void *data, const char *priorities,
> +			  unsigned int timeout);
> int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
> 			  void *data, const char *priorities,
> -			  key_serial_t cert, key_serial_t privkey);
> +			  key_serial_t cert, key_serial_t privkey,
> +			  unsigned int timeout);
> int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
> 			 void *data, const char *priorities,
> -			 key_serial_t peerid);
> +			 key_serial_t peerid, unsigned int timeout);
> int tls_server_hello(struct socket *sock, tls_done_func_t done,
> -		     void *data, const char *priorities);
> +		     void *data, const char *priorities, unsigned int timeout);
> 
> #endif /* _TLS_OFFLOAD_H */
> diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
> index 33c417cadfcb..b007e346cfc8 100644
> --- a/include/uapi/linux/handshake.h
> +++ b/include/uapi/linux/handshake.h
> @@ -64,6 +64,7 @@ enum handshake_tls_accept_attrs {
> 	HANDSHAKE_GENL_ATTR_TLS_X509_CERT,
> 	HANDSHAKE_GENL_ATTR_TLS_X509_PRIVKEY,
> 	HANDSHAKE_GENL_ATTR_TLS_PSK,
> +	HANDSHAKE_GENL_ATTR_TLS_TIMEOUT,
> 
> 	__HANDSHAKE_GENL_ATTR_TLS_ACCEPT_MAX
> };
> diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
> index 007308727395..66adac8d660a 100644
> --- a/net/tls/tls_handshake.c
> +++ b/net/tls/tls_handshake.c
> @@ -42,6 +42,7 @@ struct tls_handshake_req {
> 	const char		*th_priorities;
> 	int			th_type;
> 	int			th_auth_type;
> +	unsigned int		th_timeout;
> 	key_serial_t		th_peerid;
> 	key_serial_t		th_certificate;
> 	key_serial_t		th_privkey;
> @@ -72,6 +73,7 @@ tls_handshake_req_init(struct handshake_req *req, tls_done_func_t done,
> 	treq->th_peerid = TLS_NO_PEERID;
> 	treq->th_certificate = TLS_NO_CERT;
> 	treq->th_privkey = TLS_NO_PRIVKEY;
> +	treq->th_timeout = TLS_NO_TIMEOUT;
> 	return treq;
> }
> 
> @@ -162,6 +164,12 @@ static int tls_handshake_put_accept_resp(struct sk_buff *msg,
> 		if (ret < 0)
> 			goto out;
> 	}
> +	if (treq->th_timeout != TLS_NO_TIMEOUT) {
> +		ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_TLS_TIMEOUT,
> +				  treq->th_timeout);
> +		if (ret < 0)
> +			goto out;
> +	}
> 
> 	ret = nla_put_string(msg, HANDSHAKE_GENL_ATTR_TLS_PRIORITIES,
> 			     treq->th_priorities);
> @@ -233,6 +241,7 @@ static const struct handshake_proto tls_handshake_proto = {
>  * @done: function to call when the handshake has completed
>  * @data: token to pass back to @done
>  * @priorities: GnuTLS TLS priorities string, or NULL
> + * @timeout: TLS handshake timeout (in seconds)
>  *
>  * Return values:
>  *   %0: Handshake request enqueue; ->done will be called when complete
> @@ -240,7 +249,8 @@ static const struct handshake_proto tls_handshake_proto = {
>  *   %-ENOMEM: Memory allocation failed
>  */
> int tls_client_hello_anon(struct socket *sock, tls_done_func_t done,
> -			  void *data, const char *priorities)
> +			  void *data, const char *priorities,
> +			  unsigned int timeout)
> {
> 	struct tls_handshake_req *treq;
> 	struct handshake_req *req;
> @@ -260,6 +270,8 @@ int tls_client_hello_anon(struct socket *sock, tls_done_func_t done,
> 	treq = tls_handshake_req_init(req, done, data, tp);
> 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_CLIENTHELLO;
> 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_UNAUTH;
> +	if (timeout)
> +		treq->th_timeout = timeout;
> 
> 	return handshake_req_submit(req, flags);
> }
> @@ -273,6 +285,7 @@ EXPORT_SYMBOL(tls_client_hello_anon);
>  * @priorities: GnuTLS TLS priorities string
>  * @cert: serial number of key containing client's x.509 certificate
>  * @privkey: serial number of key containing client's private key
> + * @timeout: TLS handshake timeout (in seconds)
>  *
>  * Return values:
>  *   %0: Handshake request enqueue; ->done will be called when complete
> @@ -281,7 +294,8 @@ EXPORT_SYMBOL(tls_client_hello_anon);
>  */
> int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
> 			  void *data, const char *priorities,
> -			  key_serial_t cert, key_serial_t privkey)
> +			  key_serial_t cert, key_serial_t privkey,
> +			  unsigned int timeout)
> {
> 	struct tls_handshake_req *treq;
> 	struct handshake_req *req;
> @@ -303,6 +317,8 @@ int tls_client_hello_x509(struct socket *sock, tls_done_func_t done,
> 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_X509;
> 	treq->th_certificate = cert;
> 	treq->th_privkey = privkey;
> +	if (timeout)
> +		treq->th_timeout = timeout;
> 
> 	return handshake_req_submit(req, flags);
> }
> @@ -315,6 +331,7 @@ EXPORT_SYMBOL(tls_client_hello_x509);
>  * @data: token to pass back to @done
>  * @priorities: GnuTLS TLS priorities string
>  * @peerid: serial number of key containing TLS identity
> + * @timeout: TLS handshake timeout (in seconds)
>  *
>  * Return values:
>  *   %0: Handshake request enqueue; ->done will be called when complete
> @@ -323,7 +340,7 @@ EXPORT_SYMBOL(tls_client_hello_x509);
>  */
> int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
> 			 void *data, const char *priorities,
> -			 key_serial_t peerid)
> +			 key_serial_t peerid, unsigned int timeout)
> {
> 	struct tls_handshake_req *treq;
> 	struct handshake_req *req;
> @@ -344,6 +361,8 @@ int tls_client_hello_psk(struct socket *sock, tls_done_func_t done,
> 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_CLIENTHELLO;
> 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_PSK;
> 	treq->th_peerid = peerid;
> +	if (timeout)
> +		treq->th_timeout = timeout;
> 
> 	return handshake_req_submit(req, flags);
> }
> @@ -355,6 +374,7 @@ EXPORT_SYMBOL(tls_client_hello_psk);
>  * @done: function to call when the handshake has completed
>  * @data: token to pass back to @done
>  * @priorities: GnuTLS TLS priorities string
> + * @timeout: TLS handshake timeout (in seconds)
>  *
>  * Return values:
>  *   %0: Handshake request enqueue; ->done will be called when complete
> @@ -362,7 +382,7 @@ EXPORT_SYMBOL(tls_client_hello_psk);
>  *   %-ENOMEM: Memory allocation failed
>  */
> int tls_server_hello(struct socket *sock, tls_done_func_t done,
> -		     void *data, const char *priorities)
> +		     void *data, const char *priorities, unsigned int timeout)
> {
> 	struct tls_handshake_req *treq;
> 	struct handshake_req *req;
> @@ -382,6 +402,8 @@ int tls_server_hello(struct socket *sock, tls_done_func_t done,
> 	treq = tls_handshake_req_init(req, done, data, tp);
> 	treq->th_type = HANDSHAKE_GENL_TLS_TYPE_SERVERHELLO;
> 	treq->th_auth_type = HANDSHAKE_GENL_TLS_AUTH_UNSPEC;
> +	if (timeout)
> +		treq->th_timeout = timeout;
> 
> 	return handshake_req_submit(req, flags);
> }
> -- 
> 2.35.3
> 

--
Chuck Lever

Re: [PATCH 2/4] tls-handshake: add 'keyring' netlink attribute

[Chuck Lever III]

> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> Add a 'keyring' netlink attribute to the 'request' netlink
> message to allow the kernel to communicate the keyring to use
> to userspace.

Can you add some detail about the use case? I'm not
clear about how the keyring is to be used.

Is this keyring going to be the same for all operations
with this particular tlshd? If that's the case, why not
have tlshd do a GET_KEYRING downcall once when it starts
up?


> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> include/net/tls.h              | 1 +
> include/uapi/linux/handshake.h | 1 +
> net/tls/tls_handshake.c        | 8 ++++++++
> 3 files changed, 10 insertions(+)
> 
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 51bf5a083cce..f4baf3b4b179 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -524,6 +524,7 @@ enum {
> 	TLS_NO_CERT = 0,
> 	TLS_NO_PRIVKEY = 0,
> 	TLS_NO_TIMEOUT = 0,
> +	TLS_NO_KEYRING = 0,
> };
> 
> typedef void	(*tls_done_func_t)(void *data, int status,
> diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
> index b007e346cfc8..83705b1d1e9a 100644
> --- a/include/uapi/linux/handshake.h
> +++ b/include/uapi/linux/handshake.h
> @@ -65,6 +65,7 @@ enum handshake_tls_accept_attrs {
> 	HANDSHAKE_GENL_ATTR_TLS_X509_PRIVKEY,
> 	HANDSHAKE_GENL_ATTR_TLS_PSK,
> 	HANDSHAKE_GENL_ATTR_TLS_TIMEOUT,
> +	HANDSHAKE_GENL_ATTR_TLS_KEYRING,
> 
> 	__HANDSHAKE_GENL_ATTR_TLS_ACCEPT_MAX
> };
> diff --git a/net/tls/tls_handshake.c b/net/tls/tls_handshake.c
> index 66adac8d660a..8171b3c8f3a5 100644
> --- a/net/tls/tls_handshake.c
> +++ b/net/tls/tls_handshake.c
> @@ -43,6 +43,7 @@ struct tls_handshake_req {
> 	int			th_type;
> 	int			th_auth_type;
> 	unsigned int		th_timeout;
> +	key_serial_t		th_keyring;
> 	key_serial_t		th_peerid;
> 	key_serial_t		th_certificate;
> 	key_serial_t		th_privkey;
> @@ -74,6 +75,7 @@ tls_handshake_req_init(struct handshake_req *req, tls_done_func_t done,
> 	treq->th_certificate = TLS_NO_CERT;
> 	treq->th_privkey = TLS_NO_PRIVKEY;
> 	treq->th_timeout = TLS_NO_TIMEOUT;
> +	treq->th_keyring = TLS_NO_KEYRING;
> 	return treq;
> }
> 
> @@ -170,6 +172,12 @@ static int tls_handshake_put_accept_resp(struct sk_buff *msg,
> 		if (ret < 0)
> 			goto out;
> 	}
> +	if (treq->th_keyring != TLS_NO_KEYRING) {
> +		ret = nla_put_u32(msg, HANDSHAKE_GENL_ATTR_TLS_KEYRING,
> +				  treq->th_keyring);
> +		if (ret < 0)
> +			goto out;
> +	}
> 
> 	ret = nla_put_string(msg, HANDSHAKE_GENL_ATTR_TLS_PRIORITIES,
> 			     treq->th_priorities);
> -- 
> 2.35.3
> 

--
Chuck Lever

Re: [PATCH 1/4] tls-handshake: add 'timeout' netlink attribute

[Hannes Reinecke]

On 2/17/23 12:58, Chuck Lever III wrote:
> 
> 
>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> Add a 'timeout' netlink attribute to the 'request' netlink
>> message to allow the kernel to communicate an internal timeout
>> to userspace.
> 
> Can you describe the use caae?
> 
> I'm not clear on why we need to parametrize the handshake
> timeout. Why would it need to be changed? Why would, say,
> a PSK handshake need a different timeout than x.509? And,
> why doesn't a timeout wait in the API consumer work just
> as well?
> 
> Last we left this, you were using the timeout to work
> around some socket reference counting issues. I think we
> need to address those first.
> 
The problem is that we have have a timeout on the kernel side, and 
another timeout on the userland side.
This is primarily an issue for the client, as the kernel code
needs to continue _eventually_, otherwise the connection will
stall.
So we do need a timeout for the client to ensure forward progress.
Hence the kernel client needs a timeout to eventually declare the 
handshake non-functional and either abort or retry without TLS.

But as the kernel now has a timeout, that would need to be transported
to userland to tell the application to stop trying after a certain time.
Which is what this parameter does.

The alternative would be to kill the application directly from the 
kernel (or send some other signal), but that's really nasty and then we 
would need to worry about cleanup effects. Hence I'd rather not.

This approach has the benefit that the kernel can wait slightly longer 
than the timeout sent to userspace (in my code I've added two seconds), 
giving userspace enough time to send a completion message to the kernel.
With that we can be sure that userspace has given up control of the 
socket, and we remain the sole owner.
Without such a message it's always tricky to figure out _who_ has 
control over the socket, leading to all sorts of reference counting issues.

Cheers,

Hannes

Re: [PATCH 2/4] tls-handshake: add 'keyring' netlink attribute

[Hannes Reinecke]

On 2/17/23 13:00, Chuck Lever III wrote:
> 
> 
>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> Add a 'keyring' netlink attribute to the 'request' netlink
>> message to allow the kernel to communicate the keyring to use
>> to userspace.
> 
> Can you add some detail about the use case? I'm not
> clear about how the keyring is to be used.
> 
> Is this keyring going to be the same for all operations
> with this particular tlshd? If that's the case, why not
> have tlshd do a GET_KEYRING downcall once when it starts
> up?
> 
And that's the key (sic!) here:
with this attribute each tlshd instance can have a _different_
keyring.
Idea is to prep the server side with the keyring to use, and then
reflect that choice to the userspace application such that it can
lookup the key in that keyring, too.

(Note to self: need to change the initial keyring to be the process 
keyring, not the session keyring for that to work)

With that we can be sure that both sides (kernel and userspace) will 
have access to the same set of keys.
Idea is to have only _one_ place which need to be configured; in this
case you would need to update the kernel side to allow for individual
keyrings.
Without this you would need to configure tlshd to use the correct keys, 
and it'll be rather tricky to configure different keys for individual 
connections.

Cheers,

Hannes

Re: [PATCH 1/4] tls-handshake: add 'timeout' netlink attribute

[Chuck Lever III]

> On Feb 17, 2023, at 7:17 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 2/17/23 12:58, Chuck Lever III wrote:
>>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> Add a 'timeout' netlink attribute to the 'request' netlink
>>> message to allow the kernel to communicate an internal timeout
>>> to userspace.
>> Can you describe the use caae?
>> I'm not clear on why we need to parametrize the handshake
>> timeout. Why would it need to be changed? Why would, say,
>> a PSK handshake need a different timeout than x.509? And,
>> why doesn't a timeout wait in the API consumer work just
>> as well?
>> Last we left this, you were using the timeout to work
>> around some socket reference counting issues. I think we
>> need to address those first.
> The problem is that we have have a timeout on the kernel side, and another timeout on the userland side.
> This is primarily an issue for the client, as the kernel code
> needs to continue _eventually_, otherwise the connection will
> stall.
> So we do need a timeout for the client to ensure forward progress.
> Hence the kernel client needs a timeout to eventually declare the handshake non-functional and either abort or retry without TLS.
> 
> But as the kernel now has a timeout, that would need to be transported
> to userland to tell the application to stop trying after a certain time.
> Which is what this parameter does.
> 
> The alternative would be to kill the application directly from the kernel (or send some other signal), but that's really nasty and then we would need to worry about cleanup effects. Hence I'd rather not.

Actually IMO this is a cleaner overall approach. The cleanup
here would be the same as if the handshake agent crashed, and
that needs to work properly. It also needs to work if the
user driving the application handshake request is signaled
(system shutting down, user hit ^C, etc).


> This approach has the benefit that the kernel can wait slightly longer than the timeout sent to userspace (in my code I've added two seconds), giving userspace enough time to send a completion message to the kernel.
> With that we can be sure that userspace has given up control of the socket, and we remain the sole owner.
> Without such a message it's always tricky to figure out _who_ has control over the socket, leading to all sorts of reference counting issues.

It's not difficult. Kill the handshake agent, and that closes
the user space fd. The kernel now has control of the socket.


--
Chuck Lever

Re: [PATCH 2/4] tls-handshake: add 'keyring' netlink attribute

[Chuck Lever III]

> On Feb 17, 2023, at 7:24 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 2/17/23 13:00, Chuck Lever III wrote:
>>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> Add a 'keyring' netlink attribute to the 'request' netlink
>>> message to allow the kernel to communicate the keyring to use
>>> to userspace.
>> Can you add some detail about the use case? I'm not
>> clear about how the keyring is to be used.
>> Is this keyring going to be the same for all operations
>> with this particular tlshd? If that's the case, why not
>> have tlshd do a GET_KEYRING downcall once when it starts
>> up?
> And that's the key (sic!) here:
> with this attribute each tlshd instance can have a _different_
> keyring.

What do you mean by "tlshd instance"? Do you mean each tlshd
child process needs its own keyring?


> Idea is to prep the server side with the keyring to use, and then
> reflect that choice to the userspace application such that it can
> lookup the key in that keyring, too.
> 
> (Note to self: need to change the initial keyring to be the process keyring, not the session keyring for that to work)
> 
> With that we can be sure that both sides (kernel and userspace) will have access to the same set of keys.
> Idea is to have only _one_ place which need to be configured; in this
> case you would need to update the kernel side to allow for individual
> keyrings.
> Without this you would need to configure tlshd to use the correct keys, and it'll be rather tricky to configure different keys for individual connections.

I thought the issue was keeping the keys private by setting up
a keyring that is accessible only to the kernel and that tlshd
instance? Did I miss something?

Seems like the tlshd child's process keyring is already set up
this way? I'd like to understand if there's already something
available to fill this role.

--
Chuck Lever

Re: [PATCH 1/4] tls-handshake: add 'timeout' netlink attribute

[Hannes Reinecke]

On 2/17/23 13:26, Chuck Lever III wrote:
> 
> 
>> On Feb 17, 2023, at 7:17 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 2/17/23 12:58, Chuck Lever III wrote:
>>>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>
>>>> Add a 'timeout' netlink attribute to the 'request' netlink
>>>> message to allow the kernel to communicate an internal timeout
>>>> to userspace.
>>> Can you describe the use caae?
>>> I'm not clear on why we need to parametrize the handshake
>>> timeout. Why would it need to be changed? Why would, say,
>>> a PSK handshake need a different timeout than x.509? And,
>>> why doesn't a timeout wait in the API consumer work just
>>> as well?
>>> Last we left this, you were using the timeout to work
>>> around some socket reference counting issues. I think we
>>> need to address those first.
>> The problem is that we have have a timeout on the kernel side, and another timeout on the userland side.
>> This is primarily an issue for the client, as the kernel code
>> needs to continue _eventually_, otherwise the connection will
>> stall.
>> So we do need a timeout for the client to ensure forward progress.
>> Hence the kernel client needs a timeout to eventually declare the handshake
>> non-functional and either abort or retry without TLS.
>>
>> But as the kernel now has a timeout, that would need to be transported
>> to userland to tell the application to stop trying after a certain time.
>> Which is what this parameter does.
>>
>> The alternative would be to kill the application directly from the kernel
>> (or send some other signal), but that's really nasty and then we would need
>> to worry about cleanup effects. Hence I'd rather not.
> 
> Actually IMO this is a cleaner overall approach. The cleanup
> here would be the same as if the handshake agent crashed, and
> that needs to work properly. It also needs to work if the
> user driving the application handshake request is signaled
> (system shutting down, user hit ^C, etc).
> 

True.

>> This approach has the benefit that the kernel can wait slightly longer
>> than the timeout sent to userspace (in my code I've added two seconds),
>> giving userspace enough time to send a completion message to the kernel.
>> With that we can be sure that userspace has given up control of the socket,
>> and we remain the sole owner.
>> Without such a message it's always tricky to figure out _who_ has control
>> over the socket, leading to all sorts of reference counting issues.
> 
> It's not difficult. Kill the handshake agent, and that closes
> the user space fd. The kernel now has control of the socket.
> 
Hmm. Okay, if you say so ... let's see what happens if I do that.
Timeouts are easy to trigger with my setup :-)

Cheers,

Hannes

Re: [PATCH 1/4] tls-handshake: add 'timeout' netlink attribute

[Chuck Lever III]

> On Feb 17, 2023, at 7:36 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 2/17/23 13:26, Chuck Lever III wrote:
>>> On Feb 17, 2023, at 7:17 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> On 2/17/23 12:58, Chuck Lever III wrote:
>>>>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>> 
>>>>> Add a 'timeout' netlink attribute to the 'request' netlink
>>>>> message to allow the kernel to communicate an internal timeout
>>>>> to userspace.
>>>> Can you describe the use caae?
>>>> I'm not clear on why we need to parametrize the handshake
>>>> timeout. Why would it need to be changed? Why would, say,
>>>> a PSK handshake need a different timeout than x.509? And,
>>>> why doesn't a timeout wait in the API consumer work just
>>>> as well?
>>>> Last we left this, you were using the timeout to work
>>>> around some socket reference counting issues. I think we
>>>> need to address those first.
>>> The problem is that we have have a timeout on the kernel side, and another timeout on the userland side.
>>> This is primarily an issue for the client, as the kernel code
>>> needs to continue _eventually_, otherwise the connection will
>>> stall.
>>> So we do need a timeout for the client to ensure forward progress.
>>> Hence the kernel client needs a timeout to eventually declare the handshake
>>> non-functional and either abort or retry without TLS.
>>> 
>>> But as the kernel now has a timeout, that would need to be transported
>>> to userland to tell the application to stop trying after a certain time.
>>> Which is what this parameter does.
>>> 
>>> The alternative would be to kill the application directly from the kernel
>>> (or send some other signal), but that's really nasty and then we would need
>>> to worry about cleanup effects. Hence I'd rather not.
>> Actually IMO this is a cleaner overall approach. The cleanup
>> here would be the same as if the handshake agent crashed, and
>> that needs to work properly. It also needs to work if the
>> user driving the application handshake request is signaled
>> (system shutting down, user hit ^C, etc).
> 
> True.
> 
>>> This approach has the benefit that the kernel can wait slightly longer
>>> than the timeout sent to userspace (in my code I've added two seconds),
>>> giving userspace enough time to send a completion message to the kernel.
>>> With that we can be sure that userspace has given up control of the socket,
>>> and we remain the sole owner.
>>> Without such a message it's always tricky to figure out _who_ has control
>>> over the socket, leading to all sorts of reference counting issues.
>> It's not difficult. Kill the handshake agent, and that closes
>> the user space fd. The kernel now has control of the socket.
> Hmm. Okay, if you say so ... let's see what happens if I do that.
> Timeouts are easy to trigger with my setup :-)

I'm pretty sure this won't work well with v4.

--
Chuck Lever

Re: [PATCH 2/4] tls-handshake: add 'keyring' netlink attribute

[Hannes Reinecke]

On 2/17/23 13:32, Chuck Lever III wrote:
> 
> 
>> On Feb 17, 2023, at 7:24 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 2/17/23 13:00, Chuck Lever III wrote:
>>>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>
>>>> Add a 'keyring' netlink attribute to the 'request' netlink
>>>> message to allow the kernel to communicate the keyring to use
>>>> to userspace.
>>> Can you add some detail about the use case? I'm not
>>> clear about how the keyring is to be used.
>>> Is this keyring going to be the same for all operations
>>> with this particular tlshd? If that's the case, why not
>>> have tlshd do a GET_KEYRING downcall once when it starts
>>> up?
>> And that's the key (sic!) here:
>> with this attribute each tlshd instance can have a _different_
>> keyring.
> 
> What do you mean by "tlshd instance"? Do you mean each tlshd
> child process needs its own keyring?
> 
It doesn't 'need' it. It already has (well, actually there are three 
keyring, one session-wide, one process-wide, and one per thread)
And all this 'keyring' attribute does (or should be doing) is allowing 
userspace to link that keyring into the process keyring.

> 
>> Idea is to prep the server side with the keyring to use, and then
>> reflect that choice to the userspace application such that it can
>> lookup the key in that keyring, too.
>>
>> (Note to self: need to change the initial keyring to be the process
>> keyring, not the session keyring for that to work)
>>
>> With that we can be sure that both sides (kernel and userspace) will
>> have access to the same set of keys.
>> Idea is to have only _one_ place which need to be configured; in this
>> case you would need to update the kernel side to allow for individual
>> keyrings.
>> Without this you would need to configure tlshd to use the correct keys,
>> and it'll be rather tricky to configure different keys for individual connections.
> 
> I thought the issue was keeping the keys private by setting up
> a keyring that is accessible only to the kernel and that tlshd
> instance? Did I miss something?
> 
No, that's precisely it.
By having a keyring attribute we allow for configuration of a keyring 
per connection.

> Seems like the tlshd child's process keyring is already set up
> this way? I'd like to understand if there's already something
> available to fill this role.
> 
The issue is to make the keys _available_ to that keyring.
As it stands, tlshd is using fork() for each handshake.
So we have one master process, and one process per handshake.
The session keyring will be identical for the of these instances.
The process keyring will be different for each of these.
And the per-handshake process needs to be able to lookup keys; these 
keys either must be in one of the default keyrings, linked to one of the 
default keyrings, or have correct permissions.
The last case we've dismissed as we want to restrict the permissions.
The first case (per-session keyrings) is essentially the config option
(which necessarily is identical for all handshake processed).
And this patch now covers the second option, of having _different_ 
keyrings for each handshake process.

Idea is that you configure the in-kernel callers of the handshake to 
provide the correct keyring.

EG for nvme-tcp plan is to pass the keyring with the 'nvme connect' 
call, and configure the keyring via the 'configfs' interface for the 
server side.
If different keyrings should be used. If not stick with the default 
'.tls' keyring.
For which, incidentally, I've got another patch, which I probably should 
send out, too.

Cheers,

Hannes

Re: [PATCH 2/4] tls-handshake: add 'keyring' netlink attribute

[Chuck Lever III]

> On Feb 17, 2023, at 7:48 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 2/17/23 13:32, Chuck Lever III wrote:
>>> On Feb 17, 2023, at 7:24 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> On 2/17/23 13:00, Chuck Lever III wrote:
>>>>> On Feb 17, 2023, at 6:31 AM, Hannes Reinecke <hare@suse.de> wrote:
>>>>> 
>>>>> Add a 'keyring' netlink attribute to the 'request' netlink
>>>>> message to allow the kernel to communicate the keyring to use
>>>>> to userspace.
>>>> Can you add some detail about the use case? I'm not
>>>> clear about how the keyring is to be used.
>>>> Is this keyring going to be the same for all operations
>>>> with this particular tlshd? If that's the case, why not
>>>> have tlshd do a GET_KEYRING downcall once when it starts
>>>> up?
>>> And that's the key (sic!) here:
>>> with this attribute each tlshd instance can have a _different_
>>> keyring.
>> What do you mean by "tlshd instance"? Do you mean each tlshd
>> child process needs its own keyring?
> It doesn't 'need' it. It already has (well, actually there are three keyring, one session-wide, one process-wide, and one per thread)
> And all this 'keyring' attribute does (or should be doing) is allowing userspace to link that keyring into the process keyring.
> 
>>> Idea is to prep the server side with the keyring to use, and then
>>> reflect that choice to the userspace application such that it can
>>> lookup the key in that keyring, too.
>>> 
>>> (Note to self: need to change the initial keyring to be the process
>>> keyring, not the session keyring for that to work)
>>> 
>>> With that we can be sure that both sides (kernel and userspace) will
>>> have access to the same set of keys.
>>> Idea is to have only _one_ place which need to be configured; in this
>>> case you would need to update the kernel side to allow for individual
>>> keyrings.
>>> Without this you would need to configure tlshd to use the correct keys,
>>> and it'll be rather tricky to configure different keys for individual connections.
>> I thought the issue was keeping the keys private by setting up
>> a keyring that is accessible only to the kernel and that tlshd
>> instance? Did I miss something?
> No, that's precisely it.
> By having a keyring attribute we allow for configuration of a keyring per connection.
> 
>> Seems like the tlshd child's process keyring is already set up
>> this way? I'd like to understand if there's already something
>> available to fill this role.
> The issue is to make the keys _available_ to that keyring.
> As it stands, tlshd is using fork() for each handshake.
> So we have one master process, and one process per handshake.
> The session keyring will be identical for the of these instances.
> The process keyring will be different for each of these.
> And the per-handshake process needs to be able to lookup keys; these keys either must be in one of the default keyrings, linked to one of the default keyrings, or have correct permissions.
> The last case we've dismissed as we want to restrict the permissions.
> The first case (per-session keyrings) is essentially the config option
> (which necessarily is identical for all handshake processed).
> And this patch now covers the second option, of having _different_ keyrings for each handshake process.
> 
> Idea is that you configure the in-kernel callers of the handshake to provide the correct keyring.

I'm warming to the idea of having a private keyring per handshake.
Thanks for explaining.


> EG for nvme-tcp plan is to pass the keyring with the 'nvme connect' call, and configure the keyring via the 'configfs' interface for the server side.
> If different keyrings should be used. If not stick with the default '.tls' keyring.
> For which, incidentally, I've got another patch, which I probably should send out, too.
> 
> Cheers,
> 
> Hannes

--
Chuck Lever