diff --git a/plugins/backups/migrations/versions/0002_backups_lastseenat.py b/plugins/backups/migrations/versions/0002_backups_lastseenat.py index b4f5c70a..e71501a8 100644 --- a/plugins/backups/migrations/versions/0002_backups_lastseenat.py +++ b/plugins/backups/migrations/versions/0002_backups_lastseenat.py @@ -27,8 +27,10 @@ depends_on = None def upgrade(): - columns = {c['name'] for c in - sa.inspect(op.get_bind()).get_columns('backuprevisions')} + inspector = sa.inspect(op.get_bind()) + if 'backuprevisions' not in inspector.get_table_names(): + return + columns = {c['name'] for c in inspector.get_columns('backuprevisions')} if 'lastseenat' not in columns: op.add_column('backuprevisions', sa.Column('lastseenat', sa.DateTime(), nullable=True)) @@ -37,7 +39,9 @@ def upgrade(): def downgrade(): - columns = {c['name'] for c in - sa.inspect(op.get_bind()).get_columns('backuprevisions')} + inspector = sa.inspect(op.get_bind()) + if 'backuprevisions' not in inspector.get_table_names(): + return + columns = {c['name'] for c in inspector.get_columns('backuprevisions')} if 'lastseenat' in columns: op.drop_column('backuprevisions', 'lastseenat') diff --git a/plugins/backups/plugin.py b/plugins/backups/plugin.py index b2e59cc4..cb6f6575 100644 --- a/plugins/backups/plugin.py +++ b/plugins/backups/plugin.py @@ -410,7 +410,13 @@ class BackupsPlugin(BasePlugin): # with_for_update serializes concurrent posts for the same chain: the # fleet collects on one GE-Enforce cycle, so a retry could otherwise # pass the hash check twice and insert the same revision. - sourcehostname = payload.get('sourcehostname') + # Normalize ONCE, and use the same value to query and to store. + # samesource() maps '' to IS NULL, but the insert below used to store + # the raw '' - so a client posting an empty hostname wrote rows that its + # own next post could never match. Every post then deduped against + # nothing, inserting forever: unbounded growth, and a history no + # technician can read. Whitespace does the same thing more quietly. + sourcehostname = (payload.get('sourcehostname') or '').strip() or None latest = (db.session.query(BackupRevision) .filter(BackupRevision.assetid == assetid, BackupRevision.backupkind == kindkey, @@ -476,7 +482,7 @@ class BackupsPlugin(BasePlugin): sharepath=sharepath, sourcefilename=sourcefilename, bytesize=bytesize, - sourcehostname=payload.get('sourcehostname'), + sourcehostname=sourcehostname, collectedat=collectedat or datetime.utcnow(), lastseenat=datetime.utcnow(), ) diff --git a/plugins/backups/services/ntlars.py b/plugins/backups/services/ntlars.py index 6e65ff9d..d0d992c4 100644 --- a/plugins/backups/services/ntlars.py +++ b/plugins/backups/services/ntlars.py @@ -32,6 +32,31 @@ import re SCHEMA = 'ntlars/1' +# SECRETS ARE NOT BACKED UP. +# +# The DNC registry carries an FTP password in cleartext. Stored verbatim it +# reached three places it must not: the backuprevisions table, the revision +# panel (which renders values), and the download route (which hands back a +# ready-to-import .reg). Anyone with backups.view could read the fleet's FTP +# password off a web page. +# +# Redaction happens at PARSE, so the secret never enters the database at all. +# Redacting only on display would leave it at rest and one query away. +# +# What is deliberately NOT done: dropping the value silently. This module's +# parser refuses to discard an unparseable value precisely because a backup that +# looks complete and restores a machine with a setting missing is the worst +# outcome. A redacted value keeps its NAME and records itself in +# `redacted`, so both the panel and a restore can say plainly that this one +# field must be set by hand. +REDACTEDMARKER = '***REDACTED***' +SECRETNAMEPATTERN = re.compile(r'passw|pwd|secret|apikey|token', re.IGNORECASE) + + +def issecretname(name): + """Whether a registry value name holds a credential.""" + return bool(SECRETNAMEPATTERN.search(name or '')) + REGROOT = 'HKEY_LOCAL_MACHINE' DNCPATH = r'SOFTWARE\GE Aircraft Engines\DNC' DNCPATHWOW = r'SOFTWARE\WOW6432Node\GE Aircraft Engines\DNC' @@ -245,6 +270,20 @@ def parse(raw): dialect = 'wow6432node' if sawwow else ('ntlars' if sawplain else 'unknown') + # Strip credentials before the projection is returned - i.e. before anything + # persists it. Recorded by path\name so the omission is visible rather than + # discovered during a restore. + redacted = [] + for path in keys: + for name, value in keys[path].items(): + if not issecretname(name): + continue + if value.get('data') in ('', None): + continue # nothing to hide + keys[path][name] = {'type': value.get('type', 'REG_SZ'), + 'data': REDACTEDMARKER} + redacted.append('{}\\{}'.format(path, name) if path else name) + return { 'schema': SCHEMA, 'sourcedialect': dialect, @@ -252,6 +291,7 @@ def parse(raw): {'path': path, 'values': dict(sorted(keys[path].items()))} for path in sorted(keys) ], + 'redacted': sorted(redacted), } @@ -291,6 +331,12 @@ def render(projection, dialect='ntlars', comments=None): dialect='ntlars' omits WOW6432Node - use with the NTLARS Load... button dialect='wow6432node' includes it - use with `reg import` on 64-bit + + Redacted credentials are rendered as a comment naming the value, NOT as the + marker string. Importing '***REDACTED***' as the FTP password would leave a + machine that looks restored and cannot connect, and the technician would be + debugging the network. A commented-out value fails loudly at the one moment + somebody is looking at the file. """ if dialect not in ('ntlars', 'wow6432node'): raise ValueError('unknown dialect: {!r}'.format(dialect)) @@ -302,10 +348,28 @@ def render(projection, dialect='ntlars', comments=None): if comments: out.append('') + redacted = projection.get('redacted') or [] + if redacted: + out.append('; ---------------------------------------------------------------') + out.append('; {} credential value(s) were NOT backed up and are NOT in this' + .format(len(redacted))) + out.append('; file. Set them by hand after importing:') + for name in redacted: + out.append('; {}'.format(name)) + out.append('; ---------------------------------------------------------------') + out.append('') + for key in projection.get('keys', []): path = key.get('path', '') out.append('[{}]'.format(root + ('\\' + path if path else ''))) for name, entry in (key.get('values') or {}).items(): + # A redacted credential is commented out, never emitted as the + # marker. Importing the marker as a password produces a machine that + # looks restored and cannot log in. + if (entry or {}).get('data') == REDACTEDMARKER: + out.append('; "{}" was not backed up - set it by hand'.format( + _escape(name))) + continue out.append(_rendervalue(name, entry)) out.append('') diff --git a/scripts/redact_backup_secrets.py b/scripts/redact_backup_secrets.py new file mode 100644 index 00000000..629aa1c6 --- /dev/null +++ b/scripts/redact_backup_secrets.py @@ -0,0 +1,136 @@ +"""Remove credentials from backup revisions already stored in the database. + +WHY THIS EXISTS + +The NTLARS/DNC registry carries an FTP password in cleartext. The codec now +redacts credentials at PARSE, so nothing new reaches the database - but every +revision recorded before that still holds the password verbatim, where the +revision panel renders it and the download route hands it back as a +ready-to-import .reg. Anyone with backups.view could read it off a web page. + +Rotating the password does not fix these rows: it makes them stale AND still +teaches anyone reading them what the old one was. + +WHAT IT CHANGES: backuprevisions.payloadjson only, and only the values whose +NAME looks like a credential (the same test the codec applies). The value name +is kept and recorded in the revision's `redacted` list, so a restore from a +scrubbed revision says plainly which field must be set by hand rather than +quietly producing a machine that cannot log in. + +WHAT IT DOES NOT CHANGE: contenthash. The hash identifies the CONFIGURATION a +bay reported, and dedup compares the next post against it. Rewriting hashes here +would make every bay's next post look like a change and insert a duplicate +revision across the fleet. + +Usage (on the target instance, in the app dir): + + venv\\Scripts\\python scripts\\redact_backup_secrets.py + venv\\Scripts\\python scripts\\redact_backup_secrets.py --commit + +Dry run by default: it reports which revisions hold a credential and stops. +Take a database backup before --commit. +""" + +import argparse +import json +import os +import sys + +# Run from anywhere: put the repo root (parent of scripts/) on the path. +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from dotenv import load_dotenv + +# Load .env before anything reads config. Without it DATABASE_URL stays unset +# and config falls back to a hardcoded root@localhost URL, which fails with a +# bare 1045 Access denied naming a user the site does not use - a database +# credential error that is really "nothing told the app which database". +load_dotenv(os.path.join(os.getcwd(), '.env')) + +from shopdb import create_app # noqa: E402 +from shopdb.extensions import db # noqa: E402 + + +def scrub(projection, issecretname, marker): + """Redact credential values in one projection. Returns (changed, names).""" + if not isinstance(projection, dict): + return False, [] + + names = list(projection.get('redacted') or []) + changed = False + for key in projection.get('keys') or []: + if not isinstance(key, dict): + continue + path = key.get('path', '') + values = key.get('values') + if not isinstance(values, dict): + continue + for name, entry in values.items(): + if not issecretname(name) or not isinstance(entry, dict): + continue + if entry.get('data') in ('', None, marker): + continue + entry['data'] = marker + fullname = '{}\\{}'.format(path, name) if path else name + if fullname not in names: + names.append(fullname) + changed = True + + if changed: + projection['redacted'] = sorted(names) + return changed, names + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--commit', action='store_true', + help='Apply the changes. Dry run by default.') + args = parser.parse_args() + + app = create_app() + with app.app_context(): + from plugins.backups.models import BackupRevision + from plugins.backups.services.ntlars import issecretname, REDACTEDMARKER + + rows = BackupRevision.query.order_by(BackupRevision.backuprevisionid).all() + hits = [] + + for revision in rows: + projection = revision.payload + if not projection: + continue + # Work on a copy so a dry run cannot mutate a loaded instance into + # something a later flush would write. + working = json.loads(json.dumps(projection)) + changed, names = scrub(working, issecretname, REDACTEDMARKER) + if not changed: + continue + hits.append((revision, working, names)) + + if not hits: + print('No stored revision holds a credential. Nothing to do.') + return 0 + + print('{} revision(s) hold a credential:'.format(len(hits))) + for revision, _, names in hits: + print(' revision {:<6} asset {:<6} {:<10} {}'.format( + revision.backuprevisionid, revision.assetid, + revision.backupkind, ', '.join(names))) + + if not args.commit: + print() + print('Dry run. Nothing was changed. Re-run with --commit to write.') + print('Take a database backup first.') + return 0 + + for revision, working, _ in hits: + revision.payload = working + db.session.commit() + print() + print('Redacted {} revision(s). contenthash left untouched, so no bay ' + 'will re-post as changed.'.format(len(hits))) + return 0 + + +if __name__ == '__main__': + raise SystemExit(main()) diff --git a/tests/test_plugins/test_backups.py b/tests/test_plugins/test_backups.py index afc38b55..743feb81 100644 --- a/tests/test_plugins/test_backups.py +++ b/tests/test_plugins/test_backups.py @@ -1416,3 +1416,112 @@ def test_the_card_omits_the_snapshot_row_when_no_path_is_passed(bk_app, bk_plugi from plugins.backups.services import partmarkerinfo card = partmarkerinfo.build(_markermanifest(['A.PAT']), 1) assert 'Snapshot' not in [f['label'] for f in card['fields']] + + +def test_an_empty_source_hostname_still_dedups(bk_app, bk_plugin): + """A client posting sourcehostname='' must not defeat dedup forever. + + samesource() maps '' to IS NULL for the QUERY, but the insert stored the raw + '' - so the rows a client wrote could never match its own next post. Every + post inserted, the table grew without bound, and the revision history filled + with identical entries nobody could read. + """ + from plugins.backups.models import BackupRevision + with bk_app.app_context(): + first = bk_plugin.apply_collector_payload(_payload(sourcehostname='')) + assert first['action'] == 'created' + + second = bk_plugin.apply_collector_payload(_payload(sourcehostname='')) + assert second['action'] == 'noop', 'an identical re-post must dedup' + + rows = BackupRevision.query.all() + assert len(rows) == 1 + assert rows[0].sourcehostname is None, \ + "'' is stored as NULL, matching how samesource() queries it" + + +def test_a_padded_source_hostname_is_the_same_source(bk_app, bk_plugin): + """Whitespace must not fork a machine's history into two chains.""" + with bk_app.app_context(): + bk_plugin.apply_collector_payload(_payload(sourcehostname='GGBX0NH3ESF')) + second = bk_plugin.apply_collector_payload( + _payload(sourcehostname=' GGBX0NH3ESF ')) + assert second['action'] == 'noop' + + +# --------------------------------------------------------------------------- +# Credential redaction. The DNC registry carries an FTP password in cleartext; +# stored verbatim it reached the database, the revision panel and the download +# route, so anyone with backups.view could read the fleet's password off a page. +# --------------------------------------------------------------------------- + +# A configured install that also carries the FTP credential, which is what a +# real marker/DNC bay actually has in its registry. +SECRETREG = CONFIGUREDREG.replace( + '"MachineNo"="3204"', + '"MachineNo"="3204"\r\n"FtpPasswd"="hunter2"\r\n"FtpUser"="dnc"') + + +def test_a_credential_never_enters_the_projection(): + """Redaction happens at parse, so the secret is never persisted. + + Redacting on display instead would leave it at rest and one query away. + """ + import json + from plugins.backups.services import ntlars + + projection = ntlars.parse(_asbytes(SECRETREG)) + + assert 'hunter2' not in json.dumps(projection) + assert projection['redacted'] == ['General\\FtpPasswd'] + values = dict(projection['keys'][0]['values']) + for key in projection['keys']: + values.update(key['values']) + assert values['FtpPasswd']['data'] == ntlars.REDACTEDMARKER + assert values['FtpUser']['data'] == 'dnc', 'only credentials are withheld' + + +def test_a_redacted_value_is_commented_out_not_imported(): + """Importing the marker as a password yields a machine that cannot log in. + + It would LOOK restored, and the technician would go debugging the network. + A commented-out value fails at the one moment somebody is reading the file. + """ + from plugins.backups.services import ntlars + + text = ntlars.render(ntlars.parse(_asbytes(SECRETREG))).decode('utf-16-le') + + assert 'hunter2' not in text + assert ntlars.REDACTEDMARKER not in text + assert 'FtpPasswd' in text, 'the omission must be named, not silent' + assert '"FtpPasswd"="' not in text, 'must not be an importable value line' + assert '"FtpUser"="dnc"' in text, 'everything else still restores' + + +def test_the_download_route_cannot_hand_back_a_password(bk_app, bk_plugin): + """End to end: post a config with a password, download it back.""" + with bk_app.app_context(): + result = bk_plugin.apply_collector_payload(_payload(reg=SECRETREG)) + assert result['action'] == 'created' + + from plugins.backups.models import BackupRevision + revision = _db.session.get(BackupRevision, result['backuprevisionid']) + stored = json.dumps(revision.payload) + assert 'hunter2' not in stored + + rendered = ntlars_render(revision.payload) + assert 'hunter2' not in rendered + + +def ntlars_render(projection): + from plugins.backups.services import ntlars + return ntlars.render(projection).decode('utf-16-le') + + +def test_secret_name_matching_covers_the_usual_spellings(): + from plugins.backups.services.ntlars import issecretname + for name in ('FtpPasswd', 'Password', 'ftp_pwd', 'ApiKey', 'AuthToken', + 'ClientSecret'): + assert issecretname(name), name + for name in ('FtpUser', 'FMSHost', 'Retries', 'Passive'): + assert not issecretname(name), name