Fastboot Boot support, Android 9 fix, Fixed PycryptodomeAuthSigner's sign method #152
Conversation
greateggsgreg
changed the title
|
@alusco Any chance this could be reviewed for merge? |
| @@ -323,7 +324,7 @@ def Connect(cls, usb, banner=b'notadb', rsa_keys=None, auth_timeout_ms=100): | |||
| 'Unknown AUTH response: %s %s %s' % (arg0, arg1, banner)) | |||
|
|
|||
| # Do not mangle the banner property here by converting it to a string | |||
| signed_token = rsa_key.Sign(banner) | |||
| signed_token = rsa_key.Sign(banner) + b'\0' | |||
There was a problem hiding this comment.
This is very important for Android 9 due to a change in struct apacket's 'data' field in adb.h
|
Hey I can look at merging it, though I maintain this at best effort. Give me a bit to review it at least at a high-level. |
adb/fastboot.py
Outdated
| @@ -151,24 +152,33 @@ def _AcceptResponses(self, expected_header, info_cb, timeout_ms=None): | |||
| FastbootInvalidResponse: Fastboot responded with an unknown packet type. | |||
|
|
|||
| Returns: | |||
| OKAY packet's message. | |||
| Tuple - OKAY packet's message, List of preceding Fastboot Messages | |||
There was a problem hiding this comment.
What was this change for? It seems like it just makes it so the INFO messages are now returned as well but could that break people since the return format here is new? To be honest it's not a huge deal since I'd imagine not a ton of people using FastbootProtocol directly but just curious
There was a problem hiding this comment.
Yeah I didn't like making this change because of API breakage, but in a few tools I've integrated this with I've found it's useful for callers to have a list of the FastbootMessages that were a part of the response to any SimpleCommands.
info_cb is useful for logging, but having a list of messages returned right after a call is made is good for making decisions based on specific returns from a command. 'OKAY' only returns if the command was successfully run, but does not capture if the command itself had issues (ie. fastboot boot may return 'OKAY', ['oem unlocking is not enabled'] )
| source_len = os.stat(source_file).st_size | ||
| source_file = open(source_file) | ||
| with open(source_file_path, 'rb') as fh: | ||
| source_file = BytesIO(fh.read()) |
There was a problem hiding this comment.
do we still need the data.encode('utf8') or was that a bug in the first place?
There was a problem hiding this comment.
By data.encode('utf-8') do you mean the BytesIO creation?
The behavior here was improved so that a caller to Download() can either pass in a file-like object directly or a string file path. In the event they pass in a string, we have to convert it into a file like object before it's passed into HandleDataSending(). This statement was previously leaving a file handle open, so we read the contents and put it into BytesIO so that it has file-like properties.
If this isn't what you meant then let me know!
There was a problem hiding this comment.
Oh I see what you're referring too. The diff below shows us running data.encode('utf-8') on the file contents. This was a bug because the images that are Downloaded are almost always binary/outside the utf-8 space and would fail.
|
Few minor questions but otherwise will merge it |
|
@alusco Completely missed the feedback you provided months ago, sorry! I've made corrections to the comments as requested and answered some of the questions you had above. Please let me know if there is anything else you need! |
Fixed implementation of PycryptodomeAuthSigner.sign() method to properly resign the provided token. Also made minor fix to stripping delimiters from the InteractiveShell output, eliminating a TODO