Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10617002:
Use a better registration suffix that will always be unique while respecting the MSDN rules. (Closed)
|
Created:
8 years, 6 months ago by gab Modified:
8 years, 5 months ago CC:
chromium-reviews, erikwright (departed), grt+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse a better registration suffix that will always be unique while respecting the MSDN rules.
The suffix will now be the base32 encoding of the md5 hash of the user's username. This is always unique and can be determined many times deterministically without I/O access (as opposed to other methods we considered).
It respects all MSDN constraint for properties onto which it is appended.
This replaces the prior-style username suffixes for new installs.
Old-style suffixes (i.e. unsuffixed and username suffixed) will however be kept as is through future updates.
Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit
BUG=133810, 133173
TEST= http://goo.gl/ZZ7gE
installer_util_unittests.exe --gtest_filter=ShellUtilTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143782
Patch Set 1 #
Total comments: 18
Patch Set 2 : First algorithm + test #Patch Set 3 : remove changes to base for previous sid method #Patch Set 4 : new uber-cool algorithm #Patch Set 5 : address gregs initial comments #
Total comments: 10
Patch Set 6 : addressed robert's comments #
Total comments: 1
Patch Set 7 : oops nit #
Total comments: 5
Patch Set 8 : last nits #
Messages
Total messages: 16 (0 generated)
IMPORTANT! (i.e. Release-blocker as you know...) I just finished this now, see some of my comments in line... I'll rethink the logic overnight and all, but that's the first pass and I wanted you to have a first look at it quickly given the strict timeline (and that I spent all night finishing it...!) I have only done preliminary testing, but the encoding works :)! Base 16 is sufficient to get a good length (i.e. 24 chars encoding of the whole SID), base 64 is one-character too many (i.e. I only know for sure we are allowed [A-Za-z0-9.] which is 63 chars). And base 32 is ugly because its 5 bits... easier to do masking and shifting with Base 16 (i.e. 4 bits). I'll review the design doc tomorrow to reflect those changes. Cheers and good night (or good morning I guess when you'll see this...)! Gab https://chromiumcodereview.appspot.com/10617002/diff/1/base/win/win_util.cc File base/win/win_util.cc (right): https://chromiumcodereview.appspot.com/10617002/diff/1/base/win/win_util.cc#n... base/win/win_util.cc:131: return false; I would have preferred to copy this common functionality (copied from the function above) to a common function, but it was a pain with the PSID, PISID, SID*, and all the types Windows gives to those things... maybe you can help me with that tomorrow if you want. https://chromiumcodereview.appspot.com/10617002/diff/1/base/win/win_util.cc#n... base/win/win_util.cc:135: COMPILE_ASSERT(sizeof(SID) == 12, size_of_SID_struct_is_not_as_expected_); This seems safe, but I'll need to read documentation some more... https://chromiumcodereview.appspot.com/10617002/diff/1/base/win/win_util.h File base/win/win_util.h (right): https://chromiumcodereview.appspot.com/10617002/diff/1/base/win/win_util.h#ne... base/win/win_util.h:57: BASE_EXPORT bool GetUserSidBase16Encoded(string16* user_sid); Maybe this doesn't belong here idk... https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:114: bool GetOldUserSpecificRegistrySuffix(string16* suffix) { This is not new, simply moved from lower to avoid cyclic dependency (well actually only had to move GetNewUserSpecificRegistrySuffix() to avoid cyclic-dep., but I figured they belonged together)
the SID encoding isn't quite right, since the SID struct contains a variable number of sub-authority RIDs. you'll either need to read the sub identifiers out of the struct yourself or convert the SID to a string and parse that. basically, a SID can be up to 66 bytes (67 if you count the version), which would be 134 characters in base16. i don't think a user SID will ever be that long in practice, though, so perhaps we can assume something more reasonable like only 5 sub-authorities, which would be 26 bytes + version byte (which we can probably omit). robert: do you know if there's a reasonable upper-bound on the number of sub-authorities for a user SID? from a code standpoint, here are my suggestions: - create a function that does nothing more than basefoo encode a byte array - write unit tests for this - create a function that generates a byte array for a SID (either by parsing the SID struct or by strigifying a SID and then parsing it) - write unit tests for that - create a function that gets the current user's SID and calls the above two methods put this new fancy SID stuff in installer_util. it should be moved into base only when there are consumers of it outside of installer_util.
As discussed, I'm changing the GetNewUserSpecificRegistrySuffix() call to use a base32 encoding of the md5 hash of the username. However, the rest of the CL is still reviewable as is (with that assumption in mind). Cheers, Gab On 2012/06/21 13:39:58, grt wrote: > the SID encoding isn't quite right, since the SID struct contains a variable > number of sub-authority RIDs. you'll either need to read the sub identifiers > out of the struct yourself or convert the SID to a string and parse that. > basically, a SID can be up to 66 bytes (67 if you count the version), which > would be 134 characters in base16. i don't think a user SID will ever be that > long in practice, though, so perhaps we can assume something more reasonable > like only 5 sub-authorities, which would be 26 bytes + version byte (which we > can probably omit). robert: do you know if there's a reasonable upper-bound on > the number of sub-authorities for a user SID? > > from a code standpoint, here are my suggestions: > > - create a function that does nothing more than basefoo encode a byte array > - write unit tests for this > > - create a function that generates a byte array for a SID (either by parsing the > SID struct or by strigifying a SID and then parsing it) > - write unit tests for that > > - create a function that gets the current user's SID and calls the above two > methods > > put this new fancy SID stuff in installer_util. it should be moved into base > only when there are consumers of it outside of installer_util.
On 2012/06/21 21:12:40, gab wrote: > As discussed, I'm changing the GetNewUserSpecificRegistrySuffix() call to use a > base32 encoding of the md5 hash of the username. I can't think of any reason not to simplify and just pick a random n-bit number, where the baseM encoding of the n/8 bytes is the desired size. This avoids the username and hashing altogether.
On 2012/06/21 23:14:51, grt wrote: > I can't think of any reason not to simplify and just pick a random n-bit number, > where the baseM encoding of the n/8 bytes is the desired size. This avoids the > username and hashing altogether. Oh yeah, storing the value in the registry and reading it at runtime is problematic, right?
On 2012/06/22 00:22:34, grt wrote: > On 2012/06/21 23:14:51, grt wrote: > > I can't think of any reason not to simplify and just pick a random n-bit > number, > > where the baseM encoding of the n/8 bytes is the desired size. This avoids > the > > username and hashing altogether. > > Oh yeah, storing the value in the registry and reading it at runtime is > problematic, right? Right, if its not deterministic, we have to store it somewhere and knowing it at run-time without I/O is hard (i.e. caching with multiple-processes...)
some initial comments https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:127: // Returns Chrome's ProgId as "ChromeHTML|suffix|". although there are other funcs in this file with Chrome in the name, something nags at me that it's not the best practice. among the reasons: 1) it's Chrome or Chromium depending on the build, 2) all of this is for Chrome anyway, and 3) kChromeHTMLProgId should be replaced with a method on BrowserDistribution. https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:129: string16 GetChromeHTMLProgId(const string16& suffix) { i'm not too fond of the fact that this is called all over the place and will recompute the new suffix over and over again. can you do something about that? https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:135: // We can however only make sure it's compliant with MSDN in the new-style of suggest: "Make all new registrations comply with this requirement (existing registrations must be preserved)." https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:141: LOG(DFATAL) << "The suffixed ProgId is longer than 39 characters."; suggest NOTREACHED(); since the log will be meaningless in release builds and the person who sees the NOTREACHED in practice can refer to the source to find out what it means. https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:142: chrome_html.erase(39, string16::npos); chrome_html.erase(39); https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:1124: !GetNewUserSpecificRegistrySuffix(&tested_suffix) || a failure in GetNewUserSpecificRegistrySuffix means that the following QuickIsChromeRegistered can't be done, but it doesn't mean that GetOldUserSpecificRegistrySuffix and its QuickIsChromeRegistered must also be skipped. https://chromiumcodereview.appspot.com/10617002/diff/1/chrome/installer/util/... chrome/installer/util/shell_util.cc:1130: DCHECK(InstallUtil::IsPerUserInstall(chrome_exe.c_str()) || it's not clear to me what this is checking. should it be !IsPerUserInstall || QuickIsChromeRegistered...?
Hey robert: https://memegen.googleplex.com/7745708 :) FYI, the old algorithm is in patch set 2, but I definitely think the new one is much cooler/safer (i.e. I wrote it in 10 minutes with no mistake.... ok that's after thinking about it the whole night, but still, less error-prone coding for sure) Cheers, Gab
Adresssed greg's initial comments, see below. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:127: // Returns Chrome's ProgId as "ChromeHTML|suffix|". On 2012/06/22 14:08:24, grt wrote: > although there are other funcs in this file with Chrome in the name, something > nags at me that it's not the best practice. among the reasons: 1) it's Chrome > or Chromium depending on the build, 2) all of this is for Chrome anyway, and 3) > kChromeHTMLProgId should be replaced with a method on BrowserDistribution. Done. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:129: string16 GetChromeHTMLProgId(const string16& suffix) { On 2012/06/22 14:08:24, grt wrote: > i'm not too fond of the fact that this is called all over the place and will > recompute the new suffix over and over again. can you do something about that? Done. Cached it locally as a static variable which is only initialized once. Let me know if that works for you. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:135: // We can however only make sure it's compliant with MSDN in the new-style of On 2012/06/22 14:08:24, grt wrote: > suggest: "Make all new registrations comply with this requirement (existing > registrations must be preserved)." Done. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:141: LOG(DFATAL) << "The suffixed ProgId is longer than 39 characters."; On 2012/06/22 14:08:24, grt wrote: > suggest NOTREACHED(); since the log will be meaningless in release builds and > the person who sees the NOTREACHED in practice can refer to the source to find > out what it means. Done. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:142: chrome_html.erase(39, string16::npos); On 2012/06/22 14:08:24, grt wrote: > chrome_html.erase(39); Done. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1124: !GetNewUserSpecificRegistrySuffix(&tested_suffix) || On 2012/06/22 14:08:24, grt wrote: > a failure in GetNewUserSpecificRegistrySuffix means that the following > QuickIsChromeRegistered can't be done, but it doesn't mean that > GetOldUserSpecificRegistrySuffix and its QuickIsChromeRegistered must also be > skipped. Ah yes, good catch, changed it to check old suffix if the new suffix's check failed. If both failed then assume unsuffixed registration as originally intended. It's brackets and &&/|| logic madness, let me know if you feel I should split this logic up some other way. http://codereview.chromium.org/10617002/diff/1/chrome/installer/util/shell_ut... chrome/installer/util/shell_util.cc:1130: DCHECK(InstallUtil::IsPerUserInstall(chrome_exe.c_str()) || On 2012/06/22 14:08:24, grt wrote: > it's not clear to me what this is checking. should it be !IsPerUserInstall || > QuickIsChromeRegistered...? Ah yes, good catch :)! Added a comment to clarify.
http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:843: if (!GetOldUserSpecificRegistrySuffix(&old_suffix)) { add a comment saying that this gets what the old suffix would be for the check below. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:844: return false; Add a NOTREACHED() right before this. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:845: } else if (QuickIsChromeRegistered(dist, chrome_exe, old_suffix, don't make this an else, make it a separate if statement. It should always be reached except in case of error. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1136: // 3) Unsuffixed (even worst). worst -> worse http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util_unittest.cc:409: TEST(ShellUtilTest, MD5DigestToBase32) { Please add the test cases from http://tools.ietf.org/html/rfc4648#section-10
Comments addressed. I also tested on Windows 7: new-install, over-install on Chrome 19 which is default (keeps unsuffixed registration), and over-install on Chrome 21-dev which is not default (making mine default then takes the new-style registration as expected). Cheers, Gab http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:843: if (!GetOldUserSpecificRegistrySuffix(&old_suffix)) { On 2012/06/22 19:20:22, robertshield wrote: > add a comment saying that this gets what the old suffix would be for the check > below. Done. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:844: return false; On 2012/06/22 19:20:22, robertshield wrote: > Add a NOTREACHED() right before this. Done. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:845: } else if (QuickIsChromeRegistered(dist, chrome_exe, old_suffix, On 2012/06/22 19:20:22, robertshield wrote: > don't make this an else, make it a separate if statement. > > It should always be reached except in case of error. Done. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:1136: // 3) Unsuffixed (even worst). On 2012/06/22 19:20:22, robertshield wrote: > worst -> worse Done. http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10617002/diff/12002/chrome/installer/util/shel... chrome/installer/util/shell_util_unittest.cc:409: TEST(ShellUtilTest, MD5DigestToBase32) { On 2012/06/22 19:20:22, robertshield wrote: > Please add the test cases from http://tools.ietf.org/html/rfc4648#section-10 Done (which forced me to split the implementation in two as otherwise a digest is restricted to 16 characters, so PTAL at that as well).
http://codereview.chromium.org/10617002/diff/22001/chrome/installer/util/shel... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10617002/diff/22001/chrome/installer/util/shel... chrome/installer/util/shell_util_unittest.cc:418: const unsigned char test_array[] = { 'f', 'o', 'o', 'b', 'a', 'r', 'f' }; oops, that should not contain an extra 'f', corrected.
lgtm, a few nits could be addressed later. http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:104: PLOG(DFATAL) << "GetUserName failed"; could make this a NOTREACHED(); http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:844: // OS, it should never fail... but we still need to check for that. Delete the last sentence, it doesn't add any new information. http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... File chrome/installer/util/shell_util_unittest.cc (right): http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... chrome/installer/util/shell_util_unittest.cc:417: // Tests from http://tools.ietf.org/html/rfc4648#section-10. nice :)
Thanks, done. http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:104: PLOG(DFATAL) << "GetUserName failed"; On 2012/06/22 21:15:39, robertshield wrote: > could make this a NOTREACHED(); Done. http://codereview.chromium.org/10617002/diff/27001/chrome/installer/util/shel... chrome/installer/util/shell_util.cc:844: // OS, it should never fail... but we still need to check for that. On 2012/06/22 21:15:39, robertshield wrote: > Delete the last sentence, it doesn't add any new information. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/10617002/29002
Change committed as 143782 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
