On Sun, Nov 27, 2022 at 07:04:46PM +0800, Julien Rouhaud wrote: > And here's the rebased patch for the TAP tests. I will switch the CF entry to > Needs Review.
I have been looking at that, and applied the part 1 of the test for
the positive tests to cover the basic ground I'd like to see covered
for this feature. I have done quite a few changes to this part, like
reworking the way the incrementation of the counters is done for the
rule/map numbers and the line numbers of each file. The idea to use a
global dictionary to track the current number of lines was rather
clear, so I have kept that. add_{hba,ident}_line() rely on the
files included in a directory to be ordered in the same way as the
backend, and I am fine to live with that.
+# Normalize the data directory for Windows
+$data_dir =~ s/\/\.\//\//g; # reduce /./ to /
+$data_dir =~ s/\/\//\//g; # reduce // to /
+$data_dir =~ s/\/$//; # remove trailing /
+$data_dir =~ s/\\/\//g; # change \ to /
The mysticism around the conversion of the data directory path is
something I'd like to avoid, or I suspect that we are going to have a
hard time debugging any issues in this area. For the positive cases
committed, I have gone through the approach of using the base name of
the configuration file to avoid those compatibility issues. Wouldn't
it better to do the same for the expected error messages, switching to
an approach where we only check for the configuration file name in all
these regexps?
+# Generate the expected output for the auth file view error reporting (file
+# name, file line, error), for the given array of error conditions, as
+# generated generated by add_hba_line/add_ident_line with an err_str.
+sub generate_log_err_rows
+{
generate_log_err_rows() does not strike me as a good interface. The
issues I am pointed at here is it depends directly on the result
returned by add_hba_line() or add_ident_line() when the caller passes
down an error string. As far as I get it, this interface is done
as-is to correctly increment the line number of one file, and I'd like
to believe that add_{hba,ident}_line() should be kept designed so as
they only return the expected matching entry in their catalog views,
without any code path returning something else (in your case the
triplet [ $filename, $fileline, $err_str ]). That makes the whole
harder to understand.
Attached is a rebased patch of the rest. With everything we have
dealt with in this CF, perhaps it would be better to mark this entry
as committed and switch to a new thread where the negative TAP tests
could be discussed? It looks like the part for the error pattern
checks compared with the logs could be the easiest portion of what's
remaining.
--
Michael
From adb076649534bf9ef3020c8a000ed7fd269b13b7 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 28 Nov 2022 16:08:15 +0900 Subject: [PATCH v24] Add regression tests for file inclusion in HBA end ident configuration files This covers the error tests. Author: Julien Rouhaud Reviewed-by: FIXME Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya@jrouhaud --- .../authentication/t/004_file_inclusion.pl | 454 +++++++++++++++++- 1 file changed, 452 insertions(+), 2 deletions(-) diff --git a/src/test/authentication/t/004_file_inclusion.pl b/src/test/authentication/t/004_file_inclusion.pl index c420f3ebca..4a49d55c6a 100644 --- a/src/test/authentication/t/004_file_inclusion.pl +++ b/src/test/authentication/t/004_file_inclusion.pl @@ -33,6 +33,10 @@ my %line_counters = ('hba_rule' => 0, 'ident_rule' => 0); # the general hba rule number as an include directive generates no data # in pg_hba_file_rules. # +# If an err_str is provided, it returns an arrayref containing the provided +# filename, the current line number in that file and the provided err_str. The +# err_str has to be a valid regex string. +# # This function returns the entry of pg_hba_file_rules expected when this # is loaded by the backend. sub add_hba_line @@ -40,6 +44,7 @@ sub add_hba_line my $node = shift; my $filename = shift; my $entry = shift; + my $err_str = shift; my $globline; my $fileline; my @tokens; @@ -58,11 +63,27 @@ sub add_hba_line $fileline = ++$line_counters{$filename}; # Include directive, that does not generate a view entry. - return '' if ($entry =~ qr/^include/); + if ($entry =~ qr/^include/) + { + if (defined $err_str) + { + return [ $filename, $fileline, $err_str ]; + } + else + { + return ''; + } + } # Increment pg_hba_file_rules.rule_number and save it. $globline = ++$line_counters{'hba_rule'}; + # If caller provided an err_str, just returns the needed metadata + if (defined $err_str) + { + return [ $filename, $fileline, $err_str ]; + } + # Generate the expected pg_hba_file_rules line @tokens = split(/ /, $entry); $tokens[1] = '{' . $tokens[1] . '}'; # database @@ -91,6 +112,10 @@ sub add_hba_line # the general map number as an include directive generates no data in # pg_ident_file_mappings. # +# If an err_str is provided, it returns an arrayref containing the provided +# filename, the current line number in that file and the provided err_str. The +# err_str has to be a valid regex string. +# # This works pretty much the same as add_hba_line() above, except that it # returns an entry to match with pg_ident_file_mappings. sub add_ident_line @@ -98,6 +123,7 @@ sub add_ident_line my $node = shift; my $filename = shift; my $entry = shift; + my $err_str = shift; my $globline; my $fileline; my @tokens; @@ -116,11 +142,27 @@ sub add_ident_line $fileline = ++$line_counters{$filename}; # Include directive, that does not generate a view entry. - return '' if ($entry =~ qr/^include/); + if ($entry =~ qr/^include/) + { + if (defined $err_str) + { + return [ $filename, $fileline, $err_str ]; + } + else + { + return ''; + } + } # Increment pg_ident_file_mappings.map_number and get it. $globline = ++$line_counters{'ident_rule'}; + # If caller provided an err_str, just returns the needed metadata + if (defined $err_str) + { + return [ $filename, $fileline, $err_str ]; + } + # Generate the expected pg_ident_file_mappings line @tokens = split(/ /, $entry); # Append empty error @@ -145,6 +187,12 @@ $node->start; my $data_dir = $node->data_dir; +# Normalize the data directory for Windows +$data_dir =~ s/\/\.\//\//g; # reduce /./ to / +$data_dir =~ s/\/\//\//g; # reduce // to / +$data_dir =~ s/\/$//; # remove trailing / +$data_dir =~ s/\\/\//g; # change \ to / + note "Generating HBA structure with include directives"; my $hba_expected = ''; @@ -295,4 +343,406 @@ $contents = $node->safe_psql( FROM pg_ident_file_mappings ORDER BY map_number)); is($contents, $ident_expected, 'check contents of pg_ident_file_mappings'); +# Delete pg_hba.conf and pg_ident.conf from the given node and add minimal +# entries to allow authentication. +sub reset_auth_files +{ + my $node = shift; + + unlink("$data_dir/$hba_file"); + unlink("$data_dir/$ident_file"); + + %line_counters = ('hba_rule' => 0, 'ident_rule' => 0); + + return add_hba_line($node, "$hba_file", 'local all all trust'); +} + +# Generate a list of expected error regex for the given array of error +# conditions, as generated by add_hba_line/add_ident_line with an err_str. +# +# 2 regex are generated per array entry: one for the given err_str, and one for +# the expected line in the specific file. Since all lines are independant, +# there's no guarantee that a specific failure regex and the per-line regex +# will match the same error. Calling code should add at least one test with a +# single error to make sure that the line number / file name is correct. +# +# On top of that, an extra line is generated for the general failure to process +# the main auth file. +sub generate_log_err_patterns +{ + my $node = shift; + my $raw_errors = shift; + my $is_hba_err = shift; + my @errors; + + foreach my $arr (@{$raw_errors}) + { + my $filename = @{$arr}[0]; + my $fileline = @{$arr}[1]; + my $err_str = @{$arr}[2]; + + push @errors, qr/$err_str/; + + # Context messages with the file / line location aren't always emitted + if ( $err_str !~ /maximum nesting depth exceeded/ + and $err_str !~ /could not open file/) + { + push @errors, + qr/line $fileline of configuration file "$data_dir\/$filename"/; + } + } + + push @errors, qr/could not load $data_dir\/$hba_file/ if ($is_hba_err); + + return \@errors; +} + +# Generate the expected output for the auth file view error reporting (file +# name, file line, error), for the given array of error conditions, as +# generated generated by add_hba_line/add_ident_line with an err_str. +sub generate_log_err_rows +{ + my $node = shift; + my $raw_errors = shift; + my $exp_rows = ''; + + foreach my $arr (@{$raw_errors}) + { + my $filename = @{$arr}[0]; + my $fileline = @{$arr}[1]; + my $err_str = @{$arr}[2]; + + $exp_rows .= "\n" if ($exp_rows ne ""); + + # Unescape regex patterns if any + $err_str =~ s/\\([\(\)])/$1/g; + $exp_rows .= "|$data_dir\/$filename|$fileline|$err_str"; + } + + return $exp_rows; +} + +# Reset the main auth files, append the given payload to the given config file, +# and check that the instance cannot start, raising the expected error line(s). +sub start_errors_like +{ + my $node = shift; + my $file = shift; + my $payload = shift; + my $pattern = shift; + my $should_fail = shift; + + reset_auth_files($node); + $node->append_conf($file, $payload); + + unlink($node->logfile); + my $ret = + PostgreSQL::Test::Utils::system_log('pg_ctl', '-D', $data_dir, + '-l', $node->logfile, 'start'); + + if ($should_fail) + { + ok($ret != 0, "Cannot start postgres with faulty $file"); + } + else + { + ok($ret == 0, "postgres can start with faulty $file"); + } + + my $log_contents = slurp_file($node->logfile); + + foreach (@{$pattern}) + { + like($log_contents, $_, "Expected failure found in the logs"); + } + + if (not $should_fail) + { + # We can't simply call $node->stop here as the call is optimized out + # when the server isn't started with $node->start. + my $ret = + PostgreSQL::Test::Utils::system_log('pg_ctl', '-D', + $data_dir, 'stop', '-m', 'fast'); + ok($ret == 0, "Could stop postgres"); + } +} + + +note "test log reporting for invalid data"; + +reset_auth_files($node); +$node->restart('fast'); +$node->connect_ok('dbname=postgres', + 'Connection ok after resetting auth files'); + +$node->stop('fast'); + +start_errors_like( + $node, + $hba_file, + "include ../not_a_file", + [ + qr/could not open file "$data_dir\/not_a_file": No such file or directory/, + qr/could not load $data_dir\/$hba_file/ + ], + 1); + +# include_dir, single included file +mkdir("$data_dir/hba_inc_fail"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "local all all reject"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "local all all reject"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "local all all reject"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "not_a_token"); +start_errors_like( + $node, + $hba_file, + "include_dir ../hba_inc_fail", + [ + qr/invalid connection type "not_a_token"/, + qr/line 4 of configuration file "$data_dir\/hba_inc_fail\/inc_dir\.conf"/, + qr/could not load $data_dir\/$hba_file/ + ], + 1); + +# include_dir, single included file with nested inclusion +unlink("$data_dir/hba_inc_fail/inc_dir.conf"); +my @hba_raw_errors_step1; + +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "include file1"); + +add_hba_line($node, "hba_inc_fail/file1", "include file2"); +add_hba_line($node, "hba_inc_fail/file2", "local all all reject"); +add_hba_line($node, "hba_inc_fail/file2", "include file3"); + +add_hba_line($node, "hba_inc_fail/file3", "local all all reject"); +add_hba_line($node, "hba_inc_fail/file3", "local all all reject"); +push @hba_raw_errors_step1, + add_hba_line( + $node, "hba_inc_fail/file3", + "local all all zuul", + 'invalid authentication method "zuul"'); + +start_errors_like( + $node, $hba_file, + "include_dir ../hba_inc_fail", + generate_log_err_patterns($node, \@hba_raw_errors_step1, 1), 1); + +# start_errors_like will reset the main auth files, so the previous error won't +# occur again. We keep it around as we will put back both bogus inclusions for +# the tests at step 3. +my @hba_raw_errors_step2; + +# include_if_exists, with various problems +push @hba_raw_errors_step2, + add_hba_line($node, "hba_if_exists.conf", "local", + "end-of-line before database specification"); +push @hba_raw_errors_step2, + add_hba_line($node, "hba_if_exists.conf", "local,host", + "multiple values specified for connection type"); +push @hba_raw_errors_step2, + add_hba_line($node, "hba_if_exists.conf", "local all", + "end-of-line before role specification"); +push @hba_raw_errors_step2, + add_hba_line( + $node, "hba_if_exists.conf", + "local all all", + "end-of-line before authentication method"); +push @hba_raw_errors_step2, + add_hba_line( + $node, "hba_if_exists.conf", + "host all all test/42", + 'specifying both host name and CIDR mask is invalid: "test/42"'); +push @hba_raw_errors_step2, + add_hba_line( + $node, + "hba_if_exists.conf", + 'local @dbnames_fails.conf all reject', + "could not open file \"$data_dir/dbnames_fails.conf\": No such file or directory" + ); + +add_hba_line($node, "hba_if_exists.conf", "include recurse.conf"); +push @hba_raw_errors_step2, + add_hba_line( + $node, + "recurse.conf", + "include recurse.conf", + "could not open file \"$data_dir/recurse.conf\": maximum nesting depth exceeded" + ); + +# Generate the regex for the expected errors in the logs. There's no guarantee +# that the generated "line X of file..." will be emitted for the expected line, +# but previous tests already ensured that the correct line number / file name +# was emitted, so ensuring that there's an error in all expected lines is +# enough here. +my $expected_errors = + generate_log_err_patterns($node, \@hba_raw_errors_step2, 1); + +# Not an error, but it should raise a message in the logs. Manually add an +# extra log message to detect +add_hba_line($node, "hba_if_exists.conf", "include_if_exists if_exists_none"); +push @{$expected_errors}, + qr/skipping missing authentication file "$data_dir\/if_exists_none"/; + +start_errors_like($node, $hba_file, "include_if_exists ../hba_if_exists.conf", + $expected_errors, 1); + +# Mostly the same, but for ident files +reset_auth_files($node); + +my @ident_raw_errors_step1; + +# include_dir, single included file with nested inclusion +mkdir("$data_dir/ident_inc_fail"); +add_ident_line($node, "ident_inc_fail/inc_dir.conf", "include file1"); + +add_ident_line($node, "ident_inc_fail/file1", "include file2"); +add_ident_line($node, "ident_inc_fail/file2", "ok ok ok"); +add_ident_line($node, "ident_inc_fail/file2", "include file3"); + +add_ident_line($node, "ident_inc_fail/file3", "ok ok ok"); +add_ident_line($node, "ident_inc_fail/file3", "ok ok ok"); +push @ident_raw_errors_step1, + add_ident_line( + $node, "ident_inc_fail/file3", + "failmap /(fail postgres", + 'invalid regular expression "\(fail": parentheses \(\) not balanced'); + +start_errors_like( + $node, $ident_file, + "include_dir ../ident_inc_fail", + generate_log_err_patterns($node, \@ident_raw_errors_step1, 0), 0); + +# start_errors_like will reset the main auth files, so the previous error won't +# occur again. We keep it around as we will put back both bogus inclusions for +# the tests at step 3. +my @ident_raw_errors_step2; + +# include_if_exists, with various problems +push @ident_raw_errors_step2, + add_ident_line($node, "ident_if_exists.conf", "map", + "missing entry at end of line"); +push @ident_raw_errors_step2, + add_ident_line($node, "ident_if_exists.conf", "map1,map2", + "multiple values in ident field"); +push @ident_raw_errors_step2, + add_ident_line( + $node, + "ident_if_exists.conf", + 'map @osnames_fails.conf postgres', + "could not open file \"$data_dir/osnames_fails.conf\": No such file or directory" + ); + +add_ident_line($node, "ident_if_exists.conf", "include ident_recurse.conf"); +push @ident_raw_errors_step2, + add_ident_line( + $node, + "ident_recurse.conf", + "include ident_recurse.conf", + "could not open file \"$data_dir/ident_recurse.conf\": maximum nesting depth exceeded" + ); + +start_errors_like( + $node, $ident_file, "include_if_exists ../ident_if_exists.conf", + # There's no guarantee that the generated "line X of file..." will be + # emitted for the expected line, but previous tests already ensured that + # the correct line number / file name was emitted, so ensuring that there's + # an error in all expected lines is enough here. + generate_log_err_patterns($node, \@ident_raw_errors_step2, 0), + 0); + + +note "test reporting of various error scenario"; + +reset_auth_files($node); + +$node->start; +$node->connect_ok('dbname=postgres', 'Can connect after an auth file reset'); + +is( $node->safe_psql( + 'postgres', + 'SELECT count(*) FROM pg_hba_file_rules WHERE error IS NOT NULL'), + qq(0), + 'No error expected in pg_hba_file_rules'); + +add_ident_line($node, $ident_file, ''); +is( $node->safe_psql( + 'postgres', + 'SELECT count(*) FROM pg_ident_file_mappings WHERE error IS NOT NULL' + ), + qq(0), + 'No error expected in pg_ident_file_mappings'); + +# The instance could be restarted and no error is detected. Now check if the +# build is compatible with the view error reporting (EXEC_BACKEND / win32 will +# fail when trying to connect as they always rely on the current auth files +# content) +my @hba_raw_errors; + +push @hba_raw_errors, + add_hba_line($node, $hba_file, "include ../not_a_file", + "could not open file \"$data_dir/not_a_file\": No such file or directory" + ); + +my ($stdout, $stderr); +my $cmdret = $node->psql( + 'postgres', 'SELECT 1', + stdout => \$stdout, + stderr => \$stderr); + +if ($cmdret != 0) +{ + # Connection failed. Bail out, but make sure to raise a failure if it + # didn't fail for the expected hba file modification. + like( + $stderr, + qr/connection to server.* failed: FATAL: could not load $data_dir\/$hba_file/, + "Connection failed due to loading an invalid hba file"); + + done_testing(); + diag( + "Build not compatible with auth file view error reporting, bail out.\n" + ); + exit; +} + +# Combine errors generated at step 2, in the same order. +$node->append_conf($hba_file, "include_dir ../hba_inc_fail"); +push @hba_raw_errors, @hba_raw_errors_step1; + +$node->append_conf($hba_file, "include_if_exists ../hba_if_exists.conf"); +push @hba_raw_errors, @hba_raw_errors_step2; + +$hba_expected = generate_log_err_rows($node, \@hba_raw_errors); +is( $node->safe_psql( + 'postgres', + 'SELECT rule_number, file_name, line_number, error FROM pg_hba_file_rules' + . ' WHERE error IS NOT NULL ORDER BY rule_number'), + qq($hba_expected), + 'Detected all error in hba file'); + +# and do the same for pg_ident +my @ident_raw_errors; + +push @ident_raw_errors, + add_ident_line( + $node, + $ident_file, + "include ../not_a_file", + "could not open file \"$data_dir/not_a_file\": No such file or directory" + ); + +$node->append_conf($ident_file, "include_dir ../ident_inc_fail"); +push @ident_raw_errors, @ident_raw_errors_step1; + +$node->append_conf($ident_file, "include_if_exists ../ident_if_exists.conf"); +push @ident_raw_errors, @ident_raw_errors_step2; + +$ident_expected = generate_log_err_rows($node, \@ident_raw_errors); +is( $node->safe_psql( + 'postgres', + 'SELECT map_number, file_name, line_number, error FROM pg_ident_file_mappings' + . ' WHERE error IS NOT NULL ORDER BY map_number'), + qq($ident_expected), + 'Detected all error in ident file'); + done_testing(); -- 2.38.1
signature.asc
Description: PGP signature
