Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
949323003:
Write adb public key to devices during provision. (Closed)
|
Created:
5 years, 10 months ago by navabi Modified:
5 years, 9 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWrite adb public key to devices during provision.
Add a command-line argument for adb keys to be pushed onto
the devices being provisioned. This can be used by bots to
provide adb authentication.
BUG=
Committed: https://crrev.com/00c5fd30f5991deab5ba034d2f9829f0609342c7
Cr-Commit-Position: refs/heads/master@{#318325}
Patch Set 1 #Patch Set 2 : Add adb_key file. #
Total comments: 4
Patch Set 3 : Change name of keys file to adbkeys.pub. #Patch Set 4 : Write adb public keys from $HOME/.android/ #
Total comments: 5
Patch Set 5 : Write adb keys from $HOME/.android/ directory. #
Total comments: 4
Patch Set 6 : Make --adb-key-files an argument #
Total comments: 4
Patch Set 7 : Write adb keys passed as args and seperate out write adb keys function. #
Total comments: 11
Patch Set 8 : Use os.path.dirname and allow multiple keys in adb key files. #
Total comments: 4
Patch Set 9 : Use posixpath and nit fix. #Messages
Total messages: 33 (7 generated)
navabi@google.com changed reviewers: + friedman@chromium.org, jbudorick@chromium.org, perezju@chromium.org, vadimsh@chromium.org
Context?
friedman@google.com changed reviewers: + friedman@google.com
https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/adb_key File build/android/pylib/adb_key (right): https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/adb_... build/android/pylib/adb_key:1: QAAAALMQ6qSF4WhrptFFI7/OGJHf77IBaXEyMKNSh0ih5KOP+rqDvjmWfvXqX/Za9nypQdBxddXhoeS7OAg7LJSXEF3iK5rRFpMq36b5Yhkd2O32WqalNUxGKAbUgOTXj/83D6YBT90qXw1Dz3CsbFRkByCTVdHjbkFF4TENPhwUFIu4G7wPmfqf/TO6g3gnyOenbGJx7Uc9KQu6VNK6jxhe7hbpBbpqlR6B2uuGH3w5U8qlwaYzf+dIt7KMAThNoglfL7CuLNDjOI+K8Beu9rncr6NfXVs2/6m9BRt84szYjhicpo6VWPFMyla7oUuMJANGYh6qj9uO3lPlcW7wu9v+JZaIEJbY9ffvgoKw4bIvrv7QIc32EBWijMOfIxsnm203z2YJVGzSOsJWrI0oTqQXfm0/WW+uKZ6fKv8lzovMzVqONV8FosYkK4OhGnWWrE7qXMr3sBqtJrAIpheyLP3qhrFFc0Fpf8IKjPc1d4RfSUVFCyEApCwvqdqniQZUriGfD00z08m+dMjR7551pKU24v1JZGRlr2jE+UsUDUfBw/k6QCc6e7lQl2A8ubf8S22RAWRuMi/NaPm1GHuKwThYwy9GHrJxrY8zxIaz9hIm6mB1E9Er6hVWs6fW3yxz1i8EX8dtwZLM/aR3ek/nOBqdseqpCCgDMhqFEpHQpHlRHwCEmmXIbQEAAQA= chrome-infrastructure-adb-key Rename this file adbkey.pub. https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/cons... File build/android/pylib/constants.py (right): https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/cons... build/android/pylib/constants.py:149: 'adb_key') rename this adbkey.pub
navabi@google.com changed reviewers: - friedman@google.com
John, I updated the description to include more context. https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/adb_key File build/android/pylib/adb_key (right): https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/adb_... build/android/pylib/adb_key:1: QAAAALMQ6qSF4WhrptFFI7/OGJHf77IBaXEyMKNSh0ih5KOP+rqDvjmWfvXqX/Za9nypQdBxddXhoeS7OAg7LJSXEF3iK5rRFpMq36b5Yhkd2O32WqalNUxGKAbUgOTXj/83D6YBT90qXw1Dz3CsbFRkByCTVdHjbkFF4TENPhwUFIu4G7wPmfqf/TO6g3gnyOenbGJx7Uc9KQu6VNK6jxhe7hbpBbpqlR6B2uuGH3w5U8qlwaYzf+dIt7KMAThNoglfL7CuLNDjOI+K8Beu9rncr6NfXVs2/6m9BRt84szYjhicpo6VWPFMyla7oUuMJANGYh6qj9uO3lPlcW7wu9v+JZaIEJbY9ffvgoKw4bIvrv7QIc32EBWijMOfIxsnm203z2YJVGzSOsJWrI0oTqQXfm0/WW+uKZ6fKv8lzovMzVqONV8FosYkK4OhGnWWrE7qXMr3sBqtJrAIpheyLP3qhrFFc0Fpf8IKjPc1d4RfSUVFCyEApCwvqdqniQZUriGfD00z08m+dMjR7551pKU24v1JZGRlr2jE+UsUDUfBw/k6QCc6e7lQl2A8ubf8S22RAWRuMi/NaPm1GHuKwThYwy9GHrJxrY8zxIaz9hIm6mB1E9Er6hVWs6fW3yxz1i8EX8dtwZLM/aR3ek/nOBqdseqpCCgDMhqFEpHQpHlRHwCEmmXIbQEAAQA= chrome-infrastructure-adb-key On 2015/02/24 20:47:11, friedman1 wrote: > Rename this file adbkey.pub. Done. https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/cons... File build/android/pylib/constants.py (right): https://codereview.chromium.org/949323003/diff/20001/build/android/pylib/cons... build/android/pylib/constants.py:149: 'adb_key') On 2015/02/24 20:47:11, friedman1 wrote: > rename this adbkey.pub Done.
I think this is the wrong approach to a good idea. If the private key is local to the bots, the public key should also be limited to the bots -- i.e., it should live in the infrastructure repos, not chromium. We could then add an option to provision_devices to pass a key file that we write to the device.
PTAL. How about this approach? We don't check any public adb key into the chromium directory. Instead, it looks in the host ~/.android directory and puts any adb public key in that directory on the device. This allows us to roll-out by updating our bots to have the correct adb keys in their ~/.android directory. This also supports having multiple adb keys, which we can use to roll-out this change.
PTAL. How about this approach? We don't check any public adb key into the chromium directory. Instead, it looks in the host ~/.android directory and puts any adb public key in that directory on the device. This allows us to roll-out by updating our bots to have the correct adb keys in their ~/.android directory. This also supports having multiple adb keys, which we can use to roll-out this change.
PTAL. How about this approach? We don't check any public adb key into the chromium directory. Instead, it looks in the host ~/.android directory and puts any adb public key in that directory on the device. This allows us to roll-out by updating our bots to have the correct adb keys in their ~/.android directory. This also supports having multiple adb keys, which we can use to roll-out this change.
friedman@google.com changed reviewers: + friedman@google.com
https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... build/android/provision_devices.py:28: from sets import Set set is a python builtin https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... build/android/provision_devices.py:135: adb_keys = Set([]) adb_keys = set() https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... build/android/provision_devices.py:148: adb_key_contents = '%s/n%s' % (adb_key_contents, adb_key) \n I take it this is to avoid deailing with the extra newline instead of doing +=, but you will start out with a blank newline this way...
https://codereview.chromium.org/949323003/diff/80001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/80001/build/android/provision_... build/android/provision_devices.py:130: adb_dir_exists = device.FileExists(os.path.dirname(constants.ADB_KEYS_FILE)) I still think it'd be better to pass in a file via a command-line otpion than to hard code the key file path in here. If you'd like to have the key located at ~/.android/adbkey.pub, you can still do that -- just have the recipe pass the appropriate path to provision_devices. If you want to support multiple adb keys (as you do here), you could allow the option to be specified multiple times. https://codereview.chromium.org/949323003/diff/80001/build/android/provision_... build/android/provision_devices.py:148: for adb_key in adb_keys: this is just adb_key_contents = ''.join('%s\n' % s for s in reversed(adb_keys)) right? - do you want it to be in reverse order? - do you want the newline at the end? if both are no, this could be just adb_key_contents = '\n'.join(adb_keys)
PTAL. I made it an argument and updated the description. https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... build/android/provision_devices.py:28: from sets import Set On 2015/02/24 23:33:43, friedman1 wrote: > set is a python builtin Done. https://codereview.chromium.org/949323003/diff/60001/build/android/provision_... build/android/provision_devices.py:135: adb_keys = Set([]) On 2015/02/24 23:33:43, friedman1 wrote: > adb_keys = set() Done. https://codereview.chromium.org/949323003/diff/80001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/80001/build/android/provision_... build/android/provision_devices.py:130: adb_dir_exists = device.FileExists(os.path.dirname(constants.ADB_KEYS_FILE)) On 2015/02/24 23:44:49, jbudorick wrote: > I still think it'd be better to pass in a file via a command-line otpion than to > hard code the key file path in here. If you'd like to have the key located at > ~/.android/adbkey.pub, you can still do that -- just have the recipe pass the > appropriate path to provision_devices. > > If you want to support multiple adb keys (as you do here), you could allow the > option to be specified multiple times. Done. https://codereview.chromium.org/949323003/diff/80001/build/android/provision_... build/android/provision_devices.py:148: for adb_key in adb_keys: On 2015/02/24 23:44:49, jbudorick wrote: > this is just > > adb_key_contents = ''.join('%s\n' % s for s in reversed(adb_keys)) > > right? > > - do you want it to be in reverse order? > - do you want the newline at the end? > > if both are no, this could be just > > adb_key_contents = '\n'.join(adb_keys) Done.
https://codereview.chromium.org/949323003/diff/100001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/100001/build/android/provision... build/android/provision_devices.py:164: file_path = os.path.join(os.environ['HOME'], '.android', adb_key_file) to be clear: I don't think this should be locked into ~/.android. I think the caller should pass the path, not just the file name. https://codereview.chromium.org/949323003/diff/100001/build/android/provision... build/android/provision_devices.py:168: path_list = constants.ADB_KEYS_FILE.split('/') Why is this now just duplicating the logic from WipeDeviceData?
https://codereview.chromium.org/949323003/diff/100001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/100001/build/android/provision... build/android/provision_devices.py:164: file_path = os.path.join(os.environ['HOME'], '.android', adb_key_file) On 2015/02/25 01:32:46, jbudorick wrote: > to be clear: I don't think this should be locked into ~/.android. I think the > caller should pass the path, not just the file name. Yep. This is a mistake. options.adb_key_files will be the files that are passed in as arguments (e.g. ~/.android/adbkeys.pub). https://codereview.chromium.org/949323003/diff/100001/build/android/provision... build/android/provision_devices.py:168: path_list = constants.ADB_KEYS_FILE.split('/') On 2015/02/25 01:32:46, jbudorick wrote: > Why is this now just duplicating the logic from WipeDeviceData? The only part that is duplicated, is the part that writes to the file. It will write a different set of adb keys to the file. I'll move the write to /data/misc/adb/adb_keys to a different function.
PTAL. There is now an option --adb-keys-files <file1> <file2> ... WipeData still does the same thing (i.e. restores /data/misc/adb/adb_keys). When this lands, we can run provision_devices.py with --adb-keys-files and the adb public keys, and it they will be written to the devices. The WipeDevices function will retain the adb keys. This has the advantage of not making us change the recipes to pass the arguments. Instead, we can just run the provision script once on all the hosts we need. Alternatively, we can change the recipes to take --adb-keys-files and then it will write the key files to the device, and then when it is wiped, the keys stay on the bots.
This is looking good. Just adding a few comments. https://codereview.chromium.org/949323003/diff/120001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:118: dir_path = '/'.join(path_list[:len(path_list)-1]) nit: maybe that's just me, but I would go for: dir_path = posixpath.dirname(constants.ADB_KEYS_FILE) https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:165: adb_keys = set() nit: does it really need to be a set? do we expect passing more than one file *and* have the same public key appear in multiple files? https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:168: adb_public_key = f.readlines()[0] why do we keep only the first line? (earlier versions of your CL kept all of the lines) Also, if you only want the first line you can avoid reading the whole file by doing instead: adb_public_key = next(f).rstrip()
https://codereview.chromium.org/949323003/diff/120001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:118: dir_path = '/'.join(path_list[:len(path_list)-1]) On 2015/02/25 09:35:41, perezju wrote: > nit: maybe that's just me, but I would go for: > dir_path = posixpath.dirname(constants.ADB_KEYS_FILE) I didn't know about posixpath. We should be using that elsewhere for device path manipulations... https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:171: WriteAdbKeysFile(device, adb_key_contents) This will wipe existing keys from the device. Would that cause problems before the hosts have the private keys? Should this preserve any keys on the device and just add the keys from the key files?
Thanks for the reviews. https://codereview.chromium.org/949323003/diff/120001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:118: dir_path = '/'.join(path_list[:len(path_list)-1]) On 2015/02/25 14:09:07, jbudorick wrote: > On 2015/02/25 09:35:41, perezju wrote: > > nit: maybe that's just me, but I would go for: > > dir_path = posixpath.dirname(constants.ADB_KEYS_FILE) > > I didn't know about posixpath. We should be using that elsewhere for device path > manipulations... Yeah I don't like this split. But how about using: dir_path = os.path.dirname(constants.ADB_KEYS_FILE) Is posixpath preferred over os.path.dirname? I changed to use os.path.dirname for now. https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:165: adb_keys = set() On 2015/02/25 09:35:41, perezju wrote: > nit: does it really need to be a set? do we expect passing more than one file > *and* have the same public key appear in multiple files? Not the way the current adb keys on the host are, but following up on your comment on 168, let's keep it a set in case in the future we end up with multiple adb keys in a adb key file on the host. Then there may be an intersection of adb keys between multiple files that are passed in. https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:168: adb_public_key = f.readlines()[0] On 2015/02/25 09:35:41, perezju wrote: > why do we keep only the first line? (earlier versions of your CL kept all of the > lines) > > Also, if you only want the first line you can avoid reading the whole file by > doing instead: > adb_public_key = next(f).rstrip() Because before we were reading the adb keys file on the device which have multiple lines. Here we are reading individual adb keys on the host, and the way we have it set up is that we have multiple adb key files each with one adb key. It *could* have multiple lines per adb key on the host, so I changed it. https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:171: WriteAdbKeysFile(device, adb_key_contents) On 2015/02/25 14:09:07, jbudorick wrote: > This will wipe existing keys from the device. Would that cause problems before > the hosts have the private keys? Should this preserve any keys on the device and > just add the keys from the key files? This will not cause problems before the hosts have the private keys. The reason is when we run this command to put the shared public adb key onto the devices for the first time, we will be careful to also include the existing public key on the host, which is currently used to authenticate adb on the devices. I intend this option to overwrite the adb keys. It is up to the caller to make sure they overwrite the adb keys the right way (e.g. include the existing public keys so we don't lose authentication). We have plans on how to do that. We will test first on one bot before deploying everywhere.
PTAL
https://codereview.chromium.org/949323003/diff/120001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:118: dir_path = '/'.join(path_list[:len(path_list)-1]) On 2015/02/25 18:57:25, navabi wrote: > On 2015/02/25 14:09:07, jbudorick wrote: > > On 2015/02/25 09:35:41, perezju wrote: > > > nit: maybe that's just me, but I would go for: > > > dir_path = posixpath.dirname(constants.ADB_KEYS_FILE) > > > > I didn't know about posixpath. We should be using that elsewhere for device > path > > manipulations... > > Yeah I don't like this split. But how about using: > dir_path = os.path.dirname(constants.ADB_KEYS_FILE) > > Is posixpath preferred over os.path.dirname? I changed to use os.path.dirname > for now. It depends on the path being manipulated: - for host paths, os.path is preferred - for device paths, we have historically handrolled path manipulation (e.g. '/'.join(path_components), path.split('/')). However, posixpath handles paths the way we want for devices. https://codereview.chromium.org/949323003/diff/120001/build/android/provision... build/android/provision_devices.py:171: WriteAdbKeysFile(device, adb_key_contents) On 2015/02/25 18:57:25, navabi wrote: > On 2015/02/25 14:09:07, jbudorick wrote: > > This will wipe existing keys from the device. Would that cause problems before > > the hosts have the private keys? Should this preserve any keys on the device > and > > just add the keys from the key files? > > This will not cause problems before the hosts have the private keys. The reason > is when we run this command to put the shared public adb key onto the devices > for the first time, we will be careful to also include the existing public key > on the host, which is currently used to authenticate adb on the devices. This makes me a little nervous, but ok. > > I intend this option to overwrite the adb keys. It is up to the caller to make > sure they overwrite the adb keys the right way (e.g. include the existing public > keys so we don't lose authentication). We have plans on how to do that. We will > test first on one bot before deploying everywhere. https://codereview.chromium.org/949323003/diff/140001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/140001/build/android/provision... build/android/provision_devices.py:117: dir_path = os.path.dirname(constants.ADB_KEYS_FILE) Since this is a device path, not a host path, I think switching to os.path is wrong here. https://codereview.chromium.org/949323003/diff/140001/build/android/provision... build/android/provision_devices.py:170: WriteAdbKeysFile(device, adb_key_contents) optional nit: these two lines could be collapsed into WriteAdbKeysFile(device, '\n'.join(adb_keys))
PTAL https://codereview.chromium.org/949323003/diff/140001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/949323003/diff/140001/build/android/provision... build/android/provision_devices.py:117: dir_path = os.path.dirname(constants.ADB_KEYS_FILE) On 2015/02/25 21:47:39, jbudorick wrote: > Since this is a device path, not a host path, I think switching to os.path is > wrong here. Done. Using posixpath. https://codereview.chromium.org/949323003/diff/140001/build/android/provision... build/android/provision_devices.py:170: WriteAdbKeysFile(device, adb_key_contents) On 2015/02/25 21:47:38, jbudorick wrote: > optional nit: these two lines could be collapsed into > > WriteAdbKeysFile(device, '\n'.join(adb_keys)) Done.
lgtm
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949323003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/00c5fd30f5991deab5ba034d2f9829f0609342c7 Cr-Commit-Position: refs/heads/master@{#318325} |
