SSL support for PostgreSQL hooks #331

Merged
witten merged 7 commits from :postgres_sql into master 2020-06-20 21:24:16 +00:00
3 changed files with 94 additions and 19 deletions

View File

@ -556,6 +556,37 @@ map:
documentation for details. Note that format is
ignored when the database name is "all".
example: directory
ssl_mode:
type: str
enum: ['disable', 'allow', 'prefer',
'require', 'verify-ca', 'verify-full']
desc: |
SSL mode to use to connect to the database
server. One of "disable", "allow", "prefer",
"require", "verify-ca" or "verify-full".
Defaults to "disable".
example: require
ssl_cert:
type: str
desc: |
Path to a client certificate.
example: "/root/.postgresql/postgresql.crt"
ssl_key:
type: str
desc: |
Path to a private client key.
example: "/root/.postgresql/postgresql.key"
ssl_root_cert:
type: str
desc: |
Path to a root certificate containing a list of
trusted certificate authorities.
example: "/root/.postgresql/root.crt"
ssl_crl:
type: str
s1shed marked this conversation as resolved
Review

For these various new options, their naming would be more in keeping with the rest of borgmatic's configuration options if they were more like: ssl_mode, ssl_key, ssl_root_cert, etc.

For these various new options, their naming would be more in keeping with the rest of borgmatic's configuration options if they were more like: `ssl_mode`, `ssl_key`, `ssl_root_cert`, etc.
Review

Changed

Changed
desc: |
Path to a certificate revocation list.
example: "/root/.postgresql/root.crl"
options:
type: str
desc: |
@ -570,7 +601,8 @@ map:
database dumps are added to your source directories at
runtime, backed up, and removed afterwards. Requires
pg_dump/pg_dumpall/pg_restore commands. See
https://www.postgresql.org/docs/current/app-pgdump.html for
https://www.postgresql.org/docs/current/app-pgdump.html and
https://www.postgresql.org/docs/current/libpq-ssl.html for
details.
mysql_databases:
seq:

View File

@ -15,6 +15,25 @@ def make_dump_path(location_config): # pragma: no cover
)
def make_extra_environment(database):
'''
Make the extra_environment dict from the given database configuration.
'''
extra = dict()
if 'password' in database:
extra['PGPASSWORD'] = database['password']
extra['PGSSLMODE'] = database.get('ssl_mode', 'disable')
if 'ssl_cert' in database:
extra['PGSSLCERT'] = database['ssl_cert']
if 'ssl_key' in database:
extra['PGSSLKEY'] = database['ssl_key']
if 'ssl_root_cert' in database:
extra['PGSSLROOTCERT'] = database['ssl_root_cert']
if 'ssl_crl' in database:
extra['PGSSLCRL'] = database['ssl_crl']
return extra
def dump_databases(databases, log_prefix, location_config, dry_run):
'''
Dump the given PostgreSQL databases to a named pipe. The databases are supplied as a sequence of
@ -56,7 +75,7 @@ def dump_databases(databases, log_prefix, location_config, dry_run):
# format in a particular, a named destination is required, and redirection doesn't work.
+ (('>', dump_filename) if dump_format != 'directory' else ())
)
extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None
extra_environment = make_extra_environment(database)
logger.debug(
'{}: Dumping PostgreSQL database {} to {}{}'.format(
@ -141,7 +160,7 @@ def restore_database_dump(database_config, log_prefix, location_config, dry_run,
+ (('--username', database['username']) if 'username' in database else ())
+ (() if extract_process else (dump_filename,))
)
extra_environment = {'PGPASSWORD': database['password']} if 'password' in database else None
extra_environment = make_extra_environment(database)
logger.debug(
'{}: Restoring PostgreSQL database {}{}'.format(log_prefix, database['name'], dry_run_label)

View File

@ -29,7 +29,7 @@ def test_dump_databases_runs_pg_dump_for_each_database():
'databases/localhost/{}'.format(name),
),
shell=True,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
Review

Speaking of tests, it's awesome that you updated the existing tests. Do you think you'd be able to add new tests for the additional functionality you've added? I'm happy to help if it's not straight-forward. (This is the test file where any new unit tests on this code belong.)

Speaking of tests, it's awesome that you updated the existing tests. Do you think you'd be able to add new tests for the additional functionality you've added? I'm happy to help if it's not straight-forward. (This is the test file where any new unit tests on this code belong.)
Review

These changes were tested locally by modifying pg_hba.conf to make SSL/TLS mandatory and running borgmatic while manually changing the config file to set the various options, e.g. commenting out ssl_mode to ensure the default of disabled is set (and the dump/restore fails), setting ssl_mode to verify-full and seeing pg_dump/pg_restore complain about not providing the path to the root CA cert, setting ssl_root_cert to a non-existant path and seeing the dump/restore referring to this path, etc., so I am confident that these changes are at least functionally correct. Stylistically on the other hand…

I can try to take a stab at writing tests for these new additions but might need some "hand holding" being as though I consider myself to be rather n00bish with Python and coding in general (and having close to 0 experience with writing tests).

These changes were tested locally by modifying `pg_hba.conf` to make SSL/TLS mandatory and running borgmatic while manually changing the config file to set the various options, e.g. commenting out `ssl_mode` to ensure the default of `disabled` is set (and the dump/restore fails), setting `ssl_mode` to `verify-full` and seeing `pg_dump`/`pg_restore` complain about not providing the path to the root CA cert, setting `ssl_root_cert` to a non-existant path and seeing the dump/restore referring to this path, etc., so I am confident that these changes are at least *functionally correct*. Stylistically on the other hand… I can try to take a stab at writing tests for these new additions but might need some "hand holding" being as though I consider myself to be rather n00bish with Python and coding in general (and having close to 0 experience with writing tests).
Review

So...I created two new functions in the postgres unittest file.

The changes I pushed last night:

----------- coverage: platform linux, python 3.8.3-final-0 -----------
Name                            Stmts   Miss  Cover   Missing
--------------------
borgmatic/execute.py               81      2    98%   104-105
borgmatic/hooks/postgresql.py      60      8    87%   64, 66, 68, 70, 161, 163, 165, 167
--------------------
TOTAL                            1380     10    99%

42 files skipped due to complete coverage.

With the changes I made this morning and the tests I just pushed:


----------- coverage: platform linux, python 3.8.3-final-0 -----------
Name                   Stmts   Miss  Cover   Missing
----------------------------------------------------
borgmatic/execute.py      81      2    98%   104-105
----------------------------------------------------
TOTAL                   1372      2    99%

43 files skipped due to complete coverage.

Hopefully the tests look like what is needed?

So...I created two new functions in the postgres unittest file. The changes I pushed last night: ``` ----------- coverage: platform linux, python 3.8.3-final-0 ----------- Name Stmts Miss Cover Missing -------------------- borgmatic/execute.py 81 2 98% 104-105 borgmatic/hooks/postgresql.py 60 8 87% 64, 66, 68, 70, 161, 163, 165, 167 -------------------- TOTAL 1380 10 99% 42 files skipped due to complete coverage. ``` With the changes I made this morning and the tests I just pushed: ``` ----------- coverage: platform linux, python 3.8.3-final-0 ----------- Name Stmts Miss Cover Missing ---------------------------------------------------- borgmatic/execute.py 81 2 98% 104-105 ---------------------------------------------------- TOTAL 1372 2 99% 43 files skipped due to complete coverage. ``` Hopefully the tests look like what is needed?
Review

This is looking great. I appreciate you making all the updates!

The one change I would suggest is to make a dedicated unit test of just your new make_extra_environment() function. That way, you can test only that functionality by itself separate from all of the dump/restore functionality. That'll let you get rid of test_dump_databases_runs_pg_dump_with_ssl_options() and test_restore_database_dump_runs_pg_restore_with_ssl_options(), since you'd be testing the SSL options in your dediceated test_make_extra_environment_does_whatever().

As part of that, it would probably make sense to mock out your new make_extra_environment() function in any of the existing dump/restore test functions.. So any given test just has a single actual function under test. An example of what that might look like:

def test_dump_databases_runs_pg_dump_with_options():
    ...
    flexmock(module).should_receive('make_extra_environment').and_return({})
    ...
    assert ... == ...

Given that all of this is pretty minor, I'm happy to make these updates if you just want to be done with this pull request! Either way, please let me know.

This is looking great. I appreciate you making all the updates! The one change I would suggest is to make a dedicated unit test of just your new `make_extra_environment()` function. That way, you can test only that functionality by itself separate from all of the dump/restore functionality. That'll let you get rid of `test_dump_databases_runs_pg_dump_with_ssl_options()` and `test_restore_database_dump_runs_pg_restore_with_ssl_options()`, since you'd be testing the SSL options in your dediceated `test_make_extra_environment_does_whatever()`. As part of that, it would probably make sense to mock out your new `make_extra_environment()` function in any of the existing dump/restore test functions.. So any given test just has a single actual function under test. An example of what that might look like: ``` def test_dump_databases_runs_pg_dump_with_options(): ... flexmock(module).should_receive('make_extra_environment').and_return({}) ... assert ... == ... ``` Given that all of this is pretty minor, I'm happy to make these updates if you just want to be done with this pull request! Either way, please let me know.
Review

I came up with this which shows 100% coverage for Postgres and it passes but I have my doubts about this being correct.

+def test_make_extra_environment():
+    database = {'name': 'foo',
+                'ssl_mode': 'prefer',
+                'ssl_cert': 'cert.crt',
+                'ssl_key': 'key.key',
+                'ssl_root_cert': 'root.crt',
+                'ssl_crl': 'crl.crl'}
+    extra_env = module.make_extra_environment(database)
+    expected = {'PGSSLMODE': 'prefer',
+                'PGSSLCERT': 'cert.crt',
+                'PGSSLKEY': 'key.key',
+                'PGSSLROOTCERT': 'root.crt',
+                'PGSSLCRL': 'crl.crl'}
+    flexmock(module).should_receive('make_extra_environment').and_return(expected)
+    assert extra_env == expected

If this is OK I can commit and submit. If it's still wrong, thanks in advance for your patience…

I came up with this which shows 100% coverage for Postgres and it passes but I have my doubts about this being correct. ```diff +def test_make_extra_environment(): + database = {'name': 'foo', + 'ssl_mode': 'prefer', + 'ssl_cert': 'cert.crt', + 'ssl_key': 'key.key', + 'ssl_root_cert': 'root.crt', + 'ssl_crl': 'crl.crl'} + extra_env = module.make_extra_environment(database) + expected = {'PGSSLMODE': 'prefer', + 'PGSSLCERT': 'cert.crt', + 'PGSSLKEY': 'key.key', + 'PGSSLROOTCERT': 'root.crt', + 'PGSSLCRL': 'crl.crl'} + flexmock(module).should_receive('make_extra_environment').and_return(expected) + assert extra_env == expected ``` If this is OK I can commit and submit. If it's still wrong, thanks in advance for your patience…
Review

The test function looks great except for the flexmock(module).should_receive('make_extra_environment').and_return(expected) part. That's not needed in this test function because as it's written, the test function is testing that the mock returns what's expected, rather than testing that make_extra_environment() does! If you take that line out, then I think the test will do what you want.

What I was actually suggesting about flexmock(module).should_receive('make_extra_environment') was to add it within the test_dump_databases_* and test_restore_database_* functions only. Sorry if that wasn't clear. Expanding on the example above, the way that could go might be something like:

def test_dump_databases_runs_pg_dump_with_options():
    databases = [{'name': 'foo', 'options': '--stuff=such'}]
    process = flexmock()
    flexmock(module).should_receive('make_dump_path').and_return('')
    flexmock(module.dump).should_receive('make_database_dump_filename').and_return(
        'databases/localhost/foo'
    )
    flexmock(module.dump).should_receive('create_named_pipe_for_dump')
    flexmock(module).should_receive('make_extra_environment').and_return({})
    ...

That last line is the only addition. The idea is that in test_dump_databases_*(), the only unit you want to test is dump_databases()... and therefore all other units that dump_databases() calls out to (like make_extra_environment() should get mocked out so that their implementation details don't impact the test.

Thanks for your perseverence here!

The test function looks great except for the `flexmock(module).should_receive('make_extra_environment').and_return(expected)` part. That's not needed in this test function because as it's written, the test function is testing that the mock returns what's expected, rather than testing that `make_extra_environment()` does! If you take that line out, then I think the test will do what you want. What I was actually suggesting about `flexmock(module).should_receive('make_extra_environment')` was to add it within the `test_dump_databases_*` and `test_restore_database_*` functions only. Sorry if that wasn't clear. Expanding on the example above, the way that could go might be something like: ```python def test_dump_databases_runs_pg_dump_with_options(): databases = [{'name': 'foo', 'options': '--stuff=such'}] process = flexmock() flexmock(module).should_receive('make_dump_path').and_return('') flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ) flexmock(module.dump).should_receive('create_named_pipe_for_dump') flexmock(module).should_receive('make_extra_environment').and_return({}) ... ``` That last line is the only addition. The idea is that in `test_dump_databases_*()`, the only unit you want to test is `dump_databases()`... and therefore all other units that `dump_databases()` calls out to (like `make_extra_environment()` should get mocked out so that their implementation details don't impact the test. Thanks for your perseverence here!
Review

I rebased a "fixed" version into the previous commit which added the tests. Perhaps it's now correct.

I genuinely appreciate the patience that you've demonstrated. :)

I rebased a "fixed" version into the previous commit which added the tests. Perhaps it's now correct. I genuinely appreciate the patience that you've demonstrated. :)
run_to_completion=False,
).and_return(process).once()
@ -74,7 +74,7 @@ def test_dump_databases_runs_pg_dump_with_hostname_and_port():
'databases/database.example.org/foo',
),
shell=True,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
run_to_completion=False,
).and_return(process).once()
@ -105,13 +105,34 @@ def test_dump_databases_runs_pg_dump_with_username_and_password():
'databases/localhost/foo',
),
shell=True,
extra_environment={'PGPASSWORD': 'trustsome1'},
extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'},
run_to_completion=False,
).and_return(process).once()
assert module.dump_databases(databases, 'test.yaml', {}, dry_run=False) == [process]
def test_make_extra_environment():
database = {
'name': 'foo',
'ssl_mode': 'require',
'ssl_cert': 'cert.crt',
'ssl_key': 'key.key',
'ssl_root_cert': 'root.crt',
'ssl_crl': 'crl.crl',
}
expected = {
'PGSSLMODE': 'require',
'PGSSLCERT': 'cert.crt',
'PGSSLKEY': 'key.key',
'PGSSLROOTCERT': 'root.crt',
'PGSSLCRL': 'crl.crl',
}
extra_env = module.make_extra_environment(database)
assert extra_env == expected
def test_dump_databases_runs_pg_dump_with_directory_format():
databases = [{'name': 'foo', 'format': 'directory'}]
process = flexmock()
@ -135,7 +156,7 @@ def test_dump_databases_runs_pg_dump_with_directory_format():
'foo',
),
shell=True,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
run_to_completion=False,
).and_return(process).once()
@ -151,6 +172,8 @@ def test_dump_databases_runs_pg_dump_with_options():
)
flexmock(module.dump).should_receive('create_named_pipe_for_dump')
flexmock(module).should_receive('make_extra_environment').and_return({'PGSSLMODE': 'disable'})
flexmock(module).should_receive('execute_command').with_args(
(
'pg_dump',
@ -165,7 +188,7 @@ def test_dump_databases_runs_pg_dump_with_options():
'databases/localhost/foo',
),
shell=True,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
run_to_completion=False,
).and_return(process).once()
@ -184,7 +207,7 @@ def test_dump_databases_runs_pg_dumpall_for_all_databases():
flexmock(module).should_receive('execute_command').with_args(
('pg_dumpall', '--no-password', '--clean', '--if-exists', '>', 'databases/localhost/all'),
shell=True,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
run_to_completion=False,
).and_return(process).once()
@ -210,12 +233,12 @@ def test_restore_database_dump_runs_pg_restore():
processes=[extract_process],
output_log_level=logging.DEBUG,
input_file=extract_process.stdout,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
borg_local_path='borg',
).once()
flexmock(module).should_receive('execute_command').with_args(
('psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'),
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
).once()
module.restore_database_dump(
@ -260,7 +283,7 @@ def test_restore_database_dump_runs_pg_restore_with_hostname_and_port():
processes=[extract_process],
output_log_level=logging.DEBUG,
input_file=extract_process.stdout,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
borg_local_path='borg',
).once()
flexmock(module).should_receive('execute_command').with_args(
@ -277,7 +300,7 @@ def test_restore_database_dump_runs_pg_restore_with_hostname_and_port():
'--command',
'ANALYZE',
),
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
).once()
module.restore_database_dump(
@ -306,7 +329,7 @@ def test_restore_database_dump_runs_pg_restore_with_username_and_password():
processes=[extract_process],
output_log_level=logging.DEBUG,
input_file=extract_process.stdout,
extra_environment={'PGPASSWORD': 'trustsome1'},
extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'},
borg_local_path='borg',
).once()
flexmock(module).should_receive('execute_command').with_args(
@ -321,7 +344,7 @@ def test_restore_database_dump_runs_pg_restore_with_username_and_password():
'--command',
'ANALYZE',
),
extra_environment={'PGPASSWORD': 'trustsome1'},
extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'},
).once()
module.restore_database_dump(
@ -340,11 +363,12 @@ def test_restore_database_dump_runs_psql_for_all_database_dump():
processes=[extract_process],
output_log_level=logging.DEBUG,
input_file=extract_process.stdout,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
borg_local_path='borg',
).once()
flexmock(module).should_receive('execute_command').with_args(
('psql', '--no-password', '--quiet', '--command', 'ANALYZE'), extra_environment=None
('psql', '--no-password', '--quiet', '--command', 'ANALYZE'),
extra_environment={'PGSSLMODE': 'disable'},
).once()
module.restore_database_dump(
@ -383,12 +407,12 @@ def test_restore_database_dump_without_extract_process_restores_from_disk():
processes=[],
output_log_level=logging.DEBUG,
input_file=None,
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
borg_local_path='borg',
).once()
flexmock(module).should_receive('execute_command').with_args(
('psql', '--no-password', '--quiet', '--dbname', 'foo', '--command', 'ANALYZE'),
extra_environment=None,
extra_environment={'PGSSLMODE': 'disable'},
).once()
module.restore_database_dump(