upsmon, libupsclient: keep upsmon.conf CERT* defaults, load CERTPATH files

NEWS.adoc

@@ -762,6 +762,13 @@ https://github.com/networkupstools/nut/milestone/13
       codebase layout in the library, and this in turn caused some fallout.
       Fixed `libupsclient` builds with OpenSSL or NSS when Clang treats
       declarations after executable statements as errors. [issue #3761, PR #3762]
+    * Fixed `upsmon` losing the global `CERTPATH`, `CERTFILE`, `CERTIDENT`,
+      `CERTVERIFY` and `FORCESSL` settings from `upsmon.conf` when no
+      `nutauth.conf` file was found, and OpenSSL builds of `libupsclient`
+      never loading a `CERTPATH` which names a PEM file with CA certificates
+      (only a directory worked). Also fixed `upsmon` writing into freed
+      memory when it reloaded `upsmon.conf` with such settings after a
+      `nutauth.conf` file was read. [PR #3797]
     * The Perl `UPS::Nut` module now decodes quoted values and backslash
       escapes in query/list replies and `nutauth.conf` entries, and quotes
       authentication arguments. Single quotes in configuration are literal.

clients/authconf.c

@@ -1026,9 +1026,6 @@ int upscli_read_authconf_file(const char *filename, int fatal_errors, int debug_
 {
 	char	fn[NUT_PATH_MAX + 1];
 
-	/* Ensure we start fresh if called multiple times */
-	upscli_free_authconf_list();
-
 	if (!filename) {
 		/* Select a starting point - whichever default expected file exists;
 		 * it may INCLUDE further files as wanted by user or site sysadmin.
@@ -1105,6 +1102,12 @@ int upscli_read_authconf_file(const char *filename, int fatal_errors, int debug_
 		}
 	}
 
+	/* Ensure we start fresh if called multiple times; but only once
+	 * there is a file to read: callers like upsmon may have populated
+	 * the list (e.g. global defaults from legacy CERT* keywords) before
+	 * a best-effort attempt which finds no file */
+	upscli_free_authconf_list();
+
 	return parse_authconf_file(filename, fatal_errors, 1);
 }
 
@@ -1212,6 +1215,15 @@ upscli_authconf_t *upscli_get_authconf_item(const char *user, const char *host,
 
 	upsdebugx(2, "%s: starting for [%s]@[%s]:[%s]", __func__, NUT_STRARG(user), NUT_STRARG(host), NUT_STRARG(port));
 
+	if (!host && !port && !user && !global_defaults && add_to_list) {
+		/* Global section requested before one was read from a file,
+		 * e.g. by upsmon for legacy CERT* keywords in upsmon.conf:
+		 * create it, so host-specific items inherit its settings */
+		upsdebugx(3, "%s: create new item for global defaults", __func__);
+		global_defaults = upscli_add_authconf_item(NULL);
+		return global_defaults;
+	}
+
 	/* We want this parsed always, so we can know if there
 	 * is a fixed user, or assign the section name, at least */
 	if (upscli_normalize_authconf_section_parts(

clients/authconf.h

@@ -59,6 +59,9 @@ void upscli_free_authconf_list(void);
  * (whichever is found first); then one can follow `INCLUDE` trail if needed.
  * Ultimate error, if file was not found, is posted at specified debug_level>=0
  * or logged as LOG_WARNING if negative.
+ * The current list of authentication configurations is replaced by the
+ * file contents. With a NULL filename, it is replaced only if a default
+ * file was found; otherwise it is kept as it was.
  * Returns -1 on error, 1 on success.
  */
 int upscli_read_authconf_file(const char *filename, int fatal_errors, int debug_level);
@@ -99,7 +102,8 @@ upscli_authconf_t *upscli_find_authconf_item(const char *user, const char *host,
  * not on the list (can adapt to changes in higher levels but must be freed by
  * caller), or will be edited on or added to the list (subsequent calls would
  * likely not add anything new, but memory management is easier, data is cached).
- * if all args are NULL, return the global section or NULL if none such in the list.
+ * if all args are NULL, return the global section or NULL if none such in the list
+ * (with `add_to_list`, an empty global section is created and added if missing).
  */
 upscli_authconf_t *upscli_get_authconf_item(const char *user, const char *host, const char *port, int add_to_list);
 

clients/upsclient.c

@@ -1187,22 +1187,25 @@ void *upscli_get_or_create_ssl_context(int certverify, const char *certpath,
 			break;
 		}
 
-		ret = SSL_CTX_load_verify_locations(cfg->ssl_ctx, NULL, certpath);
+		/* Try a specific PEM file first: loading a directory (CApath)
+		 * succeeds for any string, as its lookups only happen later
+		 * during verification, so a file would never be tried after it */
+		ret = SSL_CTX_load_verify_locations(cfg->ssl_ctx, certpath, NULL);
 		if (ret != 1) {
 			ssl_debug();
-			upsdebugx(1, "%s: Failed to load CA certificate(s) from directory %s", __func__, certpath);
+			upsdebugx(1, "%s: Failed to load CA certificate(s) from file %s", __func__, certpath);
 
-			/* Can it be a specific PEM file? */
-			if ((ret = SSL_CTX_load_verify_locations(cfg->ssl_ctx, certpath, NULL)) != 1) {
+			/* Can it be a directory with (hashed) CA certificate files? */
+			if ((ret = SSL_CTX_load_verify_locations(cfg->ssl_ctx, NULL, certpath)) != 1) {
 				ssl_debug();
-				upslogx(LOG_ERR, "Failed to load CA certificate(s) from directory or file %s", certpath);
+				upslogx(LOG_ERR, "Failed to load CA certificate(s) from file or directory %s", certpath);
 				upscli_ssl_context_config_free(cfg);
 				return NULL;
 			} else {
-				upsdebugx(1, "%s: ...but succeeded to load CA certificate(s) from file %s", __func__, certpath);
+				upsdebugx(1, "%s: ...but succeeded to load CA certificate(s) from directory %s", __func__, certpath);
 			}
 		} else {
-			upsdebugx(1, "%s: Succeeded to load CA certificate(s) from directory %s", __func__, certpath);
+			upsdebugx(1, "%s: Succeeded to load CA certificate(s) from file %s", __func__, certpath);
 		}
 
 		/* Adapted from https://linux.die.net/man/3/ssl_set_verify man page example */

clients/upsmon.c

@@ -2829,6 +2829,16 @@ static void loadconfig(void)
 			upsdebugx(1, "Using best-effort auth config detection");
 			upscli_read_authconf_file(NULL, 0, 1);
 		}
+
+		/* A file that was read replaced the list, including the
+		 * global section which the legacy keywords had populated
+		 * (FIXME: merge the file into it instead, as documented):
+		 * point to the current one (or create it if the file had
+		 * none), so a reload does not edit a freed entry */
+		ac_default = upscli_get_authconf_item(NULL, NULL, NULL, 1);
+		if (!ac_default) {
+			fatalx(EXIT_FAILURE, "Fatal error: unable to get default authconf entry");
+		}
 	}
 
 	/* FIXME: Per legacy behavior, we silently went on.