Make Telemetry tests check the configuration of the OSX Keychain.

Make Telemetry tests check the configuration of the OSX Keychain.

Historically, the OSX Keychain has been misconfigured on some perf bots, which
has caused Telemtry tests to stall. This CL adds checks to emit warnings if the
Keychain is misconfigured.

I added checks for three common misconfigurations:
  - Is the default Keychain locked.
  - Is the default Keychain configured to automatically lock after a period of
  time.
  - Are the ACLs of a commonly accessed key in the default keychain
  misconfigured for the perf bots.

The first and third checks are performed by short C programs since the
information is easily available through the Security framework, but not from
the security CLI.

BUG=443340
Committed: https://crrev.com/0e07c2d7073bcb7b644fe1fe02031c55f5c1ccbe
Cr-Commit-Position: refs/heads/master@{#314921}

[erikchen, 2014-12-18 02:26:23]

Patchset #1 (id:1) has been deleted

[erikchen, 2014-12-18 02:26:31]

Patchset #1 (id:20001) has been deleted

[erikchen, 2014-12-18 02:34:19]

erikchen@chromium.org changed reviewers:
+ rsesek@chromium.org

[erikchen, 2014-12-18 02:34:20]

rsesek: Looking for a review of the c files
determine_if_keychain_entry_is_decryptable.c and
determine_if_keychain_is_locked.c.

The telemetry code is in python, and only includes 1, minimal gyp file, so I
thought it best to not create any dependencies on chromium code.

If you think there are good ways to use the security CLI to replace these 2 c
programs, I'm all ears. I'm pretty sure that determine_if_keychain_is_locked is
not replaceable. It's possible to get the ACL information from "security
dump-keychain -a", but it would involve a lot of regex parsing, plus it takes
several minutes to run "security dump-keychain -a" on my desktop.

[Robert Sesek, 2014-12-18 18:32:30]

Why aren't these part of the build/have gyp files? I think
determine_if_keychain_entry_is_decryptable could benefit from ScopedCFTypeRef.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/mac/determine_if_keychain_entry_is_decryptable.c
(right):

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/determine_if_keychain_entry_is_decryptable.c:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Is there any reason to not use C++ here for scopers?

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/determine_if_keychain_entry_is_decryptable.c:58:
for (int i = 0; i < CFArrayGetCount(acl_list); ++i) {
CFIndex

[erikchen, 2014-12-18 18:54:17]

erikchen@chromium.org changed reviewers:
+ dtu@chromium.org

[erikchen, 2014-12-18 18:54:17]

rsesek: tools/telemetry/ currently contains only 1 gyp file, and that gyp file
does not introduce any dependencies on the rest of the chromium repository. I
did not want to add a dependency on base, where one did not exist before. That
being said, let's just ask the telemetry OWNER who will be reviewing the rest of
this CL.

dtu: The two C files in this CL currently have no dependencies on the rest of
the Chromium repository. If we added a gyp file and included a dependency on
base/, we could use the C++ template ScopedCFTypeRef, which would simplify
determine_if_keychain_entry_is_decryptable.c. Would you prefer that we add a gyp
file/dependency, or keep the C files as is?

[dtu, 2014-12-19 21:43:56]

On 2014/12/18 18:54:17, erikchen wrote:
> rsesek: tools/telemetry/ currently contains only 1 gyp file, and that gyp file
> does not introduce any dependencies on the rest of the chromium repository. I
> did not want to add a dependency on base, where one did not exist before. That
> being said, let's just ask the telemetry OWNER who will be reviewing the rest
of
> this CL.
> 
> dtu: The two C files in this CL currently have no dependencies on the rest of
> the Chromium repository. If we added a gyp file and included a dependency on
> base/, we could use the C++ template ScopedCFTypeRef, which would simplify
> determine_if_keychain_entry_is_decryptable.c. Would you prefer that we add a
gyp
> file/dependency, or keep the C files as is?

Please don't add an additional dependency. There are plans to move Telemetry to
its own repo.

[erikchen, 2014-12-19 21:54:31]

rsesek: PTAL

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/mac/determine_if_keychain_entry_is_decryptable.c
(right):

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/determine_if_keychain_entry_is_decryptable.c:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/12/18 18:32:30, Robert Sesek wrote:
> Is there any reason to not use C++ here for scopers?

I don't use scopers because none come bundled with the Apple frameworks, the
telemetry team doesn't want to introduce dependencies on base/, and I didn't
want to write my own for this file.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/determine_if_keychain_entry_is_decryptable.c:58:
for (int i = 0; i < CFArrayGetCount(acl_list); ++i) {
On 2014/12/18 18:32:30, Robert Sesek wrote:
> CFIndex

Done.

[dtu, 2014-12-19 22:19:04]

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_backend.py:108:
if platform.system() != "Darwin":
Might be unsafe to assume the benchmarks are running on the current platform.

if self.browser.platform.GetOSName() != 'mac':

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_backend.py:111:
warning_suffix = 'which will cause some Telemetry tests to fail when run'\
style guide: don't use \, wrap in () instead.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/mac/keychain_helper.py
(right):

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/keychain_helper.py:15:
"""This class provides information about the default OSX Keychain."""
You can put all the functions at the module level without wrapping them in a
class. Python is not Java :)

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/keychain_helper.py:18: def
_FetchExecutableAtPath(path):
If you put the sha1 files under tools/telemetry/bin/mac, you can use
telemetry.util.support_binaries.FindPath to do the download and finding logic.

[Robert Sesek, 2014-12-19 22:23:53]

Code LGTM. Should this be gyp'd at least?

[erikchen, 2015-01-30 01:59:06]

Patchset #3 (id:80001) has been deleted

[erikchen, 2015-01-30 02:07:46]

Patchset #3 (id:100001) has been deleted

[erikchen, 2015-01-30 02:09:53]

dtu: PTAL

Also, let me know if you want me to make gyp files for the C programs, or if you
want me to leave them with compilation instructions as comments inside the file.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_backend.py
(right):

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_backend.py:108:
if platform.system() != "Darwin":
On 2014/12/19 22:19:04, dtu wrote:
> Might be unsafe to assume the benchmarks are running on the current platform.
> 
> if self.browser.platform.GetOSName() != 'mac':

This logic got moved into keychain_metric.py, which already does platform
checking with sys.platform. I didn't add, change, or remove any checks.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/desktop_browser_backend.py:111:
warning_suffix = 'which will cause some Telemetry tests to fail when run'\
On 2014/12/19 22:19:04, dtu wrote:
> style guide: don't use \, wrap in () instead.

Done.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/mac/keychain_helper.py
(right):

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/keychain_helper.py:15:
"""This class provides information about the default OSX Keychain."""
On 2014/12/19 22:19:04, dtu wrote:
> You can put all the functions at the module level without wrapping them in a
> class. Python is not Java :)

Done.

https://codereview.chromium.org/809473004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/mac/keychain_helper.py:18: def
_FetchExecutableAtPath(path):
On 2014/12/19 22:19:04, dtu wrote:
> If you put the sha1 files under tools/telemetry/bin/mac, you can use
> telemetry.util.support_binaries.FindPath to do the download and finding logic.

Done.

[dtu, 2015-02-02 20:22:11]

https://codereview.chromium.org/809473004/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/mac/keychain_helper.py (right):

https://codereview.chromium.org/809473004/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/mac/keychain_helper.py:47: version =
platform.GetHostPlatform().GetOSVersionNumber()
The OSVersionNames are comparable, so instead of adding a new API, you could do
GetOSVersionNumber() < platform_backend.LION

[erikchen, 2015-02-04 00:07:38]

dtu: PTAL

https://codereview.chromium.org/809473004/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/mac/keychain_helper.py (right):

https://codereview.chromium.org/809473004/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/mac/keychain_helper.py:47: version =
platform.GetHostPlatform().GetOSVersionNumber()
On 2015/02/02 20:22:11, dtu wrote:
> The OSVersionNames are comparable, so instead of adding a new API, you could
do
> GetOSVersionNumber() < platform_backend.LION

Done.

[dtu, 2015-02-04 00:41:37]

The CQ bit was checked by dtu@chromium.org

[dtu, 2015-02-04 00:41:37]

lgtm

[dtu, 2015-02-04 00:42:16]

On 2015/02/04 00:41:37, dtu wrote:
> lgtm

Oh yeah, also the gyp file. There's already a tools/telemetry/telemetry.gyp you
might be able to use?

[commit-bot: I haz the power, 2015-02-04 00:42:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809473004/140001

[erikchen, 2015-02-04 01:23:49]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-02-04 01:42:13]

New patchsets have been uploaded after l-g-t-m from dtu@chromium.org

[erikchen, 2015-02-04 01:59:24]

Patchset #5 (id:160001) has been deleted

[erikchen, 2015-02-04 02:00:40]

On 2015/02/04 00:42:18, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/809473004/140001

dtu: PTAL

I updated the file telemetry.gyp. I left the compilation instructions inside the
C file, as I have a suspicion that it will be very useful when telemetry gets
pulled into its own repository. I didn't see a GN file for telemetry - I assume
that it doesn't exist.

[dtu, 2015-02-04 22:19:10]

On 2015/02/04 02:00:40, erikchen wrote:
> On 2015/02/04 00:42:18, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/809473004/140001
> 
> dtu: PTAL
> 
> I updated the file telemetry.gyp. I left the compilation instructions inside
the
> C file, as I have a suspicion that it will be very useful when telemetry gets
> pulled into its own repository. I didn't see a GN file for telemetry - I
assume
> that it doesn't exist.

Yes, I think all the above are true, thanks! lgtm

[erikchen, 2015-02-04 22:25:43]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-02-04 22:27:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809473004/180001

[commit-bot: I haz the power, 2015-02-04 23:10:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-04 23:10:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2015-02-05 00:17:22]

New patchsets have been uploaded after l-g-t-m from dtu@chromium.org

[erikchen, 2015-02-05 00:21:37]

On 2015/02/04 23:10:33, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

dtu: After some research, I failed to find add a gyp conditional that looked at
the OSX SDK version (the C files depend on OSX 10.7+ SDK). I saw two ways
forward:
1. Remove the GYP targets.
2. Modify the executable to still compile against the 10.6 SDK. 

(1) seemed like a better solution, so I went with it.

[erikchen, 2015-02-05 00:21:42]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-02-05 00:22:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809473004/200001

[erikchen, 2015-02-05 00:28:49]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-02-05 00:29:18]

On 2015/02/05 00:22:46, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/809473004/200001

dtu: PTAL

[dtu, 2015-02-05 22:52:43]

lgtm

[erikchen, 2015-02-05 23:31:12]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-02-05 23:32:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/809473004/200001

[commit-bot: I haz the power, 2015-02-06 00:11:47]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2015-02-06 00:12:35]

Patchset 6 (id:??) landed as
https://crrev.com/0e07c2d7073bcb7b644fe1fe02031c55f5c1ccbe
Cr-Commit-Position: refs/heads/master@{#314921}