Add pyadb keygen command to generate keypairs

joeleong commented 5 years ago

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.

coveralls commented 5 years ago

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

Halastra commented 5 years ago

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!

Halastra commented 5 years ago

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 👍

googlebot commented 5 years ago

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.

joeleong commented 5 years ago

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!

Halastra commented 4 years ago

@googlebot I consent

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Halastra commented 4 years ago

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

This test won't actually run on Python 3+ because of #159 (I will address this issue in #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!).

joeleong commented 4 years ago

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.

Halastra commented 4 years ago

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.

JeffLIrion commented 4 years ago

This pull request contains @joeleong's key generation code as well as tests: https://github.com/google/python-adb/pull/170

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

google / python-adb

Add pyadb keygen command to generate keypairs #144