Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lockdown suggestions #294

Closed
vToMy opened this issue Jul 13, 2022 · 13 comments
Closed

Lockdown suggestions #294

vToMy opened this issue Jul 13, 2022 · 13 comments

Comments

@vToMy
Copy link

vToMy commented Jul 13, 2022

  1. Is it possible to make saving/loading pairing record to the home folder optional?
  2. 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?
@doronz88
Copy link
Owner

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.

@vToMy
Copy link
Author

vToMy commented Jul 13, 2022

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.

@doronz88
Copy link
Owner

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.

@vToMy
Copy link
Author

vToMy commented Jul 14, 2022

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

@doronz88
Copy link
Owner

@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

@vToMy
Copy link
Author

vToMy commented Jul 14, 2022

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)
python/cpython#60691 (same issue in github format)
python/cpython#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.

@doronz88
Copy link
Owner

Since this is an incomplete solution I only added a TODO note referring to this issue to avoid complicating the code with extra logic

@vToMy
Copy link
Author

vToMy commented Jul 31, 2022

@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.

@vToMy
Copy link
Author

vToMy commented Sep 14, 2022

@doronz88 what do you think? I just don't like pairing records lying around on the filesystem.

@doronz88
Copy link
Owner

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

@vToMy
Copy link
Author

vToMy commented Sep 14, 2022

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.

@doronz88
Copy link
Owner

doronz88 commented Sep 14, 2022

what do you think about 6c9cfc3?

@vToMy
Copy link
Author

vToMy commented Sep 15, 2022

Great! Thanks.

@vToMy vToMy closed this as completed Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants