Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2634403002:
Use GetDeterministicMachineSpecificId instead of RLZ for device_id (Closed)
DescriptionUse GetDeterministicMachineSpecificId instead of RLZ for device_id
Propagate rlz_device_id as the legacy_device_id and use the device_id
code in components/user_prefs/tracked instead, as it will allow us to
support more platforms than the RLZ id does.
This change also stops accepting the "previously legacy" device id which
was generated by the PrefMetricsService.
BUG=500085
Review-Url: https://codereview.chromium.org/2634403002
Cr-Commit-Position: refs/heads/master@{#446092}
Committed: https://chromium.googlesource.com/chromium/src/+/e9f69602736a3f71bb6251fdd5f08c90ca210c31
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address review comments #
Total comments: 6
Patch Set 3 : Add some saftey checks and a histogram for the device_id generation #
Total comments: 6
Patch Set 4 : Address review comments on #3 #
Total comments: 4
Patch Set 5 : Address review comments on #4 #Patch Set 6 : Rebase #Messages
Total messages: 23 (7 generated)
Description was changed from ========== Use GetDeterministicMachineSpecificId instead of RLZ for device_id Propagate rlz_device_id as the legacy_device_id and use the device_id code in components/user_prefs/tracked instead, as it will allow us to support more platforms than the RLZ id does. This change also stops accepting the "previously legacy" device id which was generated by the PrefMetricsService. BUG=500085 ========== to ========== Use GetDeterministicMachineSpecificId instead of RLZ for device_id Propagate rlz_device_id as the legacy_device_id and use the device_id code in components/user_prefs/tracked instead, as it will allow us to support more platforms than the RLZ id does. This change also stops accepting the "previously legacy" device id which was generated by the PrefMetricsService. BUG=500085 ==========
proberge@chromium.org changed reviewers: + gab@chromium.org
WOOOOOOOT!!! This is awesome:)! The main reason this wasn't done already is that it was much easier to do if we also had a real ID for Mac at the same time (which there wasn't at the time). In that line of thought, I'm assuming this is the case, but just want to double-check we're sure about the device ID generated by device_id_mac.cc, changing it later will be a pain. Cheers! Gab https://codereview.chromium.org/2634403002/diff/1/chrome/browser/prefs/chrome... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2634403002/diff/1/chrome/browser/prefs/chrome... chrome/browser/prefs/chrome_pref_service_factory.cc:383: std::string rlz_device_id; Call this |legacy_device_id| here as well (i.e. on Windows legacy device ID is RLZ and on other platforms it's empty string) https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.cc:110: return VALID_SECURE_LEGACY; I think we want to return some kind of INVALID_EMPTY_LEGACY_ID here if there's a match but |legacy_device_id_.empty()|. And pipe it through to get valid UMA. That way we will: 1) Have UMA to tell us when we think it's reasonable to enable protection on Mac. 2) Not have to remember to come back here and make an empty legacy id invalid when we do enable protection on Mac (otherwise we will keep accepting the insecure empty legacy id and won't be protecting adequately on Mac). https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.h (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.h:27: // |legacy_device_id|. The same parameters must be used in order to nit: - extra space before "same" https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator_unittest.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator_unittest.cc:185: TEST(PrefHashCalculatorTest, TestCompatibilityWithLegacyPrefMetricsServiceId) { Rename test. https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator_unittest.cc:190: "B374B2450B5C305582190D62E3BC37832F8540BE189C698BC85E2FB0B70F5485"; So far what I've been trying to do is that every time there's a migration change: the test which exercises the legacy id is using a MAC that was present in an existing test. That way there's continuity that proves the old values can still be achieved. In this case you could confirm that using static const char kSeed[] = "0123456789ABCDEF0123456789ABCDEF"; static const char kLegacyDeviceId[] = "test_device_id1"; static const char kNewDeviceId[] = "new_test_device_id1"; // As in PrefHashCalculatorTest.CatchHashChanges. const base::StringValue string_value( "testing with special chars:\n<>{}:^^@#$\\/"); static const char kExpectedValue[] = "05ACCBD3B05C45C36CD06190F63EC577112311929D8380E26E5F13182EB68318"; EXPECT_EQ( PrefHashCalculator::VALID_SECURE_LEGACY, PrefHashCalculator(kSeed, kNewDeviceId, kLegacyDeviceId) .Validate("pref.path", &string_value, kExpectedValue)); https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_store_impl.cc:16: namespace { nit: empty lines after opening and before closing namespace https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_store_impl.cc:25: } nit: } // namespace
Also, FYI, you'll be able to track this graph to see the migration happening: https://uma.googleplex.com/timeline_v2?sid=a7333760f37123489592ef56930f7226 :)
Thanks for the quick review! https://codereview.chromium.org/2634403002/diff/1/chrome/browser/prefs/chrome... File chrome/browser/prefs/chrome_pref_service_factory.cc (right): https://codereview.chromium.org/2634403002/diff/1/chrome/browser/prefs/chrome... chrome/browser/prefs/chrome_pref_service_factory.cc:383: std::string rlz_device_id; On 2017/01/17 21:03:47, gab wrote: > Call this |legacy_device_id| here as well (i.e. on Windows legacy device ID is > RLZ and on other platforms it's empty string) Done. https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.cc:110: return VALID_SECURE_LEGACY; On 2017/01/17 21:03:47, gab wrote: > I think we want to return some kind of INVALID_EMPTY_LEGACY_ID here if there's a > match but |legacy_device_id_.empty()|. And pipe it through to get valid UMA. > That way we will: > > 1) Have UMA to tell us when we think it's reasonable to enable protection on > Mac. > 2) Not have to remember to come back here and make an empty legacy id invalid > when we do enable protection on Mac (otherwise we will keep accepting the > insecure empty legacy id and won't be protecting adequately on Mac). Wouldn't this break the OSX pref migration? Currently the prefs are signed with an empty device id. If we return INVALID_EMPTY_LEGACY_ID, we would reset those preferences when migrating them to Secure Preferences. For #1, we could use https://uma.googleplex.com/timeline_v2?sid=a7cd2ce6ff0ca014bdb6a3aad9b17214 https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.h (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.h:27: // |legacy_device_id|. The same parameters must be used in order to On 2017/01/17 21:03:47, gab wrote: > nit: - extra space before "same" Done. https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator_unittest.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator_unittest.cc:185: TEST(PrefHashCalculatorTest, TestCompatibilityWithLegacyPrefMetricsServiceId) { On 2017/01/17 21:03:47, gab wrote: > Rename test. Done. https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator_unittest.cc:190: "B374B2450B5C305582190D62E3BC37832F8540BE189C698BC85E2FB0B70F5485"; On 2017/01/17 21:03:47, gab wrote: > So far what I've been trying to do is that every time there's a migration > change: the test which exercises the legacy id is using a MAC that was present > in an existing test. That way there's continuity that proves the old values can > still be achieved. > > In this case you could confirm that using > > static const char kSeed[] = "0123456789ABCDEF0123456789ABCDEF"; > static const char kLegacyDeviceId[] = "test_device_id1"; > static const char kNewDeviceId[] = "new_test_device_id1"; > > // As in PrefHashCalculatorTest.CatchHashChanges. > const base::StringValue string_value( > "testing with special chars:\n<>{}:^^@#$\\/"); > static const char kExpectedValue[] = > "05ACCBD3B05C45C36CD06190F63EC577112311929D8380E26E5F13182EB68318"; > > EXPECT_EQ( > PrefHashCalculator::VALID_SECURE_LEGACY, > PrefHashCalculator(kSeed, kNewDeviceId, kLegacyDeviceId) > .Validate("pref.path", &string_value, kExpectedValue)); Done. https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_store_impl.cc:16: namespace { On 2017/01/17 21:03:48, gab wrote: > nit: empty lines after opening and before closing namespace Done. https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_store_impl.cc:25: } On 2017/01/17 21:03:48, gab wrote: > nit: } // namespace Done. https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:19: std::string GenerateDeviceId() { Possible optimization: This method is called up to 6 times on Windows on startup. Maybe we could cache the result static-ly to avoid calls; I don't know how fast the GetDeterministicMachineSpecificId methods are on Windows or MacOSX. wdyt? https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:21: if (GetDeterministicMachineSpecificId(&device_id) == MachineIdStatus::SUCCESS) One thing I worry about is the stability of this method. I assume that the implementation is correct and that the returned ID is deterministic, but what if there are intermittent failures? It seems like this method would return the empty string and we would reset preferences on Windows. I think I'll add a DCHECK and an histogram. wdyt?
https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.cc:110: return VALID_SECURE_LEGACY; On 2017/01/18 16:10:58, proberge wrote: > On 2017/01/17 21:03:47, gab wrote: > > I think we want to return some kind of INVALID_EMPTY_LEGACY_ID here if there's > a > > match but |legacy_device_id_.empty()|. And pipe it through to get valid UMA. > > That way we will: > > > > 1) Have UMA to tell us when we think it's reasonable to enable protection on > > Mac. > > 2) Not have to remember to come back here and make an empty legacy id invalid > > when we do enable protection on Mac (otherwise we will keep accepting the > > insecure empty legacy id and won't be protecting adequately on Mac). > > Wouldn't this break the OSX pref migration? Currently the prefs are signed with > an empty device id. If we return INVALID_EMPTY_LEGACY_ID, we would reset those > preferences when migrating them to Secure Preferences. > > For #1, we could use > https://uma.googleplex.com/timeline_v2?sid=a7cd2ce6ff0ca014bdb6a3aad9b17214 It wouldn't as we wouldn't enable hardening for a while (until that signal gets low enough) and as such those would trigger WantedResets, not Resets. Depends on whether you want to consider empty device IDs as "secure". We never have but you may decide to change your policy here. https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:19: std::string GenerateDeviceId() { On 2017/01/18 16:10:58, proberge wrote: > Possible optimization: > > This method is called up to 6 times on Windows on startup. Maybe we could cache > the result static-ly to avoid calls; I don't know how fast the > GetDeterministicMachineSpecificId methods are on Windows or MacOSX. wdyt? Ah yes, that's a great idea let's do it. A static variable in this method would do just fine, thanks https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:21: if (GetDeterministicMachineSpecificId(&device_id) == MachineIdStatus::SUCCESS) On 2017/01/18 16:10:58, proberge wrote: > One thing I worry about is the stability of this method. I assume that the > implementation is correct and that the returned ID is deterministic, but what if > there are intermittent failures? It seems like this method would return the > empty string and we would reset preferences on Windows. > > I think I'll add a DCHECK and an histogram. wdyt? It better be 100% deterministic and non-flaky. I'm pretty sure the Windows one is because it's a subset of the logic for the RLZ id we've been using for a while and have never had complaints about AFAIK. Let's confirm that the Mac one also is. A DCHECK and a histogram sounds good to me. In fact we might even want to CHECK against failure because failure here is very serious and would result in erasing user settings... but I'm happy to start with a histogram. https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:24: UMA_HISTOGRAM_ENUMERATION("Settings.MachineIdGenerationStatus", status, I don't think we want to report not implemented, i.e.: if (status != MachineIdStatus::NOT_IMPLEMENTED) { UMA_HISTOGRAM_BOOLEAN("Settings.MachineIdGenerationSucess", status == MachineIdStatus::SUCCESS); } with a "BooleanSuccess" histogram type in histograms.xml instead of the enum. https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:30: #if defined(OS_WIN) || defined(OS_MACOSX) Use MachineIdStatus::NOT_IMPLEMENTED vs FAILURE instead of ifdefs here. https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:33: DCHECK("Failed to generate machine specific id for tracked preferences."); To generate a DCHECK message you need to use << (right now this is checking that the char* for this string is non-null). Overall I think the histogram is more useful than the DCHECK so let's remove the DCHECK for now (we can re-evaluate whether to make it into a CHECK after we get histogram results).
https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.cc:110: return VALID_SECURE_LEGACY; On 2017/01/19 18:52:05, gab (slow at conference) wrote: > On 2017/01/18 16:10:58, proberge wrote: > > On 2017/01/17 21:03:47, gab wrote: > > > I think we want to return some kind of INVALID_EMPTY_LEGACY_ID here if > there's > > a > > > match but |legacy_device_id_.empty()|. And pipe it through to get valid UMA. > > > That way we will: > > > > > > 1) Have UMA to tell us when we think it's reasonable to enable protection on > > > Mac. > > > 2) Not have to remember to come back here and make an empty legacy id > invalid > > > when we do enable protection on Mac (otherwise we will keep accepting the > > > insecure empty legacy id and won't be protecting adequately on Mac). > > > > Wouldn't this break the OSX pref migration? Currently the prefs are signed > with > > an empty device id. If we return INVALID_EMPTY_LEGACY_ID, we would reset those > > preferences when migrating them to Secure Preferences. > > > > For #1, we could use > > https://uma.googleplex.com/timeline_v2?sid=a7cd2ce6ff0ca014bdb6a3aad9b17214 > > It wouldn't as we wouldn't enable hardening for a while (until that signal gets > low enough) and as such those would trigger WantedResets, not Resets. > > Depends on whether you want to consider empty device IDs as "secure". We never > have but you may decide to change your policy here. You're right, INVALID_EMPTY_LEGACY_DEVICE_ID isn't an issue unless we start enforcing early. I'll update this once we make a decision. https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:19: std::string GenerateDeviceId() { On 2017/01/19 18:52:05, gab (slow at conference) wrote: > On 2017/01/18 16:10:58, proberge wrote: > > Possible optimization: > > > > This method is called up to 6 times on Windows on startup. Maybe we could > cache > > the result static-ly to avoid calls; I don't know how fast the > > GetDeterministicMachineSpecificId methods are on Windows or MacOSX. wdyt? > > Ah yes, that's a great idea let's do it. A static variable in this method would > do just fine, thanks Done. https://codereview.chromium.org/2634403002/diff/20001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:21: if (GetDeterministicMachineSpecificId(&device_id) == MachineIdStatus::SUCCESS) On 2017/01/19 18:52:05, gab (slow at conference) wrote: > On 2017/01/18 16:10:58, proberge wrote: > > One thing I worry about is the stability of this method. I assume that the > > implementation is correct and that the returned ID is deterministic, but what > if > > there are intermittent failures? It seems like this method would return the > > empty string and we would reset preferences on Windows. > > > > I think I'll add a DCHECK and an histogram. wdyt? > > It better be 100% deterministic and non-flaky. I'm pretty sure the Windows one > is because it's a subset of the logic for the RLZ id we've been using for a > while and have never had complaints about AFAIK. > > Let's confirm that the Mac one also is. > > A DCHECK and a histogram sounds good to me. In fact we might even want to CHECK > against failure because failure here is very serious and would result in erasing > user settings... but I'm happy to start with a histogram. Done. https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:24: UMA_HISTOGRAM_ENUMERATION("Settings.MachineIdGenerationStatus", status, On 2017/01/19 18:52:05, gab (slow at conference) wrote: > I don't think we want to report not implemented, i.e.: > > if (status != MachineIdStatus::NOT_IMPLEMENTED) { > UMA_HISTOGRAM_BOOLEAN("Settings.MachineIdGenerationSucess", > status == MachineIdStatus::SUCCESS); > } > > with a "BooleanSuccess" histogram type in histograms.xml instead of the enum. Done. https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:30: #if defined(OS_WIN) || defined(OS_MACOSX) On 2017/01/19 18:52:05, gab (slow at conference) wrote: > Use MachineIdStatus::NOT_IMPLEMENTED vs FAILURE instead of ifdefs here. Done. https://codereview.chromium.org/2634403002/diff/40001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:33: DCHECK("Failed to generate machine specific id for tracked preferences."); On 2017/01/19 18:52:05, gab (slow at conference) wrote: > To generate a DCHECK message you need to use << (right now this is checking that > the char* for this string is non-null). > > Overall I think the histogram is more useful than the DCHECK so let's remove the > DCHECK for now (we can re-evaluate whether to make it into a CHECK after we get > histogram results). doh! Good catch. Removed the DCHECK for now.
proberge@chromium.org changed reviewers: + jwd@chromium.org
++jwd for histograms.xml
histograms LGTM
https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.cc:110: return VALID_SECURE_LEGACY; On 2017/01/19 20:42:59, proberge wrote: > On 2017/01/19 18:52:05, gab (slow at conference) wrote: > > On 2017/01/18 16:10:58, proberge wrote: > > > On 2017/01/17 21:03:47, gab wrote: > > > > I think we want to return some kind of INVALID_EMPTY_LEGACY_ID here if > > there's > > > a > > > > match but |legacy_device_id_.empty()|. And pipe it through to get valid > UMA. > > > > That way we will: > > > > > > > > 1) Have UMA to tell us when we think it's reasonable to enable protection > on > > > > Mac. > > > > 2) Not have to remember to come back here and make an empty legacy id > > invalid > > > > when we do enable protection on Mac (otherwise we will keep accepting the > > > > insecure empty legacy id and won't be protecting adequately on Mac). > > > > > > Wouldn't this break the OSX pref migration? Currently the prefs are signed > > with > > > an empty device id. If we return INVALID_EMPTY_LEGACY_ID, we would reset > those > > > preferences when migrating them to Secure Preferences. > > > > > > For #1, we could use > > > https://uma.googleplex.com/timeline_v2?sid=a7cd2ce6ff0ca014bdb6a3aad9b17214 > > > > It wouldn't as we wouldn't enable hardening for a while (until that signal > gets > > low enough) and as such those would trigger WantedResets, not Resets. > > > > Depends on whether you want to consider empty device IDs as "secure". We never > > have but you may decide to change your policy here. > > You're right, INVALID_EMPTY_LEGACY_DEVICE_ID isn't an issue unless we start > enforcing early. I'll update this once we make a decision. What did we settle on? i.e. is this the impl I should review? :) https://codereview.chromium.org/2634403002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:21: if (cached_device_id.length() > 0) !empty() https://codereview.chromium.org/2634403002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:29: status == MachineIdStatus::SUCCESS); Put behind != NOT_IMPLEMENTED
https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... File components/user_prefs/tracked/pref_hash_calculator.cc (right): https://codereview.chromium.org/2634403002/diff/1/components/user_prefs/track... components/user_prefs/tracked/pref_hash_calculator.cc:110: return VALID_SECURE_LEGACY; On 2017/01/24 20:32:04, gab wrote: > On 2017/01/19 20:42:59, proberge wrote: > > On 2017/01/19 18:52:05, gab (slow at conference) wrote: > > > On 2017/01/18 16:10:58, proberge wrote: > > > > On 2017/01/17 21:03:47, gab wrote: > > > > > I think we want to return some kind of INVALID_EMPTY_LEGACY_ID here if > > > there's > > > > a > > > > > match but |legacy_device_id_.empty()|. And pipe it through to get valid > > UMA. > > > > > That way we will: > > > > > > > > > > 1) Have UMA to tell us when we think it's reasonable to enable > protection > > on > > > > > Mac. > > > > > 2) Not have to remember to come back here and make an empty legacy id > > > invalid > > > > > when we do enable protection on Mac (otherwise we will keep accepting > the > > > > > insecure empty legacy id and won't be protecting adequately on Mac). > > > > > > > > Wouldn't this break the OSX pref migration? Currently the prefs are signed > > > with > > > > an empty device id. If we return INVALID_EMPTY_LEGACY_ID, we would reset > > those > > > > preferences when migrating them to Secure Preferences. > > > > > > > > For #1, we could use > > > > > https://uma.googleplex.com/timeline_v2?sid=a7cd2ce6ff0ca014bdb6a3aad9b17214 > > > > > > It wouldn't as we wouldn't enable hardening for a while (until that signal > > gets > > > low enough) and as such those would trigger WantedResets, not Resets. > > > > > > Depends on whether you want to consider empty device IDs as "secure". We > never > > > have but you may decide to change your policy here. > > > > You're right, INVALID_EMPTY_LEGACY_DEVICE_ID isn't an issue unless we start > > enforcing early. I'll update this once we make a decision. > > What did we settle on? i.e. is this the impl I should review? :) We might start enforcing early. Please review the impl as-is. https://codereview.chromium.org/2634403002/diff/60001/components/user_prefs/t... File components/user_prefs/tracked/pref_hash_store_impl.cc (right): https://codereview.chromium.org/2634403002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:21: if (cached_device_id.length() > 0) On 2017/01/24 20:32:05, gab wrote: > !empty() Done. https://codereview.chromium.org/2634403002/diff/60001/components/user_prefs/t... components/user_prefs/tracked/pref_hash_store_impl.cc:29: status == MachineIdStatus::SUCCESS); On 2017/01/24 20:32:05, gab wrote: > Put behind != NOT_IMPLEMENTED Done.
SGTM, lgtm, you'll be able to track this migration in UMA via "Settings.TrackedPreferenceMigratedLegacyDeviceId" on Windows and Mac. Cheers and best of luck! Gab
On 2017/01/24 21:07:34, gab wrote: > SGTM, lgtm, you'll be able to track this migration in UMA via > "Settings.TrackedPreferenceMigratedLegacyDeviceId" on Windows and Mac. Oh and PS: you've probably done this already but just in case... Since it's very destructive to get this wrong, you'll want to make sure this indeed triggers as you expect in a local build (i.e. clear User Data, build master, run chrome + set some prefs, build branch, run new chrome, check chrome://histograms/Settings.Tracked). Wiping people's preferences, even on Canary, is not cool..! > > Cheers and best of luck! > Gab
On 2017/01/24 21:10:31, gab wrote: > On 2017/01/24 21:07:34, gab wrote: > > SGTM, lgtm, you'll be able to track this migration in UMA via > > "Settings.TrackedPreferenceMigratedLegacyDeviceId" on Windows and Mac. > > Oh and PS: you've probably done this already but just in case... Since it's very > destructive to get this wrong, you'll want to make sure this indeed triggers as > you expect in a local build (i.e. clear User Data, build master, run chrome + > set some prefs, build branch, run new chrome, check > chrome://histograms/Settings.Tracked). Wiping people's preferences, even on > Canary, is not cool..! > > > > > Cheers and best of luck! > > Gab Thanks for the review! I did test locally beforehand, but will double-check before submitting. The largest risk is if we have to revert this CL for any reason. Canary users' prefs would be signed with a device_id which isn't valid in the rolled-back build; we would have to temporarily disable enforcement on Canary.
On 2017/01/24 21:53:39, proberge wrote: > On 2017/01/24 21:10:31, gab wrote: > > On 2017/01/24 21:07:34, gab wrote: > > > SGTM, lgtm, you'll be able to track this migration in UMA via > > > "Settings.TrackedPreferenceMigratedLegacyDeviceId" on Windows and Mac. > > > > Oh and PS: you've probably done this already but just in case... Since it's > very > > destructive to get this wrong, you'll want to make sure this indeed triggers > as > > you expect in a local build (i.e. clear User Data, build master, run chrome + > > set some prefs, build branch, run new chrome, check > > chrome://histograms/Settings.Tracked). Wiping people's preferences, even on > > Canary, is not cool..! > > > > > > > > Cheers and best of luck! > > > Gab > > Thanks for the review! I did test locally beforehand, but will double-check > before submitting. > The largest risk is if we have to revert this CL for any reason. Canary users' > prefs would be signed > with a device_id which isn't valid in the rolled-back build; we would have to > temporarily disable > enforcement on Canary. Right, reverts are always hard with this code, that's why testing it manually ahead of time is crucial.
The CQ bit was checked by proberge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2634403002/#ps100001 (title: "Rebase")
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": 1485368244919170,
"parent_rev": "007ea866683b527ab8b25c3ed0b9a3a82a43e758", "commit_rev":
"e9f69602736a3f71bb6251fdd5f08c90ca210c31"}
Message was sent while issue was closed.
Description was changed from ========== Use GetDeterministicMachineSpecificId instead of RLZ for device_id Propagate rlz_device_id as the legacy_device_id and use the device_id code in components/user_prefs/tracked instead, as it will allow us to support more platforms than the RLZ id does. This change also stops accepting the "previously legacy" device id which was generated by the PrefMetricsService. BUG=500085 ========== to ========== Use GetDeterministicMachineSpecificId instead of RLZ for device_id Propagate rlz_device_id as the legacy_device_id and use the device_id code in components/user_prefs/tracked instead, as it will allow us to support more platforms than the RLZ id does. This change also stops accepting the "previously legacy" device id which was generated by the PrefMetricsService. BUG=500085 Review-Url: https://codereview.chromium.org/2634403002 Cr-Commit-Position: refs/heads/master@{#446092} Committed: https://chromium.googlesource.com/chromium/src/+/e9f69602736a3f71bb6251fdd5f0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e9f69602736a3f71bb6251fdd5f0... |
