SSL support for PostgreSQL hooks #331

s1shed · 2020-06-18T23:18:10Z

s1shed commented

2020-06-18 23:18:10 +00:00

No description provided.

witten reviewed 2020-06-19 03:04:09 +00:00

witten left a comment

Thank you so much for taking the time to develop and submit this! Looks like a great addition to the Postgres hook. A few minor comments and requests follow.

borgmatic/config/schema.yaml

						
				@ -559,0 +583,4 @@

				                            type: str

				                            desc: |

				                                Path to a certificate revocation list.

				                            example: "/root/.postgresql/root.crl"

witten commented

2020-06-19 02:56:30 +00:00

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.

s1shed commented

2020-06-19 11:12:22 +00:00

Changed

s1shed marked this conversation as resolved

borgmatic/hooks/postgresql.py Outdated

						
				@ -60,0 +59,4 @@

				        extra_environment = dict()

				        if 'password' in database:

				            extra_environment['PGPASSWORD'] = database['password']

				        extra_environment['PGSSLMODE'] = database['sslmode'] if 'sslmode' in database else 'disable'

witten commented

2020-06-19 02:57:36 +00:00

Equivalent:

extra_environment['PGSSLMODE'] = database.get('sslmode', 'disable')

I don't feel strongly either way.

Equivalent: ``` extra_environment['PGSSLMODE'] = database.get('sslmode', 'disable') ``` I don't feel strongly either way.

s1shed commented

2020-06-19 11:13:09 +00:00

That looks better. Changed.

s1shed marked this conversation as resolved

borgmatic/hooks/postgresql.py Outdated

						
				@ -145,0 +164,4 @@

				    if 'sslrootcert' in database:

				        extra_environment['PGSSLROOTCERT'] = database['sslrootcert']

				    if 'sslcrl' in database:

				        extra_environment['PGSSLCRL'] = database['sslcrl']

witten commented

2020-06-19 03:00:05 +00:00

Given that this code is repeated twice, you could factor it out into a common function for creating Postgres SSL environment variables from a borgmatic database dict.

Do not feel strongly about that in terms of factoring. However, it may make writing tests easier, because then you could test that factored-out function only once.

Given that this code is repeated twice, you *could* factor it out into a common function for creating Postgres SSL environment variables from a borgmatic database dict. Do not feel strongly about that in terms of factoring. However, it may make writing tests easier, because then you could test that factored-out function only once.

s1shed commented

2020-06-19 11:14:32 +00:00

Changed, though whether the name that I chose is acceptable might be debatable.

s1shed marked this conversation as resolved

tests/unit/hooks/test_postgresql.py

						
				@ -30,3 +30,3 @@

				            ),

				            shell=True,

				            extra_environment=None,

				            extra_environment={'PGSSLMODE': 'disable'},

witten commented

2020-06-19 03:02:22 +00:00

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.)

s1shed commented

2020-06-19 11:11:58 +00:00

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).

s1shed commented

2020-06-19 13:33:49 +00:00

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?

witten commented

2020-06-19 17:42:40 +00:00

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.

s1shed commented

2020-06-19 22:20:08 +00:00

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…

witten commented

2020-06-20 01:50:45 +00:00

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!

s1shed commented

2020-06-20 10:55:48 +00:00

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. :)

s1shed commented

2020-06-19 10:49:23 +00:00

Thank you for your review and kind crititques. The suggestions have been implemented. I added a few more commits to wrap some of my schema lines which were > 80 characters.

s1shed commented

2020-06-20 15:12:05 +00:00

If all of this looks OK I can rebase the changes to fix up the commits, if that'd be desirable (it'd look like https://projects.torsion.org/s1shed/borgmatic/commits/branch/postgres_ssl )