Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
391783003:
Enabling CRLSet for ChromeOS (Closed)
|
Created:
6 years, 5 months ago by leecam Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionEnabling CRLSet for ChromeOS
BUG=202947
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284921
Patch Set 1 #Patch Set 2 : Correcting BUG= #
Total comments: 3
Patch Set 3 : Moving registration to user login #Patch Set 4 : Removing unused header #Patch Set 5 : Not Registering for Users without a username_hash #
Total comments: 8
Patch Set 6 : Moving path location to a higher level #Patch Set 7 : Added checks for valid crl object #
Total comments: 5
Patch Set 8 : Codereview nits #Patch Set 9 : Using GetProfilePathByUserIdHash #
Total comments: 3
Patch Set 10 : Fixing comment idents #
Total comments: 1
Patch Set 11 : Tidy ifdefs #
Messages
Total messages: 33 (0 generated)
So this breaks 'nacl_integration' tests. Its due to GetProfileByUserIdHash which gets does this call... ProfileManager* profile_manager = g_browser_process->profile_manager(); profile_manager() checks that its being run the on the correct thread and I'm running its on BrowserThread::FILE. Will fix.
So, does it make sense to actually initialize/start this object on ChromeOS? It seems like you can defer initialization on CrOS until the profile startup code, ignoring the failing fetches. That said, it is unfortunate that it creates a gap of opportunity for situations where we have a CRLSet of known-bad certs, but we still allow connections through anyways. I can understand not being able to fetch the latest CRL, but should we be able to make network requests before then? https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... File chrome/browser/net/crl_set_fetcher.cc (right): https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.cc:91: // until a user logs in. 1) Pronouns generally considered harmful in comments ( https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... ) 2) Is this the correct behaviour for all platforms? In particular, what, if anything, does it change for the desktop builds? Will we be doing spurious network activity now for Chromium-on-a-stick builds (or whatever they're called?)
https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... File chrome/browser/net/crl_set_fetcher.cc (right): https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.cc:41: // For ChromeOS we place the file in the Profile Dir Why ChromeOS is different from desktop chrome and needs to put it under user profile? Shouldn't the data be shared for all users/profiles?
On 2014/07/15 01:36:17, xiyuan wrote: > https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... > File chrome/browser/net/crl_set_fetcher.cc (right): > > https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... > chrome/browser/net/crl_set_fetcher.cc:41: // For ChromeOS we place the file in > the Profile Dir > Why ChromeOS is different from desktop chrome and needs to put it under user > profile? Shouldn't the data be shared for all users/profiles? See the linked bug.
On 2014/07/15 01:41:30, Ryan Sleevi wrote: > On 2014/07/15 01:36:17, xiyuan wrote: > > > https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... > > File chrome/browser/net/crl_set_fetcher.cc (right): > > > > > https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... > > chrome/browser/net/crl_set_fetcher.cc:41: // For ChromeOS we place the file in > > the Profile Dir > > Why ChromeOS is different from desktop chrome and needs to put it under user > > profile? Shouldn't the data be shared for all users/profiles? > > See the linked bug. I see https://code.google.com/p/chromium/issues/detail?id=202947#c32 but don't see enough justification. DIR_USER_DATA on ChromeOS is /home/chronos, which is not part of rootfs and is writable to chrome. I don't see why it needs to different from desktop chrome. What did I miss?
On 2014/07/15 02:20:55, xiyuan wrote: > On 2014/07/15 01:41:30, Ryan Sleevi wrote: > > On 2014/07/15 01:36:17, xiyuan wrote: > > > > > > https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... > > > File chrome/browser/net/crl_set_fetcher.cc (right): > > > > > > > > > https://codereview.chromium.org/391783003/diff/20001/chrome/browser/net/crl_s... > > > chrome/browser/net/crl_set_fetcher.cc:41: // For ChromeOS we place the file > in > > > the Profile Dir > > > Why ChromeOS is different from desktop chrome and needs to put it under user > > > profile? Shouldn't the data be shared for all users/profiles? > > > > See the linked bug. > > I see https://code.google.com/p/chromium/issues/detail?id=202947#c32 but don't > see enough justification. DIR_USER_DATA on ChromeOS is /home/chronos, which is > not part of rootfs and is writable to chrome. I don't see why it needs to > different from desktop chrome. What did I miss? We want to avoid a possible cross-user exploitation scenario. User 1 is compromised, attacker modifies CRLSet, then user 2 is compromised. Under Linux, DIR_USER_DATA is not shared between OS users, because for user 1 the CRLSets will be in /home/user1, and for user 2 they will be in /home/user2. On Chrome OS, /home/chronos can be accessed by *all* users, hence we need to keep stuff inside /home/chronos/user.
Thanks Jorge for the clarification. It is more clear to me now. https://codereview.chromium.org/391783003/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/391783003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main.cc:411: g_browser_process->crl_set_fetcher()->StartInitialLoad(cus); The initial load should probably happen after the primary user is logged in. Since we don't seem to need the profile, we could do the initial load in UserManagerImpl::UserLoggedIn. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... If we have to register with |cus| here, then maybe split the registration part and do it here for ChromeOS but move the initial load to the suggested place.
I've moved the registration to xiyuan's suggested spot. Now the CRLSet is loaded as you would expect. I have a bug which is making the tests fail. From a quick scan it looks like its when the user doesn't have a hash but I've got to leave early this afternoon. Will fix and put a new patch up tomorrow AM
Some style comments while you fix the test. As noted on the comment, you can use git cl format or clang-format to format your CLs. You can even hook it up to Sublime. https://codereview.chromium.org/391783003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/users/user_manager_impl.cc (right): https://codereview.chromium.org/391783003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/users/user_manager_impl.cc:442: Style nit: no need to leave empty line here. You can use git cl format/clang-format to format your patches. https://codereview.chromium.org/391783003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/users/user_manager_impl.cc:443: // Register CRLSet now home dir is mounted. "now that"? https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... File chrome/browser/net/crl_set_fetcher.cc (right): https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.cc:43: path.Append(chromeos::ProfileHelper::GetUserProfileDir(*user_id_hash)); Four-space indentation (double indentation) after newline inside a statement. https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... File chrome/browser/net/crl_set_fetcher.h (right): https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.h:53: // Set the path of the CRL set file in the user data dir. "Sets", comments are usually not written in imperative form. https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.h:54: // Optional |user_id_hash| that is used by ChromeOS. "Chrome OS" with a space.
I'm not a fan of pushing the path knowledge in here. If the path is giving you trouble because it's in the CRLSetFetcher, we should instead be looking to make the path configurable, and supply DIR_USER_DATA or the profile dir as appropriate in the higher, invoking layer. https://codereview.chromium.org/391783003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/users/user_manager_impl.cc (right): https://codereview.chromium.org/391783003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/users/user_manager_impl.cc:443: // Register CRLSet now home dir is mounted. On 2014/07/16 16:36:34, Jorge Lucangeli Obes wrote: > "now that"? "now that the" ? https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... File chrome/browser/net/crl_set_fetcher.cc (right): https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.cc:40: #if defined(OS_CHROMEOS) Every #ifdef makes for a sad panda. We should try to abstract this knowledge into something more meaningful/platform agnostic. Ideal world, zero #ifdefs, and any special functionality is gated on compile time. Acceptable world, #ifdefs would be limited to the IO thread initializer (or similarly, singular place) https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... File chrome/browser/net/crl_set_fetcher.h (right): https://codereview.chromium.org/391783003/diff/80001/chrome/browser/net/crl_s... chrome/browser/net/crl_set_fetcher.h:32: const std::string& user_id_hash); This seems like a very ChromeOS-specific concept, in that it doesn't really make sense (at an interface level) on our other platforms. We should find a way to abstract this knowledge, so that it's truly ChromeOS-only.
So I moved the choice of path to outside of CRLSet. No more ifdefs....
https://codereview.chromium.org/391783003/diff/120001/chrome/browser/net/crl_... File chrome/browser/net/crl_set_fetcher.h (right): https://codereview.chromium.org/391783003/diff/120001/chrome/browser/net/crl_... chrome/browser/net/crl_set_fetcher.h:52: // Optional |user_id_hash| that is used by Chrome OS. Oops, stray comment...will remove!
I think this works for me. It's a little grotty in chrome_browser_main, but I don't see any clever ways around that. https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:406: // network. Perhaps add a comment as to where ChromeOS does initialization, so it's clear about the #ifdef-outing here
LGTM with one nit https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/user_manager_impl.cc (right): https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/user_manager_impl.cc:448: chromeos::ProfileHelper::GetUserProfileDir(username_hash)); You can use ProfileHelper::GetProfilePathByUserIdHash to get full profile path instead of appending manually here.
lgtm % rsleevi, xiyuan comments.
lgtm
Thanks guys. Nits done. See note re: xiyuan's recommended call. https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/user_manager_impl.cc (right): https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/user_manager_impl.cc:448: chromeos::ProfileHelper::GetUserProfileDir(username_hash)); On 2014/07/18 19:38:27, xiyuan wrote: > You can use ProfileHelper::GetProfilePathByUserIdHash to get full profile path > instead of appending manually here. I tried this call first but it uses the profile manager g_browser_process->profile_manager() and the the profile for this user is not set up yet.
https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/user_manager_impl.cc (right): https://codereview.chromium.org/391783003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/user_manager_impl.cc:448: chromeos::ProfileHelper::GetUserProfileDir(username_hash)); On 2014/07/19 06:36:39, leecam wrote: > On 2014/07/18 19:38:27, xiyuan wrote: > > You can use ProfileHelper::GetProfilePathByUserIdHash to get full profile path > > instead of appending manually here. > > I tried this call first but it uses the profile manager > g_browser_process->profile_manager() and the the profile for this user is not > set up yet. GetProfilePathByUserIdHash only needs the user_data_dir from profile manager. I don't see where it tries to access the user profile. What error did you get?
The CQ bit was checked by leecam@chromium.org
The CQ bit was unchecked by leecam@chromium.org
I changed it to that function. Apologies I was using ProfileHelper to get the hash which isn't set, but the user_dir is.
The CQ bit was checked by leecam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leecam@chromium.org/391783003/160001
@thestig - adding for chrome/browser/chrome_browser_main.cc
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
https://codereview.chromium.org/391783003/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (left): https://codereview.chromium.org/391783003/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:403: // CRLSetFetcher attempts to load a CRL set from either the local disk or Indentation here should match the code below. https://codereview.chromium.org/391783003/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:408: // delete it from the local disk of those how may have downloaded it. Here too. https://codereview.chromium.org/391783003/diff/160001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/391783003/diff/160001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:407: // For Chrome OS this registration is delayed until user login. Here too.
chrome_browser_main.cc lgtm https://codereview.chromium.org/391783003/diff/180001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/391783003/diff/180001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:404: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) I realize you didn't originally write this, but since you are going to touch all these lines anyway, you can slightly simplify the #ifdefs: #if defined(OS_ANDROID) ... #elif !defined(OS_CHROMEOS) ... #endif
The CQ bit was checked by leecam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leecam@chromium.org/391783003/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
Message was sent while issue was closed.
Change committed as 284921 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
