Initialize NSS with no DB in the renderer process

Initialize NSS with no DB in the renderer process

This change will allow the use of SSLClientSocketNSS to be used in the renderer
process. This is needed by chromoting client to establish SSL connections.

There are several concerns that are cleared:
1. NSS_NoDB_Init() does open user's cert DB and user modules.
2. NSS_NoDB_Init() doesn't keep file descriptors open.

It tries to open /pkcs11.txt and /secmod.db which is a bug and filed:
https://bugzilla.mozilla.org/show_bug.cgi?id=641052

However the above bug is not considered harmful.

BUG=75834
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78317

[wtc, 2011-03-15 00:21:45]

hclam: thank you for making this work.  Here are my review
comments.

High-Level Comments:

1. Rather than adding EnsureNSSNoDBInit, I'd prefer a
function that must be called before initializing NSS to
set an NSS initialization option.  This way code that
uses EnsureNSSInit can automatically initialize NSS in
NoDB mode if this new function is called first.

Note that this is actually how you implemented
EnsureNSSNoDBInit -- you added an internal function
ForceNoDBInit.

2. Why does the NoDB init mode skip so much code?
It seems that it should only affect how we initialize
NSS.  It should still call PK11_SetPasswordFunc and
InitDefaultRootCerts.

Note: PK11_GetInternalKeySlot should return NULL when
NSS is initialized with no DB, so the code to initialize
the NSS DB password should automatically be skipped.

Detailed Comments:

See below.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h
File base/nss_util.h (right):

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode40
base/nss_util.h:40: // Initialize NSS without a persistent DB.  This is used the
special case
Nit: add "in" or "for" before "the special case"

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode42
base/nss_util.h:42: // the renderer process to enable use of the NSS crypto
library.
Avoid mentioning "renderer process" in the 'base' and 'net'
modules, because these modules may potentially be used by
applications other than Chromium.

I'd delete this sentence, and make the previous sentence
more general, to say "access of persistent DB is unnecessary
or prohibited".

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode45
base/nss_util.h:45: // Access to user security modules is also not allowed.
This should say:
  NSS will be initialized without loading any user security
  modules, including the built-in root certificates module.
  User security modules need to be loaded manually after NSS
  initialization.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode52
base/nss_util.h:52: // *ONLY USE THIS IN THE RENDERER PROCESS*
This warning should be removed, or made more generic, without
reference to the renderer process.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode55
base/nss_util.h:55: // This methos is used to disable checks in NSS when used in
a forked process.
Typo: methos => method

Please document that DisableNSSForkCheck must be called
before NSS is initialized.

I suggest writing the warning like this:

WARNING: use this with caution.

http://codereview.chromium.org/6684018/diff/2001/chrome/renderer/render_proce...
File chrome/renderer/render_process_impl.cc (right):

http://codereview.chromium.org/6684018/diff/2001/chrome/renderer/render_proce...
chrome/renderer/render_process_impl.cc:170: base::EnsureNSPRInit();
EnsureNSSNoDBInit calls EnsureNSPRInit, so you don't need
to call EnsureNSPRInit.

http://codereview.chromium.org/6684018/diff/2001/content/browser/zygote_host_...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/6684018/diff/2001/content/browser/zygote_host_...
content/browser/zygote_host_linux.cc:100: switches::kEnableRemoting,  // Load
NSS for remoting.
The switch says "enable remoting", with no reference to NSS.
So the "Load NSS for remoting" comment looks strange.

http://codereview.chromium.org/6684018/diff/2001/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6684018/diff/2001/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:615: base::EnsureNSPRInit();
Remove this EnsureNSPRInit call.

[Alpha Left Google, 2011-03-15 01:27:51]

Thanks for reviewing.

Here's the patch to address your comments.

To answer your higher level questions:
1. I changed EnsureNSSNoDBInit() to ForceNSSNoDBInit() so that we just need to
call that before calling EnsureNSSInit().

2. The reason the nodb code path runs so little is I try to be consistent with
the windows and mac implementation (see the ifdefs). Since essentially we are
running NSS just like windows and mac, I would like to minimize the change in
logic and stick with win and mac as much as possible.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h
File base/nss_util.h (right):

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode40
base/nss_util.h:40: // Initialize NSS without a persistent DB.  This is used the
special case
On 2011/03/15 00:21:45, wtc wrote:
> Nit: add "in" or "for" before "the special case"

Done.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode42
base/nss_util.h:42: // the renderer process to enable use of the NSS crypto
library.
On 2011/03/15 00:21:45, wtc wrote:
> Avoid mentioning "renderer process" in the 'base' and 'net'
> modules, because these modules may potentially be used by
> applications other than Chromium.
> 
> I'd delete this sentence, and make the previous sentence
> more general, to say "access of persistent DB is unnecessary
> or prohibited".

Done.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode52
base/nss_util.h:52: // *ONLY USE THIS IN THE RENDERER PROCESS*
On 2011/03/15 00:21:45, wtc wrote:
> This warning should be removed, or made more generic, without
> reference to the renderer process.

Done.

http://codereview.chromium.org/6684018/diff/2001/base/nss_util.h#newcode55
base/nss_util.h:55: // This methos is used to disable checks in NSS when used in
a forked process.
On 2011/03/15 00:21:45, wtc wrote:
> Typo: methos => method
> 
> Please document that DisableNSSForkCheck must be called
> before NSS is initialized.
> 
> I suggest writing the warning like this:
> 
> WARNING: use this with caution.

Done.

http://codereview.chromium.org/6684018/diff/2001/chrome/renderer/render_proce...
File chrome/renderer/render_process_impl.cc (right):

http://codereview.chromium.org/6684018/diff/2001/chrome/renderer/render_proce...
chrome/renderer/render_process_impl.cc:170: base::EnsureNSPRInit();
On 2011/03/15 00:21:45, wtc wrote:
> EnsureNSSNoDBInit calls EnsureNSPRInit, so you don't need
> to call EnsureNSPRInit.

Done.

http://codereview.chromium.org/6684018/diff/2001/content/browser/zygote_host_...
File content/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/6684018/diff/2001/content/browser/zygote_host_...
content/browser/zygote_host_linux.cc:100: switches::kEnableRemoting,  // Load
NSS for remoting.
On 2011/03/15 00:21:45, wtc wrote:
> The switch says "enable remoting", with no reference to NSS.
> So the "Load NSS for remoting" comment looks strange.

Removed the comments.

[wtc, 2011-03-15 21:37:20]

LGTM.

Thank you for explaining the motivation for omitting so
much code in the NoDB mode (to match how we initialize NSS
on Mac and Windows).  I have a suggested change related to
that below, at base/nss_util.h:46.

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.cc#newcode207
base/nss_util.cc:207: // This method is used to force NSS to ne initialized
without a DB.
Typo: ne => be

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.cc#newcode306
base/nss_util.cc:306: root_ = InitDefaultRootCerts();
This function and PKCS11PasswordFunc at line 292 above
aren't defined for the !defined(USE_NSS) case.

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.cc#newcode366
base/nss_util.cc:366: bool NSSInitSingleton::force_nodb_init_ = false;
Nit: add a comment:
// static
before this line.

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.h
File base/nss_util.h (right):

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.h#newcode42
base/nss_util.h:42: // persistent DB is prohibited.
Please document that this function is applicable to the
USE_NSS case only.  On Mac and Windows, NSS is always
initialized without a persistent DB.

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.h#newcode46
base/nss_util.h:46: // loaded manually after NSS initialization.
I still think omitting the InitDefaultRootCerts call
is a non-obvious side effect of ForceNSSNoDBInit().

If you really do not want to load the default root certs
on Linux, it may be better to add a DisableDefaultRootCerts
function.

http://codereview.chromium.org/6684018/diff/7002/base/nss_util.h#newcode54
base/nss_util.h:54: // NSS is fork-sensitive to avoid problems when using user
security modules in
Nit: what does "fork-sensitive" mean?  Please reword this
comment.

http://codereview.chromium.org/6684018/diff/7002/content/browser/zygote_main_...
File content/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/6684018/diff/7002/content/browser/zygote_main_...
content/browser/zygote_main_linux.cc:24: #include "base/base64.h"
Nit: why does the new code need "base/base64.h"?