Update db export script to generate PII scrubbed copy of the Job Server production database

[nishtha-kalra]

Description

Issue - https://github.com/opensafely-core/security/issues/33

• Updated the dump_db script to now generate a sanitised backup of database with scrubbed PII.
• Updated the relevant document accordingly.
• PR for ADR - ADR for PII scrubbed copy of Job Server database for local development #5461
• Tested this locally by changing the DATABASE_URL in .env to point to my local db instead which had copy of dump from production.
• Slack thread related to this can be found here for anyone interested

Manual testing on new setup:

• clone and switch to branch nk/export-script
• switch to virtual env
• copy dotenv-sample and rename copy to .env

scripts/dev-env.sh .env

• Start docker and install requirements

just docker/db
just devenv

• In another terminal, download and restore the latest production database dump to a separate database. We need to do this so that the sanitised db script can use this production like db when creating a sanitised dump.

scp dokku4:/var/lib/dokku/data/storage/job-server/jobserver.dump jobserver.dump
createdb -h localhost -p 6543 -U user jobserver_prod 
pg_restore -h localhost -p 6543 -U user -d jobserver_prod --no-owner --no-acl jobserver.dump

• Update DATABASE_URL in .env to point to this db

DATABASE_URL=postgres://user:pass@localhost:6543/jobserver_prod 

• Build assets

just assets-rebuild

• Load changes in .env and run the sanitised db job to create a sanitised dump.
  Make sure the OUTPUT_PATH mentioned in the script is available on your system. I temporarily change the path to ./sanitised_jobserver.dump.

set -a; source .env; set +a
just manage runjob dump_sanitised_db

• Restore this sanitised db

just docker/restore-db sanitised_jobserver.dump

• Update DATABASE_URL in .env to point back to db

DATABASE_URL=postgres://user:pass@localhost:6543/jobserver

• Load changes in .env and run

set -a; source .env; set +a 
just manage migrate
just run

• Go to Account -> staff area -> users -> select any user and check that the email is now fake
• Attaching screenshot too for help

[mikerkelly]

Overall

This script seems like a well-structured solution to a very knotty problem that was complicated by our single-node infrastructure. I think the overall approach is good, given our constraints, and appreciate how you've broken down the main function into several smaller functions for structure. However, I do think there are several issues that must be resolved before we can merge this, some of them quite significant. I have some comments and questions below and inline. I appreciate this is quite a lot of feedback all at once, on a big complicated topic, so I've tried to keep it high level rather than too many line-by-line code comments.

Included/excluded fields

I think you decided which fields to include or exclude during this work. This was an area mentioned to be explored in the issue. How did you decide on what to include or exclude? It is not easy to analyse what is excluded from the allow list. There are many more inclusions than exclusions but the exclusions are the most significant information, and are only implicit in the allow list. They will also change over time as fields are added. How will someone looking at this in future understand what they are? Is there anything that can be documented about your thought process? If you used any temporary tooling for this, can it be preserved somewhere in case it is useful if we need to revisit this?

What will happen if a field is renamed, removed, or changes data type; or a developer forgets to add a new or changed one to the allow list? Will the script fail? Will the dump be valid?

I am concerned that _fake_expression may lead to errors if used on a table with a uniqueness constraint. I also think it assumes every table contains an id field when that may not be the case in future if we defined a different primary key field on a model, so I wonder if we should be robust to that? I also think we may want to end the program with an error if used on an unhandled data type, not return NULL, as I think this is unexpected and ought not continue silently. This could cause the script to fail if some of the changes mentioned above are not adjusted for. Probably it's better if the script fails and we hear about it.

I think Peter's suggestion from the Slack thread you link may address some of those issues, did you discard it? Also testing and alerting may mitigate it.

Testing

This script contributes to an IG requirement, the approach and details involve some complexity, and it makes potentially destructive changes directly in the production database. So I think strong testing here is important. This is also important so if we need to maintain or change the script we can do so confidently and efficiently.

Tested this locally by changing the DATABASE_URL in .env to point to my local db instead which had copy of dump from production.

We can possibly rely only on manual testing if automated testing is unfeasible, but we should record and execute a very thorough test plan for assurance in that case. That seems particularly important if there are no automated tests.

I think it will be good to have automated testing if possible. I was imagining some level of fairly detailed unit and some integration testing would be needed. I think that is broadly in line with the approach we discussed in Slack towards the start of the work. What do you think?

Deployment

    @monitor(
        monitor_slug="dump_sanitised_db", monitor_config=monitor_config("0 14 * * *")
    )

I think this adjusts the Sentry monitor, not the execution schedule. That's controlled by
app.json.

I am uncomfortable running this at an arbitrary time during the day after we first deploy it. As it runs directly in the production database, even with very thorough testing there is the risk that if something went wrong we could suffer material irretrievable data loss since the last backup. Instead, I suggest we initially set the schedule of this job to yearly, as that isn't run by the dokku cron process configured in app.json. So it won't run automatically.

We can test how the script behaves in production by taking a manual backup then running this script either via dokku or directly by the runjob management command to manually generate a sanitised backup. Once we're comfortable, we can set a schedule and Sentry monitor, but only once we're ready to run it on a schedule instead of dump_db.

What happens if we need a raw unsanitised copy of prod data? There are valid reasons why we might want that locally, temporarily, including debugging this script or other (potentially urgent) production issues. Maybe we should keep both scripts but dump_db is only run on demand?

Documentation

I think the script would benefit significantly from in-source documentation such as comments and docstrings, and perhaps more explanation in the git history. This script represents a significant amount of work and is implementing some complicated behaviour. So, significant commentary is probably needed for this to be easily understandable and therefore maintainable.

At one point you had 'section header' comments in the execute function like "Ensure output directory exists" that I think were a really useful starting point for immediately understanding the structure of the main execute function code blocks. I think the private functions are fairly well named and marked up with type hints, but I think what some of them are doing could be clearer if the code was restructured to be less compact, the more complicated logic merits some detailed commentary, and most of the private functions would benefit from a docstring.

Other points on the script

Have you thought about how we can best defend against accidental SQL injection? I wonder if we should be using something like quote_ident to wrap the expressions rather than doing it 'manually' in many F-strings. Is the level of escaping sufficient? Is it easy to get it right 'manually' if it needs to change? What do you think?

I am not sure I understand why the script doesn't terminate if there is no allowlist and this behaviour is not documented that I can see. I am not sure we need _run_pg_dump_schema_only at all. I don't think this is part of the requirements. Maybe you used this while developing or testing? If you agree it's not necessary, can we simplify by removing this? Or is is still useful?

Further on _fake_expression, I don't think it needs to handle arbitrary schemas, or data types we don't currently exclude, and some of those replacements are hard to reason about, so could you reduce it down to just what we need at the moment as a starting point?

Use of git

I found it quite hard to follow the git commits and their comments. In general, please ensure your commits and commit messages follow the advice at https://cbea.ms/git-commit/, and keep your commits atomic and separate each functional change, and don't mix those with restructures or refactors. In this case, as you are adding a new script it may be easier to leave the commits as they are while developing and by the time you merge you just rebase and edit down to have one commit that adds the new script, with some high-level explanation and most of the commentary in the script. In future we won't need the detailed history of your development process, but it can be useful during review.

[mikerkelly]

Thanks a lot, Nishtha. This is much improved. The overview documentation is excellent, explaining the "why" as well as the "what". The script to get the denied columns is very helpful.

There is a fair amount of new content since my first review, sorry it has taken a while to look at. There are a number of mostly minor inline comments, and I think we need to consider the points below. We are getting there!

Future work

We have discussed here and in Slack some possible future work. Do you have a list of those issues, will you create them?

SQL safety

I am concerned that the script may still be mixing safe and unsafe string interpolation and construction for SQL statements. I think that it is important to get this right as the script may be used on arbitrary data in future, depending how the database schema evolves.

From reading the psycopg2 documentation, I think that it is risky to construct SQL via f-string formatting, str concatenation, % formatting, or by composing any strings generated in these ways. It's just too hard to get the quoting and escaping right and consider all the edge cases. I would much rather fully follow the approach intended by that module and offload all the validation and safety to it, rather than partially roll our own validation with _valid_ident and mix strings validated through that with the other approach, it is hard to reason about whether that has been applied sufficiently and correctly in all instances to produce safe statements. Here is the approach described:

Usually you should express the template of your query as an SQL instance with {}-style placeholders and use format() to merge the variable parts into them, all of which must be Composable subclasses.

I think _get_column_metadata, _build_select_expressions, _fake_expression, test_create_safe_schema_scrubs_disallowed_columns all interpolate unsafely or include unsafe fragments. I think _create_safe_schema_and_copy and _drop_temp_schema use the right approach. I am not sure we need _valid_ident or _normalize_table_name if we contruct everything through the sql module.

Requirements / correctness

The script executes each line of SQL as it constructs it. I think that this risks the copied tables being inconsistent with related tables if they are modified by another process between lines being written. So should we do all our change inside a transaction and/or take a write lock so nothing can vary while we create the dump?

The script can be triggered manually, so I think we should at least think about what happens if it is run twice at the same time and both edit the non-public schema. I think that would not have a good result. We could add a lockfile or other mechanism so that can not happen, but maybe the approach from the previous paragraph is sufficient (database locking or transactions)?

I wondered it is worth recording row counts for every table in the source and destination schemas and comparing, to check that at least those are consistent. What do you think?

Performance/observability

I have not looked at performance, have you? Other than noting that it runs in some 10s of seconds locally. I think that is enough until we have evidence it is a problem. For observability, can we add an otel span around the whole script and show that on some suitable dashboard or a new one to make it easy to keep an eye on?

There is no logging at all at the moment. Can you please use structlog to record at least start/end of the main function and key params? Logging relevant details in the event of error would also be useful.

Testing

safety: I think each function that can produce SQL statements should have a test with a bad/malicious expression testing that the result is properly escaped sql.sql. I find it hard to be confident we are doing the right thing without that.

observation, no action required: A fuller integration test seems tricky, not sure if we would create a new database just in that one test, or do a a lot of database and file access patching still. Just testing _create_safe_schema_and_copy with one database row and short allowlist as you have done seems like a reasonable compromise.

optional: It would be good to have at least one test of the main execute function, with some appropriate level of patching/mocking. That code is currently untested, I think. Not essential though, manual testing can compensate, and I suppose any future development would want to do manual testing also.

[mikerkelly]

minor, documentation: Could use a comment here explaining that if restoring the sanitised dump we need to alter the scratch schema name to public, and a reference to the overview documentation.

[mikerkelly]

minor, wording:

Suggested change

	4. In another terminal, download and restore the latest production database dump to a separate database. We need to do this so that the sanitised db script can use this production like db when creating a sanitised dump. In real case scenario, we would not need to do this as we will be pointing the db on dokku server.
	4. In another terminal, download and restore the latest production database dump to a separate database so we can run the script against it without changing the local development `jobserver` database. In production, the script runs directly against the production `jobserver` database.

[mikerkelly]

Question: is this step necessary?

[mikerkelly]

minor, wording:

Suggested change

	8. Restore this sanitised db
	8. Restore the sanitised database dump into your local `jobserver` database, deleting all existing data.

[mikerkelly]

minor: can we warn this will overwrite all their local data and suggest taking a backup before if required?

[mikerkelly]

naming: why is this "safe"?
documentation: I think it is clearer to say it is "an empty destination table with the same structure as the src".
SQL safety: I think this is only safe if the parameters are sql.Identifier. At the moment they are, but that would be easy to get wrong in future. At least we should provide a type hint and/or mention it in the docstring. I think it would be safer to check types and raise ValueError if not as required. I am also tempted to say we could maybe not have this function at all as it is only used once, I am not sure the indirection adds that much value.

[mikerkelly]

taste, optional: I am not sure why this is an integration test, could you explain? I think it might live happily in its own file in the root of tests.

[mikerkelly]

This is brittle to the detail of the faking. Can we assert that rows 2 and 3 are not equal to the original value, instead?

[mikerkelly]

optional: The sql parameter names in these classes shadow the imported sql. I think it is prudent to use a different name like query.

[mikerkelly]

optional, performance: I think your allow list tests are hitting the real file system, which is slow and unnecessary. Can you mock the file access instead?