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." + ) +)