Add pyadb keygen command to generate keypairs by joeleong · Pull Request #144 · google/python-adb · GitHub

Should address #95 and #131

(I think) I reimplemented the custom public key structure from AOSP.
Add pyadb keygen command to mimic adb keygen

Note: I did update the dependencies since the pubkey code uses pycryptodome and six.
I was having problems similar to #109, so I think you need M2Crypto or rsa and pycryptodome (2 libraries). There is probably a way to implement the pubkey stuff with the other crypto libraries but I didn't bother. Happy to change things if necessary.

Coverage decreased (-2.2%) to 41.385% when pulling 01614ea on joeleong:add-custom-pubkey-encoding into d0be33c on google:master.

Let's help this thing reach master!

I'm not experienced in creating proper test cases, but I'm willing to try.
I have a minor improvement suggestion for struct usage in decode_pubkey (will post proposed patch soon).
I'd also like to try and make this work with the cryptography library (recently supported by this project).

Hopefully I'll be able to do it soon.
Thanks for your great work!

Started a review for adb/android_pubkey.py.
This is currently just a suggested cleanup, to increase readability for future reviews.
I will soon be inspecting the RSA-related code, with regards to:

The correctness of the algorithm
Library usage (specifically, porting to cryptography if possible).

Let me know what you think!

Verify endianness argument:
assert endianess in ('little', 'big')

Hey! Thanks for the review. I like your changes overall.

The only ones I'm unsure about are the size_in changes.
I'm not sure it's much clearer that way and might just be adding some unnecessary machinery, especially because it looks like its only used in two places. I'm probably open to it if you feel strongly about it, but how would you feel about leaving it the way it is, but maybe adding some comments to make it clearer?

I'll take a closer look when I get a chance and probably incorporate most/all of them.
It should be pretty easy to port to the other crypto lib but I haven't looked at it specifically.
I can help with that if you'd like.
The tests are probably most important.

Hi. I completely agree :)
Let me explain, and then tell me what you think. Maybe it's redundant after all.

The size_in function lets you convert sizes between different units.
In our case, we convert 2048 bits to 256 bytes and then 64 words.

But what if the modulus length is not a multiple of 32 or 8 bits?
e.g. 2047 bits would still require 256 bytes, and 2040 bits would require 255 bytes which are still 64 words.

The size_in is a simple formula which handles these cases.
I don't really know cryptography really well. Perhaps these cases are irrelevant by design.

Also agreed on the tests, probably most important. I'll try to get to those soon 👍

Co-Authored-By: Halastra <turchy@gmail.com>

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Implementation and some comments from the following: - [auth.cpp] (https://github.com/aosp-mirror/platform_system_core/blob/c55fab4a59cfa461857c6a61d8a0f1ae4591900c/adb/client/auth.cpp) - [android_pubkey.c] (https://github.com/aosp-mirror/platform_system_core/blob/c55fab4a59cfa461857c6a61d8a0f1ae4591900c/libcrypto_utils/android_pubkey.c) - [android_pubkey.h] (https://github.com/aosp-mirror/platform_system_core/blob/c55fab4a59cfa461857c6a61d8a0f1ae4591900c/libcrypto_utils/include/crypto_utils/android_pubkey.h) Note: It looks like [2dc4ca] (aosp-mirror/platform_system_core@2dc4cab) removes the use of the public key file and directly extracts the public key from the private key.

Address (google#95 and google#131)

With help from @Halastra suggestions from code review of google#144 - Define ANDROID_RSAPUBLICKEY_STRUCT - Remove use of six package - Fix some flake8 warnings Co-Authored-By: Halastra <turchy@gmail.com>

I rebased and incorporated most of your suggestions in 0791b9.
I think I'd prefer to skip the size_in suggestion for now, just because they're constants.
I also forgot to include your suggestion about asserting the endianess argument, but I don't think that's a big deal since it defaults to little if gibberish is passed and its a local function anyway.

I also ported it to use py-cryptography instead of pycryptodome.

Let me know what you think!

@googlebot I consent

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Well that took longer than expected.. but here goes.
A few quick notes:

This test won't actually run on Python 3+ because of TypeError: from_buffer() cannot return the address of a unicode object #159 (I will address this issue in Update sign_cryptography.py #157 or a new PR)
I'm misusing this platform because I don't know how to add commits to this PR. My apologies.
This test is simple and naive, but should be enough to cover most of your added functionality.
Let's hope TemporaryDirectory is allowed to run on the CI server :0

Add the following code under test/test_cryptography.py:

import unittest
from adb import sign_cryptography
from adb import android_pubkey
from tempfile import TemporaryDirectory
from os import path

class CryptographyTest(unittest.TestCase):
    def testKeygenAndSigner(self):
        private_key_file_name = 'adbkey'
        with TemporaryDirectory() as temp_dir:
            private_key_file_path = path.join(
                temp_dir, 
                private_key_file_name
            )
            android_pubkey.keygen(private_key_file_path)
            signer = sign_cryptography.CryptographySigner(private_key_file_path)

Regarding your previous notes:

I really like your work!
I might be tempted to add the size_in function in a future PR.
Same goes for the endianess (+ better default to 'little', to match bytes.fromhex)

Please add the above changes. I will review and approve them (promise!).

Hey thanks for your suggestions!

I haven't really been keeping up with this project, particularly #167 and all the stuff @JeffLIrion has been contributing, so it is a little unclear to me what the best path forward is. It's very possible his PR's could supersede this one.

I'm happy to incorporate your suggestions to this PR either directly or you can maybe submit a PR to https://github.com/joeleong/python-adb/tree/add-custom-pubkey-encoding and I'll push your commits here.

You are right in your observations. His work is indeed an improvement in many areas!
But since there's still no apparent USB support in adb-shell, I'll be using this package.

So I guess I'll submit a PR to your repository as you suggested.

This pull request contains @joeleong's key generation code as well as tests: #170

I also copied it over to adb-shell (and attributed it to @joeleong), along with tests.

Halastra reviewed Jul 23, 2019

View reviewed changes

joeleong commented Jul 23, 2019

View reviewed changes

joeleong added a commit to joeleong/python-adb that referenced this pull request Jul 24, 2019

Apply @Halastra suggestions from code review of google#144

1a9a9cc

Co-Authored-By: Halastra <turchy@gmail.com>

joeleong and others added 3 commits July 24, 2019 15:42

Add pyadb keygen command to generate keypairs

efa2a49

Address (google#95 and google#131)

Refactor android_pubkey.py struct usage

0a791b9

With help from @Halastra suggestions from code review of google#144 - Define ANDROID_RSAPUBLICKEY_STRUCT - Remove use of six package - Fix some flake8 warnings Co-Authored-By: Halastra <turchy@gmail.com>

joeleong force-pushed the add-custom-pubkey-encoding branch from 1a9a9cc to 0a791b9 Compare July 24, 2019 22:42

Replace pubkey pycryptodome with cryptography

01614ea

JeffLIrion mentioned this pull request Sep 24, 2019

New package adb-shell #167

Open

+              def _to_bytes(n, length, endianess='big'):
+                  """partial python2 compatibility with int.to_bytes
+                  https://stackoverflow.com/a/20793663"""
+                  if six.PY2:

Add pyadb keygen command to generate keypairs by joeleong · Pull Request #144 · google/python-adb · GitHub

Uh oh!

Add pyadb keygen command to generate keypairs - #144

Add pyadb keygen command to generate keypairs#144joeleong wants to merge 4 commits intogoogle:mastergoogle/python-adb:masterfrom joeleong:add-custom-pubkey-encodingjoeleong/python-adb:add-custom-pubkey-encodingCopy head branch name to clipboard

Conversation

joeleong commented Jan 7, 2019

Uh oh!

coveralls commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Halastra commented Jul 17, 2019

Uh oh!

Halastra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Halastra Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joeleong left a comment

Choose a reason for hiding this comment

Uh oh!

Halastra commented Jul 24, 2019

Uh oh!

googlebot commented Jul 24, 2019

Uh oh!

joeleong commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Halastra commented Nov 28, 2019

Uh oh!

googlebot commented Nov 28, 2019

Uh oh!

Halastra commented Nov 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joeleong commented Nov 28, 2019

Uh oh!

Halastra commented Nov 28, 2019

Uh oh!

JeffLIrion commented Nov 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add pyadb keygen command to generate keypairs#144
joeleong wants to merge 4 commits into
google:masterfrom
joeleong:add-custom-pubkey-encoding

coveralls commented Jan 7, 2019 •
edited
Loading

joeleong commented Jul 25, 2019 •
edited
Loading

Halastra commented Nov 28, 2019 •
edited
Loading