[PATCH 0/5] Kick off ktls-utils 1.2 development

[PATCH 0/5] Kick off ktls-utils 1.2 development

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

This is a small series collects a few recently discussed code
changes and other clean-ups. These have been pushed to
oracle/ktls-utils in a ktls-utils-1.2-dev branch.

By the way, I recently enabled CodeQL on chucklever/ktls-utils and
that's where patch 3/5 comes from. If anyone knows of sensible
static analysis that can be easily enabled via GitHub Action, let me
know and I can try to set that up.

Chuck Lever (5):
  tlshd: Fix a minor race
  tlshd: Remove unneeded variable "error"
  workflows: Limit permission of the makefile.yml action
  tlshd: Add default keyrings for NFS
  tlshd: Relocate TLSHD_ALLPERMS

 .github/workflows/makefile.yml |  1 +
 src/tlshd/config.c             | 93 +++++++++++++---------------------
 src/tlshd/tlshd.conf.man       |  8 ++-
 3 files changed, 42 insertions(+), 60 deletions(-)

-- 
2.49.0

[PATCH 1/5] tlshd: Fix a minor race

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Parfait complains about using a pathname to perform an access(2)
and then passing the same pathname to open(2). Between the access(2)
and the open(2) calls, the permissions can change. I think this is
harmless for tlshd, but all the same, let's clean this up.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/tlshd/config.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/src/tlshd/config.c b/src/tlshd/config.c
index be5d472c466a..f4907ebb1a54 100644
--- a/src/tlshd/config.c
+++ b/src/tlshd/config.c
@@ -140,7 +140,11 @@ static bool tlshd_config_read_datum(const char *pathname, gnutls_datum_t *data,
 
 	fd = open(pathname, O_RDONLY);
 	if (fd == -1) {
-		tlshd_log_perror("open");
+		if (access(pathname, F_OK))
+			tlshd_log_debug("tlshd cannot access \"%s\"",
+					pathname);
+		else
+			tlshd_log_perror("open");
 		goto out;
 	}
 	if (fstat(fd, &statbuf)) {
@@ -198,7 +202,7 @@ bool tlshd_config_get_client_truststore(char **bundle)
 		g_error_free(error);
 		return false;
 	} else if (access(pathname, F_OK)) {
-		tlshd_log_debug("client x509.truststore pathname \"%s\" is not accessible", pathname);
+		tlshd_log_debug("tlshd cannot access \"%s\"", pathname);
 		g_free(pathname);
 		return false;
 	}
@@ -234,10 +238,6 @@ bool tlshd_config_get_client_certs(gnutls_pcert_st *certs,
 	if (!pathname) {
 		g_error_free(error);
 		return false;
-	} else if (access(pathname, F_OK)) {
-		tlshd_log_debug("client x509.certificate pathname \"%s\" is not accessible", pathname);
-		g_free(pathname);
-		return false;
 	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
@@ -282,10 +282,6 @@ bool tlshd_config_get_client_privkey(gnutls_privkey_t *privkey)
 	if (!pathname) {
 		g_error_free(error);
 		return false;
-	} else if (access(pathname, F_OK)) {
-		tlshd_log_debug("client x509.private_key pathname \"%s\" is not accessible", pathname);
-		g_free(pathname);
-		return false;
 	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
@@ -336,7 +332,7 @@ bool tlshd_config_get_server_truststore(char **bundle)
 		g_error_free(error);
 		return false;
 	} else if (access(pathname, F_OK)) {
-		tlshd_log_debug("server x509.truststore pathname \"%s\" is not accessible", pathname);
+		tlshd_log_debug("tlshd cannot access \"%s\"", pathname);
 		g_free(pathname);
 		return false;
 	}
@@ -372,10 +368,6 @@ bool tlshd_config_get_server_certs(gnutls_pcert_st *certs,
 	if (!pathname) {
 		g_error_free(error);
 		return false;
-	} else if (access(pathname, F_OK)) {
-		tlshd_log_debug("server x509.certificate pathname \"%s\" is not accessible", pathname);
-		g_free(pathname);
-		return false;
 	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
@@ -420,10 +412,6 @@ bool tlshd_config_get_server_privkey(gnutls_privkey_t *privkey)
 	if (!pathname) {
 		g_error_free(error);
 		return false;
-	} else if (access(pathname, F_OK)) {
-		tlshd_log_debug("server x509.privkey pathname \"%s\" is not accessible", pathname);
-		g_free(pathname);
-		return false;
 	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
-- 
2.49.0

[PATCH 2/5] tlshd: Remove unneeded variable "error"

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up: Commit c380357cf8f1 ("tlshd: Demote warnings about missing
options") removed some debug messages that used the local "error"
variable. "error" is passed to glib, but the returned value is no
longer checked or used.

Fixes: c380357cf8f1 ("tlshd: Demote warnings about missing options")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/tlshd/config.c | 44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/src/tlshd/config.c b/src/tlshd/config.c
index f4907ebb1a54..e050d3df6050 100644
--- a/src/tlshd/config.c
+++ b/src/tlshd/config.c
@@ -193,15 +193,13 @@ out:
  */
 bool tlshd_config_get_client_truststore(char **bundle)
 {
-	GError *error = NULL;
 	gchar *pathname;
 
 	pathname = g_key_file_get_string(tlshd_configuration, "authenticate.client",
-					 "x509.truststore", &error);
-	if (!pathname) {
-		g_error_free(error);
+					 "x509.truststore", NULL);
+	if (!pathname)
 		return false;
-	} else if (access(pathname, F_OK)) {
+	if (access(pathname, F_OK)) {
 		tlshd_log_debug("tlshd cannot access \"%s\"", pathname);
 		g_free(pathname);
 		return false;
@@ -228,17 +226,14 @@ bool tlshd_config_get_client_truststore(char **bundle)
 bool tlshd_config_get_client_certs(gnutls_pcert_st *certs,
 				   unsigned int *certs_len)
 {
-	GError *error = NULL;
 	gnutls_datum_t data;
 	gchar *pathname;
 	int ret;
 
 	pathname = g_key_file_get_string(tlshd_configuration, "authenticate.client",
-					"x509.certificate", &error);
-	if (!pathname) {
-		g_error_free(error);
+					"x509.certificate", NULL);
+	if (!pathname)
 		return false;
-	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
 				     TLSHD_CERT_MODE)) {
@@ -272,17 +267,14 @@ bool tlshd_config_get_client_certs(gnutls_pcert_st *certs,
  */
 bool tlshd_config_get_client_privkey(gnutls_privkey_t *privkey)
 {
-	GError *error = NULL;
 	gnutls_datum_t data;
 	gchar *pathname;
 	int ret;
 
 	pathname = g_key_file_get_string(tlshd_configuration, "authenticate.client",
-					"x509.private_key", &error);
-	if (!pathname) {
-		g_error_free(error);
+					"x509.private_key", NULL);
+	if (!pathname)
 		return false;
-	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
 				     TLSHD_PRIVKEY_MODE)) {
@@ -323,15 +315,13 @@ bool tlshd_config_get_client_privkey(gnutls_privkey_t *privkey)
  */
 bool tlshd_config_get_server_truststore(char **bundle)
 {
-	GError *error = NULL;
 	gchar *pathname;
 
 	pathname = g_key_file_get_string(tlshd_configuration, "authenticate.server",
-					 "x509.truststore", &error);
-	if (!pathname) {
-		g_error_free(error);
+					 "x509.truststore", NULL);
+	if (!pathname)
 		return false;
-	} else if (access(pathname, F_OK)) {
+	if (access(pathname, F_OK)) {
 		tlshd_log_debug("tlshd cannot access \"%s\"", pathname);
 		g_free(pathname);
 		return false;
@@ -358,17 +348,14 @@ bool tlshd_config_get_server_truststore(char **bundle)
 bool tlshd_config_get_server_certs(gnutls_pcert_st *certs,
 				   unsigned int *certs_len)
 {
-	GError *error = NULL;
 	gnutls_datum_t data;
 	gchar *pathname;
 	int ret;
 
 	pathname = g_key_file_get_string(tlshd_configuration, "authenticate.server",
-					"x509.certificate", &error);
-	if (!pathname) {
-		g_error_free(error);
+					"x509.certificate", NULL);
+	if (!pathname)
 		return false;
-	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
 				     TLSHD_CERT_MODE)) {
@@ -402,17 +389,14 @@ bool tlshd_config_get_server_certs(gnutls_pcert_st *certs,
  */
 bool tlshd_config_get_server_privkey(gnutls_privkey_t *privkey)
 {
-	GError *error = NULL;
 	gnutls_datum_t data;
 	gchar *pathname;
 	int ret;
 
 	pathname = g_key_file_get_string(tlshd_configuration, "authenticate.server",
-					"x509.private_key", &error);
-	if (!pathname) {
-		g_error_free(error);
+					"x509.private_key", NULL);
+	if (!pathname)
 		return false;
-	}
 
 	if (!tlshd_config_read_datum(pathname, &data, TLSHD_OWNER,
 				     TLSHD_PRIVKEY_MODE)) {
-- 
2.49.0

[PATCH 3/5] workflows: Limit permission of the makefile.yml action

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Suggested by CodeQL.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 .github/workflows/makefile.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.github/workflows/makefile.yml b/.github/workflows/makefile.yml
index 5d16bd9193c9..3a0ee4faeba0 100644
--- a/.github/workflows/makefile.yml
+++ b/.github/workflows/makefile.yml
@@ -6,6 +6,7 @@ jobs:
   build:
 
     runs-on: ubuntu-latest
+    permissions: read-all
     strategy:
       fail-fast: false
       matrix:
-- 
2.49.0

[PATCH 4/5] tlshd: Add default keyrings for NFS

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

The NFS mount command is to add keys to the .nfs keyring. Also add a
keyring for NFSD configuration.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/tlshd/config.c       | 11 +++++++----
 src/tlshd/tlshd.conf.man |  8 +++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/tlshd/config.c b/src/tlshd/config.c
index e050d3df6050..b41051e40c08 100644
--- a/src/tlshd/config.c
+++ b/src/tlshd/config.c
@@ -99,15 +99,18 @@ bool tlshd_config_init(const gchar *pathname)
 		for (i = 0; i < length; i++) {
 			if (!strcmp(keyrings[i], ".nvme"))
 				continue;
+			if (!strcmp(keyrings[i], ".nfs"))
+				continue;
+			if (!strcmp(keyrings[i], ".nfsd"))
+				continue;
 			tlshd_keyring_link_session(keyrings[i]);
 		}
 		g_strfreev(keyrings);
 	}
-	/*
-	 * Always link the default nvme subsystem keyring into the
-	 * session.
-	 */
+	/* The ".nvme", ".nfs", and ".nfsd" keyrings cannot be disabled. */
 	tlshd_keyring_link_session(".nvme");
+	tlshd_keyring_link_session(".nfs");
+	tlshd_keyring_link_session(".nfsd");
 
 	return true;
 }
diff --git a/src/tlshd/tlshd.conf.man b/src/tlshd/tlshd.conf.man
index 9d6d92f521ca..abb2f9917467 100644
--- a/src/tlshd/tlshd.conf.man
+++ b/src/tlshd/tlshd.conf.man
@@ -79,7 +79,13 @@ that contain handshake authentication tokens.
 .B tlshd
 links these keyrings into its session keyring.
 The configuration file may specify either a keyring's name or serial number.
-The default is to provide no keyring.
+.B tlshd
+always includes the
+.IR .nvme ,
+.IR .nfs ,
+and
+.I .nfsd
+keyrings on its session keyring.
 .P
 And, in this section, there are two subsections:
 .I [client]
-- 
2.49.0

[PATCH 5/5] tlshd: Relocate TLSHD_ALLPERMS

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up: Move it next to the other related macro definitions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 src/tlshd/config.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/tlshd/config.c b/src/tlshd/config.c
index b41051e40c08..df26663d3a69 100644
--- a/src/tlshd/config.c
+++ b/src/tlshd/config.c
@@ -46,12 +46,6 @@
 
 static GKeyFile *tlshd_configuration;
 
-/**
- * ALLPERMS exists in glibc, but not on musl, so we
- * manually define TLSHD_ACCESSPERMS instead of using ALLPERMS.
- */
-#define TLSHD_ACCESSPERMS	(S_IRWXU|S_IRWXG|S_IRWXO)
-
 /**
  * tlshd_config_init - Read tlshd's config file
  * @pathname: Pathname to config file
@@ -120,6 +114,12 @@ void tlshd_config_shutdown(void)
 	g_key_file_free(tlshd_configuration);
 }
 
+/**
+ * ALLPERMS exists in glibc, but not on musl, so we manually
+ * define TLSHD_ACCESSPERMS instead of using ALLPERMS.
+ */
+#define TLSHD_ACCESSPERMS	(S_IRWXU|S_IRWXG|S_IRWXO)
+
 /*
  * Expected file attributes
  */
-- 
2.49.0