Implement an option to skip editing of HBA rules by pg_autoctl #169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DimCitus
changed the title
mtuncer
reviewed
Feb 25, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
| @@ -693,7 +695,7 @@ create_database_and_extension(Keeper *keeper) | |||
| pgSetup->dbname, | |||
| config->nodename, | |||
| pg_setup_get_username(pgSetup), | |||
| "trust", | |||
| pg_setup_get_auth_method(pgSetup), | |||
There was a problem hiding this comment.
function returns "trust" as default value
There was a problem hiding this comment.
Indeed. And when the user said --auth cert or --auth scram-sha-256, that's what we're going to use here when opening the LAN for a whole formation of nodes.
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
| { | ||
| authMethod = pg_setup_get_auth_method(postgresSetup); | ||
| /* most authentication methods require a password */ | ||
| if (strcmp(authMethod, "trust") != 0 |
There was a problem hiding this comment.
consider using a macro like HBA_TRUST, so that we won't have hardcoded "trust" in the c code
There was a problem hiding this comment.
I don't think we would gain enough in terms of maintenance that way. Maybe we could have a quick parser for HBA authentication methods and use an enum internally? I didn't do that, as I'm not sure it's necessary to go that far at the moment.
mtuncer
reviewed
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
mtuncer
suggested changes
Feb 28, 2020
mtuncer
reviewed
Feb 28, 2020
The setting should be found in the configuration file and re-installed in memory at pg_autoctl start-up, like it is the case for the monitor. This looks like it was forgotten and bugs never actually showed-up.
When an option is not set (replication.password in this testing session), don't try to dereference its strValue pointer: it's NULL.
In cases we hide the logs to the users, and skip some ERROR messages. It happens that sometimes we want to see the exact same error messages at the DEBUG level in those cases.
This is implemented using a new authentication method: "skip". This allows minimal changes to the internals of the code, and to issue a nice WARNing message to our users so that they can easily figure out what changes they need to make to their HBA files.
Include testing of connection strings, allowing for better debugging of HBA rules. That's going to be useful for --skip-pg-hba users.
This is work in progress and allows to test placing one's own SSL certificates and setup and HBA rules when bypassing the default authMethod provided by pg_autoctl usually.
This time we use the full SSL settings in a way that we have authentication without passwords.
Also update some more that we missed in previous rounds.
We can't set the pg_autoctl configuration file entry for the replication password until the configuration file has been created, which happens in the pg_autoctl create command. That said, pg_basebackup fails in the create command because the replication password is not set yet. The dirty fix is to do it twice and set the replication password in between the two calls. A better fix would be to introduce a way to break the circular dependency: - provide a way to set the replication password at create time, - or separate the registration step from the pg_basebackup step. The second approach is more appealing because it avoids yet another password handling mechanism to implement, or passing it in the clear.
DimCitus
force-pushed
the
feature/skip-hba
branch
from
885b4e8
to
26ed02c
Compare
mtuncer
reviewed
Feb 28, 2020
mtuncer
approved these changes
Feb 28, 2020
Weel actually... our test environment *requires* some changes to the HBA, so even when using --skip-pg-hba the HBA file is edited to add two rules. Take that into account in the test suite thanks to diff --ignore-matching-lines.
DimCitus
added a commit
that referenced
this pull request
Mar 19, 2020
* Add forgotten --auth method to keeper's config. The setting should be found in the configuration file and re-installed in memory at pg_autoctl start-up, like it is the case for the monitor. This looks like it was forgotten and bugs never actually showed-up. * Fix a segmentation fault hazard. When an option is not set (replication.password in this testing session), don't try to dereference its strValue pointer: it's NULL. * Improve "silent" error logging. In cases we hide the logs to the users, and skip some ERROR messages. It happens that sometimes we want to see the exact same error messages at the DEBUG level in those cases. * Implement --skip-pg-hba. This is implemented using a new authentication method: "skip". This allows minimal changes to the internals of the code, and to issue a nice WARNing message to our users so that they can easily figure out what changes they need to make to their HBA files. * Improve pg_autoctl config check. Include testing of connection strings, allowing for better debugging of HBA rules. That's going to be useful for --skip-pg-hba users. * Require --auth trust rather than making it the silent default. * Improve log output of subcommands. * Add a test case for --skip-pg-hba. This is work in progress and allows to test placing one's own SSL certificates and setup and HBA rules when bypassing the default authMethod provided by pg_autoctl usually. * Now use sslmode=verify-ca in the test for --skip-pg-hba. * Implement certificate based authentication in --skip-pg-hba test. This time we use the full SSL settings in a way that we have authentication without passwords. * Update Python version on Travis. * Update documentation with --auth and --skip-pg-hba options. Also update some more that we missed in previous rounds. * Quick & dirty fix for a bootstrap issue with replication password. We can't set the pg_autoctl configuration file entry for the replication password until the configuration file has been created, which happens in the pg_autoctl create command. That said, pg_basebackup fails in the create command because the replication password is not set yet. The dirty fix is to do it twice and set the replication password in between the two calls. A better fix would be to introduce a way to break the circular dependency: - provide a way to set the replication password at create time, - or separate the registration step from the pg_basebackup step. The second approach is more appealing because it avoids yet another password handling mechanism to implement, or passing it in the clear. * Apply review comments. * Test that the HBA files are not editing when using --skip-pg-hba. Weel actually... our test environment *requires* some changes to the HBA, so even when using --skip-pg-hba the HBA file is edited to add two rules. Take that into account in the test suite thanks to diff --ignore-matching-lines.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow users to
--skip-pg-hbarules editing by pg_autoctl. In that case, we issue WARNing messages with the rules that pg_autoctl refrains from adding, to make it easy enough for our users to review their own provisioning.Fixes #91, fixes #62.