Обсуждение: Extended test coverage and docs for SSL passphrase commands
When I was writing tests for the SSL SNI patch [0] I realized that the current tests for ssl passphrase commands aren't fully exercising the feature, so I extended them to better understand how it works. Attached is an extended set of tests for passphrase protected keys where connection and reloads are tested as well as their different characteristics on Windows. The patchset also contains a small doc addition which documents the fact that passphrase command reloading must be on when running on Windows (EXEC_BACKEND) since every backend will issue a SSL configuration reload. -- Daniel Gustafsson
Вложения
On 07.11.25 21:26, Daniel Gustafsson wrote: > When I was writing tests for the SSL SNI patch [0] I realized that the current > tests for ssl passphrase commands aren't fully exercising the feature, so I > extended them to better understand how it works. Attached is an extended set > of tests for passphrase protected keys where connection and reloads are tested > as well as their different characteristics on Windows. > > The patchset also contains a small doc addition which documents the fact that > passphrase command reloading must be on when running on Windows (EXEC_BACKEND) > since every backend will issue a SSL configuration reload. Your test code conflates $windows_os with EXEC_BACKEND. It should work to enable EXEC_BACKEND on a non-Windows system and have everything work. So I think that code needs to extract the actual EXEC_BACKEND setting somehow, instead of using the OS identity as a proxy. About the behavior that your documentation patch describes, I would like to have some kind of reflection of that in the code as well. At least a comment near default_openssl_tls_init() maybe? I haven't traced the code through, but I would be curious about what is different in an EXEC_BACKEND environment. For example, is the argument isServerStart also true if it's not a server start? Or should the setting actually be enforced directly on the GUC system?
> On 12 Nov 2025, at 15:15, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 07.11.25 21:26, Daniel Gustafsson wrote:
>> When I was writing tests for the SSL SNI patch [0] I realized that the current
>> tests for ssl passphrase commands aren't fully exercising the feature, so I
>> extended them to better understand how it works. Attached is an extended set
>> of tests for passphrase protected keys where connection and reloads are tested
>> as well as their different characteristics on Windows.
>> The patchset also contains a small doc addition which documents the fact that
>> passphrase command reloading must be on when running on Windows (EXEC_BACKEND)
>> since every backend will issue a SSL configuration reload.
>
> Your test code conflates $windows_os with EXEC_BACKEND. It should work to enable EXEC_BACKEND on a non-Windows
systemand have everything work. So I think that code needs to extract the actual EXEC_BACKEND setting somehow, instead
ofusing the OS identity as a proxy.
As far as I know the only way to programmatically learn that from the Perl
testcode would be to check for the presence of the CONFIG_EXEC_PARAMS file in
$self->data_dir, which should be easy enough to do. Do you know of a better
way?
> About the behavior that your documentation patch describes, I would like to have some kind of reflection of that in
thecode as well. At least a comment near default_openssl_tls_init() maybe? I haven't traced the code through, but I
wouldbe curious about what is different in an EXEC_BACKEND environment. For example, is the argument isServerStart
alsotrue if it's not a server start? Or should the setting actually be enforced directly on the GUC system?
It is documented in src/backend/tcop/backend_startup.c with the following
comment in BackendMain():
#ifdef EXEC_BACKEND
/*
* Need to reinitialize the SSL library in the backend, since the context
* structures contain function pointers and cannot be passed through the
* parameter file.
*
* If for some reason reload fails (maybe the user installed broken key
* files), soldier on without SSL; that's better than all connections
* becoming impossible.
*
* XXX should we do this in all child processes? For the moment it's
* enough to do it in backend children.
*/
#ifdef USE_SSL
if (EnableSSL)
{
if (secure_initialize(false) == 0)
LoadedSSL = true;
Calling secure_initialize with isServerStart == false will force a reload which
in turn requires the passphrase command to be reloadable if it is to work at
all.
Not sure if we need too much more than that, but maybe a note could be added to
be_tls_init that isServerStart will reflect config reloads as well as
EXEC_BACKEND?
--
Daniel Gustafsson
On 2025-Nov-12, Daniel Gustafsson wrote: > As far as I know the only way to programmatically learn that from the Perl > testcode would be to check for the presence of the CONFIG_EXEC_PARAMS file in > $self->data_dir, which should be easy enough to do. Do you know of a better > way? We have check_pg_config(), which reads pg_config.h. For EXEC_BACKEND you need pg_config_manual.h, but I think you could go roughly in the same direction ... for instance you could add an argument to check_pg_config() to say which file to read -- though I think the easiest is to read both files always and return the grep count in both. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Officer Krupke, what are we to do? Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")
> On 12 Nov 2025, at 18:47, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2025-Nov-12, Daniel Gustafsson wrote: > >> As far as I know the only way to programmatically learn that from the Perl >> testcode would be to check for the presence of the CONFIG_EXEC_PARAMS file in >> $self->data_dir, which should be easy enough to do. Do you know of a better >> way? > > We have check_pg_config(), which reads pg_config.h. For EXEC_BACKEND > you need pg_config_manual.h, Right, but they can't be treated the same since EXEC_BACKEND will always be matched by such a grep and the presence of WIN32 and !__CYGWIN__ mst be tested for. Or am I thinking about it the wrong way? This is why I figured checking for the exec_params file could be an option, but with the drawback that it would only work for a running cluster so it wouldn't be a generic function but coded directly in the SSL TAP test file. -- Daniel Gustafsson
> On 13 Nov 2025, at 00:12, Daniel Gustafsson <daniel@yesql.se> wrote: > >> On 12 Nov 2025, at 18:47, Álvaro Herrera <alvherre@kurilemu.de> wrote: >> >> On 2025-Nov-12, Daniel Gustafsson wrote: >> >>> As far as I know the only way to programmatically learn that from the Perl >>> testcode would be to check for the presence of the CONFIG_EXEC_PARAMS file in >>> $self->data_dir, which should be easy enough to do. Do you know of a better >>> way? >> >> We have check_pg_config(), which reads pg_config.h. For EXEC_BACKEND >> you need pg_config_manual.h, > > Right, but they can't be treated the same since EXEC_BACKEND will always be > matched by such a grep and the presence of WIN32 and !__CYGWIN__ mst be tested > for. The attached v2 adds a GUC debug_exec_backend which can be used to get the state of the running cluster, much like how debug_assertions will tell whether or not assertions were compiled in or not. (Per an idea off-list conversation about this.) This will be operating system independent and reusable in other tests as well. The rest of the patches are the same, just adapted to use this GUC in the SSL test. -- Daniel Gustafsson
Вложения
Hi Daniel, I just reviewed the patch and got a few comments. > On Nov 22, 2025, at 06:38, Daniel Gustafsson <daniel@yesql.se> wrote: > > > The attached v2 adds a GUC debug_exec_backend which can be used to get the > state of the running cluster, much like how debug_assertions will tell whether > or not assertions were compiled in or not. (Per an idea off-list conversation > about this.) This will be operating system independent and reusable in other > tests as well. > > The rest of the patches are the same, just adapted to use this GUC in the SSL > test. > > -- > Daniel Gustafsson > > <v2-0001-Add-GUC-to-show-EXEC_BACKEND-state.patch><v2-0002-doc-Clarify-passphrase-command-reloading-on-Windo.patch><v2-0003-ssl-Add-connection-and-reload-tests-for-key-passp.patch> 1 - 0001 ``` + short_desc => 'Shows whether the running server is running in EXEC_BACKEND mode.', ``` The GUC is added like a mirror of debug_assertions. However, I think a small difference is that, assertions will impact everythingat runtime, while EXEC_BACKEND don’t really impact PG’s behavior, instead it only impacts how backend processesare spawned. Thus, I feel “running server is EXEC_BACKEND mode” is a little bit inaccurate, maybe just say “showwhether the running server is built with EXEC_BACKEND”. 2 - 0002 ``` + This parameter must be set to <literal>on</literal> when running on + <systemitem class="osname">Windows</systemitem> since all connections ``` This is not a comment. I’m just thinking that, as EXEC_BACKEND is compile flag, when a server is started, it knows if EXEC_BACKENDis enabled or not. So that, if ssl_passphrase_command must be turned on, why cannot we automatically turn onit? 3 - 0003 ``` +$node->log_check( + "passhprase could reload private key", ``` Typo: passhprase -> passphrase Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On 2025-Nov-21, Daniel Gustafsson wrote: > The attached v2 adds a GUC debug_exec_backend which can be used to get the > state of the running cluster, Nice idea. I think the parallel to debug_assertions is not perfect, because you can turn off debug_assertions in a server built with them included, but you cannot turn off EXEC_BACKEND. This says we shouldn't name the symbol with the DEFAULT word: it should just be "EXEC_BACKEND_ENABLED". The value already cannot be changed, which is good, and there are tests for this behavior of PGC_INTERNAL gucs in src/test/modules/unsafe_tests/sql/guc_privs.sql, so I think we don't need anything additional. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Once again, thank you and all of the developers for your hard work on PostgreSQL. This is by far the most pleasant management experience of any database I've worked on." (Dan Harris) http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
> On 22 Nov 2025, at 10:30, Chao Li <li.evan.chao@gmail.com> wrote: > I just reviewed the patch and got a few comments. Thanks! An updated version will come downthread. > The GUC is added like a mirror of debug_assertions. However, I think a small difference is that, assertions will impacteverything at runtime, while EXEC_BACKEND don’t really impact PG’s behavior, instead it only impacts how backend processesare spawned. Thus, I feel “running server is EXEC_BACKEND mode” is a little bit inaccurate, maybe just say “showwhether the running server is built with EXEC_BACKEND”. That's a good point, the docementation hunk had it right where this part got it wrong. Fixed. > This is not a comment. I’m just thinking that, as EXEC_BACKEND is compile flag, when a server is started, it knows if EXEC_BACKENDis enabled or not. So that, if ssl_passphrase_command must be turned on, why cannot we automatically turn onit? Technically we might be able to, but I don't want to override the administrator when it comes to sensitive configuration settings. Better to document what needs to be done and have the user make informed decisions. > Typo: passhprase -> passphrase Fixed. -- Daniel Gustafsson
> On 22 Nov 2025, at 14:00, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2025-Nov-21, Daniel Gustafsson wrote: > >> The attached v2 adds a GUC debug_exec_backend which can be used to get the >> state of the running cluster, > > Nice idea. > > I think the parallel to debug_assertions is not perfect, because you can > turn off debug_assertions in a server built with them included, but you > cannot turn off EXEC_BACKEND. debug_assertions cannot be disabled anymore, since 3bdcf6a5a755. > This says we shouldn't name the symbol > with the DEFAULT word: it should just be "EXEC_BACKEND_ENABLED". Regardless, I totally agree with that, fixed in the attached along with the review comments from upthread. -- Daniel Gustafsson
Вложения
On 2025-Nov-23, Daniel Gustafsson wrote: > > On 22 Nov 2025, at 14:00, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > I think the parallel to debug_assertions is not perfect, because you > > can turn off debug_assertions in a server built with them included, > > but you cannot turn off EXEC_BACKEND. > > debug_assertions cannot be disabled anymore, since 3bdcf6a5a755. Hah, 2014 -- goes to show that I don't try very often ;-) (I do have a separate build tree for no-assertions, which I don't use very often either.) > > This says we shouldn't name the symbol with the DEFAULT word: it > > should just be "EXEC_BACKEND_ENABLED". > > Regardless, I totally agree with that, fixed in the attached along > with the review comments from upthread. This looks reasonable to me, thanks. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No nos atrevemos a muchas cosas porque son difíciles, pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)
On 23.11.25 20:56, Daniel Gustafsson wrote: >> On 22 Nov 2025, at 14:00, Álvaro Herrera <alvherre@kurilemu.de> wrote: >> >> On 2025-Nov-21, Daniel Gustafsson wrote: >> >>> The attached v2 adds a GUC debug_exec_backend which can be used to get the >>> state of the running cluster, >> >> Nice idea. >> >> I think the parallel to debug_assertions is not perfect, because you can >> turn off debug_assertions in a server built with them included, but you >> cannot turn off EXEC_BACKEND. > > debug_assertions cannot be disabled anymore, since 3bdcf6a5a755. > >> This says we shouldn't name the symbol >> with the DEFAULT word: it should just be "EXEC_BACKEND_ENABLED". > > Regardless, I totally agree with that, fixed in the attached along with the > review comments from upthread. This looks good to me.