*wip* NSS: handle chromeos system slot.

net/cert/nss_profile_filter_chromeos.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/nss_profile_filter_chromeos.h"
 
 #include "base/strings/string_piece.h"
 #include "base/strings/stringprintf.h"
 #include "net/cert/x509_certificate.h"
 
@@ skipping 25 matching lines @@
 NSSProfileFilterChromeOS::NSSProfileFilterChromeOS() {}
 
 NSSProfileFilterChromeOS::NSSProfileFilterChromeOS(
     const NSSProfileFilterChromeOS& other) {
   public_slot_.reset(other.public_slot_ ?
       PK11_ReferenceSlot(other.public_slot_.get()) :
       NULL);
   private_slot_.reset(other.private_slot_ ?
       PK11_ReferenceSlot(other.private_slot_.get()) :
       NULL);
+  system_slot_.reset(other.system_slot_ ?
+      PK11_ReferenceSlot(other.system_slot_.get()) :

[pneubeck (no reviews), 2014/06/13 12:40:42]

btw. I think this is one of the best examples, why scoped_ptr seems to be the
wrong wrapper for slots (ref counted in NSS vs. single explicit owner declared
with scoped_ptr).

[mattm, 2014/06/24 03:16:45]

yep. I guess just no one has felt like writing a general "external refcounted
object" wrapper class.

+      NULL);
 }
 
 NSSProfileFilterChromeOS::~NSSProfileFilterChromeOS() {}
 
 NSSProfileFilterChromeOS& NSSProfileFilterChromeOS::operator=(
     const NSSProfileFilterChromeOS& other) {
   public_slot_.reset(other.public_slot_ ?
       PK11_ReferenceSlot(other.public_slot_.get()) :
       NULL);
   private_slot_.reset(other.private_slot_ ?
       PK11_ReferenceSlot(other.private_slot_.get()) :
       NULL);
+  system_slot_.reset(other.system_slot_ ?
+      PK11_ReferenceSlot(other.system_slot_.get()) :
+      NULL);
   return *this;
 }
 
 void NSSProfileFilterChromeOS::Init(crypto::ScopedPK11Slot public_slot,
-                                    crypto::ScopedPK11Slot private_slot) {
+                                    crypto::ScopedPK11Slot private_slot,
+                                    crypto::ScopedPK11Slot system_slot) {
   // crypto::ScopedPK11Slot actually holds a reference counted object.
   // Because scoped_ptr<T> assignment is a no-op if it already points to
   // the same pointer, a reference would be leaked because .Pass() does
   // not release its reference, and the receiving object won't free
   // its copy.
   if (public_slot_.get() != public_slot.get())
     public_slot_ = public_slot.Pass();
   if (private_slot_.get() != private_slot.get())
     private_slot_ = private_slot.Pass();
+  if (system_slot_.get() != system_slot.get())
+    system_slot_ = system_slot.Pass();
 }
 
 bool NSSProfileFilterChromeOS::IsModuleAllowed(PK11SlotInfo* slot) const {
-  // If this is one of the public/private slots for this profile, allow it.
-  if (slot == public_slot_.get() || slot == private_slot_.get())
+  // If this is one of the public/private slots for this profile or the system
+  // slot, allow it.
+  if (slot == public_slot_.get() || slot == private_slot_.get() ||
+      slot == system_slot_.get())

[pneubeck (no reviews), 2014/06/13 12:40:42]

nit: missing {}

[mattm, 2014/06/24 03:16:45]

Done.

     return true;
   // Allow the root certs module.
   if (PK11_HasRootCerts(slot))
     return true;
   // If it's from the read-only slots, allow it.
   if (PK11_IsInternal(slot) && !PK11_IsRemovable(slot))
     return true;
   // If |public_slot_| or |private_slot_| is null, there isn't a way to get the
   // modules to use in the final test.
   if (!public_slot_.get() || !private_slot_.get())
     return false;
   // If this is not the internal (file-system) module or the TPM module, allow
-  // it.
+  // it. (This assumes that private_slot_ and system_slot_ are on the same

[pneubeck (no reviews), 2014/06/13 12:40:42]

could you ensure this with a DCHECK?

[mattm, 2014/06/24 03:16:46]

Done.

+  // module.)
   SECMODModule* module_for_slot = PK11_GetModule(slot);
   if (module_for_slot != PK11_GetModule(public_slot_.get()) &&
       module_for_slot != PK11_GetModule(private_slot_.get()))

[pneubeck (no reviews), 2014/06/13 12:40:42]

nit: missing {}

[mattm, 2014/06/24 03:16:46]

Done.

     return true;

[pneubeck (no reviews), 2014/06/13 12:40:42]

Although you didn't modify it, could you still add a rational why we would
accept (CA) certs of other modules? In which cases would this case occur and why
is it what we want?

Note that the existing class comment is extremely unspecific in what the purpose
of this class/function is: "...if a given slot or certificate should be used for
a given user."

[mattm, 2014/06/24 03:16:45]

This allows for some hypothetical future where we might support plugging in
smartcards/etc. I don't think it actually should happen currently. (I suppose in
dev mode someone could do this today)

   return false;
 }
 
 bool NSSProfileFilterChromeOS::IsCertAllowed(CERTCertificate* cert) const {
   crypto::ScopedPK11SlotList slots_for_cert(
       PK11_GetAllSlotsForCert(cert, NULL));
   if (!slots_for_cert) {
     DVLOG(2) << "cert no slots: " << base::StringPiece(cert->nickname);
     return false;
   }
@@ skipping 28 matching lines @@
     ModuleNotAllowedForProfilePredicate(const NSSProfileFilterChromeOS& filter)
     : filter_(filter) {}
 
 bool NSSProfileFilterChromeOS::ModuleNotAllowedForProfilePredicate::operator()(
     const scoped_refptr<CryptoModule>& module) const {
   return !filter_.IsModuleAllowed(module->os_module_handle());
 }
 
 }  // namespace net