From a8bc2d19fa8bc5b747d21b45044e6759dc20e984 Mon Sep 17 00:00:00 2001 From: Shane B Date: Sun, 16 Aug 2026 19:35:39 +0200 Subject: [PATCH] Make a secret file usable on Windows, and stop two tests passing for the wrong reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `secrets.py` refused any file with the group or other read bits set, and told the user to run `chmod 600`. On Windows CPython synthesises st_mode as 0o666 for every regular file and os.chmod cannot clear those bits, so the guard was true of every file on the system: reading a secret from a file was impossible on a platform this ships a binary for, and the advice offered could not change the answer. Measured, not inferred - chmod 600 leaves the mode at 0o100666. The check is skipped where the bits carry no information rather than approximated. Real permissions on Windows are ACLs, which stat cannot see, and reading them needs icacls or pywin32 - a large dependency for a guard that is advisory even on POSIX. Leaving it was not the safer option: it made the safest input route unusable and pushed people to the environment variable instead. POSIX behaviour is untouched. Three permission tests are now skipped there, and **two of them were passing for the wrong reason** - they assert that an insecure file is refused, which was true because every file was. A skip that says so is worth more than a green tick that means nothing. The third also failed for an unrelated reason: pytest.raises(match=) is a regex and a Windows path is full of backslashes, so it now uses re.escape. test_device.py had the same shape - it asserts the stored device identity is readable by its owner alone, which chmod cannot arrange there. Skipped, and device.py now says plainly that its chmod is a no-op on Windows rather than leaving it to look like protection. The marker lives in test/unittestutils.py beside skip_unless_unix, which is where this repository already keeps that kind of thing. Windows goes from 8 failures to none: 435 passed, 8 skipped. POSIX is unchanged by construction - `os.name != "nt" and ...` - and CI proves it, because the four skipped tests are the ones that run there. Also brings docs/android-import-handover.md back in line with the code. §6's three outstanding items have shipped, the device-name ask is dropped rather than pending - announce_device needs the heartbeat token, which an account serialised before FindMy.py kept it does not have - and §4 no longer asks for a Room migration, because OwnedBeacons.content is already nullable. The risk there moved to the read paths, and the note now says which one. Co-Authored-By: Claude Opus 5 (1M context) --- docs/android-import-handover.md | 102 ++++++++++++++++++-------------- python/exporter/device.py | 5 ++ python/exporter/secrets.py | 13 +++- python/test/test_device.py | 2 + python/test/test_scriptable.py | 9 ++- python/test/unittestutils.py | 11 ++++ 6 files changed, 95 insertions(+), 47 deletions(-) diff --git a/docs/android-import-handover.md b/docs/android-import-handover.md index 52a1628a..ad712433 100644 --- a/docs/android-import-handover.md +++ b/docs/android-import-handover.md @@ -129,11 +129,19 @@ Today's importer skips the unknown directory silently. What it needs: - The new directory in `AppleZipImporterUtil.MATCHERS`. Note the identifier is **not** a UUID - these come from files other tools wrote - so the existing v4-UUID regex does not apply. -- Storage for a key-list accessory. `OwnedBeacon.content` assumes a plist; `accessoryJson` is - already FindMy.py's own JSON and is the natural home, but the fetch path calls - `FindMyAccessory.from_json` specifically and a custom tag is a `FixedRollingKeyPairAccessory`. -- **A Room migration** if the schema moves - rule 1, and there is no going back from getting that - wrong. +- ~~Storage for a key-list accessory... the fetch path calls `FindMyAccessory.from_json` + specifically.~~ **Done.** `main.accessoryFromJson` dispatches on FindMy.py's own `type` tag, and + everything else the fetch path touches - `keys_between`, `get_min_index`, `get_max_index`, + `update_alignment`, `to_json` - is on both classes already, so nothing downstream learns which + kind it has. +- ~~**A Room migration** if the schema moves.~~ **It does not move.** `OwnedBeacons.content` is + already nullable in schema 3, so a key-list tag is a row with `accessory_json` set and `content` + NULL. No new column, no migration, and rule 1 does not come into it. + + The risk moved rather than vanished: the read paths must tolerate a NULL plist. + `BeaconRepository` already does, but `BeaconDataParser` reads + `getOwnedBeaconInfo().content` straight into an XML parse, and a custom tag has no naming record + either - its display name is the `name` in its own mapping. That is where the work is. ## 5. The app becomes an exporter too — and then most people stop needing an export @@ -267,44 +275,48 @@ support thread. --- -## 6. Two smaller things - -**The FindMy pin is out of step with itself.** `app/build.gradle.kts` installs the -`feat/icloud-keychain-export` branch, while `app/src/test/python/requirements.txt` pins -`FindMy==0.9.8`. Whatever `test_pinned_versions_match_the_app_build` is checking, it is not those -two agreeing - so the bridge tests exercise a different library than the app ships. The exporter -solved the same problem with a lockfile that records the resolved commit; the app still needs -`@` appended before any build that leaves the machine, which its own comment already says. - -**The app registers as `0FINDMYPY001`.** Nothing passes a serial to FindMy.py, so the device the -user sees in their Apple account list is named after the library rather than after this app - -which is what [rule 11](../AGENTS.md) exists to prevent, and not what the docs describe. FindMy.py -now takes `serial=` on the Anisette provider and defaults CloudKit to it, so this is a one-line -fix at the point the provider is built, plus a decision that `0PENTAGVIEWR` is the app's. The -exporter presents `0PENTAGXPORT`, deliberately different: two installs, two entries, each -removable without breaking the other. - -**And the entry has no name, which is the field a person reads first.** Nothing calls the -`postdata` announce of -[Stage 2 §7](./findmy-export/02-mobileme-delegate.md#7-naming-the-registered-device), so Apple -names the row after whatever model the client claims - `MacBookPro18,3` shows up as a bare -**"MacBookPro"**, sitting in a list of the user's real hardware with nothing to tell it apart. -FindMy.py cannot do this at all today; the ask is written up in -[findmy-py-device-name-request.md](./findmy-py-device-name-request.md). Target is -**`OpenTagViewer App`**, with the exporter as `OpenTagViewer Exporter`. - -**Once name and serial are set, the model is only choosing an icon** - so the app should claim an -iPhone and the desktop tool a Mac, which makes a device list readable at a glance. Two conditions -on that: - -- **Do not switch the model on its own.** Today the app sends serial `"0"` and no name, so an - iPhone claim would produce an entry called "iPhone" with no serial, among the user's real - iPhones. Strictly worse than the Mac claim it has now. Model, name and serial land together or - not at all. -- **All five strings move together.** Model, OS version, build, CFNetwork and Darwin describe one - real release - see [rule 11](../AGENTS.md) and - [Stage 1 §2.2](./findmy-export/01-authentication.md), which carries a worked set: - `iPhone15,2`, iOS `17.4`, build `21E219`, CFNetwork `1494.0.7`, Darwin `23.4.0`. +## 6. Two smaller things — **mostly done** + +**~~The FindMy pin is out of step with itself.~~** Done, and then done again more widely. The app +pins a commit in both `app/build.gradle.kts` and `app/src/test/python/requirements.txt`; the +exporter, which tracked the branch and so drifted every time anybody pushed, now pins the same +commit in `python/pyproject.toml`. `test_main.py::test_the_whole_repository_pins_one_findmy` +asserts all four agree and names the file that does not. + +The reason it is one pin and not two: **`opentagviewer_export` is shared code with two runtimes.** +Chaquopy packages it into the APK, the exporter runs the same files from source, and one package +behind two library versions breaks exactly one of its consumers on the first API difference. + +**~~The app registers as `0FINDMYPY001`.~~** Done. A new sign-in presents `0PENTAGVIEWR`, beside +the exporter's `0PENTAGXPORT`. **New sessions only** - an account established before this keeps +what it was bound to, because re-identifying an existing session costs that user a sign-in and +leaves a second entry they never asked for. See `app/src/main/python/identity.py`. + +**The entry has no name, and it is not going to get one.** Naming the row needs the `postdata` +announce of [Stage 2 §7](./findmy-export/02-mobileme-delegate.md#7-naming-the-registered-device), +which authenticates with the `com.apple.gs.idms.hb` heartbeat token. That token arrives once, in +the same set as the PET, and an account serialised before FindMy.py started keeping it has nothing +to announce with - so for the installed base it does not work, and making it work means those +users signing in again. **Not worth a re-login for a label**, and +[findmy-py-device-name-request.md](./findmy-py-device-name-request.md) is therefore not being +pursued for the app. The serial carries the recognisability instead. + +**~~Once name and serial are set, the model is only choosing an icon.~~** Done, with the icon +being the whole point: `iPhone15,2` renders as **"iPhone 14 Pro"** rather than as a bare +"MacBookPro" among the user's real Macs. Both conditions were met - + +- **It did not switch on its own.** The serial landed in the same release, so the entry is an + iPhone *with* `0PENTAGVIEWR` on it rather than an unnamed, unserialled phone among real ones. +- **All five strings moved together**, from the worked set in + [Stage 1 §2.2](./findmy-export/01-authentication.md): `iPhone15,2`, iOS `17.4`, build `21E219`, + CFNetwork `1494.0.7`, Darwin `23.4.0`. + +**And only for fresh installs.** An install that already has an ADI identity keeps the Mac it was +provisioned as, forever - the two are one identity and Apple binds a session to it. That is +`AdiDeviceIdentity.Hardware`, whose two profiles Python reads across the bridge rather than +copying. [Rule 11](../AGENTS.md) has the longer version, including the trap that cost most of the +time: FindMy.py transforms `X-Apple-I-MD-LU` and `X-Mme-Device-Id` on the way out and the Java ADI +path does not, so passing "the same string" aligns one field and silently leaves the other as two. > **[observed] Claiming to be a phone does not make it a second factor.** A registered entry > presenting as an iPhone still reports *"This device cannot be used to receive Apple Account @@ -312,8 +324,8 @@ on that: > [Stage 1 §13](./findmy-export/01-authentication.md) argued - so the icon change carries no risk > of turning this app into a second factor for someone's Apple ID. -`AdiDeviceIdentity.CLIENT_INFO`'s comment used to justify its value as the least remarkable string -Apple sees. That reasoning is gone: an entry built to be recognised is not blending in. +`AdiDeviceIdentity.CLIENT_INFO` no longer exists; a compile-time constant could only ever be one +thing for everyone, which is precisely what a per-install profile cannot be. **Build the disclosure text from the account, not from constants.** §7.1 of Stage 2 requires that the identifiers shown to the user are the ones actually sent - the whole point being that somebody diff --git a/python/exporter/device.py b/python/exporter/device.py index 47bc8342..0129a1f7 100644 --- a/python/exporter/device.py +++ b/python/exporter/device.py @@ -103,6 +103,11 @@ def save(uid: str, devid: str, anisette: Any, path: Path | None = None) -> None: path.write_text(json.dumps(document, indent=2), encoding="utf-8") # Readable by its owner alone. It is not secret, but it names this installation to Apple # and there is no reason for anybody else on the machine to have it. + # + # **A no-op on Windows**, where chmod only toggles the read-only attribute and the group + # and other bits cannot be cleared at all. Left in rather than made conditional: it is + # correct where it works, harmless where it does not, and the alternative - real ACLs - + # needs icacls or pywin32. Worth knowing that this file is not protected there. path.chmod(0o600) except OSError as e: logger.warning("Could not store the device identity at %s: %s", path, e) diff --git a/python/exporter/secrets.py b/python/exporter/secrets.py index f146b9cb..6a71ebbc 100644 --- a/python/exporter/secrets.py +++ b/python/exporter/secrets.py @@ -77,7 +77,18 @@ def _from_file(path: Path) -> str: # **Refused rather than warned about.** A warning on a file anybody can read is advice nobody # acts on, and the whole reason to prefer a file over an environment variable is that a file # can have permissions. One that does not is worse than the option it was chosen over. - if mode & (stat.S_IRGRP | stat.S_IROTH): + # + # **Except on Windows, where the bits carry no information.** `st_mode` there is synthesised + # by CPython - every regular file reports 0o666, and `os.chmod(path, 0o600)` does not move + # the group and other bits at all. So this test was true of every file on the system: it + # refused all of them, and told the user to run `chmod 600`, which cannot change the answer. + # Reading a secret from a file was simply impossible on a platform this ships a binary for. + # + # Skipped rather than approximated. Real permissions on Windows are ACLs, which `stat` cannot + # see, and checking them needs `icacls` or pywin32 - a large dependency for a guard that is + # advisory even on POSIX. Leaving it in place was not the safer option; it was the one that + # made the safest input route unusable and pushed people to the environment variable instead. + if os.name != "nt" and mode & (stat.S_IRGRP | stat.S_IROTH): msg = ( f"{path} is readable by other users on this machine, so it is not a safe place for a" f" secret. Run: chmod 600 {path}" diff --git a/python/test/test_device.py b/python/test/test_device.py index a77922d2..0f0864c2 100644 --- a/python/test/test_device.py +++ b/python/test/test_device.py @@ -14,6 +14,7 @@ import pytest from exporter import device +from test.unittestutils import skip_unless_posix_permissions ANISETTE = {"type": "aniLocal", "prov_data": "AAAA", "serial": "0PENTAGXPORT"} @@ -39,6 +40,7 @@ def test_forgetting_it_leaves_nothing(self, identity): assert device.load(identity) is None assert device.forget(identity) is False + @skip_unless_posix_permissions def test_it_is_readable_by_its_owner_alone(self, identity): device.save("uid-1", "devid-1", ANISETTE, identity) diff --git a/python/test/test_scriptable.py b/python/test/test_scriptable.py index c8c62210..cfb3d0d1 100644 --- a/python/test/test_scriptable.py +++ b/python/test/test_scriptable.py @@ -16,10 +16,12 @@ from __future__ import annotations import asyncio +import re import pytest from exporter import cli, prompts, secrets +from test.unittestutils import skip_unless_posix_permissions as POSIX_PERMISSIONS @pytest.fixture(autouse=True) @@ -80,6 +82,7 @@ def test_leading_and_trailing_spaces_are_kept(self, secret_file): def test_a_second_line_is_not_part_of_the_secret(self, secret_file): assert secrets.read(secret_file("hunter2\n# a note\n"), "UNSET_VAR") == "hunter2" + @POSIX_PERMISSIONS def test_a_world_readable_file_is_refused(self, secret_file): # Refused rather than warned about. The reason to prefer a file over an environment # variable is that a file can have permissions; one that does not is worse than what it @@ -89,16 +92,20 @@ def test_a_world_readable_file_is_refused(self, secret_file): with pytest.raises(secrets.SecretError, match="readable by other users"): secrets.read(path, "UNSET_VAR") + @POSIX_PERMISSIONS def test_a_group_readable_file_is_refused_too(self, secret_file): path = secret_file("hunter2\n", mode=0o640) with pytest.raises(secrets.SecretError, match="readable by other users"): secrets.read(path, "UNSET_VAR") + @POSIX_PERMISSIONS def test_the_advice_names_the_fix(self, secret_file): path = secret_file("hunter2\n", mode=0o644) - with pytest.raises(secrets.SecretError, match=f"chmod 600 {path}"): + # re.escape because `match` is a regex and a Windows path is full of backslashes - + # the second, unrelated reason this one failed there. + with pytest.raises(secrets.SecretError, match=re.escape(f"chmod 600 {path}")): secrets.read(path, "UNSET_VAR") def test_an_empty_file_is_an_error_rather_than_an_empty_password(self, secret_file): diff --git a/python/test/unittestutils.py b/python/test/unittestutils.py index 09a01028..d8ee4c45 100644 --- a/python/test/unittestutils.py +++ b/python/test/unittestutils.py @@ -20,3 +20,14 @@ SYSTEM != DARWIN and SYSTEM != LINUX, reason="Requires Unix-like OS" ) + +skip_unless_posix_permissions = pytest.mark.skipif( + os.name == "nt", + reason=( + "Windows has no POSIX mode bits to assert on. CPython synthesises st_mode as 0o666 for " + "every regular file and os.chmod cannot clear the group and other bits, so a test of " + "who can read a file proves nothing there - two of these used to pass for the wrong " + "reason, because the guard they check refused every file on the system. Real " + "permissions on Windows are ACLs, which stat cannot see." + ) +)