Use NSS for symmetric key crypto operations on Windows and Mac.

Use NSS for symmetric key crypto operations on Windows and Mac.

Encryptor, HMAC, and SymmetricKey now use NSS on all platforms except Android.
This allows us to use them inside the sandbox, something that was not possible
when using the platform APIs.

On Windows, Native Client 64-bit builds still use the the platform APIs.

BUG=127803, 124741
TEST=Existing tests since there is no change in functionality.

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

[Ryan Sleevi, 2012-06-13 20:39:51]

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode70
crypto/crypto.gyp:70: 'symmetric_key_mac.cc',
nit: sorting
nit: line wrapping

issue: You already removed these from the 'sources' list (line 146-219), so this
is not needed. Perhaps that's what you meant by the TODO, in which case, it's
TODONE?

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode80
crypto/crypto.gyp:80: ['exclude', 'signature_verifier_nss\.cc$'],
You need to update this section with the changes you made on 56-59

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode112
crypto/crypto.gyp:112: '../third_party/nss/nss.gyp:nss',
Just to confirm this is absolutely necessary. It should only be the case if
[encryptor.h / hmac.h / symmetric_key.h] are including NSS/NSPR headers.

If so, can we forward declare the types, as done in [signature_verifier.h /
ec_private_key.h / ec_signature_creator.h]?

If not, I don't think I'll lose sleep over it, but it's just potentially one
more build serialization step that would be nice to avoid if possible.

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode317
crypto/crypto.gyp:317: 'NACL_WIN64',
This should be '<@(nacl_win64_defines)'

http://codereview.chromium.org/10543146/diff/2001/crypto/encryptor.h
File crypto/encryptor.h (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/encryptor.h#newcode17
crypto/encryptor.h:17: #if !defined(USE_OPENSSL)
For all code like this, my preference is to use positive assertions, rather than
negative. While longer, it makes it clear what the conditions are for depending
on something.

#if defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)

Same also for line 122.

http://codereview.chromium.org/10543146/diff/2001/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:16: #elif !defined(USE_OPENSSL)
See previous comments

Also line 62/94

[ddorwin, 2012-06-13 21:53:23]

Thanks. I made the requested changes. Running bots now.

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode70
crypto/crypto.gyp:70: 'symmetric_key_mac.cc',
On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> nit: sorting
> nit: line wrapping
> 
> issue: You already removed these from the 'sources' list (line 146-219), so
this
> is not needed. Perhaps that's what you meant by the TODO, in which case, it's
> TODONE?

Done.
I misunderstood and thought *_win.* were automatically added.

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode80
crypto/crypto.gyp:80: ['exclude', 'signature_verifier_nss\.cc$'],
On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> You need to update this section with the changes you made on 56-59

Done.

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode112
crypto/crypto.gyp:112: '../third_party/nss/nss.gyp:nss',
On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> Just to confirm this is absolutely necessary. It should only be the case if
> [encryptor.h / hmac.h / symmetric_key.h] are including NSS/NSPR headers.
> 
> If so, can we forward declare the types, as done in [signature_verifier.h /
> ec_private_key.h / ec_signature_creator.h]?
> 
> If not, I don't think I'll lose sleep over it, but it's just potentially one
> more build serialization step that would be nice to avoid if possible.

I got this from Matt. I'll try building without it.

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode317
crypto/crypto.gyp:317: 'NACL_WIN64',
On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> This should be '<@(nacl_win64_defines)'

Done.

http://codereview.chromium.org/10543146/diff/2001/crypto/encryptor.h
File crypto/encryptor.h (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/encryptor.h#newcode17
crypto/encryptor.h:17: #if !defined(USE_OPENSSL)
On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> For all code like this, my preference is to use positive assertions, rather
than
> negative. While longer, it makes it clear what the conditions are for
depending
> on something.
> 
> #if defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)
> 
> Same also for line 122.

Done.

http://codereview.chromium.org/10543146/diff/2001/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:16: #elif !defined(USE_OPENSSL)
On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> See previous comments
> 
> Also line 62/94

Done.

[ddorwin, 2012-06-13 22:43:42]

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/10543146/diff/2001/crypto/crypto.gyp#newcode112
crypto/crypto.gyp:112: '../third_party/nss/nss.gyp:nss',
On 2012/06/13 21:53:23, ddorwin wrote:
> On 2012/06/13 20:39:51, Ryan Sleevi wrote:
> > Just to confirm this is absolutely necessary. It should only be the case if
> > [encryptor.h / hmac.h / symmetric_key.h] are including NSS/NSPR headers.
> > 
> > If so, can we forward declare the types, as done in [signature_verifier.h /
> > ec_private_key.h / ec_signature_creator.h]?
> > 
> > If not, I don't think I'll lose sleep over it, but it's just potentially one
> > more build serialization step that would be nice to avoid if possible.
> 
> I got this from Matt. I'll try building without it.

Removing them caused the following error on Mac:

In file included from ../../media/crypto/aes_decryptor.cc:10:
In file included from ../../crypto/encryptor.h:18:
../../crypto/scoped_nss_types.h:9:10: fatal error: 'keyhi.h' file not found
#include <keyhi.h>
         ^

I spent a while trying to remove the pk11pub.h includes from scoped_nss_types.h,
but I think it will require some major refactoring.  For example,
symmetric_key.h defines SymmetricKey, which has a ScopedPK11SymKey member. I
think SymmetricKey would need to define an interface and/or use a PImpl.

So, I'd like to leave this here for now. I can file a bug if you want.

[ddorwin, 2012-06-14 19:21:53]

Passes all try bots, Windows shared library builds now work (once
http://codereview.chromium.org/10537178/ lands and is rolled in DEPS), and Clear
Key works without --no-sandbox.

[Ryan Sleevi, 2012-06-14 19:35:26]

+cc ncbray, who can hopefully indicate whether or not this looks right for NaCL
and, if not, how they can be tested.

Otherwise this looks fine, but I'll want to hold off on a more detailed review
until Nick weighs in.

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:190: // (On all platforms, RNG
is primed in WebKit (for window.crypto.getRandom()).
You can remove this second comment.

http://codereview.chromium.org/10543146/diff/2015/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/10543146/diff/2015/crypto/crypto.gyp#newcode22
crypto/crypto.gyp:22: 'NACL_WIN64',
You do not need this variable definition. It comes from build/common.gypi ,
which is the point of using the GYP variable :)

http://code.google.com/searchframe#OAMlx_jo-ck/src/build/common.gypi&exact_pa...

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor.h
File crypto/encryptor.h (right):

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor.h#newcode34
crypto/encryptor.h:34: class CRYPTO_EXPORT Counter {
You shouldn't have to export the child class; because the parent is exported,
these symbols should be visible.

Got a link to a compile error?

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor_unittest.cc
File crypto/encryptor_unittest.cc (right):

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor_unittest.c...
crypto/encryptor_unittest.cc:152: const uint8 kTest1[] = {0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0};
unsigned char, to be consistent with the rest of this file.

[Nick Bray, 2012-06-14 20:37:06]

+cc bradnelson who helped me with the original gyp changes.

There's a lot about this CL I don't know enough about to say anything
intelligent.

In general, all we need is HMAC/SHA256.  Nothing else.  All the "platform" stuff
I've seen on Windows is dead code - crypto/third_party/nss/ is where most of the
live code lives.  As I understand the term, NaCl does not use the platform
libraries, they're just referenced by a bunch of code paths that never get
exercised.

A simple, hermetic HMAC/SHA256 implementation would make our lives easier on all
platforms. At the time, I decided not to do that to avoid duplicating code,
which is something I now regret due to the headaches the NSS library has caused
us on Linux and ChromeOS.  (Chrome has papered over these problems, but we ended
up re-discovering them because we are building crypto into binaries other than
the zygote.)

Would us going hermetic be a net win for crypto/?  It would allow you to
jettison the windows "platform" code?

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:189: // On Linux, NSS uses
system libraries, so the .so's must be loaded.
On a side note, ChromeOS uses a patched version of NSS that behaves differently
than Linux in some cases.  I've been burned by this.  Probably not relevant for
this CL, but beware of NSS skew for ChromeOS hardware.

[ddorwin, 2012-06-14 20:44:19]

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:190: // (On all platforms, RNG
is primed in WebKit (for window.crypto.getRandom()).
On 2012/06/14 19:35:26, Ryan Sleevi wrote:
> You can remove this second comment.

Done.

http://codereview.chromium.org/10543146/diff/2015/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/10543146/diff/2015/crypto/crypto.gyp#newcode22
crypto/crypto.gyp:22: 'NACL_WIN64',
:) Ah, I thought that was a new define in the patch I inherited.

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor.h
File crypto/encryptor.h (right):

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor.h#newcode34
crypto/encryptor.h:34: class CRYPTO_EXPORT Counter {
On 2012/06/14 19:35:26, Ryan Sleevi wrote:
> You shouldn't have to export the child class; because the parent is exported,
> these symbols should be visible.
> 
> Got a link to a compile error?

Tell that to the Windows shared libraries build. :)

Before I added it, I got these link errors:

LINK crypto_unittests.exe
FAILED: python gyp-win-tool link-wrapper "C:\Program Files (x86)\Microsoft
Visual Studio 10.0\VC\bin\link.exe" /nologo /OUT:crypto_unittests.exe
/PDB:crypto_uni
ttests.exe.pdb @crypto_unittests.exe.rsp
crypto_unittests.encryptor_unittest.obj : error LNK2019: unresolved external
symbol "public: __thiscall crypto::Encryptor::Counter::~Counter(void)"
(??1Counter@
Encryptor@crypto@@QAE@XZ) referenced in function "private: virtual void
__thiscall EncryptorTest_CTRCounter_Test::TestBody(void)"
(?TestBody@EncryptorTest_CTRCo
unter_Test@@EAEXXZ)
crypto_unittests.encryptor_unittest.obj : error LNK2019: unresolved external
symbol "public: void __thiscall crypto::Encryptor::Counter::Write(void *)"
(?Write@
Counter@Encryptor@crypto@@QAEXPAX@Z) referenced in function "private: virtual
void __thiscall EncryptorTest_CTRCounter_Test::TestBody(void)"
(?TestBody@Encrypto
rTest_CTRCounter_Test@@EAEXXZ)
crypto_unittests.encryptor_unittest.obj : error LNK2019: unresolved external
symbol "public: bool __thiscall crypto::Encryptor::Counter::Increment(void)"
(?Incr
ement@Counter@Encryptor@crypto@@QAE_NXZ) referenced in function "private:
virtual void __thiscall EncryptorTest_CTRCounter_Test::TestBody(void)"
(?TestBody@Encr
yptorTest_CTRCounter_Test@@EAEXXZ)
crypto_unittests.encryptor_unittest.obj : error LNK2019: unresolved external
symbol "public: __thiscall crypto::Encryptor::Counter::Counter(class
base::BasicStr
ingPiece<class std::basic_string<char,struct std::char_traits<char>,class
std::allocator<char> > > const &)"
(??0Counter@Encryptor@crypto@@QAE@ABV?$BasicStringP
iece@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@base@@@Z)
referenced in function "private: virtual void __thiscall
EncryptorTest_CTRCounter_
Test::TestBody(void)" (?TestBody@EncryptorTest_CTRCounter_Test@@EAEXXZ)
crypto_unittests.exe : fatal error LNK1120: 4 unresolved externals
ninja: build stopped: subcommand failed.

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor_unittest.cc
File crypto/encryptor_unittest.cc (right):

http://codereview.chromium.org/10543146/diff/2015/crypto/encryptor_unittest.c...
crypto/encryptor_unittest.cc:152: const uint8 kTest1[] = {0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0};
On 2012/06/14 19:35:26, Ryan Sleevi wrote:
> unsigned char, to be consistent with the rest of this file.

Done.

[Ryan Sleevi, 2012-06-14 20:54:48]

I'm very curious the problems that you think Chrome has papered over. We're
certainly not trying to do that. If you're seeing problems, would love to know
so we can try and help :)

The problem with hermetic crypto (and I appreciate that you didn't go this
route) is that there are a number of incentives to keeping crypto code
centralized, and not duplicated across multiple places in the tree. Both from a
regulatory compliance view (eg: FIPS), but also from a security and auditing
view (eg: the same bug in three different implementations, all of which have to
be fixed).

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/10543146/diff/2015/chrome/renderer/chrome_rend...
chrome/renderer/chrome_render_process_observer.cc:189: // On Linux, NSS uses
system libraries, so the .so's must be loaded.
On 2012/06/14 20:37:06, Nick Bray wrote:
> On a side note, ChromeOS uses a patched version of NSS that behaves
differently
> than Linux in some cases.  I've been burned by this.  Probably not relevant
for
> this CL, but beware of NSS skew for ChromeOS hardware.

Do you have an example of this?

Chrome on Linux/CrOS makes no statements about what version the system NSS must
be. It uses shared objects for both of them. There are no patches to NSS that
Chrome for Linux uses.

[Nick Bray, 2012-06-14 21:16:43]

> I'm very curious the problems that you think Chrome has papered over. We're
> certainly not trying to do that. If you're seeing problems, would love to know
> so we can try and help :)

The one that prevented me from launching a feature on ChromeOS for M21 was the
zygote overriding fopen to implement opening /dev/urandom by duplicating and
existing file handle.  This override allows NSS to completely init inside the
Chrome sandbox.  nacl_bootstrap (a special Linux binary for launching NaCl)
didn't have this override.  Opening /dev/urandom failed in the Linux version of
Chrome, but we didn't notice or care because NaCl didn't use that part of NSS. 
HOWEVER ChromeOS patches NSS to abort the entire process if opening /dev/urandom
fails.  Boom.  NaCl fails to start on ChromeOS hardware (but not for a
chromeos=1 build run on the desktop) due to a non-obvious dependency between a
patched version of NSS and a patched version of fopen in the zygote. :/  Not
only that, but the patch to NSS prints out an error message referring to a line
number that doesn't actually exist until you patch the sources - so unless you
know the patching exists, figuring out where the heck the error message comes
from is very frustrating. :)

I understand adapting NSS to run inside the sandbox is tough, but the
intricacies and existing lore were not clear to me from the outset.  (No DB init
(70ms performance regression until I tracked this down), load the shared
libraries before Sandbox init or else they will be lazily loaded later (a
regression that was not caught by the bots because they had no sandbox), the
ChromeOS bug I just detailed, etc.)

So I'll repeat my refain: all I wanted was HMAC/SHA256! :)

[wtc, 2012-06-14 21:28:48]

Review comments on patch set 8:

I didn't read the changes to the GYP files.  This seems
correct.  I have just some suggested changes to comments.

http://codereview.chromium.org/10543146/diff/11006/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/10543146/diff/11006/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_process_observer.cc:189: // On Linux, NSS uses
system libraries, so the .so's must be loaded.
Nit: this should read:
  On Linux, we use system NSS libraries, so ...

It may be better to just say "On the platforms where we use
system NSS libraries, the .so's must be loaded".  The
platforms are more than just Linux:
    defined(OS_POSIX) && !defined(OS_MACOSX)

http://codereview.chromium.org/10543146/diff/11006/crypto/encryptor_unittest.cc
File crypto/encryptor_unittest.cc (right):

http://codereview.chromium.org/10543146/diff/11006/crypto/encryptor_unittest....
crypto/encryptor_unittest.cc:94: // (crbug.com/124434) and Mac OS X 10.7
(crbug.com/127586).

Delete " and Mac OS X 10.7 (crbug.com/127586)" from this
comment.

http://codereview.chromium.org/10543146/diff/11006/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/10543146/diff/11006/crypto/symmetric_key.h#new...
crypto/symmetric_key.h:14: #if defined(NACL_WIN64)

Please add a comment to explain NACL_WIN64, and point out
it must be tested before OS_WIN.

[ddorwin, 2012-06-14 22:01:55]

http://codereview.chromium.org/10543146/diff/11006/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/10543146/diff/11006/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_process_observer.cc:189: // On Linux, NSS uses
system libraries, so the .so's must be loaded.
On 2012/06/14 21:28:48, wtc wrote:
> Nit: this should read:
>   On Linux, we use system NSS libraries, so ...
> 
> It may be better to just say "On the platforms where we use
> system NSS libraries, the .so's must be loaded".  The
> platforms are more than just Linux:
>     defined(OS_POSIX) && !defined(OS_MACOSX)

Done.

http://codereview.chromium.org/10543146/diff/11006/crypto/encryptor_unittest.cc
File crypto/encryptor_unittest.cc (right):

http://codereview.chromium.org/10543146/diff/11006/crypto/encryptor_unittest....
crypto/encryptor_unittest.cc:94: // (crbug.com/124434) and Mac OS X 10.7
(crbug.com/127586).
On 2012/06/14 21:28:48, wtc wrote:
> 
> Delete " and Mac OS X 10.7 (crbug.com/127586)" from this
> comment.

Done.

http://codereview.chromium.org/10543146/diff/11006/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/10543146/diff/11006/crypto/symmetric_key.h#new...
crypto/symmetric_key.h:14: #if defined(NACL_WIN64)
On 2012/06/14 21:28:48, wtc wrote:
> 
> Please add a comment to explain NACL_WIN64, and point out
> it must be tested before OS_WIN.

Done.
I explained it in crypto.gyp (I hope I got that right) and referred to that
here. It seems crypto.gyp is the right place to explain this since there are so
many files affected.

[wtc, 2012-06-14 23:01:19]

Patch set 9 LGTM.  I didn't review crypto.gyp.

[Ryan Sleevi, 2012-06-14 23:04:39]

Note: Please hold off on committing until Brad or Nick can further review.

I'm not sure how the changes to the 'crypto' target will affect NaCl.
'crypto_nacl_win64' is clear, but it's less clear on 'crypto', so I want to make
sure it's tested appropriately for NaCl before landing.

[bradn, 2012-06-15 00:10:27]

At a glance your just bringing in more injected stuff from nss, since it looks
like mostly include_dirs, seems unlikely to affect the corner of the library
nick is using.
LGTM

[Ryan Sleevi, 2012-06-15 00:13:40]

On 2012/06/15 00:10:27, bradn wrote:
> At a glance your just bringing in more injected stuff from nss, since it looks
> like mostly include_dirs, seems unlikely to affect the corner of the library
> nick is using.
> LGTM

Just to confirm you're on board:

For Windows binaries (and Mac and Linux, but the interesting point is Windows),
depending on crypto/ explicitly means NSS will get linked in and used.

Previously any dependency on NSS by crypto/ was able to be optimized out,
because the code (eg: ECC signatures) was never used.

If you have standalone binaries (eg: nacl_helper), they will now include "most"
of NSS, EXCEPT for the Win64 build.

Is the nacl win64 binary the only standalone nacl binary?

[bradn, 2012-06-15 00:31:42]

Sounds good.


On Thu, Jun 14, 2012 at 5:13 PM, <rsleevi@chromium.org> wrote:

> On 2012/06/15 00:10:27, bradn wrote:
>
>> At a glance your just bringing in more injected stuff from nss, since it
>> looks
>> like mostly include_dirs, seems unlikely to affect the corner of the
>> library
>> nick is using.
>> LGTM
>>
>
> Just to confirm you're on board:
>
> For Windows binaries (and Mac and Linux, but the interesting point is
> Windows),
> depending on crypto/ explicitly means NSS will get linked in and used.
>
> Previously any dependency on NSS by crypto/ was able to be optimized out,
> because the code (eg: ECC signatures) was never used.
>
> If you have standalone binaries (eg: nacl_helper), they will now include
> "most"
> of NSS, EXCEPT for the Win64 build.
>
> Is the nacl win64 binary the only standalone nacl binary?
>
>
http://codereview.chromium.**org/10543146/<http://codereview.chromium.org/105...
>

[Nick Bray, 2012-06-15 00:31:59]

nacl64.exe (Win64) and nacl_helper (Linux) are the two standalone NaCl binaries.
 On Win32 and Mac, NaCl is part of the Chrome shared library.

I was scratching my head to figure out how else this might cause NaCl problems
and I just thought of a way.

See: src/chrome/nacl/nacl_validation_query.cc line 45.  On win32/mac we'll use
the crypto library in Chrome, but in a new process.  This change will result in
us using NSS in these cases?  Should the preprocessor define therefore be
changed, otherwise we'll get a performance regression?

[wtc, 2012-06-15 00:32:16]

http://codereview.chromium.org/10543146/diff/1011/DEPS
File DEPS (right):

http://codereview.chromium.org/10543146/diff/1011/DEPS#newcode65
DEPS:65: "nss_revision": "142187",

ddorwin:  If I commit my CL http://codereview.chromium.org/10548059/
first, you will get a merge conflict here.  If that happens,
please exclude this file from your CL.

[Ryan Sleevi, 2012-06-15 00:37:37]

On 2012/06/15 00:31:59, Nick Bray wrote:
> nacl64.exe (Win64) and nacl_helper (Linux) are the two standalone NaCl
binaries.
>  On Win32 and Mac, NaCl is part of the Chrome shared library.
> 
> I was scratching my head to figure out how else this might cause NaCl problems
> and I just thought of a way.
> 
> See: src/chrome/nacl/nacl_validation_query.cc line 45.  On win32/mac we'll use
> the crypto library in Chrome, but in a new process.  This change will result
in
> us using NSS in these cases?  Should the preprocessor define therefore be
> changed, otherwise we'll get a performance regression?

Right. This is exactly the place of concern and why I wanted to make sure you
guys were looped in.

Could you clarify what you mean 'in a new process'? Is this 'in a standalone
exe' (eg: nacl64.exe), or do you just mean in a new process type (eg: not a GPU,
renderer, or other zygote-derived process type?)

The previous implementation used system APIs on OS X, and either system APIs or
(effectively) a fork of NSS.

The new implementation will use the statically compiled version of NSS
(Win/Mac). If this is used in a standalone process (not part of the Chrome
shared library), then this will likely cause a size regression. If this code is
part of the Chrome shared library, then the net result is that you'll be
indirecting through several more function calls to get to the HMAC
implementation.

[Ryan Sleevi, 2012-06-15 01:15:11]

Met with Nick, talked about all the dependencies, sounds like we're fine.

LGTM.

[ddorwin, 2012-06-15 04:35:25]

Thanks everyone!

http://codereview.chromium.org/10543146/diff/1011/DEPS
File DEPS (right):

http://codereview.chromium.org/10543146/diff/1011/DEPS#newcode65
DEPS:65: "nss_revision": "142187",
On 2012/06/15 00:32:16, wtc wrote:
> 
> ddorwin:  If I commit my CL http://codereview.chromium.org/10548059/
> first, you will get a merge conflict here.  If that happens,
> please exclude this file from your CL.

Done.

[commit-bot: I haz the power, 2012-06-15 05:39:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/10543146/3026

[commit-bot: I haz the power, 2012-06-15 08:11:53]

Change committed as 142356