[nmbro]

Two client-side TLS bugs, found while testing upsmon with SSL on Windows (both are platform-independent and reproduce on Linux with current master).

Problem

1. upsmon ignored the global CERTPATH, CERTFILE, CERTIDENT, CERTVERIFY and FORCESSL from upsmon.conf when there is no AUTHCONF line and no nutauth.conf on the system. So the CA was never loaded and FORCESSL/CERTVERIFY had no effect. Two causes in clients/authconf.c:
   • upsmon keeps these settings in the entry it gets with upscli_get_authconf_item(NULL, NULL, NULL, 1). With an empty list, that call did not create the global section but a @localhost:3493 item (renamed by upsmon to a NULL section), which is not known as global defaults. So the items created later for the monitored hosts did not inherit from it.
   • upsmon then calls the best-effort upscli_read_authconf_file(NULL, 0, 1), which freed the whole list before looking for a file. With no nutauth.conf, the parsed settings were discarded (and upsmon kept a pointer to the freed entry).
2. A CERTPATH naming a PEM file (OpenSSL) was never loaded, although upsmon.conf(5) documents that form (e.g. CERTPATH /etc/ssl/certs/ca-certificates.crt). upscli_get_or_create_ssl_context() tried SSL_CTX_load_verify_locations() with CERTPATH as a directory first and as a file only if that failed. But loading a directory (CApath) succeeds for any string, because OpenSSL only looks into it during verification, so the file was never tried, and verification failed even with the right CA in the file.

Changes

1. clients/authconf.c: keep upsmon.conf CERT* defaults

   • upscli_get_authconf_item(): with all-NULL arguments, add_to_list set and no global section yet, create (and add) the global section, as the header documents for all-NULL arguments. Host items made later are cloned from it, as the code already does for global defaults read from a file.
   • upscli_read_authconf_file(): free the current list only once a file was found to be read; if none is found, the list is kept.

2. clients/upsclient.c: load a CERTPATH file as a CA bundle

   • Try CERTPATH as a file (CAfile) first, then as a directory (CApath). A CAfile load of a directory fails cleanly, so directories work as before. This also avoids stat() (which e.g. rejects a trailing slash on Windows). The NSS code path is not changed.

3. clients/upsmon.c: re-fetch default authconf entry

   • After reading a nutauth.conf file, upsmon fetches its default (global) entry again, creating it if the file had no global section. Before, its pointer to that entry was left dangling once a file replaced the list, so a reload with CERT* keywords in upsmon.conf wrote into freed memory.
   • authconf.h: clarify that an explicitly named file always replaces the list.

Not changed here: reading a nutauth.conf file still replaces the list, so the upsmon.conf globals and CERTHOST entries are dropped. UPGRADING.adoc and upsmon.conf(5) describe a merge instead (legacy keywords as the global defaults, the file loaded on top), so the code is what needs fixing. As discussed below, that goes into a follow-up with a mode option for upscli_read_authconf_file().

Testing

upsmon -DDDDD with a upsmon.conf holding only MONITOR, CERTPATH, CERTVERIFY 1 and FORCESSL 1 (no AUTHCONF, no nutauth.conf), on Windows builds under Wine (i686 and x86_64 MSVCRT, x86_64 UCRT):

• before: the Collected AUTHCONF entries: dump is empty, and the entry for the monitored host is created empty;
• after: the dump shows CERTPATH, CERTVERIFY = 1, FORCESSL = 1, and the host entry is cloned from these global defaults.

upsc -A nutauth.conf (CERTVERIFY = 1, FORCESSL = 1) against a minimal TLS test server (answers STARTTLS, then TLS with a certificate signed by a test CA), with CERTPATH set to the right or a wrong CA, as a PEM file or as a hashed directory:

Build	file, right CA	file, wrong CA	dir, right CA	dir, wrong CA
Linux, master	fails verify	refused	verified	refused
Linux, this PR	verified	refused	verified	refused
Windows (Wine), before	fails verify	refused	verified	refused
Windows (Wine), this PR	verified	refused	verified	refused

Both Linux builds use --with-openssl --enable-Werror. The first commit was also tested end to end on Windows test VMs with a real upsd (directory CERTPATH: right CA verifies, wrong CA refused). Not tested with NSS builds (that code path is unchanged).

The third commit was tested on Linux (Debian, --with-openssl --enable-Werror, AddressSanitizer): upsmon -F -DDDDDD with CERTPATH, CERTVERIFY 1 and FORCESSL 1 in upsmon.conf and a nutauth.conf found in the config directory, then upsmon -c reload. Before: ASan reports a heap-use-after-free in parse_conf_arg() (from reload_conf()). After: the reload completes with no ASan report.

Checklist

• Described the changes in the PR submission
• Multiple commits, each with a functional change
• Use of coding helper tools and AI disclosed: Claude Code was used for development assistance (diagnosis, changes and tests); the commits carry its Co-Authored-By trailer
• Revised for NUT code style; ASCII only
• Errors and progress via upsdebugx()/upslogx()
• Coding style follows the files
• NEWS.adoc bullet (NUT client libraries section)

🤖 Generated with Claude Code

https://claude.ai/code/session_01Ju3WEqpswgy9dN7EQB3Fjn

[github-actions]

A ZIP file with standard source tarball and another tarball with pre-built docs for commit 6a204c6 is temporarily available: NUT-tarballs-PR-3797.zip.

[jimklimov]

Good points, sorry for overlooking that - in #3607 I suppose. Luckily this regression (as well as whole nutauth.conf feature) was not published in any numbered release yet :D

Regarding the question of which side wins (AUTHCONF or legacy keywords), I think the minimal simple and correct solution which can go into this PR is to document that this depends on ordering:

• An AUTHCONF keyword in upsmon.conf resets the security part of configuration, starting it from scratch
• Legacy keyword lines seen after any AUTHCONF processing (if any) would merge into the run-time configuration, replacing data from nutauth.conf in case of conflicts (assuming/hoping that's what happens now - right?)

Any improvements to this logic, such as not-removing the existing list made from legacy keywords and merging whatever is explicitly spelled in a nutauth.conf file into those existing list items, including the global default section, would be probably welcome before the release but can be evaluated as a separate PR (along with documented behavior change).

While at it, can you/Claude please double-check (second set of eyes is always better):

• ....That the description above fits reality?
• ...Whether the problem also impacted MONITOR keywords, or do we still store their data points outside authconf-based list?
• ...That CERTFILE/CERTPATH lines now adjust (create only if missing) specifically the default "section" which user@host:port sections would later instantiate from (and that such instantiation DOES happen later, not during configuration parsing so a later change to these lines in the default section would not impact actual connection attempts)?
• ...That CERTHOST lines define or update existing (from nutauth.conf) template entries for the host(:port?) in the list and eventually the host_cert_list?

[jimklimov]

In fact, I think in that follow-up PR it may make sense to extend upscli_read_authconf_file() with an option whether this read always starts from a clean slate (two values, for "including the case of missing nutauth.conf files" and "only if there was a file") or (third value) updates whatever was parsed or otherwise learned before.

Then the upscli_free_authconf_list() check before (removed in this PR) or after (added in this PR) the file presence check would depend on this new option, adding some flexibility for NUT client programs. I think the majority would want old behavior as default (clear before checking) as they lacked options to populate any other setting anyway, and upsmon specifically would use the option to update whatever existed.

This may avalanche into changes across the ecosystem however (similar in C++/Perl/Python/Java bindings, maybe update the API consumer in WmNUT for new method parameter, API man pages...)

[nmbro]

Thanks! I double-checked against the code:

• Description: correct, except for one detail. For an explicit AUTHCONF <path>, the list is still freed before parsing; only the search-path case checks for the file first. In upsmon that case uses fatal_errors=1, so it exits anyway. I'll fix the header comment.
• Ordering: it doesn't work that way now. The AUTHCONF line only records the path, and the file is read after all of upsmon.conf is parsed, wherever the line was. If a file is found, it frees the whole list, so legacy globals and CERTHOST items are dropped whatever the line order, and ac_default is left dangling. A SIGHUP reload with a CERTPATH line then writes into freed memory.
  That wiping is the bug, not the docs: UPGRADING.adoc (NUT SSL clients can only use one identity and one trust store per process #3439) says the legacy keywords "act as a 'global defaults' section" and the file "gets loaded on top of these data", upsmon.conf(5) calls them "the preferred global default settings", and the comment right before the read in loadconfig() says loading a file "would at best populate any missing points". So the intended behavior is a merge, with legacy values winning over the file's global directives, and per-host file sections overriding the defaults as usual.
  For this PR I'd add only a one-liner that re-fetches ac_default = upscli_get_authconf_item(NULL, NULL, NULL, 1) after the read, which fixes the UAF. The merge itself fits your follow-up with the upscli_read_authconf_file() mode option: in "update" mode, don't free the list, and have the file's global directives (authconf.c around line 956, now an unconditional set_authconf_val(global_defaults, ...)) fill only NULL/-1 fields.
• MONITOR: not affected. Credentials stay in utype_t, and try_connect() looks up with user=NULL, so only @host:port and global sections apply. FWIW, that means a [user@host] section in nutauth.conf is never matched by upsmon.
• CERT*: yes, these only touch the global section. Items for MONITOR hosts are cloned lazily on the first try_connect(), after parsing. Later lookups merge globals only into NULL/-1 fields, so changes after the first connect (e.g. on reload) don't override values already cloned.
• CERTHOST: creates the @host:port item, or edits it in place, cloned from the globals as they are at that point; a CERTPATH that comes later is still merged in at connect time. It can't update items from nutauth.conf, because the file is read afterwards and wipes them when found. Also, host_cert_list keeps the first entry per host:port and isn't cleared with the authconf list, so a CERTHOST from upsmon.conf beats the file's certhost there. I'd add that to the follow-up as well.

Edit: corrected the "Ordering" point. I first suggested documenting that the file replaces the legacy settings, but the existing docs describe a merge, so the code is what needs fixing.

This reply was drafted with Claude Code (Opus 5.5) and reviewed by me.

[AppVeyorBot]

✅ Build nut 2.8.5.5906-master completed (commit ea7f04b3b4 by @nmbro)

• artifacts
• A 7-zip archive with built NUT for Windows debug binaries is temporarily available: NUT-for-Windows-x86_64-SNAPSHOT-2.8.5.5906-master.7z
• ccache: 388 of 410 compilations were cache hits (94.6%); cache size 750 of 750 MiB after cleanup