Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
572123002:
Installs test CA to android devices as part of browser startup so that apps can be tested with prox… (Closed)
DescriptionInstalls test CA to android devices as part of browser startup so that apps can be tested with proxy HTTPS server without code changes.
BUG=412826
Committed: https://crrev.com/f28d9cace2df4a9dcaa992c4df5b85e3b2674dbb
Cr-Commit-Position: refs/heads/master@{#296619}
Committed: https://crrev.com/10eb77a2c2843664e7d55d8a4535d66b7f95d9c8
Cr-Commit-Position: refs/heads/master@{#297653}
Patch Set 1 #
Total comments: 8
Patch Set 2 : code cleanup per Ned's recommendtions #
Total comments: 2
Patch Set 3 : calling functions directly to install/remove cert #
Total comments: 4
Patch Set 4 : generates CA cert per run #
Total comments: 8
Patch Set 5 : update #Patch Set 6 : resolve merge conflict #Patch Set 7 : use tempdir #
Total comments: 3
Patch Set 8 : update #
Total comments: 1
Messages
Total messages: 37 (9 generated)
wuhu@google.com changed reviewers: + nednguyen@google.com, slamm@google.com, tonyg@chromium.org
Please review when you have time. Thanks.
https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:28: os.pardir, os.pardir, os.pardir)) Use telemetry.core.util.GetChromiumSrcDir() instead. https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:35: signal.signal(signal.SIGINT, signal.SIG_DFL) This shouldn't be a public method. Some education for me: what does this do and why do we need this? https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:37: class CAError(Exception): CAError is not a very descriptive name. https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:271: if sys.platform.startswith('linux') or sys.platform == 'darwin': Use platform.GetHostPlatform().GetOSName() instead. https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:279: #TODO(wuhu) Implement removal once the API function has been added to WPR Hmhhh. What would happen if we don't uninstall test CA on the device after test?
See comments below. Thanks https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:35: signal.signal(signal.SIGINT, signal.SIG_DFL) On 2014/09/16 16:21:45, nednguyen wrote: > This shouldn't be a public method. > > Some education for me: what does this do and why do we need this? This is taken from webpagereplay.py and basically makes clean up of child process nicer: "Signal masks on Linux are inherited from parent processes. If anything invoking us accidentally masks SIGINT (e.g. by putting a process in the background from a shell script), sending a SIGINT to the child will fail to terminate it. Running this signal handler before execing should fix that problem" https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:279: #TODO(wuhu) Implement removal once the API function has been added to WPR On 2014/09/16 16:21:45, nednguyen wrote: > Hmhhh. What would happen if we don't uninstall test CA on the device after test? It's not a huge problem to leave the test CA on the device. Supposely, someone could potentially used certificate signed with the test CA and use it to serve malicious content, but that's very unlikely. For cleanliness, I think we should remove it if possible.
slamm@chromium.org changed reviewers: + slamm@chromium.org
https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:35: signal.signal(signal.SIGINT, signal.SIG_DFL) On 2014/09/16 17:42:58, wuhu wrote: > On 2014/09/16 16:21:45, nednguyen wrote: > > This shouldn't be a public method. > > > > Some education for me: what does this do and why do we need this? > > This is taken from webpagereplay.py and basically makes clean up of child > process nicer: > > "Signal masks on Linux are inherited from parent processes. If anything > invoking us accidentally masks SIGINT (e.g. by putting a process in the > background from a shell script), sending a SIGINT to the child will fail > to terminate it. Running this signal handler before execing should fix that > problem" Instead of forking a process, perhaps you could make the WPR call from this code. The server needs a separate process because it is long running.
Thanks Steve, that makes things a lot simpler. I'll modify adb_install_cert to throw appropriate exceptions so we can know when the calls failed. I'll add a dependency between the two issues.
https://codereview.chromium.org/572123002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:246: def _InstallTestCA(self): _InstallTestCA -> _InstallTestCa Treat "CA" like a word in CamelCase. https://codereview.chromium.org/572123002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:251: self._replay_dir + '/wpr_cert.pem') Would it be better to generate a new root CA in a temporary directory? That way, you would not need the replay directory here. https://codereview.chromium.org/572123002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:256: def _UninstallTestCA(self): _UninstallTestCA -> _RemoveTestCa https://codereview.chromium.org/572123002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:260: def _OpenCALogFile(self): This method is no longer needed.
Thanks for the feedback, changes have been made.
The CQ bit was checked by wuhu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/572123002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/572123002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:28: REPLAY_DIR = os.path.join( Please make this constant and the one below private as well. https://codereview.chromium.org/572123002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:277: if subprocess.call(cmd_line, **kwargs) != 0: Based on slamm's comment, can't you use the direct function call AndroidCertInstaller(...).install_cert(...) instead?
Ned, I think you commented on patch 2 instead of 4 :)
https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:27: import adb_install_cert #pylint: disable=F0401 nits: lack a space between '#' and 'pylint' https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:206: 'webpagereplay_logs', 'testca.pem') Is there any reason we can't use tmpdir for this? https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:266: os.remove(ca_cert_prefix + '-cert.p12') It looks like cleaning up these artifacts should be handled by remove_cert() method. Or you should create a directory that contain these and use shutil.rmtree() on that directory.
Please see comments below. Will update with a new patch to address the issues raised. https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:206: 'webpagereplay_logs', 'testca.pem') On 2014/09/22 19:38:56, nednguyen wrote: > Is there any reason we can't use tmpdir for this? This directory is already created for storing wpr logs, so I figured we can use it to keep all wpr related stuff together. https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:266: os.remove(ca_cert_prefix + '-cert.p12') On 2014/09/22 19:38:56, nednguyen wrote: > It looks like cleaning up these artifacts should be handled by remove_cert() > method. Or you should create a directory that contain these and use > shutil.rmtree() on that directory. remove_cert is a general API that simply removes a cert from the device. We probably don't want to put other side effects inside. But it's a good idea to put stuff in one directory to cutdown on code.
https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:206: 'webpagereplay_logs', 'testca.pem') On 2014/09/22 20:43:53, wuhu wrote: > On 2014/09/22 19:38:56, nednguyen wrote: > > Is there any reason we can't use tmpdir for this? > > This directory is already created for storing wpr logs, so I figured we can use > it to keep all wpr related stuff together. If we delete it anyway, just make it a tempdir.
https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:206: 'webpagereplay_logs', 'testca.pem') On 2014/09/22 21:16:58, nednguyen wrote: > On 2014/09/22 20:43:53, wuhu wrote: > > On 2014/09/22 19:38:56, nednguyen wrote: > > > Is there any reason we can't use tmpdir for this? > > > > This directory is already created for storing wpr logs, so I figured we can > use > > it to keep all wpr related stuff together. > > If we delete it anyway, just make it a tempdir. A tempdir makes for sense to me too -- especially since the logs directory name is specific to logs. https://codereview.chromium.org/572123002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:254: ) Save some vertical space? self._cert_util = adb_install_cert.AndroidCertInstaller( self._adb.device_serial(), None, self._ca_cert_path)
LGTM Please wait for Slamm's approval.
LGTM... after some minor changes. https://codereview.chromium.org/572123002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:201: self._ca_cert_path = os.path.join(tempfile.mkdtemp(), Keep on one line since it fits. self._ca_cert_path = os.path.join(tempfile.mkdtemp(), 'testca.pem') https://codereview.chromium.org/572123002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:215: # Install test CA. Remove this comment since it does not add information. https://codereview.chromium.org/572123002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:258: ca_cert_dir = self._ca_cert_path[:-10] ca_cert_dir = os.path.dirname(self._ca_cert_path)
The CQ bit was checked by wuhu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/572123002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as d004b781792cf292fad5960a601f5be6e45623cb
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f28d9cace2df4a9dcaa992c4df5b85e3b2674dbb Cr-Commit-Position: refs/heads/master@{#296619}
Message was sent while issue was closed.
http://chromegw/i/chromium.perf/builders/Android%20Nexus7v2%20Perf/builds/1720 Traceback (most recent call last): <module> at tools/perf/run_benchmark:25 sys.exit(test_runner.main()) main at tools/telemetry/telemetry/test_runner.py:382 return command().Run(options) Run at tools/telemetry/telemetry/test_runner.py:189 return min(255, self._test().Run(args)) Run at tools/telemetry/telemetry/benchmark.py:95 page_runner.Run(pt, ps, expectations, finder_options, results) Run at tools/telemetry/telemetry/page/page_runner.py:428 page, credentials_path, possible_browser, results, state) _PrepareAndRunPage at tools/telemetry/telemetry/page/page_runner.py:259 finder_options) StartBrowserIfNeeded at tools/telemetry/telemetry/page/page_runner.py:55 self.browser = possible_browser.Create() Create at tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:118 android_platform_backend=self._platform_backend) __init__ at tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:214 self._InstallTestCa() _InstallTestCa at tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:247 cert_path=self._ca_cert_path) generate_dummy_ca_cert at third_party/webpagereplay/certutils.py:113 raise openssl_import_error ImportError: No module named OpenSSL
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/606533002/ by nednguyen@google.com. The reason for reverting is: Making the Android nexus perf bot all red..
The CQ bit was checked by wuhu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/572123002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 2a330387534b178b0f7e259023b75344eeb5ca1a
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/10eb77a2c2843664e7d55d8a4535d66b7f95d9c8 Cr-Commit-Position: refs/heads/master@{#297653}
Message was sent while issue was closed.
hjd@chromium.org changed reviewers: + hjd@chromium.org
Message was sent while issue was closed.
+cc: Armand This CL seems to have broken the bot here: http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_perf This is the build in question: http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_p... Based on the error message looks like we need to provision the bot (build15-a1) with the OpenSSL Python library but I'm not sure what the preferred strategy is for this (I assume that sshing in and randomly pip installing things is frowned upon :P) does anybody know what I need to do/who I should ask? https://codereview.chromium.org/572123002/diff/140001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/572123002/diff/140001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:30: import certutils # pylint: disable=F0401 This import seems to have broken http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_perf This is the build in question: http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_p... based on the error message looks like we need to provision the bot with the OpenSSL Python library.
Message was sent while issue was closed.
On 2014/10/01 18:05:01, hjd wrote: > +cc: Armand > > This CL seems to have broken the bot here: > http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_perf > > This is the build in question: > http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_p... > > Based on the error message looks like we need to provision the bot (build15-a1) > with the OpenSSL Python library but I'm not sure what the preferred strategy is > for this (I assume that sshing in and randomly pip installing things is frowned > upon :P) does anybody know what I need to do/who I should ask? > Hi hjd, Dirk has add OpenSSL Python library to build/install-build-deps.sh here: https://codereview.chromium.org/287053007/ +friedman@: I thought added the library to all linuxed based bot in: https://code.google.com/p/chromium/issues/detail?id=418309. Can you take a look at this issue? > https://codereview.chromium.org/572123002/diff/140001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py > (right): > > https://codereview.chromium.org/572123002/diff/140001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:30: > import certutils # pylint: disable=F0401 > This import seems to have broken > http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_perf > This is the build in question: > http://build.chromium.org/p/chromium.perf.fyi/builders/android_webview_aosp_p... > based on the error message looks like we need to provision the bot with the > OpenSSL Python library.
Message was sent while issue was closed.
nednguyen@google.com changed reviewers: + friedman@chromium.org
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/617263004/ by tonyg@chromium.org. The reason for reverting is: Userdebug build perf bots are still missing dependencies: ImportError: No module named OpenSSL Also, we'll need to gracefully degrade on user builds: CertInstallError: Cert Install Failed.
tonyg@chromium.org changed reviewers: - tonyg@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
