Certificate Transparency: EV certificates whitelist support for ChromeOS

Certificate Transparency: EV certificates whitelist support for ChromeOS

This is a small change to store the EV certificates whitelist in the user's profile, in a similar
manner to the CRL Set.
That means that after the initial component update for the EV whitelist,
users will have the EV whitelist available and will get the EV indicator
immediately after logging in to ChromeOS, rather than having to wait for
the component updater every time.

Implementation is similar to CRLSet as well: During registration of the EVWhitelistComponentInstaller a task is also posted to do an initial load from disk.

BUG=339128
Committed: https://crrev.com/31ff3e17a9534631c704094d75651ba38a63b685
Cr-Commit-Position: refs/heads/master@{#307437}

[Eran Messeri, 2014-11-04 10:50:28]

eranm@chromium.org changed reviewers:
+ sorin@google.com

[Eran Messeri, 2014-11-04 10:51:26]

Sorin, please take a look, if changes to the EVWhitelistComponentInstaller look
decent, I'll rope in other reviewers for the ChromeOS and net code.

[Eran Messeri, 2014-11-07 13:42:27]

eranm@chromium.org changed reviewers:
+ waffles@chromium.org

[Eran Messeri, 2014-11-07 13:43:25]

Added waffles@ as a reviewer because I'm not sure who the right reviewer is.

[Sorin Jianu, 2014-11-07 17:30:43]

sorin@chromium.org changed reviewers:
+ sorin@chromium.org

[Sorin Jianu, 2014-11-07 17:30:45]

Thank you. I apologize for the late answer, when that happens, ping me in chat,
since I usually drop anything to do the code reviews, if I see them coming.

Are there any changes to UTs needed, or new UTs to be written?

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:434: // Android does not currently have
the EV indicator. No need to get the
Can you please clarify the comment?

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:1120:
base::FilePath path;
path can be initialized here and be declared const.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.h (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.h:282: // Starts
loading EV Certificates whitelist
Needs . at the end.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/component...
File chrome/browser/component_updater/ev_whitelist_component_installer.cc
(right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.cc:49: int
list_size = base::checked_cast<int>(compressed_list.size());
const?

Also, is the cast to int needed? What happens if we overflow?

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.cc:182:
content::BrowserThread::FILE, FROM_HERE,
Can we use a thread from the blocking pool for this instead of the FILE thread?
As a general idea, we want to be off the FILE thread as much as possible.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist.h (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist.h:83: void SetNewEVCertsWhitelist(
Would it be ok to rename the function as |SetEVCertsWhitelist|? New seems
redundant in this case since a setter sets a "new" value.

[Eran Messeri, 2014-11-10 22:54:56]

Thanks for the review, addressed all comments, PTAL.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:434: // Android does not currently have
the EV indicator. No need to get the
On 2014/11/07 17:30:44, Sorin Jianu wrote:
> Can you please clarify the comment? 

Done.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:1120:
base::FilePath path;
On 2014/11/07 17:30:44, Sorin Jianu wrote:
> path can be initialized here and be declared const.

Done.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.h (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.h:282: // Starts
loading EV Certificates whitelist
On 2014/11/07 17:30:44, Sorin Jianu wrote:
> Needs . at the end.

Done.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/component...
File chrome/browser/component_updater/ev_whitelist_component_installer.cc
(right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.cc:49: int
list_size = base::checked_cast<int>(compressed_list.size());
On 2014/11/07 17:30:44, Sorin Jianu wrote:
> const?
Done
> 
> Also, is the cast to int needed? What happens if we overflow?
checked_cast has a DCHECK for these cases, doesn't it?
The list really shouldn't be >4GB in size, so I think a checked_cast here is
appropriate.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.cc:182:
content::BrowserThread::FILE, FROM_HERE,
On 2014/11/07 17:30:44, Sorin Jianu wrote:
> Can we use a thread from the blocking pool for this instead of the FILE
thread?
> As a general idea, we want to be off the FILE thread as much as possible.

Done.

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist.h (right):

https://codereview.chromium.org/657613002/diff/40001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist.h:83: void SetNewEVCertsWhitelist(
On 2014/11/07 17:30:45, Sorin Jianu wrote:
> Would it be ok to rename the function as |SetEVCertsWhitelist|? New seems
> redundant in this case since a setter sets a "new" value.

Done.

[Sorin Jianu, 2014-11-11 20:09:28]

lgtm

Thank you Eran. Minor comments below, I don't have a strong opinion about any of
them, let me know what you think, otherwise, it looks good.

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/ev_whitelist_component_installer.cc
(right):

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.cc:49: const
int list_size = base::checked_cast<int>(compressed_list.size());
It seems that the cast is needed since that is what base::WriteFile expects.
However, when the cast fails because the cast is not satisfied, then the program
CHECKS, and Chrome will crash. Is this the expected behavior? Do we need to
handle this error gracefully?

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/ev_whitelist_component_installer.h
(right):

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.h:46:
base::FilePath ev_whitelist_path_;
can this data member be const? In seems it is initialized by the ctor.

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist.h (right):

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist.h:84:
scoped_refptr<net::ct::EVCertsWhitelist> new_whitelist);
I think we can avoid the new_ prefix for the name of the function parameter as
well.

[Sorin Jianu, 2014-11-17 19:33:02]

bump

[Eran Messeri, 2014-11-18 10:44:52]

On 2014/11/17 19:33:02, Sorin Jianu wrote:
> bump

Should have updated to let you know why your comments weren't addressed yet:
* I still haven't tested it on real ChromeOS hardware.
* leecam@ found a regression with CRLSets not being fetched for the guest
profile on ChromeOS and this change may be affected by the same regression.

I intend to test this change on ChromeOS this week.

[Eran Messeri, 2014-11-20 17:55:05]

Sorin, I've addressed all your comments and added registration of the
EVCertsWhitelist in the restore_after_crash_session_manager (based on the bug
leecam@ found, see https://codereview.chromium.org/737043002).

I've also tested on a physical ChromeOS device and confirmed it's working.

Please take another look. Thanks!

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/ev_whitelist_component_installer.cc
(right):

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.cc:49: const
int list_size = base::checked_cast<int>(compressed_list.size());
On 2014/11/11 20:09:28, Sorin Jianu wrote:
> It seems that the cast is needed since that is what base::WriteFile expects.
> However, when the cast fails because the cast is not satisfied, then the
program
> CHECKS, and Chrome will crash. Is this the expected behavior? Do we need to
> handle this error gracefully?

The compressed_list size should always be very small (700K - a few MB), IIUC the
cast would fail if the size would be > 2GB, in which case there's definitely
something wrong.
I've added a check if the value is in range, which skips writing the file in
this case.

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
File chrome/browser/component_updater/ev_whitelist_component_installer.h
(right):

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/component...
chrome/browser/component_updater/ev_whitelist_component_installer.h:46:
base::FilePath ev_whitelist_path_;
On 2014/11/11 20:09:28, Sorin Jianu wrote:
> can this data member be const? In seems it is initialized by the ctor.

Done.

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist.h (right):

https://codereview.chromium.org/657613002/diff/60001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist.h:84:
scoped_refptr<net::ct::EVCertsWhitelist> new_whitelist);
On 2014/11/11 20:09:28, Sorin Jianu wrote:
> I think we can avoid the new_ prefix for the name of the function parameter as
> well. 

Done.

[Sorin Jianu, 2014-11-20 23:58:14]

lgtm

Hi Eran,
thank you, minor comments below.

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.cc
(right):

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.cc:45:
(user_manager->GetActiveUser());
The formatting seems incorrect, unless this is a ChromeOS style issue.

As a side issue, if you have not done it already, please run git cl lint and git
cl format on the files in the component_updater which are owned by us.

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist.h (right):

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist.h:78: // if |whitelist| is not valid,
does nothing.
The comment seems incomplete and it could miss some punctuation.

[Sorin Jianu, 2014-11-20 23:58:16]

lgtm

Hi Eran,
thank you, minor comments below.

[Eran Messeri, 2014-11-26 17:14:31]

All done, pushing now. Thanks.

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.cc
(right):

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.cc:45:
(user_manager->GetActiveUser());
On 2014/11/20 23:58:14, Sorin Jianu wrote:
> The formatting seems incorrect, unless this is a ChromeOS style issue.
> 
> As a side issue, if you have not done it already, please run git cl lint and
git
> cl format on the files in the component_updater which are owned by us.

Done (git-cl formatted).

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/net/packe...
File chrome/browser/net/packed_ct_ev_whitelist.h (right):

https://codereview.chromium.org/657613002/diff/80001/chrome/browser/net/packe...
chrome/browser/net/packed_ct_ev_whitelist.h:78: // if |whitelist| is not valid,
does nothing.
On 2014/11/20 23:58:14, Sorin Jianu wrote:
> The comment seems incomplete and it could miss some punctuation.

Done.

[Eran Messeri, 2014-11-26 17:14:40]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-11-26 17:15:10]

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

[commit-bot: I haz the power, 2014-11-26 17:19:03]

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

[commit-bot: I haz the power, 2014-11-26 17:19:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eran Messeri, 2014-11-26 17:24:26]

eranm@chromium.org changed reviewers:
+ agl@chromium.org, nkostylev@chromium.org

[Eran Messeri, 2014-11-26 17:25:11]

Adding agl@ to for owners review of chrome/browser/net code and nkostylev for
owners review of code under chrome/browser/chromeos/login/session/

[Nikita (slow), 2014-11-26 17:34:15]

On 2014/11/26 17:25:11, Eran wrote:
> Adding agl@ to for owners review of chrome/browser/net code and nkostylev for
> owners review of code under chrome/browser/chromeos/login/session/

lgtm

[agl, 2014-12-08 19:16:21]

lgtm

[Eran Messeri, 2014-12-08 21:15:10]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-12-08 21:16:18]

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

[commit-bot: I haz the power, 2014-12-08 21:21:14]

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

[commit-bot: I haz the power, 2014-12-08 21:21:15]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eran Messeri, 2014-12-08 21:25:53]

eranm@chromium.org changed reviewers:
+ jam@chromium.org

[Eran Messeri, 2014-12-08 21:27:17]

Added jam@ for owners approval of chrome/browser/chrome_browser_main.cc, as
owners of more specific directories have approved.

[jam, 2014-12-08 22:59:12]

lgtm

[Eran Messeri, 2014-12-09 09:08:54]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2014-12-09 09:09:39]

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

[commit-bot: I haz the power, 2014-12-09 09:11:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-09 09:12:35]

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