From f62e907e9ffbb4ee45a0554b81fc390db862642d Mon Sep 17 00:00:00 2001 From: jmoore-skild Date: Wed, 5 Aug 2026 20:05:23 -0400 Subject: [PATCH] fix(backup): refuse the whole LDAP family on restore, not just its password (#2656) A settings restore could substitute the instance'"'"'s authentication source. auth.py reads the LDAP config live from the settings table on every login, and none of ldap_server_url, ldap_user_filter, ldap_auto_provision or ldap_default_group is credential-shaped, so the secret-key hints never saw them and only the four auth-policy keys were protected. ldap_enabled was covered by the companion-credential rule instead, and that rule asks the wrong question. It judges availability - "will the integration still work?" - and an anonymous bind works, so a payload that simply OMITS ldap_bind_password skips the refusal and has its toggle written. Omitting the credential is exactly what an attacker authoring the file would do: they own the directory being pointed at, so they need no bind credential from us. Left unrefused, a backup repository anyone can write to yields admin: point ldap_server_url at your own directory, set ldap_auto_provision and ldap_default_group=Administrators, and the next login on a fresh username is provisioned into the admin group. Overwrite-off is enough on an instance that never configured LDAP - there are no rows to skip. Refused by prefix so a key added to the LDAP schema later is refused by default, and matched case-insensitively because the key comes from the backup JSON rather than from our own writer. ldap_enabled leaves _COMPANION_CREDENTIALS rather than sitting there as dead code, since _is_protected_setting_key runs first. The two tests asserting an anonymous bind was a false positive are inverted - they encoded the hole - and the refusal reuses the existing settingsAuthSkipped note, which already points at Settings > Authentication. --- backend/app/services/github_restore.py | 41 +++++++- backend/tests/unit/test_github_restore.py | 110 +++++++++++++++++++--- 2 files changed, 135 insertions(+), 16 deletions(-) diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index ee2e15074..e118ca5c7 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -108,9 +108,10 @@ _MQTT_SETTING_KEYS = { # * bypass the lockout refusals ``update_settings`` enforces (a # ``local_login_enabled=false`` with no enabled OIDC provider, or with no # OIDC link on the caller, is a 400 there — #1589). -# * cross a permission boundary: /github-backup/restore is gated on -# GITHUB_RESTORE alone, so this would be a way to rewrite auth config -# without SETTINGS_UPDATE. +# * cross a permission boundary: a restore would be a way to rewrite auth +# config without SETTINGS_UPDATE. (The endpoint gates each category on the +# permission owning its rows now, but that is settings:update — still not +# the auth UI's own guards, which is what these keys actually need.) # # Auth is reconfigured through the auth UI, which has the guards. Restoring it # from a snapshot has no safe reading. @@ -121,6 +122,25 @@ _PROTECTED_SETTING_KEYS = { "setup_completed", } +# The LDAP family, refused for the same reason and by prefix rather than by +# name, so a key added to the schema later is refused by default. +# +# These are not "how the instance behaves" settings — together they name *which +# directory server decides who you are*. auth.py reads them live from this table +# on every login (see the ldap_keys list in _get_ldap_settings), so a restore +# that writes them substitutes the authentication source wholesale: +# ldap_server_url points at another directory, ldap_auto_provision creates a +# local account for whoever it vouches for, and ldap_default_group decides what +# that account gets — Administrators, if the backup says so. +# +# The companion rule does NOT cover this, which is the trap. ldap_enabled is +# paired with ldap_bind_password there, but an *anonymous* bind is a working +# config, so a backup that simply omits the password skips the refusal at the +# _COMPANION_EXPOSURE_TOGGLES check and the toggle is written. Omitting a +# credential is exactly what an attacker authoring this file would do — they own +# the directory being pointed at, so they need no bind credential from us. +_PROTECTED_SETTING_PREFIXES = ("ldap_",) + # Nozzle diameters the backup collector iterates. A path outside this set means # the backup was written by a newer version, so accept it rather than dropping # data, but keep the list for validation messages. @@ -175,7 +195,11 @@ def _is_blocked_setting_key(key: str) -> bool: def _is_protected_setting_key(key: str) -> bool: - return key in _PROTECTED_SETTING_KEYS + # Lowered for the prefix test for the same reason _is_blocked_setting_key + # lowers: the key comes from the backup's JSON, not from our own writer, so + # its casing is whatever the file says. An exact-match name stays exact — + # those four are ours and are only ever written lowercase. + return key in _PROTECTED_SETTING_KEYS or key.lower().startswith(_PROTECTED_SETTING_PREFIXES) # There used to be an ``_is_skipped_setting_key`` here, the union of the two @@ -202,9 +226,16 @@ def _is_protected_setting_key(key: str) -> bool: # virtual_printer_enabled is largely vestigial post-migration — core/database.py # copies the rows into the virtual_printers table — but it is the same shape, and # refusing a vestigial toggle is a harmless no-op. +# +# ldap_enabled is deliberately NOT here. It was, paired with +# ldap_bind_password — but this rule judges availability ("will the integration +# work?"), and that is the wrong question for an authentication source. An +# anonymous bind is a working config, so the pair let a backup omit the password +# and have the toggle written; the whole LDAP family is refused by prefix above +# instead. _is_protected_setting_key runs first in _plan_settings, so leaving the +# entry here would be dead code that reads like coverage. _COMPANION_CREDENTIALS = { "prometheus_enabled": "prometheus_token", - "ldap_enabled": "ldap_bind_password", "mqtt_enabled": "mqtt_password", "ha_enabled": "ha_token", "virtual_printer_enabled": "virtual_printer_access_code", diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index 31ffd89a4..1e7494fd2 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -109,10 +109,54 @@ class TestSettingKeyBlocklist: assert _is_blocked_setting_key(key) is False assert _is_protected_setting_key(key) is True - @pytest.mark.parametrize("key", ["currency", "ldap_enabled", "auth_secret_key"]) - def test_protected_set_is_only_the_auth_policy_keys(self, key): + @pytest.mark.parametrize("key", ["currency", "auth_secret_key", "mqtt_enabled", "prometheus_enabled"]) + def test_protected_set_does_not_swallow_ordinary_or_credential_keys(self, key): assert _is_protected_setting_key(key) is False + @pytest.mark.parametrize( + "key", + [ + "ldap_enabled", + "ldap_server_url", + "ldap_search_base", + "ldap_user_filter", + "ldap_security", + "ldap_group_mapping", + "ldap_auto_provision", + "ldap_ca_cert_path", + "ldap_default_group", + "ldap_bind_dn", + "LDAP_ENABLED", + "ldap_something_added_later", + ], + ) + def test_the_whole_ldap_family_is_protected(self, key): + """Together these name *which directory decides who you are*. + + ``auth.py`` reads them live from this table on every login, so a restore + that writes them substitutes the authentication source: point + ``ldap_server_url`` at another directory, set ``ldap_auto_provision``, + and ``ldap_default_group`` decides what the account it creates gets. + + The companion rule did not cover this and could not: it pairs + ``ldap_enabled`` with ``ldap_bind_password`` and asks whether the + integration will *work*, and an anonymous bind works — so a payload that + simply omitted the password had its toggle written. Refused by prefix so + a key added to the LDAP schema later is refused by default, and matched + case-insensitively because the key comes from the backup's JSON rather + than from our own writer. + """ + assert _is_protected_setting_key(key) is True + + def test_ldap_enabled_is_not_also_a_companion_toggle(self): + """It was, and the pair is what let the family through. + + Kept as a test rather than a comment because re-adding it would read as + tightening the rule while actually being dead code — + ``_is_protected_setting_key`` runs first in ``_plan_settings``. + """ + assert "ldap_enabled" not in _COMPANION_CREDENTIALS + def test_ha_token_from_env_is_deliberately_not_carved_out(self): """Recorded so the review's question about it is not re-litigated. @@ -348,6 +392,39 @@ class TestCompanionCredentials: await self._restore(db_session, **{toggle: "true", credential: "s3cret"}) assert toggle not in await self._rows(db_session) + @pytest.mark.asyncio + async def test_an_authored_ldap_payload_cannot_substitute_the_directory(self, db_session): + """The attack the companion rule could not see, refused end to end. + + Anyone who can write to the backup repository can author this file, and + the shape that beat the old rule is the natural one for an attacker: + *omit* ``ldap_bind_password``. They own the directory being pointed at, + so they need no bind credential from us — and an anonymous bind is a + working config, which is exactly what the availability rule was built to + allow through. + + Left unrefused, the next login against a fresh username binds to + ``ldap_server_url``, ``ldap_auto_provision`` creates the local account, + and ``ldap_default_group`` decides it is an Administrator. Overwrite-off + is enough on an instance that never configured LDAP: there are no rows + to skip. + """ + tally = await self._restore( + db_session, + currency="EUR", + ldap_enabled="true", + ldap_server_url="ldaps://evil.example.com:636", + ldap_security="ldaps", + ldap_search_base="dc=evil,dc=com", + ldap_user_filter="(uid={username})", + ldap_auto_provision="true", + ldap_default_group="Administrators", + ) + + rows = await self._rows(db_session) + assert rows == {"currency": "EUR"}, "not one LDAP row may land" + assert any("authentication" in note.lower() for note in _messages(tally)) + @pytest.mark.asyncio async def test_ha_toggle_is_refused_when_the_environment_has_no_token(self, db_session, monkeypatch): monkeypatch.delenv("HA_TOKEN", raising=False) @@ -508,14 +585,19 @@ class TestCompanionCredentials: async def test_the_availability_class_keeps_the_backup_credential_condition(self, db_session): """The other half of the same change: only Prometheus loses condition 2. - Absent is treated like blank here — an anonymous broker or bind is a - working config, so refusing it would be a false positive. + Absent is treated like blank here — an anonymous broker is a working + config, so refusing it would be a false positive. + + LDAP used to be in this list and is not any more: the same reasoning that + makes an anonymous bind legitimate is what let an authored payload point + the instance at another directory, so the family is refused outright + rather than judged on availability. See + ``test_the_whole_ldap_family_is_protected``. """ - await self._restore(db_session, mqtt_enabled="true", ldap_enabled="true", virtual_printer_enabled="true") + await self._restore(db_session, mqtt_enabled="true", virtual_printer_enabled="true") rows = await self._rows(db_session) assert rows["mqtt_enabled"] == "true" - assert rows["ldap_enabled"] == "true" assert rows["virtual_printer_enabled"] == "true" def test_every_exposure_toggle_is_a_companion_toggle(self): @@ -564,12 +646,18 @@ class TestCompanionCredentials: assert not any("switched off" in note for note in _messages(tally)) @pytest.mark.asyncio - async def test_an_anonymous_ldap_bind_is_not_a_false_positive(self, db_session): - """Same for a backup that carries the key with a blank value.""" - tally = await self._restore(db_session, ldap_enabled="true", ldap_bind_password=" ") + async def test_a_blank_ldap_bind_password_no_longer_lets_the_toggle_through(self, db_session): + """The inverted control, and the reason the LDAP pair had to go. - assert (await self._rows(db_session))["ldap_enabled"] == "true" - assert not any("switched off" in note for note in _messages(tally)) + A blank bind password used to read as "anonymous bind, a working config, + do not over-refuse". It reads the same way to an attacker authoring the + file, who wants no bind credential precisely because the directory is + theirs — so the availability question cannot be asked about an + authentication source at all. + """ + await self._restore(db_session, ldap_enabled="true", ldap_bind_password=" ") + + assert "ldap_enabled" not in await self._rows(db_session) @pytest.mark.asyncio async def test_turning_a_toggle_off_is_always_written(self, db_session):