Adding macro to enable changing SSL library (Part 1)

Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=591545
Committed: https://crrev.com/a1714ab3703201b3a3c424fa42788f0e4d060b26
Cr-Commit-Position: refs/heads/master@{#382077}

[svaldez, 2016-03-17 17:17:59]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2016-03-17 19:48:09]

https://codereview.chromium.org/1808963004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1808963004/diff/1/build/common.gypi#newcode743
build/common.gypi:743: # NSS verifier usage.
Nit: Maybe add "On non-OpenSSL iOS configurations, certificates use the
operating system library, but the verifier uses the bundled copy of NSS."

https://codereview.chromium.org/1808963004/diff/1/build/config/crypto.gni
File build/config/crypto.gni (right):

https://codereview.chromium.org/1808963004/diff/1/build/config/crypto.gni#new...
build/config/crypto.gni:32: # True if NSS is used for certificate verification.
Nit: Maybe add "On non-OpenSSL iOS configurations, certificates use the
operating system library, but verification uses the bundled copy of NSS."

https://codereview.chromium.org/1808963004/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/1808963004/diff/1/crypto/nss_util.cc#newcode718
crypto/nss_util.cc:718: #if defined(OS_IOS) && !defined(USE_OPENSSL)
Shouldn't OS_IOS && USE_OPENSSL be impossible in this file?

https://codereview.chromium.org/1808963004/diff/1/crypto/nss_util.cc#newcode806
crypto/nss_util.cc:806: #if defined(USE_NSS_VERIFIER)
Are there any cases where this ifdef is false and we build this? (I probably
should have just removed this earlier.)

https://codereview.chromium.org/1808963004/diff/1/ios/crnet/crnet_environment.mm
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1808963004/diff/1/ios/crnet/crnet_environment...
ios/crnet/crnet_environment.mm:157: #if defined(USE_NSS_VERIFIER)
(I suppose, strictly speaking, this line is also used for the SSL stack and
crypto bits. If we somehow had !USE_OPENSSL and !USE_NSS_VERIFIER, we'd want
this here too. But meh. I don't think it's really worth fussing about.)

https://codereview.chromium.org/1808963004/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1808963004/diff/1/net/BUILD.gn#newcode71
net/BUILD.gn:71: }
This is already in build/config/BUILD.gn, right?

https://codereview.chromium.org/1808963004/diff/1/net/BUILD.gn#newcode293
net/BUILD.gn:293: if (!use_nss_verifier) {
Nit: Should this be outside of the !use_nss_certs block? I think it's
equivalent, but the other use_nss_verifier checks are treated as independent.

https://codereview.chromium.org/1808963004/diff/1/net/cert/test_root_certs.h
File net/cert/test_root_certs.h (right):

https://codereview.chromium.org/1808963004/diff/1/net/cert/test_root_certs.h#...
net/cert/test_root_certs.h:70: #elif defined(OS_MACOSX) &&
!defined(USE_NSS_VERIFIER)
I'm assuming we'll be using this code in the new iOS configuration.

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp#newcode244
net/net.gyp:244: # Only include this test when using system NSS for cert
verification.
system NSS -> NSS

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp#newcode249
net/net.gyp:249: [ 'use_nss_verifier == 0 and OS == "ios"', {
== 1?

https://codereview.chromium.org/1808963004/diff/1/net/net_common.gypi
File net/net_common.gypi (right):

https://codereview.chromium.org/1808963004/diff/1/net/net_common.gypi#newcode120
net/net_common.gypi:120: 'cert/x509_certificate_ios.cc',
I'm assuming you're planning on adding a separate file here?

https://codereview.chromium.org/1808963004/diff/1/net/net_common.gypi#newcode199
net/net_common.gypi:199: [ 'use_nss_certs == 1 or use_nss_verifier == 1', {
Other files have just been doing use_nss_verifier and assuming use_nss_verifier
=> use_nss_certs.

https://codereview.chromium.org/1808963004/diff/1/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/1808963004/diff/1/tools/grit/grit_rule.gni#ne...
tools/grit/grit_rule.gni:167: }
Are we expecting to use this in grd files?

[svaldez, 2016-03-17 20:04:19]

https://codereview.chromium.org/1808963004/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1808963004/diff/1/build/common.gypi#newcode743
build/common.gypi:743: # NSS verifier usage.
On 2016/03/17 19:48:08, davidben wrote:
> Nit: Maybe add "On non-OpenSSL iOS configurations, certificates use the
> operating system library, but the verifier uses the bundled copy of NSS."

Done.

https://codereview.chromium.org/1808963004/diff/1/build/config/crypto.gni
File build/config/crypto.gni (right):

https://codereview.chromium.org/1808963004/diff/1/build/config/crypto.gni#new...
build/config/crypto.gni:32: # True if NSS is used for certificate verification.
On 2016/03/17 19:48:08, davidben wrote:
> Nit: Maybe add "On non-OpenSSL iOS configurations, certificates use the
> operating system library, but verification uses the bundled copy of NSS."

Done.

https://codereview.chromium.org/1808963004/diff/1/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/1808963004/diff/1/crypto/nss_util.cc#newcode718
crypto/nss_util.cc:718: #if defined(OS_IOS) && !defined(USE_OPENSSL)
On 2016/03/17 19:48:08, davidben wrote:
> Shouldn't OS_IOS && USE_OPENSSL be impossible in this file?

Done.

https://codereview.chromium.org/1808963004/diff/1/crypto/nss_util.cc#newcode806
crypto/nss_util.cc:806: #if defined(USE_NSS_VERIFIER)
On 2016/03/17 19:48:08, davidben wrote:
> Are there any cases where this ifdef is false and we build this? (I probably
> should have just removed this earlier.)
Done.

https://codereview.chromium.org/1808963004/diff/1/ios/crnet/crnet_environment.mm
File ios/crnet/crnet_environment.mm (right):

https://codereview.chromium.org/1808963004/diff/1/ios/crnet/crnet_environment...
ios/crnet/crnet_environment.mm:157: #if defined(USE_NSS_VERIFIER)
On 2016/03/17 19:48:08, davidben wrote:
> (I suppose, strictly speaking, this line is also used for the SSL stack and
> crypto bits. If we somehow had !USE_OPENSSL and !USE_NSS_VERIFIER, we'd want
> this here too. But meh. I don't think it's really worth fussing about.)

Acknowledged.

https://codereview.chromium.org/1808963004/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1808963004/diff/1/net/BUILD.gn#newcode71
net/BUILD.gn:71: }
On 2016/03/17 19:48:08, davidben wrote:
> This is already in build/config/BUILD.gn, right?

Done.

https://codereview.chromium.org/1808963004/diff/1/net/BUILD.gn#newcode293
net/BUILD.gn:293: if (!use_nss_verifier) {
On 2016/03/17 19:48:08, davidben wrote:
> Nit: Should this be outside of the !use_nss_certs block? I think it's
> equivalent, but the other use_nss_verifier checks are treated as independent.

Done.

https://codereview.chromium.org/1808963004/diff/1/net/cert/test_root_certs.h
File net/cert/test_root_certs.h (right):

https://codereview.chromium.org/1808963004/diff/1/net/cert/test_root_certs.h#...
net/cert/test_root_certs.h:70: #elif defined(OS_MACOSX) &&
!defined(USE_NSS_VERIFIER)
On 2016/03/17 19:48:08, davidben wrote:
> I'm assuming we'll be using this code in the new iOS configuration.

Acknowledged.

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp#newcode244
net/net.gyp:244: # Only include this test when using system NSS for cert
verification.
On 2016/03/17 19:48:08, davidben wrote:
> system NSS -> NSS

Done.

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp#newcode249
net/net.gyp:249: [ 'use_nss_verifier == 0 and OS == "ios"', {
On 2016/03/17 19:48:08, davidben wrote:
> == 1?

Nope, since this whole thing is subtractive.

https://codereview.chromium.org/1808963004/diff/1/net/net_common.gypi
File net/net_common.gypi (right):

https://codereview.chromium.org/1808963004/diff/1/net/net_common.gypi#newcode120
net/net_common.gypi:120: 'cert/x509_certificate_ios.cc',
On 2016/03/17 19:48:09, davidben wrote:
> I'm assuming you're planning on adding a separate file here?

Acknowledged.

https://codereview.chromium.org/1808963004/diff/1/net/net_common.gypi#newcode199
net/net_common.gypi:199: [ 'use_nss_certs == 1 or use_nss_verifier == 1', {
On 2016/03/17 19:48:09, davidben wrote:
> Other files have just been doing use_nss_verifier and assuming
use_nss_verifier
> => use_nss_certs.

Done.

https://codereview.chromium.org/1808963004/diff/1/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/1808963004/diff/1/tools/grit/grit_rule.gni#ne...
tools/grit/grit_rule.gni:167: }
On 2016/03/17 19:48:09, davidben wrote:
> Are we expecting to use this in grd files?

Mostly added for sake of completion.

[davidben, 2016-03-17 20:19:45]

lgtm. Could you stable this to the appropriate bug?

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1808963004/diff/1/net/net.gyp#newcode249
net/net.gyp:249: [ 'use_nss_verifier == 0 and OS == "ios"', {
On 2016/03/17 20:04:18, svaldez wrote:
> On 2016/03/17 19:48:08, davidben wrote:
> > == 1?
> 
> Nope, since this whole thing is subtractive.

Acknowledged.

https://codereview.chromium.org/1808963004/diff/1/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/1808963004/diff/1/tools/grit/grit_rule.gni#ne...
tools/grit/grit_rule.gni:167: }
On 2016/03/17 20:04:18, svaldez wrote:
> On 2016/03/17 19:48:09, davidben wrote:
> > Are we expecting to use this in grd files?
> 
> Mostly added for sake of completion.

Fair enough. I don't have a good sense of whether we add things here as-needed
or by default.

https://codereview.chromium.org/1808963004/diff/40001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1808963004/diff/40001/net/BUILD.gn#newcode185
net/BUILD.gn:185: net_shared_sources -= [ "cert/x509_util_ios.cc" ]
Nit: This corresponds to line 310 on the GRD file, right? Probably also want to
remove the header file, not that it matters.

[svaldez, 2016-03-17 21:30:04]

Description was changed from

==========
Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=
==========

to

==========
Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=591545
==========

[svaldez, 2016-03-18 13:42:32]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-03-18 13:42:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1808963004/#ps60001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-03-18 13:42:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808963004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808963004/60001

[commit-bot: I haz the power, 2016-03-18 13:51:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-18 13:51:28]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[svaldez, 2016-03-18 14:02:43]

svaldez@chromium.org changed reviewers:
+ eugenebut@chromium.org, sergeyu@chromium.org, thakis@chromium.org

[svaldez, 2016-03-18 14:02:44]

Hi,

This is the first CL in a set to allow us to change the SSL library used in iOS.
Could you please take a look at and review the following files:

thakis:
 build/
 tools/

eugenebut:
 ios/

sergeyu:
 remoting/

Thanks,
Steven

[Nico, 2016-03-18 14:06:20]

Exciting! Generally lgtm, but one comment below.

Once this is in, I guess we'll be able to remove all these SSL lib build config
variables?

https://codereview.chromium.org/1808963004/diff/60001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1808963004/diff/60001/build/config/BUILD.gn#n...
build/config/BUILD.gn:133: }
is it possible to use the system described in "[chromium-dev] New system for
feature enable/disable flags" here instead?

[svaldez, 2016-03-18 14:57:33]

Yeah, eventually we'll just drop most of the NSS code.

https://codereview.chromium.org/1808963004/diff/60001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1808963004/diff/60001/build/config/BUILD.gn#n...
build/config/BUILD.gn:133: }
On 2016/03/18 14:06:20, Nico wrote:
> is it possible to use the system described in "[chromium-dev] New system for
> feature enable/disable flags" here instead?

Done.

[Eugene But (OOO till 7-30), 2016-03-18 15:15:44]

lgtm

[svaldez, 2016-03-18 15:30:51]

On 2016/03/18 14:57:33, svaldez wrote:
> Yeah, eventually we'll just drop most of the NSS code.
> 
> https://codereview.chromium.org/1808963004/diff/60001/build/config/BUILD.gn
> File build/config/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1808963004/diff/60001/build/config/BUILD.gn#n...
> build/config/BUILD.gn:133: }
> On 2016/03/18 14:06:20, Nico wrote:
> > is it possible to use the system described in "[chromium-dev] New system for
> > feature enable/disable flags" here instead?
> 
> Done.

Actually I might need to use the old format since we want the build flag to be
dependent on whether the target is build as nacl_untrusted_build.

[Nico, 2016-03-18 18:37:26]

ok, old approach lgtm too then

[svaldez, 2016-03-18 19:23:01]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-03-18 19:23:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, eugenebut@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1808963004/#ps140001
(title: "Remove remoting_nacl change.")

[commit-bot: I haz the power, 2016-03-18 19:23:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808963004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808963004/140001

[commit-bot: I haz the power, 2016-03-18 20:48:01]

Description was changed from

==========
Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=591545
==========

to

==========
Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=591545
==========

[commit-bot: I haz the power, 2016-03-18 20:48:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-18 20:50:35]

Description was changed from

==========
Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=591545
==========

to

==========
Adding macro to enable changing SSL library (Part 1)

This changes the macro used to be USE_NSS_VERIFIER and modifies build
files to no longer assume iOS is using NSS.

BUG=591545

Committed: https://crrev.com/a1714ab3703201b3a3c424fa42788f0e4d060b26
Cr-Commit-Position: refs/heads/master@{#382077}
==========

[commit-bot: I haz the power, 2016-03-18 20:50:37]

Patchset 8 (id:??) landed as
https://crrev.com/a1714ab3703201b3a3c424fa42788f0e4d060b26
Cr-Commit-Position: refs/heads/master@{#382077}