Verify TLS certificates, instead of disabling verification library-wide - #261
Open
parawanderer wants to merge 4 commits into
Open
Verify TLS certificates, instead of disabling verification library-wide#261parawanderer wants to merge 4 commits into
parawanderer wants to merge 4 commits into
Conversation
`HttpSession` passed `ssl=False` on every request, so every connection this
library makes -- authentication, CloudKit, the setup delegate -- was
unauthenticated and interceptable by anything on the network path.
The reason was one host. `gsa.apple.com` chains to the 2006 Apple Root CA, which
certifi does not ship and Mozilla-derived stores do not carry, so it fails
verification wherever those are in use while every other Apple host verifies
fine:
ssl.SSLCertVerificationError: certificate verify failed:
self-signed certificate in certificate chain
So bundle that one root and add it to the platform defaults, rather than
switching verification off. Adding a trust anchor does not weaken the others, and
the certificate is checked against its published SHA-256 before it is loaded, so
a mangled copy fails immediately rather than at the first handshake.
Measured live, comparing certifi's store against the context this builds:
gsa.apple.com fails under certifi and passes here; gateway.icloud.com,
setup.icloud.com and the escrow proxy pass under both, and were being sent
unverified for no reason at all.
Deliberately not pinning a leaf or an intermediate. A general TLS path has to
survive certificate rotation, and a client that pins one breaks on a day Apple
chooses; adding a root is the right strength.
One opt-out, on `RemoteAnisetteProvider(allow_unverified_https=True)`, off by
default and reaching no request but the one to that server. It exists for a
self-hosted Anisette server with a self-signed certificate; a server reached over
plain http:// needs nothing, since there is no TLS to verify. It is serialized
only when true, so existing account files are unchanged and reloading one cannot
silently turn a working self-signed setup into a failing one.
Owner
|
Good addition, thanks. This indeed originates from one of the first versions of the library, but this is definitely the right way to do it. I don't really see the advantage of checking the certs hash? It's embedded into the Python script, if someone can tamper with the certificate they can also just remove that check. Although I guess it doesn't hurt much, either. Also, you got keychain exporting working in Python? That's awesome! I've been a bit hesitant to do that because of potential changes by Apple in the future, but I'm curious to see how well it works for you. If you can fix the errors I'd be happy to merge. |
**`test (3.14)`**: the test asserted `len(context.get_ca_certs()) > 1` to show Apple's root was added on top of the platform's store rather than instead of it. Whether a platform loads its store eagerly from a file or lazily from a hashed directory differs by OS and by Python build — the runner returns one entry where this machine returns hundreds — so that count measured the machine, not the code. It now asserts the default context's anchors are a subset of this one's, which is the property meant and holds under both loading strategies. **Pre-commit**: ten missing docstrings and an import order in the new test file. Worth recording why they were invisible locally: `pyproject.toml` excludes `tests/`, but pre-commit passes explicit filenames and ruff only honours `exclude` during traversal unless `force-exclude` is set — so CI lints tests and `ruff check findmy examples scripts` never did. Reproduced with `ruff check $(git ls-files '*.py')`, which is what the hook actually does. Verified against the version that failed: the suite passes on 3.14.
Author
|
Quick response regarding the keychain export thing, from implementing it and documenting the spec I highly doubt they can break it in any major way because of coupling and backwards compatibility needs. |
basedpyright's `reportTypedDictNotRequiredAccess`: `allow_unverified_https` is written only when true, so it is `NotRequired` on the mapping and a subscript is an access that can raise. The assertion is unchanged — present, and true — and `.get` says what the type already said. Same cause as the previous commit and worth stating once: my verification was narrower than CI's on both linters. `ruff check findmy examples scripts` and `basedpyright findmy` never saw `tests/`, while pre-commit passes every tracked Python file explicitly. The reproduction is `<tool> $(git ls-files '*.py')`.
Owner
|
This is Apple, they will just build a new system, have devices migrate keychain items over and use both systems side by side for the next 10 years, while slowly crippling the old system :-). But good to know, I might reconsider integration then. I'm on vacation currently and reviewing from my phone doesn't work very well, but I'll try to get back to this next week. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi, this is part of a larger fork I made of the library that integrates a Clean-Room'ed implementation of native iCloud FindMy device fetching into this library, mainly for the purposes of this app I made before using it, and just to make the whole apple auth logic available to the public under MIT.
This was an issue I noticed while implementing the fork.
HttpSessionpassedssl=Falseon every request, so every connection this library makes -- authentication, CloudKit, the setup delegate -- was unauthenticated and interceptable by anything on the network path.The reason (from what I could derive) was probably that you were having an issue with one host:
gsa.apple.comchains to the 2006 Apple Root CA, which certifi does not ship and Mozilla-derived stores do not carry, so it fails verification wherever those are in use while every other Apple host verifies fine:So the proposal is to instead bundle that one root and add it to the platform defaults on every platform, rather than switching verification off entirely. Adding a trust anchor does not weaken the others, and the certificate is checked against its published SHA-256 before it is loaded, so a mangled copy fails immediately rather than at the first handshake.
Measured live, comparing certifi's store against the context this builds:
gsa.apple.comfails under certifi and passes here. Other apple-related endpoints likegateway.icloud.com,setup.icloud.comand the escrow proxy pass that my iCloud auth implementation fork uses pass under both, and would be sent unverified for no reason at all.Deliberately not pinning a leaf or an intermediate. A general TLS path has to survive certificate rotation, and a client that pins one breaks on a day Apple chooses; adding a root is the right strength.
Includes an opt-out via:
RemoteAnisetteProvider(allow_unverified_https=True), which is off by default and reaching no request but the one to that one server, addressing the use-case of a self-hosted Anisette server with a self-signed certificate (nothttp://servers which need nothing as there is no TLS to verify). It is serialized only when true, so existing account files are unchanged and reloading one cannot silently turn a working self-signed setup into a failing one.