Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
384413004:
Remove default key slot from KeygenHandler. (Closed)
|
Created:
6 years, 5 months ago by pneubeck (no reviews) Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRemove default key slot from KeygenHandler.
KeygenHandler defaulted to use the persistent key slot. However, the Handler is also used in ChromeOS where no such global persistent slot exists.
This change removes this default behavior from KeygenHandler.
Instead only the slot provided by the CryptoModuleDelegate will be used.
If no CryptoModuleDelegate is set, then an error will be thrown on keygen usage.
The unit test of KeygenHandler now explicitly provides the slot through a StubCryptoModuleDelegate.
In a follow up, the crypto::GetPersistentNSSKeySlot() call will be removed from the unit tests as well and instead the test slot will be get()'ed from the ScopedTestNSSDB.
BUG=210525
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283634
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Fixed compilation, addressed suggestions. #
Total comments: 1
Patch Set 3 : Added a comment. #Patch Set 4 : Adapted build dependencies. #Patch Set 5 : Pass scoped instead of raw slot pointer. #Patch Set 6 : Pass the KeygenHandler in the ShellResourceContext impl. #Patch Set 7 : Fixing android_webview. #
Total comments: 3
Patch Set 8 : Less invasive approach. #
Total comments: 4
Patch Set 9 : Addressed comments: adding ifdefs. #Messages
Total messages: 20 (0 generated)
Maybe you want to take an early look at this one (I didn't get to fixing all compile errors of the other platforms yet). This change is required for the ScopedTestNSSDB change that we discussed, namely keygen_handler_unittest will use ScopedTestNSSDB to setup a test slot without affecting any slot getters in crypto/nss_util . So, feel free to wait for that next change, which should be reviewable tomorrow.
https://codereview.chromium.org/384413004/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/384413004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.cc:331: keygen_handler->set_key_slot(slot.get()); BUG?: This may potentially close |slot|. Should we update the KeygenHandler to make it clear it's taking a ScopedPK11Slot (and thus keeping it alive?) https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler.h File net/base/keygen_handler.h (right): https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler.... net/base/keygen_handler.h:46: void set_key_slot(PK11SlotInfo* slot); Any reason this shouldn't be a crypto::ScopedPK11Slot, so indicate ownership xfer is happening? https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:41: #if defined(OS_CHROMEOS) Is there any reason you restrict this to ChromeOS? Seems like we'd *always* want it in a test DB, with your changes. https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:98: #endif I'm not at all a fan of embedding #ifdefs# like this. Is there some way we can abstract this out to the minimal bits necessary? https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:140: &results[i]), Perhaps wrapping this base::Bind()ing into a helper function that creates a callback, as appropriate?
https://codereview.chromium.org/384413004/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/384413004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.cc:331: keygen_handler->set_key_slot(slot.get()); On 2014/07/14 21:54:24, Ryan Sleevi wrote: > BUG?: This may potentially close |slot|. Should we update the KeygenHandler to > make it clear it's taking a ScopedPK11Slot (and thus keeping it alive?) I didn't see a point in passing ownership of a PK11Slot: 1) Anybody can increase the ref-count to the slot 2) Someone sets up and will destroy the slot. That someone should have the real ownership (or pass it on, together with the responsibility to destroy). Passing around multiple scoped ptrs to the same slot obscures that ownership/responsibility. Following this argument, there should be exactly one ScopedPK11Slot at a time and all other users have a PK11SlotInfo*. But maybe I'm missing an important part of NSS memory management. I agree that relying on 1) isn't ideal because it introduces dependencies on PK11_ReferenceSlot all over the place. And in this function it's confusing that CreateKeygenHandlerWithSlot has ownership but doesn't pass it on, because I didn't touch GetPrivateNSSKeySlotForResourceContext. What's your usual strategy of passing PK11Slots around? https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:41: #if defined(OS_CHROMEOS) On 2014/07/14 21:54:25, Ryan Sleevi wrote: > Is there any reason you restrict this to ChromeOS? > > Seems like we'd *always* want it in a test DB, with your changes. Ack. Not sure why the restriction was there before. https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:98: #endif On 2014/07/14 21:54:25, Ryan Sleevi wrote: > I'm not at all a fan of embedding #ifdefs# like this. Is there some way we can > abstract this out to the minimal bits necessary? Yeah, this was a rather quick hack to see whether the change will work. Cleaned this up now. https://codereview.chromium.org/384413004/diff/40001/net/base/keygen_handler_... net/base/keygen_handler_unittest.cc:140: &results[i]), On 2014/07/14 21:54:25, Ryan Sleevi wrote: > Perhaps wrapping this base::Bind()ing into a helper function that creates a > callback, as appropriate? The handler doesn't have to be constructed in on the worker thread. Creating and setting it up before now.
https://codereview.chromium.org/384413004/diff/140001/net/base/keygen_handler.h File net/base/keygen_handler.h (right): https://codereview.chromium.org/384413004/diff/140001/net/base/keygen_handler... net/base/keygen_handler.h:17: #include "crypto/scoped_nss_types.h" hmpf. This leaks nss includes into content/shell which doesn't set the required include folders. If you have a better idea than a) splitting KeygenHandler into an abstract interface and an implementation or b) putting NSS members into a forward declared 'struct NSSDetails' which is defined in the _nss.cc (this also requires to abandon the shared KeygenHandler constructor) , I'd like to hear it.
https://codereview.chromium.org/384413004/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/384413004/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.cc:331: keygen_handler->set_key_slot(slot.get()); On 2014/07/15 13:22:21, pneubeck wrote: > On 2014/07/14 21:54:24, Ryan Sleevi wrote: > > BUG?: This may potentially close |slot|. Should we update the KeygenHandler to > > make it clear it's taking a ScopedPK11Slot (and thus keeping it alive?) > > I didn't see a point in passing ownership of a PK11Slot: > 1) Anybody can increase the ref-count to the slot > 2) Someone sets up and will destroy the slot. That someone should have the real > ownership (or pass it on, together with the responsibility to destroy). Passing > around multiple scoped ptrs to the same slot obscures that > ownership/responsibility. Following this argument, there should be exactly one > ScopedPK11Slot at a time and all other users have a PK11SlotInfo*. But maybe I'm > missing an important part of NSS memory management. > > I agree that relying on 1) isn't ideal because it introduces dependencies on > PK11_ReferenceSlot all over the place. And in this function it's confusing that > CreateKeygenHandlerWithSlot has ownership but doesn't pass it on, because I > didn't touch GetPrivateNSSKeySlotForResourceContext. > > > What's your usual strategy of passing PK11Slots around? We treat it like scoped_refptr, because the objects are reference counted. (Scoped_ptr is appropriate, and how Chrome generally handles reference counted types that are external, because few systems give insight into the actual count to permit sane releasing strategies) That's why I requested (and still request) scopedPtr, since it forces you to increment the ref count beforehand (PK11_ReferenceSlot) and then pass ownership of that reference through to the class, which will ensure the slot is references for the duration of the class.
ptal
sky@ please take a look at: content/browser content/public pfeldman@ please take a look at the changes for the content shell if USE_NSS is defined.
LGTM note that I'm not an owner of content/public/browser/resource_context.h .
avi@ please take a look at the change in content/public/browser/resource_context.h
brettw@ please take a look at the change in content/public/browser/resource_context.h mnaganov@ please review the android changes.
So, a bit more about the design here: The original, preexisting code uses the NSSCryptoModuleDelegate for two reasons: 1) The prompting UI (CryptoModuleDelegate) is contingent/directly related to the slot (ScopedPK11Slot) being accessed. Your new design de-couples these two aspects, and I'm not sure what effect that will have. 2) The intent was to support where content/ embedders can prompt the user which token to store in. For example, Firefox does this when multiple tokens are present, and we've received requests to support this in the past. I *think* your code is safe in that, but I'm having a bit of trouble reasoning about it, and the decoupling in 1 is a little concerning. Matt's a much better person to review this, unfortunately, he's OOO. https://codereview.chromium.org/384413004/diff/230001/content/browser/loader/... File content/browser/loader/resource_scheduler_unittest.cc (right): https://codereview.chromium.org/384413004/diff/230001/content/browser/loader/... content/browser/loader/resource_scheduler_unittest.cc:96: OVERRIDE {} Do you not need to call the callback with NULL? What will happen here? A hang? https://codereview.chromium.org/384413004/diff/230001/content/public/test/moc... File content/public/test/mock_resource_context.cc (right): https://codereview.chromium.org/384413004/diff/230001/content/public/test/moc... content/public/test/mock_resource_context.cc:34: NOTREACHED(); Ditto here https://codereview.chromium.org/384413004/diff/230001/net/base/keygen_handler.h File net/base/keygen_handler.h (right): https://codereview.chromium.org/384413004/diff/230001/net/base/keygen_handler... net/base/keygen_handler.h:55: // password callback is okay here. This comment is out of date.
android_webview LGTM
2nd much less invasive attempt. Not sure why I didn't do this in the first place. I agree that the first approach changed more than initially anticipated.
https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... net/base/keygen_handler_unittest.cc:30: class StubCryptoModuleDelegate : public crypto::NSSCryptoModuleDelegate { #ifdefs ? https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... net/base/keygen_handler_unittest.cc:61: crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot())))); Depending on ordering of your CLs, and to confirm my own understanding, in a future CL, you'll be changing this to something like test_nss_db_.Slot(), like we discussed, right?
https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... net/base/keygen_handler_unittest.cc:61: crypto::ScopedPK11Slot(crypto::GetPersistentNSSKeySlot())))); On 2014/07/16 20:05:42, Ryan Sleevi wrote: > Depending on ordering of your CLs, and to confirm my own understanding, in a > future CL, you'll be changing this to something like test_nss_db_.Slot(), like > we discussed, right? Yes, exactly! I didn't want to do that in one step.
lgtm
Updated the commit message as well. https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... File net/base/keygen_handler_unittest.cc (right): https://codereview.chromium.org/384413004/diff/270001/net/base/keygen_handler... net/base/keygen_handler_unittest.cc:30: class StubCryptoModuleDelegate : public crypto::NSSCryptoModuleDelegate { On 2014/07/16 20:05:42, Ryan Sleevi wrote: > #ifdefs ? Done.
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/384413004/290001
Message was sent while issue was closed.
Change committed as 283634 |
