Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks by loading the system TPM slot
on chrome start-up and using it as an additional source of client
certificates for CertLoader.
CertLoader supports working with only one database (e.g. only system
token) or both databases (system and user token).
If CertLoader works with both databases, it also supports the special case that the user's database allows access to the system token (e.g. for affiliated users) and prevents duplicate certificates in its output.

BUG=655266
Review-Url: https://codereview.chromium.org/2858113003
Cr-Commit-Position: refs/heads/master@{#471280}
Committed: https://chromium.googlesource.com/chromium/src/+/2f149a05d31ad319ef6b620911b74e390dbee354

[pmarko, 2017-05-04 14:51:34]

Description was changed from

==========
[TEMP] Enable device-wide EAP-TLS networks

Current state of implementation - not ready for review.

BUG=
==========

to

==========
[TEMP] Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks by loading the system TPM slot
on chrome start-up and using it as an additional source of client
certificates for CertLoader.
CertLoader supports working with only one database (e.g. only system
token) or both databases (system and user token).

BUG=655266
==========

[pmarko, 2017-05-09 10:56:08]

Description was changed from

==========
[TEMP] Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks by loading the system TPM slot
on chrome start-up and using it as an additional source of client
certificates for CertLoader.
CertLoader supports working with only one database (e.g. only system
token) or both databases (system and user token).

BUG=655266
==========

to

==========
Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks by loading the system TPM slot
on chrome start-up and using it as an additional source of client
certificates for CertLoader.
CertLoader supports working with only one database (e.g. only system
token) or both databases (system and user token).
If CertLoader works with both databases, it also supports the special case that
the user's database allows access to the system token (e.g. for affiliated
users) and prevents duplicate certificates in its output.

BUG=655266
==========

[pmarko, 2017-05-09 10:57:58]

pmarko@chromium.org changed reviewers:
+ stevenjb@chromium.org

[pmarko, 2017-05-09 10:57:59]

Hi Maksim and Steven,

please review this CL. This is a follow-up to
https://codereview.chromium.org/2828713002/ , now finally loading the system
token on start-up and using it for device-wide EAP-TLS networks, independently
of the user's NSS database.

[stevenjb, 2017-05-09 17:04:02]

Did you mean to include maksim on the review?

Unfortunately I'm not as up to speed on the certificate stuff as I would like to
be (but I should be paying more attention to it soon).

https://codereview.chromium.org/2858113003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:113: }
Looking through the code, I don't see access to any of these methods outside of
chrome_browser_main_chromeos.cc.

Is there any reason not to make this part of CertLoader or
ChromeBrowserMainPartsChromeos (encapsulated as a helper class)?

The reason I ask is that for mus+ash I expect we will need to access
certificates from both the Chrome and Ash processes, and I don't think we will
want this code to be part of the Browser at that point. Putting the code here
will make that extraction a bit more complicated.

https://codereview.chromium.org/2858113003/diff/60001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/60001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:53: net::NSSCertDatabase* GetNSSDB() { return database_;
}
nit: database()
(and maybe rename the member/accessor nss_database)

https://codereview.chromium.org/2858113003/diff/60001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:254: user_cert_cache_->certificates_loaded();
Is this correct?

I'm a bit fuzzy on whether both system_cert_cache_ and user_cert_cache_ can be
active? I assume the can be, otherwise we wouldn't need both, correct? In that
case shouldn't this be && ?

[pmarko, 2017-05-10 11:43:36]

Patchset #5 (id:80001) has been deleted

[pmarko, 2017-05-10 11:48:00]

pmarko@chromium.org changed reviewers:
+ emaxx@chromium.org

[pmarko, 2017-05-10 11:48:01]

Thank you for your review, and indeed I wanted to include Maksim!
Sorry for the delay Maksim, for some reason you were not a reviewer ( though I
am sure I typed your name into the Reviewers field :) ) - Something went wrong
there.

https://codereview.chromium.org/2858113003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:113: }
On 2017/05/09 17:04:02, stevenjb wrote:
> Looking through the code, I don't see access to any of these methods outside
of
> chrome_browser_main_chromeos.cc.
> 
> Is there any reason not to make this part of CertLoader or
> ChromeBrowserMainPartsChromeos (encapsulated as a helper class)?
> 
> The reason I ask is that for mus+ash I expect we will need to access
> certificates from both the Chrome and Ash processes, and I don't think we will
> want this code to be part of the Browser at that point. Putting the code here
> will make that extraction a bit more complicated.

Good point. I expected other clients to use this global system token database in
the future (e.g. extensions on the sign-in screen, as currently implemented by
Yves), but now I think the nice solution would be to inject that into the
sign-in profile from ChromeBrowserMainPArtsChromeos. So: Done.

https://codereview.chromium.org/2858113003/diff/60001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/60001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:53: net::NSSCertDatabase* GetNSSDB() { return database_;
}
On 2017/05/09 17:04:02, stevenjb wrote:
> nit: database()
> (and maybe rename the member/accessor nss_database)

Done.

https://codereview.chromium.org/2858113003/diff/60001/chromeos/cert_loader.cc...
chromeos/cert_loader.cc:254: user_cert_cache_->certificates_loaded();
On 2017/05/09 17:04:02, stevenjb wrote:
> Is this correct?
> 
> I'm a bit fuzzy on whether both system_cert_cache_ and user_cert_cache_ can be
> active? I assume the can be, otherwise we wouldn't need both, correct? In that
> case shouldn't this be && ?

CertLoader can be used with one or both - I've improved the comments on the
class to make this more clear.
The idea is that:
- Before user sign-in, we start with only the system token NSSCertDatabase, so
only system_cert_cache_ has data.
- After user sign-in, both system_cert_cache_ and user_cert_cache_ have data.

I've documented (in the header) that certificates_loaded() returns true if any
certificates have been loaded, so || makes sense.

[emaxx, 2017-05-11 02:57:46]

I'm fine with the chosen approach in general, but I believe the merging logic
needs a closer look (please see below).

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:379:
base::Unretained(this)));
It seems to be unsafe to use Unretained(this) throughout this class. I don't
know whether it can be ever destroyed in the current architecture, but it's
still better to use a cleaner approach, like WeakPtrFactory.

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:388:
crypto::ScopedPK11Slot system_nss_slot =
nit: #include "crypto/scoped_nss_types.h"

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409:
std::move(system_slot), crypto::ScopedPK11Slot());
nit: I'd prefer having some hints here for what these arguments actually mean,
e.g.:
... /* public_slot */, ... /* private_slot */

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:410:
database->SetSystemSlot(std::move(system_slot_copy));
I think this trick with passing the same slot twice (both as a public and as a
system slot) deserves an explanatory comment (probably, in the beginning of the
function body).

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:417:
std::unique_ptr<net::NSSCertDatabase> system_token_cert_database_;
nit: #include "net/cert/nss_cert_database.h"

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:40: CHECK(!nss_database_);
nit: #include "base/logging.h"

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:59: void LoadCertificates() {
nit: Make private?

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:80: void
UpdateCertificates(std::unique_ptr<net::CertificateList> cert_list) {
nit: Make private?

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:104: bool CertificatesLoading() const {
nit: Rename to lowercase, as it's a simple accessor just like the following two
ones?

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:117: // Flags describing current CertLoader state.
nit: Remove the reference to the CertLoader class.
(And I'd prefer having more detailed comment for each of these flags.)

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:122: // The user-specific NSS certificate database from
which the certificates
nit: The "user-specific" characteristic is not actual anymore.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:127: std::unique_ptr<net::CertificateList> cert_list_;
Is the smart pointer actually needed there?

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:132: };
nit: DISALLOW_COPY_AND_ASSIGN (for consistency).

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:309: std::unique_ptr<net::CertificateList> all_certs =
Can this logic lead to problems in the case when user_cert_cache_ hasn't loaded
yet? IIUC, the CertLoader will respond with an empty list of certs in this case,
despite that it could have returned non-empty list of system-slot certs before.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:321: std::unique_ptr<net::CertificateList> system_certs
=
nit: BTW, wouldn't it be possible to drop the smart pointers here and from
UpdateCertificates, FilterSystemTokenCertificates, et al? Moving
std::unique_ptr's is not much better than moving std::vector's.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:18: #include "net/cert/cert_database.h"
nit: Unnecessary include.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:78: void StartWithSystemNSSDB(net::NSSCertDatabase*
system_slot_database);
nit: I think the "StartWith..." naming scheme becomes a bit misleading, as it
suggests that only a single call of one of them will happen. Maybe
"StartObservingSystemNSSDB"/"StartObservingUserNSSDB", or simple
"SetSystemNSSDB"/"SetUserNSSDB"?

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:96: // Returns true when the certificate list has been
requested but not loaded.
nit: Please update the comment to also explain the semantics of this method when
there are two databases.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:100: // system and a user nss database, this returns true
after the certificates
nit: s/nss/NSS/

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:102: bool certificates_loaded() const;
nit: This name also becomes slightly contradicting with its actual meaning.
What about "initial_certificates_load_finished"? (is this option technically
correct?)

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:138: // True if the initial load of CertLoader is still
pending.
Seems that this member is only used for the debug output? I'd suggest removing
it and keeping the class simpler, unless there's a good reason to have this bit
of information here.

[pmarko, 2017-05-11 11:49:18]

Thank you very much!!

I've fixed everything. I was not sure about getting rid of std::unique_ptr
wrapping the CertificateLists - it would be doable but would look weird in one
place and require a copy in another. Maybe we should do it anyway to improve
readability. Please see the comments below.

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:379:
base::Unretained(this)));
On 2017/05/11 02:57:45, emaxx wrote:
> It seems to be unsafe to use Unretained(this) throughout this class. I don't
> know whether it can be ever destroyed in the current architecture, but it's
> still better to use a cleaner approach, like WeakPtrFactory.

You are right, this could get destroyed if we close the browser again during
startup. Switched to WeakPtrFactory as suggested.
Done.

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:388:
crypto::ScopedPK11Slot system_nss_slot =
On 2017/05/11 02:57:46, emaxx wrote:
> nit: #include "crypto/scoped_nss_types.h"

Done.

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:409:
std::move(system_slot), crypto::ScopedPK11Slot());
On 2017/05/11 02:57:46, emaxx wrote:
> nit: I'd prefer having some hints here for what these arguments actually mean,
> e.g.:
> ... /* public_slot */, ... /* private_slot */

Done.

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:410:
database->SetSystemSlot(std::move(system_slot_copy));
On 2017/05/11 02:57:46, emaxx wrote:
> I think this trick with passing the same slot twice (both as a public and as a
> system slot) deserves an explanatory comment (probably, in the beginning of
the
> function body).

Done.

https://codereview.chromium.org/2858113003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:417:
std::unique_ptr<net::NSSCertDatabase> system_token_cert_database_;
On 2017/05/11 02:57:45, emaxx wrote:
> nit: #include "net/cert/nss_cert_database.h"

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:40: CHECK(!nss_database_);
On 2017/05/11 02:57:46, emaxx wrote:
> nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:59: void LoadCertificates() {
On 2017/05/11 02:57:46, emaxx wrote:
> nit: Make private?

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:80: void
UpdateCertificates(std::unique_ptr<net::CertificateList> cert_list) {
On 2017/05/11 02:57:46, emaxx wrote:
> nit: Make private?

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:104: bool CertificatesLoading() const {
On 2017/05/11 02:57:46, emaxx wrote:
> nit: Rename to lowercase, as it's a simple accessor just like the following
two
> ones?

Good point. I've chosen "initial_load_running" to make the name reflect reality.
Also renamed in CertLoader:: to initial_load_of_any_database_running(). Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:117: // Flags describing current CertLoader state.
On 2017/05/11 02:57:46, emaxx wrote:
> nit: Remove the reference to the CertLoader class.
> (And I'd prefer having more detailed comment for each of these flags.)

Done. Also renamed certificates_loaded_ to initial_load_finished_.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:122: // The user-specific NSS certificate database from
which the certificates
On 2017/05/11 02:57:46, emaxx wrote:
> nit: The "user-specific" characteristic is not actual anymore.

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:127: std::unique_ptr<net::CertificateList> cert_list_;
On 2017/05/11 02:57:46, emaxx wrote:
> Is the smart pointer actually needed there?

Hrm. NSSCertDatabase->ListCerts calls its callback with
std::unique_ptr<net::CertificateList>[1], which is where this originates from.
I'm not sure if we should prefer moving from the vector pointed to there:
cert_list_ = std::move(*cert_list_ptr);
It should work but looks a bit weird. WDYT?


[1]
https://cs.chromium.org/chromium/src/net/cert/nss_cert_database.h?l=92&gs=cpp...

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:132: };
On 2017/05/11 02:57:46, emaxx wrote:
> nit: DISALLOW_COPY_AND_ASSIGN (for consistency).

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:309: std::unique_ptr<net::CertificateList> all_certs =
On 2017/05/11 02:57:46, emaxx wrote:
> Can this logic lead to problems in the case when user_cert_cache_ hasn't
loaded
> yet? IIUC, the CertLoader will respond with an empty list of certs in this
case,
> despite that it could have returned non-empty list of system-slot certs
before.

Very good find! I'd say that this line can stay as it is and
has_system_certificates() should return true only if it has loaded certificates
_and_ has access to the system token.
Added a comment here and at the method and fixed the method.

The unittests didn't catch this because:
- CacheUpdated is called on the UI thread when a *_cert_cache_ has finished
loading certs
- If it originates from the user_cert_cache_, it will work
- It would only break if user_cert_cache_ is initialized and has not finished
its initial cert load, and at that time an update comes in from
system_cert_cache_.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:321: std::unique_ptr<net::CertificateList> system_certs
=
On 2017/05/11 02:57:46, emaxx wrote:
> nit: BTW, wouldn't it be possible to drop the smart pointers here and from
> UpdateCertificates, FilterSystemTokenCertificates, et al? Moving
> std::unique_ptr's is not much better than moving std::vector's.

The only part where this is actually useful is with the
FilterSystemTokenCertificates, UpdateCertificates PostTask.*ReplyWithResult
above:
- all_certs gets moved into the UpdateCertificates callback object and later
into UpdateCertificates.
- We can still use all_certs.get() to get a pointer to the same vector to pass
into FilterSystemTokenCertificates.

Doing this with plain std::vector would be hard (we'd have to make a copy). But
making a copy once might be more efficient than the unique_ptr indirection +
more readable. WDYT?

(BTW fixed UB above by moving the all_certs.get() to a separate statement - C++
parameter evaluation order is undefined).

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:18: #include "net/cert/cert_database.h"
On 2017/05/11 02:57:46, emaxx wrote:
> nit: Unnecessary include.

Done.
We do need to include something which defines X509Certificate so
scoped_refptr<X509Certificate> used through the CertificateList typedef works -
included it directly.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:78: void StartWithSystemNSSDB(net::NSSCertDatabase*
system_slot_database);
On 2017/05/11 02:57:46, emaxx wrote:
> nit: I think the "StartWith..." naming scheme becomes a bit misleading, as it
> suggests that only a single call of one of them will happen. Maybe
> "StartObservingSystemNSSDB"/"StartObservingUserNSSDB", or simple
> "SetSystemNSSDB"/"SetUserNSSDB"?

You're absolutely right. Let's go with Set<type>NSSDB.
Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:96: // Returns true when the certificate list has been
requested but not loaded.
On 2017/05/11 02:57:46, emaxx wrote:
> nit: Please update the comment to also explain the semantics of this method
when
> there are two databases.

Also renamed to initial_load_of_any_database_running() as discussed below. Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:100: // system and a user nss database, this returns true
after the certificates
On 2017/05/11 02:57:46, emaxx wrote:
> nit: s/nss/NSS/

Done.

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:102: bool certificates_loaded() const;
On 2017/05/11 02:57:46, emaxx wrote:
> nit: This name also becomes slightly contradicting with its actual meaning.
> What about "initial_certificates_load_finished"? (is this option technically
> correct?)
Done.
Technically correct. I've decided to use "initial_load_finished" for brevity -
WDYT?

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:138: // True if the initial load of CertLoader is still
pending.
On 2017/05/11 02:57:46, emaxx wrote:
> Seems that this member is only used for the debug output? I'd suggest removing
> it and keeping the class simpler, unless there's a good reason to have this
bit
> of information here.

When CertLoader updates its Observers, it passes a bool initial_load parameter,
suggesting if this is the initial load of certificates.
The initial load is defined as the first time any certificates are loaded (see
Observer interface definition above), so it should be false when loading
additional certs due to adding another slot.

(this can be justified by two reasons:
(1) It should be transparent to CertLoader's clients that CertLoader is using
two databases internally. So the additional database load should be treated in
the same way as an update to the first database
(2) The only client using this parameter is NetworkCertMigrator, which updates
slot numbers and removes non-loaded cert references in shill. This should only
be done on the initial load. There can be no reference to certs from the
additional database when it is loaded, because those references would have been
removed on the first pass.
Instead, ClientCertResolver re-estabilishes that reference)

This boolean tracks that property. It can't be in the CertCache itself because
it is global for the CertLoader, not local per used NSSCertDatabase.

I've updated the comment to make that more clear.

[emaxx, 2017-05-11 14:36:53]

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:127: std::unique_ptr<net::CertificateList> cert_list_;
On 2017/05/11 11:49:18, pmarko wrote:
> On 2017/05/11 02:57:46, emaxx wrote:
> > Is the smart pointer actually needed there?
> 
> Hrm. NSSCertDatabase->ListCerts calls its callback with
> std::unique_ptr<net::CertificateList>[1], which is where this originates from.
> I'm not sure if we should prefer moving from the vector pointed to there:
> cert_list_ = std::move(*cert_list_ptr);
> It should work but looks a bit weird. WDYT?
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/net/cert/nss_cert_database.h?l=92&gs=cpp...

I'm not insisting, but to me ridding of these smart pointers would make it all a
bit clearer.
Yes, you'd have to move from unique_ptr<CertificateList> in that single place
(using the technique that you showed - which looks fine to me), and then you'd
be able to use move semantics with CertificateList directly.

BTW, I believe they're gradually switching to the move semantics in the
net::-related code, see e.g. this recent change:
https://chromium.googlesource.com/chromium/src/+/7ed243fc89fb72230c47f5974212...

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/2858113003/diff/100001/chromeos/cert_loader.h...
chromeos/cert_loader.h:138: // True if the initial load of CertLoader is still
pending.
On 2017/05/11 11:49:18, pmarko wrote:
> On 2017/05/11 02:57:46, emaxx wrote:
> > Seems that this member is only used for the debug output? I'd suggest
removing
> > it and keeping the class simpler, unless there's a good reason to have this
> bit
> > of information here.
> 
> When CertLoader updates its Observers, it passes a bool initial_load
parameter,
> suggesting if this is the initial load of certificates.
> The initial load is defined as the first time any certificates are loaded (see
> Observer interface definition above), so it should be false when loading
> additional certs due to adding another slot.
> 
> (this can be justified by two reasons:
> (1) It should be transparent to CertLoader's clients that CertLoader is using
> two databases internally. So the additional database load should be treated in
> the same way as an update to the first database
> (2) The only client using this parameter is NetworkCertMigrator, which updates
> slot numbers and removes non-loaded cert references in shill. This should only
> be done on the initial load. There can be no reference to certs from the
> additional database when it is loaded, because those references would have
been
> removed on the first pass.
> Instead, ClientCertResolver re-estabilishes that reference)
> 
> This boolean tracks that property. It can't be in the CertCache itself because
> it is global for the CertLoader, not local per used NSSCertDatabase.
> 
> I've updated the comment to make that more clear.

Thanks, I missed that this flag is actually used.

https://codereview.chromium.org/2858113003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:425:
CertLoader::Get()->SetSystemNSSDB(system_token_cert_database_.get());
Is CertLoader guaranteed to be initialized at this point?
Looks like it's created from
ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart() which, I guess, runs
after ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() which constructs
your SystemTokenCertDBInitializer.
It may be correct due to the InitializeDatabase() being called asynchronously,
but worth checking that (and dropping a comment on that here).

https://codereview.chromium.org/2858113003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:425:
CertLoader::Get()->SetSystemNSSDB(system_token_cert_database_.get());
Another question is whether the lifetime of this NSSCertDatabase is always
larger than the one of CertLoader.
Looks like it's true, given that CertLoader is shut down from
ChromeBrowserMainPartsChromeos::PostDestroyThreads(), but still worth commenting
on this explicitly.

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:74: bool has_system_certificates() const {
nit: It's a bit weird that this method returns a bit different thing than the
private member that is named almost the same.
So I'd suggest either just returning has_system_certificates_ here (with leaving
the magic of combining two booleans to the calling code) or inventing a better
name for this accessor.

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.h...
chromeos/cert_loader.h:22: class X509Certificate;
nit: Not necessary now.

[pmarko, 2017-05-11 17:24:57]

I've addressed the comments and switched from
std::uniqute_ptr<net::CertificateList> to net::CertificateList.

https://codereview.chromium.org/2858113003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:425:
CertLoader::Get()->SetSystemNSSDB(system_token_cert_database_.get());
On 2017/05/11 14:36:53, emaxx wrote:
> Another question is whether the lifetime of this NSSCertDatabase is always
> larger than the one of CertLoader.
> Looks like it's true, given that CertLoader is shut down from
> ChromeBrowserMainPartsChromeos::PostDestroyThreads(), but still worth
commenting
> on this explicitly.

Actually, I've added an explicit reset() to the unique_ptr holding this in
ChromeBrowserMainPartsChromeos::PostDestroyThreads.

https://codereview.chromium.org/2858113003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:425:
CertLoader::Get()->SetSystemNSSDB(system_token_cert_database_.get());
On 2017/05/11 14:36:53, emaxx wrote:
> Is CertLoader guaranteed to be initialized at this point?
> Looks like it's created from
> ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart() which, I guess,
runs
> after ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() which constructs
> your SystemTokenCertDBInitializer.
> It may be correct due to the InitializeDatabase() being called asynchronously,
> but worth checking that (and dropping a comment on that here).

Order: For a moment I thought this could be a problem indeed, but it's the other
way round :)
First PostMainMessageLoopStart is executed, then PreMainMessageLoopRun [1]. I
will drop a comment at the beginning of this class because it _is_
counter-intuitive.

[1]
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?rcl=...

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:74: bool has_system_certificates() const {
On 2017/05/11 14:36:53, emaxx wrote:
> nit: It's a bit weird that this method returns a bit different thing than the
> private member that is named almost the same.
> So I'd suggest either just returning has_system_certificates_ here (with
leaving
> the magic of combining two booleans to the calling code) or inventing a better
> name for this accessor.

Agreed and Done. (combining at callsite)

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.h
File chromeos/cert_loader.h (right):

https://codereview.chromium.org/2858113003/diff/140001/chromeos/cert_loader.h...
chromeos/cert_loader.h:22: class X509Certificate;
On 2017/05/11 14:36:53, emaxx wrote:
> nit: Not necessary now.

Done.

[emaxx, 2017-05-11 17:58:39]

LGTM.
And thanks for doing the cleanup of unique_ptr<CertificateList>.

[stevenjb, 2017-05-11 18:25:35]

lgtm

https://codereview.chromium.org/2858113003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:395:
base::Bind(&SystemTokenCertDBInitializer::GotSystemSlotOnIOThread,
nit: BindRepeating?

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:225: base::Unretained(this)))),
nit: I think this would be a little more readable if these were initialized in
the constructor body instead.

[emaxx, 2017-05-11 20:24:53]

(sorry, just ran across two more nits)

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:320: std::move(user_cert_cache_->cert_list()),
nit: std::move() has no effect here and may be omitted.

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:324: std::move(user_cert_cache_->cert_list())));
nit: std::move() has no effect here and may be omitted.

[pmarko, 2017-05-11 21:01:47]

I've addressed the comments and fixed access to weak_ptr_(factory) from the
wrong thread in chrome_browser_main_chromeos.cc. SystemTokenCertDBInitializer's
weak_ptrs are now only accessed on the UI thread. PTAL at that part. Thanks!

https://codereview.chromium.org/2858113003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2858113003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:395:
base::Bind(&SystemTokenCertDBInitializer::GotSystemSlotOnIOThread,
On 2017/05/11 18:25:35, stevenjb wrote:
> nit: BindRepeating?

Done.

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:225: base::Unretained(this)))),
On 2017/05/11 18:25:35, stevenjb wrote:
> nit: I think this would be a little more readable if these were initialized in
> the constructor body instead.

Done.

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:320: std::move(user_cert_cache_->cert_list()),
On 2017/05/11 20:24:52, emaxx wrote:
> nit: std::move() has no effect here and may be omitted.

Done.

https://codereview.chromium.org/2858113003/diff/180001/chromeos/cert_loader.c...
chromeos/cert_loader.cc:324: std::move(user_cert_cache_->cert_list())));
On 2017/05/11 20:24:52, emaxx wrote:
> nit: std::move() has no effect here and may be omitted.

Done.

[pmarko, 2017-05-12 07:23:17]

The CQ bit was checked by pmarko@chromium.org

[pmarko, 2017-05-12 07:23:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2858113003/#ps200001
(title: "Addressed comments and fixed weak_ptr access from wrong thread.")

[commit-bot: I haz the power, 2017-05-12 07:23:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 08:35:02]

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

[commit-bot: I haz the power, 2017-05-12 08:35:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pmarko, 2017-05-12 08:38:15]

The CQ bit was checked by pmarko@chromium.org

[commit-bot: I haz the power, 2017-05-12 08:38:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 09:54:40]

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

[commit-bot: I haz the power, 2017-05-12 09:54:41]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pmarko, 2017-05-12 10:04:33]

The CQ bit was checked by pmarko@chromium.org

[commit-bot: I haz the power, 2017-05-12 10:04:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 11:09:55]

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

[commit-bot: I haz the power, 2017-05-12 11:09:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pmarko, 2017-05-12 11:56:46]

The CQ bit was checked by pmarko@chromium.org

[commit-bot: I haz the power, 2017-05-12 11:57:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 12:21:48]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494590206266100,
"parent_rev": "da9bbac7c610430c9b94142d83ce56ee1d4f2f87", "commit_rev":
"2f149a05d31ad319ef6b620911b74e390dbee354"}

[commit-bot: I haz the power, 2017-05-12 12:22:02]

Description was changed from

==========
Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks by loading the system TPM slot
on chrome start-up and using it as an additional source of client
certificates for CertLoader.
CertLoader supports working with only one database (e.g. only system
token) or both databases (system and user token).
If CertLoader works with both databases, it also supports the special case that
the user's database allows access to the system token (e.g. for affiliated
users) and prevents duplicate certificates in its output.

BUG=655266
==========

to

==========
Enable device-wide EAP-TLS networks

Enable device-wide EAP-TLS networks by loading the system TPM slot
on chrome start-up and using it as an additional source of client
certificates for CertLoader.
CertLoader supports working with only one database (e.g. only system
token) or both databases (system and user token).
If CertLoader works with both databases, it also supports the special case that
the user's database allows access to the system token (e.g. for affiliated
users) and prevents duplicate certificates in its output.

BUG=655266

Review-Url: https://codereview.chromium.org/2858113003
Cr-Commit-Position: refs/heads/master@{#471280}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f149a05d31ad319ef6b620911b7...
==========

[commit-bot: I haz the power, 2017-05-12 12:22:04]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/2f149a05d31ad319ef6b620911b7...