Backups: stop storing credentials, and fix a dedup chain that never matched

The NTLARS/DNC registry carries an FTP password in cleartext and it was stored
verbatim. That put it in the backuprevisions table, on the revision panel which
renders values, and in the download route which hands back a ready-to-import
.reg - so anyone holding backups.view could read it off a web page.

Redaction happens at PARSE, so the secret never reaches the database at all.
Redacting on display would have left it at rest and one query away.

What is deliberately NOT done is dropping the value silently. This codec already
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, is listed in the projection, and renders as a
COMMENT naming what must be set by hand. Emitting a placeholder as the password
would produce a machine that looks restored and cannot connect, and the
technician would go debugging the network.

scripts/redact_backup_secrets.py scrubs revisions recorded before this, dry run
by default. It leaves contenthash alone: the hash identifies the configuration a
bay reported, so rewriting it would make every bay's next post look like a
change and insert a duplicate revision across the fleet.

Separately, sourcehostname is now normalized at ingest. samesource() maps '' to
IS NULL for the query while the insert stored the raw '', so rows written by a
client posting an empty hostname could never match their own next post: every
post inserted, the table grew without bound, and the history filled with
identical entries nobody could read. Whitespace did the same thing more quietly.

plugins/backups/migrations/versions/0002_backups_lastseenat.py

@@ -27,8 +27,10 @@ depends_on = None
 
 
 def upgrade():
-    columns = {c['name'] for c in
+    inspector = sa.inspect(op.get_bind())
-               sa.inspect(op.get_bind()).get_columns('backuprevisions')}
+    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
+    inspector = sa.inspect(op.get_bind())
-               sa.inspect(op.get_bind()).get_columns('backuprevisions')}
+    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')

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(),
         )

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('')
 

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

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