Horton

Re: Allow file inclusion in pg_hba and pg_ident files

Julien Rouhaud <rjuju123@gmail.com>

View thread View on postgr.es
From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Aleksander Alekseev <aleksander@timescale.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Nathan Bossart <nathandbossart@gmail.com>
Date: 2022-03-27T09:52:22Z
Lists: pgsql-hackers

Attachments

Hi,

On Fri, Mar 25, 2022 at 08:18:31PM +0900, Michael Paquier wrote:
>
> Now looking at 0002.  The changes in hba.c are straight-forward,
> that's a nice read.

Thanks!

>  	if (!field) { \
> -		ereport(LOG, \
> +		ereport(elevel, \
>  				(errcode(ERRCODE_CONFIG_FILE_ERROR), \
>  				 errmsg("missing entry in file \"%s\" at end of line %d", \
>  						IdentFileName, line_num))); \
> +		*err_msg = psprintf("missing entry at end of line"); \
>  		return NULL; \
>  	} \
> I think that we'd better add to err_msg the line number and the file
> name.  This would become particularly important once the facility to
> include files gets added.  We won't use IdentFileName for this
> purpose, but at least we would know which areas to change.  Also, even
> if the the view proposes line_number, there is an argument in favor of
> consistency here.

I don't really like it.  The file inclusion patch adds a file_name column in
both views so that you have a direct access to the information, whether the
line is in error or not.  Having the file name and line number in error message
doesn't add any value as it would be redundant, and just make the view output
bigger (on top of making testing more difficult).  I kept the err_msg as-is
(and fixed the ereport filename in the file inclusion patch that I indeed
missed).
>
> +select count(*) >= 0 as ok from pg_ident_file_mappings;
>
> I'd really like to see more tests for this stuff

I didn't like the various suggestions, as it would mean to scatter the tests
all over the place.  The whole point of those views is indeed to check the
current content of a file without applying the configuration change (not on
Windows or EXEC_BACKEND, but there's nothing we can do there), so let's use
this way.  I added a naive src/test/authentication/003_hba_ident_views.pl test
that validates that specific new valid and invalid lines in both files are
correctly reported.  Note that I didn't update those tests for the file
inclusion.

Note that those tests fail on Windows (and I'm assuming on EXEC_BACKEND
builds), as they're testing invalid files which by definition prevent any
further connection attempt.  I'm not sure what would be best to do here, apart
from bypassing the invalid config tests on such platforms.  I don't think that
validating that trying to connect on such platforms when an invalid
pg_hba/pg_ident file brings anything.

> +    a.pg_usernamee,
> [...]
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>pg_username</structfield> <type>text</type>
>
> Perhaps that's just a typo in the function output and you
> intended to use pg_username?

Yes that was a typo :)  It's correctly documented in catalogs.sgml, so I just
fixed pg_proc.dat and rules.out.

> +   /* Free parse_hba_line memory */
> +   MemoryContextSwitchTo(oldcxt);
> +   MemoryContextDelete(identcxt);
> Incorrect comment, this should be parse_ident_line.

Indeed.  I actually fixed it before but lost the change when rebasing after the
2nd hbafuncs.c refactoring.  I also fixed an incorrect comment about
pg_hba_file_mappings.

Commits

  1. Add TAP tests for include directives in HBA end ident files

  2. Introduce variables for initial and max nesting depth on configuration files

  3. Add support for file inclusions in HBA and ident configuration files

  4. Add missing initialization in tokenize_expand_file() for output list

  5. Rework memory contexts in charge of HBA/ident tokenization

  6. Add error context callback when tokenizing authentication files

  7. Invent open_auth_file() in hba.c to refactor authentication file opening

  8. Use AbsoluteConfigLocation() when building an included path in hba.c

  9. Move code related to configuration files in directories to new file

  10. doc: Fix some descriptions related to pg_ident_file_mappings

  11. Add rule_number to pg_hba_file_rules and map_number to pg_ident_file_mappings

  12. Refactor code handling the names of files loaded in hba.c

  13. Make consistent a couple of log messages when parsing HBA files

  14. Use hba_file/ident_file GUCs rather than pg_hba.conf/pg_ident.conf in logs

  15. Fix path reference when parsing pg_ident.conf for pg_ident_file_mappings

  16. Add system view pg_ident_file_mappings

  17. Modify query on pg_hba_file_rules to check for errors in regression tests

  18. Refactor code related to pg_hba_file_rules() into new file