pymobiledevice3
pymobiledevice3 copied to clipboard
doronz88
Lockdown suggestions
- Is it possible to make saving/loading pairing record to the home folder optional?
- Ideally we could skip writing ssl.txt to the filesystem as well, but it seems python's api doesn't let us load ssl context from byte array rather then file. Unless you can think of some other way?
Where would you rather the pairing records to be stored instead? Or do you mean to make it all non volatile entirely?
We could also just write the SSL file as a temporary instead, but it helps for wireshark debugging.
I mean, the pairing record doesn't have to be stored. If I understand correctly it's just a cache used to avoid asking for trust again. Since pairing records can give you access to personal info on the device, having the option to not store them on the filesystem would be nice. Same for the ssl files. I'm only suggesting to add the option to not store them if we don't want to.
I see, Good point. What do you think the best way to pass this parameter to lockdown? As another optional parameter? I also wanted to add the timedelta to the certificate creation so maybe its getting messy.
You mean the timedelta for the expiration date? That would actually be nice.
Maybe some kind of another optional parameter pairing_records_cache_folder which will be None if no caching is used, or could be the path of the cache (currently ~/.pymobiledevice3).
Regarding the _ssl.txt files - the issue is with context.load_cert_chain(certfile, keyfile) (in service_connection.py) that doesn't accept byte array, only files, and cannot be monkey-patched as it's an internal function.
One possible solution is to indeed use temp files (I don't really like it...)
Alternatively, it may be possible to use pyOpenSSL context object instead:
https://www.pyopenssl.org/en/stable/api/ssl.html#context-objects
but it needs testing (theoretically the API should be compatible since both python and pyopenssl are wrappers of openssl).
@vToMy what do you think of current implementation? as long as the SSL certificate is still stored there's no point in avoiding disk writes. The pyOpenSSL api isn't very friendly so if you have a working snippet feel free to PR it also
Looking at PyOpenSSL I've convinced myself that it would not be the right approach, as it would require to use its ssl connection as well, and it's probably a better idea to keep using python's build-in ssl connection.
The ability to specify certificates from memory seems to be a long-time requested feature: https://bugs.python.org/issue16487 (original issue) https://github.com/python/cpython/issues/60691 (same issue in github format) https://github.com/python/cpython/pull/2449 (a stale PR) But looks like personal feelings and ego got in the way of it actually being implemented 🤦
Anyway, I still think making the pairing record cache folder optional would be nice as preparation for the future. As for the ssl files - it's either temp files for now, and switch to memory at some later python version, or just wait for future python version.
On another note - when we do store those files, it's probably better to allow some kind of encryption. Even python's context.load_cert_chain has an optional password parameter.
Since this is an incomplete solution I only added a TODO note referring to this issue to avoid complicating the code with extra logic
@doronz88 PR looks good. I still think allowing the pairing record cache folder to be none and not saving it to the disk would be nice.
@doronz88 what do you think? I just don't like pairing records lying around on the filesystem.
Assuming you already have one, you can use it instead of the locally stored. Also, while pairing, if usbmuxd supports it, it will store to filesystem also
I understand the purpose. I'll repeat my suggestion:
Allow pairing_records_cache_folder inside LockdownClient to be None, and in this case skip saving/loading pairing records from this folder, and only use usbmuxd.