Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317613004:
Remove usage of singleton software_slot_ in nss on ChromeOS (Closed)
|
Created:
6 years, 6 months ago by tbarzic Modified:
6 years, 5 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove usage of singleton software_slot_ in nss on ChromeOS
Instead of opening primary user's public slot separately, do it like it's done
for other users: when InitializeNSSForChromeOSUser is called.
This makes primary user's public slot state not dependent on chromeos::TPMTokenLoader.
Also, with this, opening primary users public slot is not bound with enabling
TPM anymore, so the slot may get open for guest user and on Linux ChromeOS.
BUG=383663, 302062
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282817
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : splitoff browser test cleanup #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 13
Patch Set 6 : . #
Total comments: 2
Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : rebase #Patch Set 10 : . #Patch Set 11 : . #
Total comments: 17
Patch Set 12 : . #Patch Set 13 : . #
Total comments: 6
Patch Set 14 : method renames #
Total comments: 2
Patch Set 15 : split GetAndSetStartInitTPMForCHormeOSUser #Patch Set 16 : . #Patch Set 17 : rebase #Messages
Total messages: 37 (0 generated)
Can you please take a look? https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (left): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#oldco... crypto/nss_util.cc:613: PK11SlotInfo* GetPublicNSSKeySlot() { afaik, on ChromeOS, this is only referenced from default NSSDatabase implementation, which is not used anymore (as singleton NSS databse is not used). https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#newco... crypto/nss_util.cc:453: DVLOG(2) << "Using the test slot for the user's software slot."; so, the main functional change here is that the public slot gets opened when not running on cros (i.e. on Linux ChromeOS; though this has been the case for secondary users for a while) and in Guest mode. (though, this is not reached for users that do not have username hash, like in retail mode). (OpenPersistentNSSDB used to be called only when TPM was enabled) What I'm not certain about is public account user (whether username hash gets set for these, and if so whether public slot should be disabled). pneubeck: do you know anything about this?
https://codereview.chromium.org/317613004/diff/80001/chromeos/tpm_token_loade... File chromeos/tpm_token_loader.cc (right): https://codereview.chromium.org/317613004/diff/80001/chromeos/tpm_token_loade... chromeos/tpm_token_loader.cc:41: void CallEnableTPMTokenForNSS() { optional nit: could be removed. I don't think that the VLOG is worth it to keep a separate function for it. https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (left): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#oldco... crypto/nss_util.cc:613: PK11SlotInfo* GetPublicNSSKeySlot() { On 2014/06/12 01:41:37, tbarzic wrote: > afaik, on ChromeOS, this is only referenced from default NSSDatabase > implementation, which is not used anymore (as singleton NSS databse is not > used). So, this is.... surprising... The 'default' NSSCertDatabase implementation isn't used anymore on ChromeOS (::GetInstance() is never called), but we're still compiling it into the release binary and we have to maintain all it's dependencies which might not make any sense in ChromeOS (like this GetPublicNSSKeySlot() ) ?! In other words, you can't exclude this function (or at least the exposed public function) from ChromeOS compilation. It tried setting up a cleanup for this, but it would require some basic changes about NSSCertDatabase. I will not continue trying that, if there is not strong support from the owners for this... https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#newco... crypto/nss_util.cc:453: DVLOG(2) << "Using the test slot for the user's software slot."; On 2014/06/12 01:41:37, tbarzic wrote: > so, the main functional change here is that the public slot gets opened when not > running on cros (i.e. on Linux ChromeOS; though this has been the case for > secondary users for a while) and in Guest mode. > (though, this is not reached for users that do not have username hash, like in > retail mode). > (OpenPersistentNSSDB used to be called only when TPM was enabled) > > What I'm not certain about is public account user (whether username hash gets > set for these, and if so whether public slot should be disabled). > pneubeck: do you know anything about this? In all types of modes/sessions the username_hash should be set to a usable value. What makes you believe that it isn't the case for retail mode? In public sessions and guest mode, the user should not be able to persist anything beyond logout. (The device wide token that Darren is working on, however should allow use to get certificates _into_ a public session; but again likely not the other way round) Public session can be controlled by policy, whereas the guest mode should behave identical on any device no matter what was configured on the device or who is using/owning that device. Apart from that these sessions should be as similar to regular sessions. I.e. temporary installation of certs/keys should work as usual. One other thing to consider are ephemeral sessions, in which case any user account will be removed after usage (the home directory is mounted in a tmpfs). To be honest, I don't have the full picture and am not sure what your change implies. If you want to, we could VC and you could explain me some more background. https://codereview.chromium.org/317613004/diff/80001/net/base/keygen_handler_... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/317613004/diff/80001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:35: crypto::ScopedTestNSSDB test_nss_db_; will the complicated setup in these browser tests (e.g. chrome/browser/ui/webui/options/certificate_manager_browsertest.cc) be simpler now? (just want to now, can be done in a follow up)
https://codereview.chromium.org/317613004/diff/80001/chromeos/tpm_token_loade... File chromeos/tpm_token_loader.cc (right): https://codereview.chromium.org/317613004/diff/80001/chromeos/tpm_token_loade... chromeos/tpm_token_loader.cc:41: void CallEnableTPMTokenForNSS() { On 2014/06/12 15:14:54, pneubeck wrote: > optional nit: could be removed. I don't think that the VLOG is worth it to keep > a separate function for it. Done. https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (left): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#oldco... crypto/nss_util.cc:613: PK11SlotInfo* GetPublicNSSKeySlot() { On 2014/06/12 15:14:54, pneubeck wrote: > On 2014/06/12 01:41:37, tbarzic wrote: > > afaik, on ChromeOS, this is only referenced from default NSSDatabase > > implementation, which is not used anymore (as singleton NSS databse is not > > used). > > So, this is.... surprising... > The 'default' NSSCertDatabase implementation isn't used anymore on ChromeOS > (::GetInstance() is never called), but we're still compiling it into the release > binary and we have to maintain all it's dependencies which might not make any > sense in ChromeOS (like this GetPublicNSSKeySlot() ) ?! > > In other words, you can't exclude this function (or at least the exposed public > function) from ChromeOS compilation. > > It tried setting up a cleanup for this, but it would require some basic changes > about NSSCertDatabase. I will not continue trying that, if there is not strong > support from the owners for this... yeah, I'd started looking at removing this method for ChromeOS as part of this patch, but came to the similar conclusion as you :) https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#newco... crypto/nss_util.cc:453: DVLOG(2) << "Using the test slot for the user's software slot."; On 2014/06/12 15:14:54, pneubeck wrote: > On 2014/06/12 01:41:37, tbarzic wrote: > > so, the main functional change here is that the public slot gets opened when > not > > running on cros (i.e. on Linux ChromeOS; though this has been the case for > > secondary users for a while) and in Guest mode. > > (though, this is not reached for users that do not have username hash, like in > > retail mode). > > (OpenPersistentNSSDB used to be called only when TPM was enabled) > > > > What I'm not certain about is public account user (whether username hash gets > > set for these, and if so whether public slot should be disabled). > > pneubeck: do you know anything about this? > > In all types of modes/sessions the username_hash should be set to a usable > value. What makes you believe that it isn't the case for retail mode? > > In public sessions and guest mode, the user should not be able to persist > anything beyond logout. > (The device wide token that Darren is working on, however should allow use to > get certificates _into_ a public session; but again likely not the other way > round) > Public session can be controlled by policy, whereas the guest mode should behave > identical on any device no matter what was configured on the device or who is > using/owning that device. > Apart from that these sessions should be as similar to regular sessions. I.e. > temporary installation of certs/keys should work as usual. > > One other thing to consider are ephemeral sessions, in which case any user > account will be removed after usage (the home directory is mounted in a tmpfs). > > > To be honest, I don't have the full picture and am not sure what your change > implies. If you want to, we could VC and you could explain me some more > background. For users other than guest and public account, it doesn't change much. For guest/public account user, an r/w user db will be opened in the users home dir, and it would enable importing certificates (though, not binding them to hardware as TPM is not loaded in these cases). My main concerns with public account user was whether allowing temporary certificate installation makes sense, and if it does making sure that certificates is not persistent. Though, looking at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr..., cryptohome for public account is mounted as ephemeral, so it's home dir should be discarded on log out (and with it the user DB opened here). https://codereview.chromium.org/317613004/diff/80001/net/base/keygen_handler_... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/317613004/diff/80001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:35: crypto::ScopedTestNSSDB test_nss_db_; On 2014/06/12 15:14:54, pneubeck wrote: > will the complicated setup in these browser tests (e.g. > chrome/browser/ui/webui/options/certificate_manager_browsertest.cc) be simpler > now? > > (just want to now, can be done in a follow up) yes, I originally had that here, but decided to split it out to a follow up cl: https://codereview.chromium.org/325313004/
https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#newco... crypto/nss_util.cc:453: DVLOG(2) << "Using the test slot for the user's software slot."; On 2014/06/12 18:38:12, tbarzic wrote: > On 2014/06/12 15:14:54, pneubeck wrote: > > On 2014/06/12 01:41:37, tbarzic wrote: > > > so, the main functional change here is that the public slot gets opened when > > not > > > running on cros (i.e. on Linux ChromeOS; though this has been the case for > > > secondary users for a while) and in Guest mode. > > > (though, this is not reached for users that do not have username hash, like > in > > > retail mode). > > > (OpenPersistentNSSDB used to be called only when TPM was enabled) > > > > > > What I'm not certain about is public account user (whether username hash > gets > > > set for these, and if so whether public slot should be disabled). > > > pneubeck: do you know anything about this? > > > > In all types of modes/sessions the username_hash should be set to a usable > > value. What makes you believe that it isn't the case for retail mode? > > > > In public sessions and guest mode, the user should not be able to persist > > anything beyond logout. > > (The device wide token that Darren is working on, however should allow use to > > get certificates _into_ a public session; but again likely not the other way > > round) > > Public session can be controlled by policy, whereas the guest mode should > behave > > identical on any device no matter what was configured on the device or who is > > using/owning that device. > > Apart from that these sessions should be as similar to regular sessions. I.e. > > temporary installation of certs/keys should work as usual. > > > > One other thing to consider are ephemeral sessions, in which case any user > > account will be removed after usage (the home directory is mounted in a > tmpfs). > > > > > > To be honest, I don't have the full picture and am not sure what your change > > implies. If you want to, we could VC and you could explain me some more > > background. > > For users other than guest and public account, it doesn't change much. > For guest/public account user, an r/w user db will be opened in the users home > dir, and it would enable importing certificates (though, not binding them to > hardware as TPM is not loaded in these cases). > > My main concerns with public account user was whether allowing temporary > certificate installation makes sense, and if it does making sure that > certificates is not persistent. Though, looking at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr..., > cryptohome for public account is mounted as ephemeral, so it's home dir should > be discarded on log out (and with it the user DB opened here). btw. we used to open user db for guest users, but http://crrev.com/215720 started bypassing OpenPersistentNSSDb step for guest users. +stevenjb, in case there was a reason I'm not aware of to do this (I'll nee an OWNER approval for chromeos/ regardless)
https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (left): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#oldco... crypto/nss_util.cc:613: PK11SlotInfo* GetPublicNSSKeySlot() { On 2014/06/12 18:38:12, tbarzic wrote: > On 2014/06/12 15:14:54, pneubeck wrote: > > On 2014/06/12 01:41:37, tbarzic wrote: > > > afaik, on ChromeOS, this is only referenced from default NSSDatabase > > > implementation, which is not used anymore (as singleton NSS databse is not > > > used). > > > > So, this is.... surprising... > > The 'default' NSSCertDatabase implementation isn't used anymore on ChromeOS > > (::GetInstance() is never called), but we're still compiling it into the > release > > binary and we have to maintain all it's dependencies which might not make any > > sense in ChromeOS (like this GetPublicNSSKeySlot() ) ?! > > > > In other words, you can't exclude this function (or at least the exposed > public > > function) from ChromeOS compilation. > > > > It tried setting up a cleanup for this, but it would require some basic > changes > > about NSSCertDatabase. I will not continue trying that, if there is not strong > > support from the owners for this... > > yeah, I'd started looking at removing this method for ChromeOS as part of this > patch, but came to the similar conclusion as you :) Sorry, I haven't looked at the ChromeOS side, but what support/concerns do you need addressed?
https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc File crypto/nss_util.cc (left): https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#oldco... crypto/nss_util.cc:613: PK11SlotInfo* GetPublicNSSKeySlot() { On 2014/06/12 18:44:04, Ryan Sleevi wrote: > On 2014/06/12 18:38:12, tbarzic wrote: > > On 2014/06/12 15:14:54, pneubeck wrote: > > > On 2014/06/12 01:41:37, tbarzic wrote: > > > > afaik, on ChromeOS, this is only referenced from default NSSDatabase > > > > implementation, which is not used anymore (as singleton NSS databse is not > > > > used). > > > > > > So, this is.... surprising... > > > The 'default' NSSCertDatabase implementation isn't used anymore on ChromeOS > > > (::GetInstance() is never called), but we're still compiling it into the > > release > > > binary and we have to maintain all it's dependencies which might not make > any > > > sense in ChromeOS (like this GetPublicNSSKeySlot() ) ?! > > > > > > In other words, you can't exclude this function (or at least the exposed > > public > > > function) from ChromeOS compilation. > > > > > > It tried setting up a cleanup for this, but it would require some basic > > changes > > > about NSSCertDatabase. I will not continue trying that, if there is not > strong > > > support from the owners for this... > > > > yeah, I'd started looking at removing this method for ChromeOS as part of this > > patch, but came to the similar conclusion as you :) > > Sorry, I haven't looked at the ChromeOS side, but what support/concerns do you > need addressed? I think that discussion will sidetrack from the main purpose of this CL. But I'll write a mail to explain what I meant.
On 2014/06/12 18:38:13, tbarzic wrote: > https://codereview.chromium.org/317613004/diff/80001/chromeos/tpm_token_loade... > File chromeos/tpm_token_loader.cc (right): > > https://codereview.chromium.org/317613004/diff/80001/chromeos/tpm_token_loade... > chromeos/tpm_token_loader.cc:41: void CallEnableTPMTokenForNSS() { > On 2014/06/12 15:14:54, pneubeck wrote: > > optional nit: could be removed. I don't think that the VLOG is worth it to > keep > > a separate function for it. > > Done. > > https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc > File crypto/nss_util.cc (left): > > https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#oldco... > crypto/nss_util.cc:613: PK11SlotInfo* GetPublicNSSKeySlot() { > On 2014/06/12 15:14:54, pneubeck wrote: > > On 2014/06/12 01:41:37, tbarzic wrote: > > > afaik, on ChromeOS, this is only referenced from default NSSDatabase > > > implementation, which is not used anymore (as singleton NSS databse is not > > > used). > > > > So, this is.... surprising... > > The 'default' NSSCertDatabase implementation isn't used anymore on ChromeOS > > (::GetInstance() is never called), but we're still compiling it into the > release > > binary and we have to maintain all it's dependencies which might not make any > > sense in ChromeOS (like this GetPublicNSSKeySlot() ) ?! > > > > In other words, you can't exclude this function (or at least the exposed > public > > function) from ChromeOS compilation. > > > > It tried setting up a cleanup for this, but it would require some basic > changes > > about NSSCertDatabase. I will not continue trying that, if there is not strong > > support from the owners for this... > > yeah, I'd started looking at removing this method for ChromeOS as part of this > patch, but came to the similar conclusion as you :) > > https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc > File crypto/nss_util.cc (right): > > https://codereview.chromium.org/317613004/diff/80001/crypto/nss_util.cc#newco... > crypto/nss_util.cc:453: DVLOG(2) << "Using the test slot for the user's software > slot."; > On 2014/06/12 15:14:54, pneubeck wrote: > > On 2014/06/12 01:41:37, tbarzic wrote: > > > so, the main functional change here is that the public slot gets opened when > > not > > > running on cros (i.e. on Linux ChromeOS; though this has been the case for > > > secondary users for a while) and in Guest mode. > > > (though, this is not reached for users that do not have username hash, like > in > > > retail mode). > > > (OpenPersistentNSSDB used to be called only when TPM was enabled) > > > > > > What I'm not certain about is public account user (whether username hash > gets > > > set for these, and if so whether public slot should be disabled). > > > pneubeck: do you know anything about this? > > > > In all types of modes/sessions the username_hash should be set to a usable > > value. What makes you believe that it isn't the case for retail mode? > > > > In public sessions and guest mode, the user should not be able to persist > > anything beyond logout. > > (The device wide token that Darren is working on, however should allow use to > > get certificates _into_ a public session; but again likely not the other way > > round) > > Public session can be controlled by policy, whereas the guest mode should > behave > > identical on any device no matter what was configured on the device or who is > > using/owning that device. > > Apart from that these sessions should be as similar to regular sessions. I.e. > > temporary installation of certs/keys should work as usual. > > > > One other thing to consider are ephemeral sessions, in which case any user > > account will be removed after usage (the home directory is mounted in a > tmpfs). > > > > > > To be honest, I don't have the full picture and am not sure what your change > > implies. If you want to, we could VC and you could explain me some more > > background. > > For users other than guest and public account, it doesn't change much. > For guest/public account user, an r/w user db will be opened in the users home > dir, and it would enable importing certificates (though, not binding them to > hardware as TPM is not loaded in these cases). > > My main concerns with public account user was whether allowing temporary > certificate installation makes sense, and if it does making sure that > certificates is not persistent. Though, looking at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr..., > cryptohome for public account is mounted as ephemeral, so it's home dir should > be discarded on log out (and with it the user DB opened here). > > https://codereview.chromium.org/317613004/diff/80001/net/base/keygen_handler_... > File net/base/keygen_handler_unittest.cc (right): > > https://codereview.chromium.org/317613004/diff/80001/net/base/keygen_handler_... > net/base/keygen_handler_unittest.cc:35: crypto::ScopedTestNSSDB test_nss_db_; > On 2014/06/12 15:14:54, pneubeck wrote: > > will the complicated setup in these browser tests (e.g. > > chrome/browser/ui/webui/options/certificate_manager_browsertest.cc) be simpler > > now? > > > > (just want to now, can be done in a follow up) > > yes, I originally had that here, but decided to split it out to a follow up cl: > https://codereview.chromium.org/325313004/ awesome!
https://codereview.chromium.org/317613004/diff/100001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/100001/crypto/nss_util.cc#newc... crypto/nss_util.cc:454: public_slot.reset(PK11_ReferenceSlot(test_slot_)); this isn't necessary (GetPublicSlotForChromeOSUser already returns the test_slot if it's set)
https://codereview.chromium.org/317613004/diff/100001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/100001/crypto/nss_util.cc#newc... crypto/nss_util.cc:454: public_slot.reset(PK11_ReferenceSlot(test_slot_)); On 2014/06/13 22:04:59, mattm wrote: > this isn't necessary (GetPublicSlotForChromeOSUser already returns the test_slot > if it's set) good point; I'll bail out here if test slot is set.
I'll leave Matt to stamp this - he knows this code better than I. My quick review is that it Looks.Good.To.Me
oops, lgtm.
can this be committed?
On 2014/06/26 09:59:56, pneubeck wrote: > can this be committed? sorry to bother you, but this change would be very helpful because it simplifies a few tests using NSSCertDatabase.
lgtm
rsleevi, stevenjb: I need OWNER approvals from you for crypto/ and chromeos/ adding rlp for OWNER approval for chrome/browser/profiles/
How is the device owner determined now (the key lives in the owners NSS DB, not chaps)?
On 2014/06/30 18:40:44, Ryan Sleevi wrote: > How is the device owner determined now (the key lives in the owners NSS DB, not > chaps)? The way the ownership is determined should not change. The user's nss db still gets loaded on login, but instead of when TPMToken loader starts, it happens during the user's profile initialization (both of these start when the user logs in).
On 2014/06/30 22:08:06, tbarzic wrote: > On 2014/06/30 18:40:44, Ryan Sleevi wrote: > > How is the device owner determined now (the key lives in the owners NSS DB, > not > > chaps)? > > The way the ownership is determined should not change. The user's nss db still > gets loaded on login, but instead of when TPMToken loader starts, it happens > during the user's profile initialization (both of these start when the user logs > in). Actually, I missed a case where opening nss db during profile initialization may not be soon enough: If we detect that the device policy file is corrupted, we'll allow login only if the user is owner (which would have to happen before profile creation starts) :( I'll try to rework this...
profiles LGTM (it sounds like things might change a bit. just let me know if they do in profiles/ and I can take another look)
https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/auth/parallel_authenticator.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/auth/parallel_authenticator.cc:511: if (LoginState::IsInitialized()) { When you added this, you wrote that SafeMode depends on the CertLoader: https://chromiumcodereview.appspot.com/23684033 Even in the old version of CertLoader when it included the TPM initialization, why is this dependency? SafeMode only requires the private key from the software token and neither certs nor the TPM being initialized, IIUC. The purpose of the certificate loading in CertLoader is also solely to have a list of certs available for the UI thread. If the dependency is required, how was the old code supposed to be synchronized with the cert loading? the loading is asynchronous and I don't see any waiting for events. https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:207: // Make sure NSS is initialzied and NSS DB is loaded for the user before typo: initialzied -> initialized https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:208: // serching for the owner key. typo: serching -> searching https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:216: true /* provisional */), How about always initializing NSS in the parallel authenticator (e.g. in ParallelAuthenticator::ResolveState()) instead of in the UserManager? Then we wouldn't need this provisional flag and would always initialize the slots at the same time.
https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/auth/parallel_authenticator.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/auth/parallel_authenticator.cc:511: if (LoginState::IsInitialized()) { On 2014/07/01 14:37:17, pneubeck wrote: > When you added this, you wrote that SafeMode depends on the CertLoader: > https://chromiumcodereview.appspot.com/23684033 > > Even in the old version of CertLoader when it included the TPM initialization, > why is this dependency? SafeMode only requires the private key from the software > token and neither certs nor the TPM being initialized, IIUC. The purpose of the > certificate loading in CertLoader is also solely to have a list of certs > available for the UI thread. > > If the dependency is required, how was the old code supposed to be synchronized > with the cert loading? the loading is asynchronous and I don't see any waiting > for events. It depended on CertLoader because CertLoader was actually where software token used to be loaded (OpenPersistentNSSDB in TPMTokenLoader used to be part of CertLoader class). Though, I admit that that cl description could have been more precise. https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:216: true /* provisional */), On 2014/07/01 14:37:17, pneubeck wrote: > How about always initializing NSS in the parallel authenticator (e.g. in > ParallelAuthenticator::ResolveState()) instead of in the UserManager? Then we > wouldn't need this provisional flag and would always initialize the slots at the > same time. The NSS is actually initialized in profile_io_data.cc. The reason I need a flag like this is not to block initialization there. What's initialized in UserManager is CertLoader.
https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/auth/parallel_authenticator_unittest.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/auth/parallel_authenticator_unittest.cc:97: state_.reset(new TestAttemptState(user_context_, false)); Random aside; Why is this all in SetUp/TearDown, as opposed to ctor/dtor? And why are you DCHECKing in a unit test (line 77)? May not be your code, just lots of wtfs. https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.h (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.h:74: static void IsPrivateKeyExistAsync(const IsOwnerCallback& callback); If this is a static method, can't you really just move it into the .cc definition? Why make it a static class member? https://codereview.chromium.org/317613004/diff/200001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/200001/crypto/nss_util.cc#newc... crypto/nss_util.cc:465: } This logic strikes me as a little weird, as the comment doesn't fully explain the possible conditions. If I set provisional to false, and load, it will return true. If I then attempt to load again, this time with provisional=true, it will return false (even though it's loaded and usable) If I set provisional to true, and load, it will return true. If I then attempt to load again, this time with provisional=false, it will true, but not actually do any work (it just does set_provisional) Regardless of the provisional flag, these methods all have side-effects on the overall NSS state. https://codereview.chromium.org/317613004/diff/200001/crypto/nss_util_internal.h File crypto/nss_util_internal.h (right): https://codereview.chromium.org/317613004/diff/200001/crypto/nss_util_interna... crypto/nss_util_internal.h:53: // Returns true if the user was added or confimed, false if it already typo: confirmed?
https://codereview.chromium.org/317613004/diff/200001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/200001/crypto/nss_util.cc#newc... crypto/nss_util.cc:465: } On 2014/07/01 18:51:33, Ryan Sleevi wrote: > This logic strikes me as a little weird, as the comment doesn't fully explain > the possible conditions. > > If I set provisional to false, and load, it will return true. If I then attempt > to load again, this time with provisional=true, it will return false (even > though it's loaded and usable) > > If I set provisional to true, and load, it will return true. If I then attempt > to load again, this time with provisional=false, it will true, but not actually > do any work (it just does set_provisional) > > Regardless of the provisional flag, these methods all have side-effects on the > overall NSS state. Yeah, I'm not to content with semantics here.. I need to come up with better naming, and improve the comment (I've planned to do that today). Stay tuned. (Basically, the purpose for this is to let profile_io_data.cc know whether it should continue with initializing private slot for the user or not)
PTAL, I think the approach in the new patchset is a bit cleaner.. adding ygorshenin (as an owner in c/b/c/login/) https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/auth/parallel_authenticator_unittest.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/auth/parallel_authenticator_unittest.cc:97: state_.reset(new TestAttemptState(user_context_, false)); On 2014/07/01 18:51:33, Ryan Sleevi wrote: > Random aside; Why is this all in SetUp/TearDown, as opposed to ctor/dtor? And > why are you DCHECKing in a unit test (line 77)? > > May not be your code, just lots of wtfs. Not sure about background for these.. Though, I think most of the unit tests in chrome/browser/chromeos setup various mocked/fake services in SetUp method rather than in ctor. I'll remove the dcheck https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:207: // Make sure NSS is initialzied and NSS DB is loaded for the user before On 2014/07/01 14:37:17, pneubeck wrote: > typo: initialzied -> initialized Done. https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:208: // serching for the owner key. On 2014/07/01 14:37:17, pneubeck wrote: > typo: serching -> searching Done. https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.h (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.h:74: static void IsPrivateKeyExistAsync(const IsOwnerCallback& callback); On 2014/07/01 18:51:33, Ryan Sleevi wrote: > If this is a static method, can't you really just move it into the .cc > definition? Why make it a static class member? no valid reason; moved to .cc
https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc#newc... crypto/nss_util.cc:476: bool RequestInitializeTPMForChromeOSUser( New approach seems good. Not entirely sure about the naming... request makes it sort of sound like the caller is requesting nss_util to do the initialization. ShouldInitializeTPMForChromeOSUser sounds better, but less clear that it has side effects. wdyt?
https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc#newc... crypto/nss_util.cc:476: bool RequestInitializeTPMForChromeOSUser( On 2014/07/02 22:31:39, mattm wrote: > New approach seems good. > Not entirely sure about the naming... request makes it sort of sound like the > caller is requesting nss_util to do the initialization. > ShouldInitializeTPMForChromeOSUser sounds better, but less clear that it has > side effects. > wdyt? Yeah, I had exactly the same thoughts about naming. Another idea I had was (Own|Lock|AttemptStart)TPMInitializationForChromeOSUser and . If I don't come up with anything better, I'll change it to ShouldInitialize...
This is largely out of my depth, but I don't see any red flags, just nits and the name change, lgtm. https://codereview.chromium.org/317613004/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.cc (right): https://codereview.chromium.org/317613004/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:119: void IsPrivateKeyExistAsync( nit: Can we rename this DoesPrivateKeyExistAsync? :) https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc#newc... crypto/nss_util.cc:476: bool RequestInitializeTPMForChromeOSUser( On 2014/07/02 22:50:23, tbarzic wrote: > On 2014/07/02 22:31:39, mattm wrote: > > New approach seems good. > > Not entirely sure about the naming... request makes it sort of sound like the > > caller is requesting nss_util to do the initialization. > > ShouldInitializeTPMForChromeOSUser sounds better, but less clear that it has > > side effects. > > wdyt? > > Yeah, I had exactly the same thoughts about naming. Another idea I had was > (Own|Lock|AttemptStart)TPMInitializationForChromeOSUser and . > If I don't come up with anything better, I'll change it to ShouldInitialize... So, I think the most clear thing to do here would be to separate Was and Set + TPMInitializeForChromeOSUserRequested. If that is inconvenient then I would be explicit: CheckIfTPMInitializeForChromeOSUserRequestedAndSet(). A bit of a mouthufull, but it is clear at least...
chrome/browser/chromeos/ownership/* lgtm.
https://codereview.chromium.org/317613004/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/ownership/owner_settings_service.cc (right): https://codereview.chromium.org/317613004/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/ownership/owner_settings_service.cc:119: void IsPrivateKeyExistAsync( On 2014/07/02 23:06:02, stevenjb wrote: > nit: Can we rename this DoesPrivateKeyExistAsync? :) Done. https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/317613004/diff/240001/crypto/nss_util.cc#newc... crypto/nss_util.cc:476: bool RequestInitializeTPMForChromeOSUser( On 2014/07/02 23:06:03, stevenjb wrote: > On 2014/07/02 22:50:23, tbarzic wrote: > > On 2014/07/02 22:31:39, mattm wrote: > > > New approach seems good. > > > Not entirely sure about the naming... request makes it sort of sound like > the > > > caller is requesting nss_util to do the initialization. > > > ShouldInitializeTPMForChromeOSUser sounds better, but less clear that it has > > > side effects. > > > wdyt? > > > > Yeah, I had exactly the same thoughts about naming. Another idea I had was > > (Own|Lock|AttemptStart)TPMInitializationForChromeOSUser and . > > If I don't come up with anything better, I'll change it to ShouldInitialize... > > So, I think the most clear thing to do here would be to separate Was and Set + > TPMInitializeForChromeOSUserRequested. If that is inconvenient then I would be > explicit: CheckIfTPMInitializeForChromeOSUserRequestedAndSet(). A bit of a > mouthufull, but it is clear at least... Went with something similar: GetAndSetStartedInitializeTPMForChromeOSUser
https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/auth/parallel_authenticator.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/auth/parallel_authenticator.cc:511: if (LoginState::IsInitialized()) { On 2014/07/01 16:33:07, tbarzic wrote: > On 2014/07/01 14:37:17, pneubeck wrote: > > When you added this, you wrote that SafeMode depends on the CertLoader: > > https://chromiumcodereview.appspot.com/23684033 > > > > Even in the old version of CertLoader when it included the TPM initialization, > > why is this dependency? SafeMode only requires the private key from the > software > > token and neither certs nor the TPM being initialized, IIUC. The purpose of > the > > certificate loading in CertLoader is also solely to have a list of certs > > available for the UI thread. > > > > If the dependency is required, how was the old code supposed to be > synchronized > > with the cert loading? the loading is asynchronous and I don't see any waiting > > for events. > > It depended on CertLoader because CertLoader was actually where software token > used to be loaded (OpenPersistentNSSDB in TPMTokenLoader used to be part of > CertLoader class). Though, I admit that that cl description could have been more > precise. Since you're removing OpenPersistentNSSDB from TPMTokenLoader and instead initializing the NSSDB in IsOwnerForSafeModeAsync, isn't this comment incorrect now?
Sorry for the delays. I'm still having a bit of trouble wrapping my head around some of this. https://codereview.chromium.org/317613004/diff/260001/crypto/nss_util_internal.h File crypto/nss_util_internal.h (right): https://codereview.chromium.org/317613004/diff/260001/crypto/nss_util_interna... crypto/nss_util_internal.h:60: CRYPTO_EXPORT bool GetAndSetStartedInitializeTPMForChromeOSUser( Naming still confused the hell out of me. ShouldInitializeTPMForChromeOSUser? I'm guessing the issue is that once this API returns true, it expects the caller to initialize, because it's going to start returning false for everyone else? That seems like a dangerous sort of API. IsResponsibleToInitializeTPMForChromeOSUser? Alternatively, split the ShouldInit and SetWillInit into two methods.
https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/auth/parallel_authenticator.cc (right): https://codereview.chromium.org/317613004/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/auth/parallel_authenticator.cc:511: if (LoginState::IsInitialized()) { On 2014/07/09 16:08:11, pneubeck wrote: > On 2014/07/01 16:33:07, tbarzic wrote: > > On 2014/07/01 14:37:17, pneubeck wrote: > > > When you added this, you wrote that SafeMode depends on the CertLoader: > > > https://chromiumcodereview.appspot.com/23684033 > > > > > > Even in the old version of CertLoader when it included the TPM > initialization, > > > why is this dependency? SafeMode only requires the private key from the > > software > > > token and neither certs nor the TPM being initialized, IIUC. The purpose of > > the > > > certificate loading in CertLoader is also solely to have a list of certs > > > available for the UI thread. > > > > > > If the dependency is required, how was the old code supposed to be > > synchronized > > > with the cert loading? the loading is asynchronous and I don't see any > waiting > > > for events. > > > > It depended on CertLoader because CertLoader was actually where software token > > used to be loaded (OpenPersistentNSSDB in TPMTokenLoader used to be part of > > CertLoader class). Though, I admit that that cl description could have been > more > > precise. > > Since you're removing OpenPersistentNSSDB from TPMTokenLoader and instead > initializing the NSSDB in IsOwnerForSafeModeAsync, > isn't this comment incorrect now? Changed the comment, though, it may make sense to remove this logged in state after this lands. https://codereview.chromium.org/317613004/diff/260001/crypto/nss_util_internal.h File crypto/nss_util_internal.h (right): https://codereview.chromium.org/317613004/diff/260001/crypto/nss_util_interna... crypto/nss_util_internal.h:60: CRYPTO_EXPORT bool GetAndSetStartedInitializeTPMForChromeOSUser( On 2014/07/09 19:05:40, Ryan Sleevi wrote: > Naming still confused the hell out of me. > > ShouldInitializeTPMForChromeOSUser? > > I'm guessing the issue is that once this API returns true, it expects the caller > to initialize, because it's going to start returning false for everyone else? > That seems like a dangerous sort of API. > Yeah, we had the same expectation with old API, but somewhat hidden in InitializeNSSForChromeOSUser. > IsResponsibleToInitializeTPMForChromeOSUser? > > Alternatively, split the ShouldInit and SetWillInit into two methods. OK, I'll go with this. Should be less confusing.
I think this now only needs a last lgtm from Ryan :-)
lgtm
The CQ bit was checked by tbarzic@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/317613004/320001
Message was sent while issue was closed.
Change committed as 282817 |
