Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721333005:
Reauthorize Keychain to Replace Developer ID Certificate (Closed)
|
Created:
3 years, 9 months ago by Greg K Modified:
3 years, 9 months ago CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, wfh+watch_chromium.org, asvitkine+watch_chromium.org, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReauthorize Keychain to Replace Developer ID Certificate
This adds the code to reauthorize the Safe Storage Keychain entry to prepare for
replacing Chrome's Developer ID certificate. This signs Chrome with a
new designated requirement (DR), that includes both the current and new
certificate. It then deletes and re-adds our safe storage entry to the
keychain. This is because a keychain entry's ACL is the application's DR
by default, and an app cannot modify the ACL, but it can delete its own
item. The re-authorization code is not yet called at Chrome's launch.
BUG=629906
Review-Url: https://codereview.chromium.org/2721333005
Cr-Commit-Position: refs/heads/master@{#454649}
Committed: https://chromium.googlesource.com/chromium/src/+/e29cfa202aa6f41e862093ddb24353aa27638afb
Patch Set 1 #Patch Set 2 : Reauthorize Keychain to Replace Developer ID Certificate #
Total comments: 35
Patch Set 3 : Changes per review #
Total comments: 15
Patch Set 4 : Cleanup the comments, and error handling logic #
Total comments: 8
Patch Set 5 : Updates per review. #
Total comments: 2
Patch Set 6 : Remove histogram.h include #
Messages
Total messages: 41 (26 generated)
kerrnel@chromium.org changed reviewers: + mark@chromium.org
PTAL. Thanks, Greg
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reauthorize Keychain to Replace Developer ID Certificate This reauthorizes the Safe Storage Keychain entry to prepare for replacing Chrome's Developer ID certificate. This signs Chrome with a new designated requirement (DR), that includes both the current and new certificate. It then deletes and re-adds our safe storage entry to the keychain. This is because a keychain entry's ACL is the applications DR by default, and an app cannot modify the AC, but it can delete its own item. BUG=629906 ========== to ========== Reauthorize Keychain to Replace Developer ID Certificate This reauthorizes the Safe Storage Keychain entry to prepare for replacing Chrome's Developer ID certificate. This signs Chrome with a new designated requirement (DR), that includes both the current and new certificate. It then deletes and re-adds our safe storage entry to the keychain. This is because a keychain entry's ACL is the applications DR by default, and an app cannot modify the ACL, but it can delete its own item. BUG=629906 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
No structural or logical problems really. Should be able to clean these up and land this quickly. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. CL description: applications→application’s https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. The CL description makes it sound like this change activates the new code, but it doesn’t. Make that clear. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:28: // because the keychain uses an apps designated requirement as the default ACL app’s https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:28: // because the keychain uses an apps designated requirement as the default ACL “default ACL for”→“ACL for reading” https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:29: // for an item. We are signing Chrome with the current certificate (so it can Don’t say “we.” Also, the bit about signing will become stale, because signing happens outside of this codebase. So this should just say that Chrome will be signed with a designated requirement that allows the item to be read by Chromes signed with both the old and new certificates. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:39: NULL, strlen(KeychainPassword::service_name), Use nullptr now in new code. Hooray for C++11! https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:40: KeychainPassword::service_name, strlen(KeychainPassword::account_name), If git cl format formatted this, then it’s fine because I guess I’m required to say so, but if it didn’t format this, then I’ll say that it’s ugly. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:44: std::string password = One of the sanitizers may not like this if !password_data, and !password_data seems to be a possibility based on line 46. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:47: memset(password_data, 0xAA, pw_length); This is good. memset_s() would be even better. But you never bother scrambling the std::string version of password, so it kind of doesn’t even matter that you’ve done this. You should scramble any copies that you make. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:52: return false; Wanna say something about this? https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:55: LOG(ERROR) << "KeychainReauthorize failed. Cannot delete item."; Aha! Whenever you’re logging something about an OSStatus like you are here, you can use OSSTATUS_LOG from base/mac/mac_logging.h. The log message will include a description of what went wrong. It’s amazing! https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:59: error = keychain.AddGenericPassword( I’m pretty sure that the answer is no, but we can’t Find-Add without a Delete in the middle, right? https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:75: [user_defaults synchronize]; I don’t believe that this one will do anything, because you haven’t written anything yet. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:98: ++pref_value; The comment in the header was explicit that this would happen BEFORE the reauthorize attempt. It’s nice to do it before, because if KeychainReauthorize() turns out to be crashy, just launch Chrome a few more times to recover. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:105: NSString* success_pref_key = [pref_key stringByAppendingString:@"Success"]; You already computed this above, on line 82. You should only do it once. https://codereview.chromium.org/2721333005/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2721333005/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_app.sh.in:52: certificate leaf = H\"34f0bdf7f87d4f3a955862c351472e52250e4c2b\") \ I will double-check this later. https://codereview.chromium.org/2721333005/diff/20001/components/os_crypt/key... File components/os_crypt/keychain_password_mac.mm (right): https://codereview.chromium.org/2721333005/diff/20001/components/os_crypt/key... components/os_crypt/keychain_password_mac.mm:64: OSStatus error = keychain_.FindGenericPassword( Again with the clang-format? “Before” was so much more readable. Oh well, progress. (?) https://codereview.chromium.org/2721333005/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721333005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43753: + The event records the number of previous times that the keychain Probably lop off the first three words of each of these four summaries.
Description was changed from ========== Reauthorize Keychain to Replace Developer ID Certificate This reauthorizes the Safe Storage Keychain entry to prepare for replacing Chrome's Developer ID certificate. This signs Chrome with a new designated requirement (DR), that includes both the current and new certificate. It then deletes and re-adds our safe storage entry to the keychain. This is because a keychain entry's ACL is the applications DR by default, and an app cannot modify the ACL, but it can delete its own item. BUG=629906 ========== to ========== Reauthorize Keychain to Replace Developer ID Certificate This adds the code to reauthorize the Safe Storage Keychain entry to prepare for replacing Chrome's Developer ID certificate. This signs Chrome with a new designated requirement (DR), that includes both the current and new certificate. It then deletes and re-adds our safe storage entry to the keychain. This is because a keychain entry's ACL is the application's DR by default, and an app cannot modify the ACL, but it can delete its own item. The re-authorization code is not yet called at Chrome's launch. BUG=629906 ==========
https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2017/03/01 22:00:30, Mark Mentovai wrote: > The CL description makes it sound like this change activates the new code, but > it doesn’t. Make that clear. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2017/03/01 22:00:30, Mark Mentovai wrote: > The CL description makes it sound like this change activates the new code, but > it doesn’t. Make that clear. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:28: // because the keychain uses an apps designated requirement as the default ACL On 2017/03/01 22:00:31, Mark Mentovai wrote: > app’s Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:28: // because the keychain uses an apps designated requirement as the default ACL On 2017/03/01 22:00:30, Mark Mentovai wrote: > “default ACL for”→“ACL for reading” Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:29: // for an item. We are signing Chrome with the current certificate (so it can On 2017/03/01 22:00:31, Mark Mentovai wrote: > Don’t say “we.” Also, the bit about signing will become stale, because signing > happens outside of this codebase. So this should just say that Chrome will be > signed with a designated requirement that allows the item to be read by Chromes > signed with both the old and new certificates. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:39: NULL, strlen(KeychainPassword::service_name), On 2017/03/01 22:00:31, Mark Mentovai wrote: > Use nullptr now in new code. Hooray for C++11! Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:40: KeychainPassword::service_name, strlen(KeychainPassword::account_name), On 2017/03/01 22:00:31, Mark Mentovai wrote: > If git cl format formatted this, then it’s fine because I guess I’m required to > say so, but if it didn’t format this, then I’ll say that it’s ugly. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:44: std::string password = On 2017/03/01 22:00:30, Mark Mentovai wrote: > One of the sanitizers may not like this if !password_data, and !password_data > seems to be a possibility based on line 46. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:47: memset(password_data, 0xAA, pw_length); On 2017/03/01 22:00:31, Mark Mentovai wrote: > This is good. memset_s() would be even better. But you never bother scrambling > the std::string version of password, so it kind of doesn’t even matter that > you’ve done this. You should scramble any copies that you make. memset_s requires some LIBC extension, but let's definitely scramble the string. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:52: return false; On 2017/03/01 22:00:30, Mark Mentovai wrote: > Wanna say something about this? Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:59: error = keychain.AddGenericPassword( On 2017/03/01 22:00:31, Mark Mentovai wrote: > I’m pretty sure that the answer is no, but we can’t Find-Add without a Delete in > the middle, right? Definitely good to test, but it failed. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:75: [user_defaults synchronize]; On 2017/03/01 22:00:31, Mark Mentovai wrote: > I don’t believe that this one will do anything, because you haven’t written > anything yet. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:98: ++pref_value; On 2017/03/01 22:00:31, Mark Mentovai wrote: > The comment in the header was explicit that this would happen BEFORE the > reauthorize attempt. It’s nice to do it before, because if KeychainReauthorize() > turns out to be crashy, just launch Chrome a few more times to recover. That makes sense, I didn't think about that. https://codereview.chromium.org/2721333005/diff/20001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:105: NSString* success_pref_key = [pref_key stringByAppendingString:@"Success"]; On 2017/03/01 22:00:31, Mark Mentovai wrote: > You already computed this above, on line 82. You should only do it once. Done. https://codereview.chromium.org/2721333005/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2721333005/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_app.sh.in:52: certificate leaf = H\"34f0bdf7f87d4f3a955862c351472e52250e4c2b\") \ On 2017/03/01 22:00:31, Mark Mentovai wrote: > I will double-check this later. Acknowledged. https://codereview.chromium.org/2721333005/diff/20001/components/os_crypt/key... File components/os_crypt/keychain_password_mac.mm (right): https://codereview.chromium.org/2721333005/diff/20001/components/os_crypt/key... components/os_crypt/keychain_password_mac.mm:64: OSStatus error = keychain_.FindGenericPassword( On 2017/03/01 22:00:31, Mark Mentovai wrote: > Again with the clang-format? “Before” was so much more readable. Oh well, > progress. (?) Done. https://codereview.chromium.org/2721333005/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721333005/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43753: + The event records the number of previous times that the keychain On 2017/03/01 22:00:31, Mark Mentovai wrote: > Probably lop off the first three words of each of these four summaries. Done.
LGTM otherwise https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.h (right): https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.h:16: // Calls KeychainReauthorize, but only if it's determined that it's necessary. KeychainReauthorize is a hidden function, an implementation detail of the other file. It doesn’t make any sense out here. We shouldn’t talk about it here, but its name comes up in this comment a few times. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.h:22: // max_tries to prevent further attempts, and the preference name with the It doesn’t set it to max_tries, it just sets the success one, which is also checked before jumping into the reauthorization. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:32: memset(buf->data(), 0xBA5EBA11, buf->size()); That’s cute, but you’re really just using 0x11 here and at your other memset(). You could use the nonstandard Apple-ism memset_pattern4() if you really want to toss baseballs into memory badly. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:59: if (password_data) { I think you should be working off of error here, not password_data. Ought to be OK to just move the (error) check up above here, and then get rid of the (password_data) check. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:73: if (password.get() == nullptr) { I want to say that this can’t happen and isn’t worth checking for, but maybe you know better. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:90: password.get()->data(), NULL); nullptr https://codereview.chromium.org/2721333005/diff/40001/chrome/installer/mac/si... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2721333005/diff/40001/chrome/installer/mac/si... chrome/installer/mac/sign_app.sh.in:52: certificate leaf = H\"34f0bdf7f87d4f3a955862c351472e52250e4c2b\") \ Verifying that 34f0bdf7f87d4f3a955862c351472e52250e4c2b is correct. That looks like what you have here. The “new” one expires 2019-07-31. https://codereview.chromium.org/2721333005/diff/40001/components/os_crypt/key... File components/os_crypt/keychain_password_mac.mm (right): https://codereview.chromium.org/2721333005/diff/40001/components/os_crypt/key... components/os_crypt/keychain_password_mac.mm:65: NULL, strlen(service_name), service_name, strlen(account_name), nullptr
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.h (right): https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.h:16: // Calls KeychainReauthorize, but only if it's determined that it's necessary. On 2017/03/02 02:59:32, Mark Mentovai wrote: > KeychainReauthorize is a hidden function, an implementation detail of the other > file. It doesn’t make any sense out here. We shouldn’t talk about it here, but > its name comes up in this comment a few times. Done. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.h:22: // max_tries to prevent further attempts, and the preference name with the On 2017/03/02 02:59:32, Mark Mentovai wrote: > It doesn’t set it to max_tries, it just sets the success one, which is also > checked before jumping into the reauthorization. Done. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:32: memset(buf->data(), 0xBA5EBA11, buf->size()); On 2017/03/02 02:59:32, Mark Mentovai wrote: > That’s cute, but you’re really just using 0x11 here and at your other memset(). > You could use the nonstandard Apple-ism memset_pattern4() if you really want to > toss baseballs into memory badly. Given that this is pretty important and time sensitive, let's jut write 0x11 and be done with it. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:59: if (password_data) { On 2017/03/02 02:59:32, Mark Mentovai wrote: > I think you should be working off of error here, not password_data. Ought to be > OK to just move the (error) check up above here, and then get rid of the > (password_data) check. Done. https://codereview.chromium.org/2721333005/diff/40001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:73: if (password.get() == nullptr) { On 2017/03/02 02:59:32, Mark Mentovai wrote: > I want to say that this can’t happen and isn’t worth checking for, but maybe you > know better. Done. https://codereview.chromium.org/2721333005/diff/40001/chrome/installer/mac/si... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2721333005/diff/40001/chrome/installer/mac/si... chrome/installer/mac/sign_app.sh.in:52: certificate leaf = H\"34f0bdf7f87d4f3a955862c351472e52250e4c2b\") \ On 2017/03/02 02:59:32, Mark Mentovai wrote: > Verifying that 34f0bdf7f87d4f3a955862c351472e52250e4c2b is correct. That looks > like what you have here. The “new” one expires 2019-07-31. Acknowledged. https://codereview.chromium.org/2721333005/diff/40001/components/os_crypt/key... File components/os_crypt/keychain_password_mac.mm (right): https://codereview.chromium.org/2721333005/diff/40001/components/os_crypt/key... components/os_crypt/keychain_password_mac.mm:65: NULL, strlen(service_name), service_name, strlen(account_name), On 2017/03/02 02:59:32, Mark Mentovai wrote: > nullptr Done.
kerrnel@chromium.org changed reviewers: + haraken@chromium.org, thestig@chromium.org
Time for an OWNERS review. haraken@chromium.org: Please review changes in histogram.xml thestig@chromium.org: Please review changes in components/ Thanks, Greg
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
haraken@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms. (I'm eligible to review UseCounter-only changes.)
kerrnel@chromium.org changed reviewers: - thestig@chromium.org
kerrnel@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne@chromium.org, can you do an OWNERS review for components/? We are trying to land this CL soon so we can merge it into a release branch. Thank you, Greg
components/ lgtm
https://codereview.chromium.org/2721333005/diff/60001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/60001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:93: void KeychainReauthorizeIfNeeded(NSString* pref_key, int max_tries) { It looks like this function is never called. Is that intentional? https://codereview.chromium.org/2721333005/diff/60001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:108: UMA_HISTOGRAM_COUNTS("OSX.KeychainReauthorizeIfNeeded", pref_value); This histogram has an upper bound of 1000000, and records data in exponentially-sized buckets. What sorts of values are you expecting to see? Might an UMA_HISTOGRAM_SPARSE_SLOWLY call better suit your needs? https://codereview.chromium.org/2721333005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721333005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43754: + complete. Please document when this is recorded. Ditto for the other metrics. https://codereview.chromium.org/2721333005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43780: + How many times the keychain reauthorization ran before finally succeeding. This metric counts runs that are not at update time, right? Please document that; ditto for the other like-minded metric.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2721333005/diff/60001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/60001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:93: void KeychainReauthorizeIfNeeded(NSString* pref_key, int max_tries) { On 2017/03/03 01:36:12, Ilya Sherman wrote: > It looks like this function is never called. Is that intentional? Yes, the next CL will add some backup protection and then call this code to do the re-auth. https://codereview.chromium.org/2721333005/diff/60001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:108: UMA_HISTOGRAM_COUNTS("OSX.KeychainReauthorizeIfNeeded", pref_value); On 2017/03/03 01:36:12, Ilya Sherman wrote: > This histogram has an upper bound of 1000000, and records data in > exponentially-sized buckets. What sorts of values are you expecting to see? > Might an UMA_HISTOGRAM_SPARSE_SLOWLY call better suit your needs? Done. https://codereview.chromium.org/2721333005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2721333005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43754: + complete. On 2017/03/03 01:36:12, Ilya Sherman wrote: > Please document when this is recorded. Ditto for the other metrics. Done. https://codereview.chromium.org/2721333005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43780: + How many times the keychain reauthorization ran before finally succeeding. On 2017/03/03 01:36:12, Ilya Sherman wrote: > This metric counts runs that are not at update time, right? Please document > that; ditto for the other like-minded metric. Done.
Thanks. LGTM % a nit: https://codereview.chromium.org/2721333005/diff/80001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/80001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:18: #include "base/metrics/histogram.h" nit: It doesn't look like this #include is needed.
https://codereview.chromium.org/2721333005/diff/80001/chrome/browser/mac/keyc... File chrome/browser/mac/keychain_reauthorize.mm (right): https://codereview.chromium.org/2721333005/diff/80001/chrome/browser/mac/keyc... chrome/browser/mac/keychain_reauthorize.mm:18: #include "base/metrics/histogram.h" On 2017/03/03 18:19:55, Ilya Sherman wrote: > nit: It doesn't look like this #include is needed. Done.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, sdefresne@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2721333005/#ps100001 (title: "Remove histogram.h include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488569444708880,
"parent_rev": "3e367b750ae50dc01ddba5744f6b7171e57f75a0", "commit_rev":
"e29cfa202aa6f41e862093ddb24353aa27638afb"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488569444708880,
"parent_rev": "3e367b750ae50dc01ddba5744f6b7171e57f75a0", "commit_rev":
"e29cfa202aa6f41e862093ddb24353aa27638afb"}
Message was sent while issue was closed.
Description was changed from ========== Reauthorize Keychain to Replace Developer ID Certificate This adds the code to reauthorize the Safe Storage Keychain entry to prepare for replacing Chrome's Developer ID certificate. This signs Chrome with a new designated requirement (DR), that includes both the current and new certificate. It then deletes and re-adds our safe storage entry to the keychain. This is because a keychain entry's ACL is the application's DR by default, and an app cannot modify the ACL, but it can delete its own item. The re-authorization code is not yet called at Chrome's launch. BUG=629906 ========== to ========== Reauthorize Keychain to Replace Developer ID Certificate This adds the code to reauthorize the Safe Storage Keychain entry to prepare for replacing Chrome's Developer ID certificate. This signs Chrome with a new designated requirement (DR), that includes both the current and new certificate. It then deletes and re-adds our safe storage entry to the keychain. This is because a keychain entry's ACL is the application's DR by default, and an app cannot modify the ACL, but it can delete its own item. The re-authorization code is not yet called at Chrome's launch. BUG=629906 Review-Url: https://codereview.chromium.org/2721333005 Cr-Commit-Position: refs/heads/master@{#454649} Committed: https://chromium.googlesource.com/chromium/src/+/e29cfa202aa6f41e862093ddb243... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e29cfa202aa6f41e862093ddb243... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
