Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
206453002:
Introduce USE_OPENSSL_CERTS for certificate handling. (Closed)
|
Created:
6 years, 9 months ago by haavardm Modified:
6 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionIntroduce USE_OPENSSL_CERTS for certificate handling.
See discussion at chromium issue 338885.
When USE_OPENSSL_CERTS is defined, X509::OSCertHandle is now
typedef'ed to struct X509*.
When USE_OPENSSL is defined, USE_OPENSSL_CERTS will now be
defined for linux and Android, while being off for Mac and
Windows. This allows OpenSSL to be used while leaving
certificate handling to the OS.
OpenSSL cert verifying code will only be used on Linux.
This patch does not change any default behavior.
Bug=none
Test=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260152
Patch Set 1 #
Total comments: 40
Patch Set 2 : Changed meaning of USE_OPENSSL_CERTS #
Total comments: 19
Patch Set 3 : Address wtc's comments. #Patch Set 4 : Put back nss and nspr dependencies for crypto_unittests. #
Total comments: 9
Patch Set 5 : Final fixes and nits #
Messages
Total messages: 30 (0 generated)
This patch introduces USE_OPENSSL_CERTS as discussed in https://code.google.com/p/chromium/issues/detail?id=338885 . With a small patch in src/third_party/openssl (not yet submittet), this CL makes it possible to compile crypto_unittests and net_unittests with OpenSSL on OS X. I have tested the builds locally on mac/linux with and without USE_OPENSSL cert, and I've also tested the default setting for Android. The patch does not touch any gyp settings for Windows. When "ifdef logic" affecting windows has been touched, it follows Os X . I'll asked for try bot access to test compile on windows though.
Review comments on patch set 1: This seems correct, but I'd like to take another look because it is not easy to grasp what exactly USE_OPENSSL_CERTS means. High-level comments: 1. Some of the info in the CL's description should be moved to or duplicated in a source file. A good place is build/common.gypi. 2. In the CL's description, please add R=, BUG=, TEST=. It is OK to say BUG=none or TEST=none. https://codereview.chromium.org/206453002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode50 build/common.gypi:50: # Use OpenSSL instead of NSS. Under development: see http://crbug.com/62803 Please update this comment. At least, the "Under development: see http://crbug.com/ 62803" part should be removed. It would be good to describe the new, narrower meaning of use_openssl and the relation to use_openssl_certs. https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode542 build/common.gypi:542: ['(OS=="linux" or OS=="freebsd" or OS=="openbsd" or OS=="solaris") and use_openssl==0', { Nit: ideally this should be combined with the new code you added below, so that we don't repeat the (OS=="linux" or OS=="freebsd" or OS=="openbsd" or OS=="solaris") test. https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode548 build/common.gypi:548: # When OpenSSL is used for SSL on unix like systems, turn on OpenSSL This should be "for SSL and crypto". https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode551 build/common.gypi:551: 'use_openssl_certs%': 1, Should we also add an "else" block that set use_openssl_certs to 0? All the neighboring code does that, so it would be good to be consistent even if it's not necessary. https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp#newcode43 crypto/crypto.gyp:43: [ 'use_openssl == 1', { 1. The indentation level should be two spaces rather than four spaces. (I know the indentation level is not consistent in this file.) 2. It seems that we should be able to specify the dependency on openssl at a higher-level in one place, rather than in two places (here and line 64-66)). https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp#newcode223 crypto/crypto.gyp:223: [ 'use_openssl == 0 and OS == "mac"', { I think these two blocks can be simply deleted. We should get the dependency on nss and nspr from the export_dependent_settings of the "crypto" target. If not, I think at least this block can be deleted. https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h File crypto/encryptor.h (right): https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h#newcode17 crypto/encryptor.h:17: (defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)) This should be written like this: #if defined(USE_NSS) || \ (!defined(USE_OPENSSL) && (defined(OS_WIN) || defined(OS_MACOSX))) Please make the same change to similar ifdefs in other files. https://codereview.chromium.org/206453002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl_unittest.cc (right): https://codereview.chromium.org/206453002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl_unittest.cc:64: #if defined(USE_OPENSSL_CERTS) || defined(OS_ANDROID) Why do you need to comment out this code? https://codereview.chromium.org/206453002/diff/1/net/url_request/url_request_... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/206453002/diff/1/net/url_request/url_request_... net/url_request/url_request_unittest.cc:6947: #if defined(USE_OPENSSL_CERTS) || defined(OS_ANDROID) I think this function should return false on Android, but not for the reason that "OpenSSL does not support EV validation". So we probably should add || defined(OS_ANDROID) to the OS X case below and update the comment on line 6951.
drive by comments https://codereview.chromium.org/206453002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/206453002/diff/1/DEPS#newcode452 DEPS:452: "/trunk/deps/third_party/openssl@" + Var("openssl_revision"), Is this necessary? It's already part of the top level deps dict. https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp#newcode43 crypto/crypto.gyp:43: [ 'use_openssl == 1', { On 2014/03/20 21:14:56, wtc wrote: > > 1. The indentation level should be two spaces rather than four spaces. (I know > the indentation level is not consistent in this file.) > > 2. It seems that we should be able to specify the dependency on openssl at a > higher-level in one place, rather than in two places (here and line 64-66)). I agree (see https://codereview.chromium.org/205813004) https://codereview.chromium.org/206453002/diff/1/net/ssl/server_bound_cert_se... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/206453002/diff/1/net/ssl/server_bound_cert_se... net/ssl/server_bound_cert_service_unittest.cc:154: #if !defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID) It looks like crbug.com/91512 was fixed. Can these ifdefs just be removed?
Thanks for a lot of good comments. The USE_OPENSSL_CERTS define means that the certificate implementation and the certificate verifying is done using OpenSSL APIs. Android is a mix where OpenSSL is used to implement the certificate, and Android is used to very the certs. Thus USE_OPENSSL_CERTS is not defined for Android, and the needed OpenSSL code is triggered by OS_ANDROID instead. When USE_OPENSSL_CERTS is off, the certificate and verifying code is implemented using the OS libraries. After this CL, USE_OPENSSL only turns on the OpenSSL implementation of the SSL. The cert verifying is handled by the OS until USE_OPENSSL_CERTS is defined. On linux, for now, USE_OPENSSL_CERTS is turned on along with USE_OPENSSL. https://codereview.chromium.org/206453002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/206453002/diff/1/DEPS#newcode452 DEPS:452: "/trunk/deps/third_party/openssl@" + Var("openssl_revision"), On 2014/03/20 23:40:24, mattm wrote: > Is this necessary? It's already part of the top level deps dict. I can try to remove it from a clean checkout. However, if this is not needed, why is the NSS dependency just above needed? https://codereview.chromium.org/206453002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode542 build/common.gypi:542: ['(OS=="linux" or OS=="freebsd" or OS=="openbsd" or OS=="solaris") and use_openssl==0', { On 2014/03/20 21:14:56, wtc wrote: > > Nit: ideally this should be combined with the new code you added below, so that > we don't repeat the (OS=="linux" or OS=="freebsd" or OS=="openbsd" or > OS=="solaris") test. I had that at first, but the result got quite nested and was hard to read, partly in due to how the "if" is expressed using "'conditions': []". It was actually more clear having it in two separate "ifs". In general I think the ssl logic in common.gyp as a whole could be simplified. Much of the logic could probably be combined into fewer blocks. The logic has probably evolved bit by bit, and I'm certainly contributing to that now. I'm just afraid that if I simplify and rewrite the logic, I'll miss something and cause compilation trouble on some unknown profile. https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp#newcode223 crypto/crypto.gyp:223: [ 'use_openssl == 0 and OS == "mac"', { Thanks, I'll test. On 2014/03/20 21:14:56, wtc wrote: > > I think these two blocks can be simply deleted. We should get the dependency on > nss and nspr from the export_dependent_settings of the "crypto" target. > > If not, I think at least this block can be deleted. https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h File crypto/encryptor.h (right): https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h#newcode17 crypto/encryptor.h:17: (defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)) On 2014/03/20 21:14:56, wtc wrote: > > This should be written like this: > > #if defined(USE_NSS) || \ > (!defined(USE_OPENSSL) && (defined(OS_WIN) || defined(OS_MACOSX))) > > Please make the same change to similar ifdefs in other files. That's what I had first actually, but I found it a bit complex. I think they logically are the same though, as long as USE_NSS and USE_OPENSSL are not both defined. I'll put that back your version if that is what you prefer. https://codereview.chromium.org/206453002/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl_unittest.cc (right): https://codereview.chromium.org/206453002/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl_unittest.cc:64: #if defined(USE_OPENSSL_CERTS) || defined(OS_ANDROID) On 2014/03/20 21:14:56, wtc wrote: > > Why do you need to comment out this code? Thanks, I think I got this wrong. It should probably !defined(USE_OPENSSL_CERTS) here instead. On OS X/Windows the OpenSSL versions of private keys and cert store would not be implemented. The reason I kept the compilation of this file in the net.gyp is that we might want to add non cert-store tests later. The name of the file is ssl_client_socket_openssl_unittest.cc after all. https://codereview.chromium.org/206453002/diff/1/net/ssl/server_bound_cert_se... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/206453002/diff/1/net/ssl/server_bound_cert_se... net/ssl/server_bound_cert_service_unittest.cc:154: #if !defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID) On 2014/03/20 23:40:24, mattm wrote: > It looks like crbug.com/91512 was fixed. Can these ifdefs just be removed? Thanks, I wasn't aware of that. I'll try to remove. https://codereview.chromium.org/206453002/diff/1/net/url_request/url_request_... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/206453002/diff/1/net/url_request/url_request_... net/url_request/url_request_unittest.cc:6947: #if defined(USE_OPENSSL_CERTS) || defined(OS_ANDROID) On 2014/03/20 21:14:56, wtc wrote: > > I think this function should return false on Android, but not for the reason > that "OpenSSL does not support EV validation". So we probably should add || > defined(OS_ANDROID) to the OS X case below and update the comment on line 6951. Thanks, I was quite unsure about this one. I'll do some more testing.
https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp#newcode43 crypto/crypto.gyp:43: [ 'use_openssl == 1', { On 2014/03/20 23:40:24, mattm wrote: > On 2014/03/20 21:14:56, wtc wrote: > > > > 1. The indentation level should be two spaces rather than four spaces. (I know > > the indentation level is not consistent in this file.) > > > > 2. It seems that we should be able to specify the dependency on openssl at a > > higher-level in one place, rather than in two places (here and line 64-66)). > > I agree (see https://codereview.chromium.org/205813004) Right. I'll wait until your patch is committed, and update my cl accordingly.
https://codereview.chromium.org/206453002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/206453002/diff/1/DEPS#newcode452 DEPS:452: "/trunk/deps/third_party/openssl@" + Var("openssl_revision"), On 2014/03/21 13:08:21, Håvard Molland wrote: > On 2014/03/20 23:40:24, mattm wrote: > > Is this necessary? It's already part of the top level deps dict. > I can try to remove it from a clean checkout. However, if this is not needed, > why is the NSS dependency just above needed? NSS is not in the top level deps, since linux uses the system NSS libs (except for SSL, which is in net/third_party/nss)
Having read through this patch a few times, I'm still a bit confused about USE_OPENSSL_CERTS and had to keep cross-checking. Apologies if these comments display my ignorance here. I think a big part of the confusion is that I think mentally, I keep expecting USE_OPENSSL_CERTS to cover how X509Certificate::OSCertHandle is typedef'd. That is, an X509* when USE_OPENSSL_CERTS is defined, and an (OS native handle) when it's not. USE_OPENSSL matches USE_NSS - that is, it governs the SSL implementation, but can't make assumptions about X509Certificate The problem in mentally mapping the above definitions is that Android doesn't have an (OS native handle) [unless we're talking JNI objects - eww], so it also uses X509*. It also doesn't provide a solid definition of OS verification vs OpenSSL verification. I'll try to review this again in a few hours - and in particular, to see if I can come up any comment additions that would help me keep things clearer mentally - but I did want to provide initial feedback. https://codereview.chromium.org/206453002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/206453002/diff/1/DEPS#newcode452 DEPS:452: "/trunk/deps/third_party/openssl@" + Var("openssl_revision"), On 2014/03/21 13:08:21, Håvard Molland wrote: > On 2014/03/20 23:40:24, mattm wrote: > > Is this necessary? It's already part of the top level deps dict. > I can try to remove it from a clean checkout. However, if this is not needed, > why is the NSS dependency just above needed? Because on Win/Mac/iOS, we build all of NSS & NSPR, and thus have two checkouts: third_party/nss (for all of NSPR + NSS - libssl) net/third_party/nss (for all of libssl) On Linux, we only have one checkout net/third_party/nss (for all of libssl) +1 to removing this as unneeded https://codereview.chromium.org/206453002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode53 build/common.gypi:53: # Use OpenSSL for certificate handling. I'm a little confused here, when reading both this code and the later code for CertVerifier. When use_openssl_certs:1, X509Certificate::OSCertHandle will always be an X509*, right? When use_openssl_certs:1, certificate verification MAY happen with OpenSSL - or it may not (eg: Android), right? Perhaps there's a better way to express that here than just saying "certificate handling". https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode542 build/common.gypi:542: ['(OS=="linux" or OS=="freebsd" or OS=="openbsd" or OS=="solaris") and use_openssl==0', { On 2014/03/21 13:08:21, Håvard Molland wrote: > On 2014/03/20 21:14:56, wtc wrote: > > > > Nit: ideally this should be combined with the new code you added below, so > that > > we don't repeat the (OS=="linux" or OS=="freebsd" or OS=="openbsd" or > > OS=="solaris") test. > > I had that at first, but the result got quite nested and was hard to read, > partly in due to how the "if" is expressed using "'conditions': []". It was > actually more clear having it in two separate "ifs". > > In general I think the ssl logic in common.gyp as a whole could be simplified. > Much of the logic could probably be combined into fewer blocks. The logic has > probably evolved bit by bit, and I'm certainly contributing to that now. I'm > just afraid that if I simplify and rewrite the logic, I'll miss something and > cause compilation trouble on some unknown profile. Yeah, as we transition towards OpenSSL, we can simplify a lot. Part of the issue is caused by the "system NSS vs bundled NSS" hassles. https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode551 build/common.gypi:551: 'use_openssl_certs%': 1, On 2014/03/20 21:14:56, wtc wrote: > > Should we also add an "else" block that set use_openssl_certs to 0? All the > neighboring code does that, so it would be good to be consistent even if it's > not necessary. +1 for consistency. https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h File crypto/encryptor.h (right): https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h#newcode17 crypto/encryptor.h:17: (defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)) On 2014/03/21 13:08:21, Håvard Molland wrote: > On 2014/03/20 21:14:56, wtc wrote: > > > > This should be written like this: > > > > #if defined(USE_NSS) || \ > > (!defined(USE_OPENSSL) && (defined(OS_WIN) || defined(OS_MACOSX))) > > > > Please make the same change to similar ifdefs in other files. > That's what I had first actually, but I found it a bit complex. I think they > logically are the same though, as long as USE_NSS and USE_OPENSSL are not both > defined. > > I'll put that back your version if that is what you prefer. I think I agree with Wan-Teh, in that I found myself having to consult his version more than yours when trying to understand under what situations this would be used. With the && (as written), I always have to keep that mental state in place, whereas with the || (as wan-teh wrote), I can do fewer backtracking about the conditions. https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc... net/cert/cert_verify_proc.cc:23: #elif defined(USE_OPENSSL_CERTS) Shouldn't you still have a !defined(OS_ANDROID) here? https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc... net/cert/cert_verify_proc.cc:170: #elif defined(USE_OPENSSL_CERTS) ditto https://codereview.chromium.org/206453002/diff/1/net/cert/test_root_certs.h File net/cert/test_root_certs.h (right): https://codereview.chromium.org/206453002/diff/1/net/cert/test_root_certs.h#n... net/cert/test_root_certs.h:15: #elif defined(USE_OPENSSL_CERTS) I don't understand why no more OS_ANDROID check?
On 2014/03/21 19:47:50, Ryan Sleevi wrote: > Having read through this patch a few times, I'm still a bit confused about > USE_OPENSSL_CERTS and had to keep cross-checking. Apologies if these comments > display my ignorance here. > > I think a big part of the confusion is that I think mentally, I keep expecting > USE_OPENSSL_CERTS to cover how X509Certificate::OSCertHandle is typedef'd. That > is, an X509* when USE_OPENSSL_CERTS is defined, and an (OS native handle) when > it's not. > > USE_OPENSSL matches USE_NSS - that is, it governs the SSL implementation, but > can't make assumptions about X509Certificate Isn't this backwards? Doesn't USE_NSS currently means use nss for everything, !USE_NSS means just use NSS for SSL (unless USE_OPENSSL).
On 2014/03/21 22:37:40, mattm wrote: > On 2014/03/21 19:47:50, Ryan Sleevi wrote: > > Having read through this patch a few times, I'm still a bit confused about > > USE_OPENSSL_CERTS and had to keep cross-checking. Apologies if these comments > > display my ignorance here. > > > > I think a big part of the confusion is that I think mentally, I keep expecting > > USE_OPENSSL_CERTS to cover how X509Certificate::OSCertHandle is typedef'd. > That > > is, an X509* when USE_OPENSSL_CERTS is defined, and an (OS native handle) when > > it's not. > > > > USE_OPENSSL matches USE_NSS - that is, it governs the SSL implementation, but > > can't make assumptions about X509Certificate > > Isn't this backwards? Doesn't USE_NSS currently means use nss for everything, > !USE_NSS means just use NSS for SSL (unless USE_OPENSSL). Gah, yes. Sorry.
On 2014/03/21 19:47:50, Ryan Sleevi wrote: > Having read through this patch a few times, I'm still a bit confused about > USE_OPENSSL_CERTS and had to keep cross-checking. Apologies if these comments > display my ignorance here. > > I think a big part of the confusion is that I think mentally, I keep expecting > USE_OPENSSL_CERTS to cover how X509Certificate::OSCertHandle is typedef'd. That > is, an X509* when USE_OPENSSL_CERTS is defined, and an (OS native handle) when > it's not. Since Ryan was also confused about USE_OPENSSL_CERTS, I suggest that we define USE_OPENSSL_CERTS to simply mean X509Certificate::OSCertHandle is typedef'd as an X509*. Then, in the few places where we need to know what type of CertVerifyProc is used, we add special cases for OS_ANDROID.
As WTC suggested, I've changed the meaning of USE_OPENSSL_CERTS. When USE_OPENSSL_CERTS is defined, X509::OsCertHandle is now typedef'ed to struct X509*. When USE_OPENSSL is defined, USE_OPENSSL_CERTS will now be defined for linux and android, while being off for Mac and Windows. OpenSSL cert verifying code will only be compiled on linux. https://codereview.chromium.org/206453002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/206453002/diff/1/DEPS#newcode452 DEPS:452: "/trunk/deps/third_party/openssl@" + Var("openssl_revision"), On 2014/03/21 19:47:51, Ryan Sleevi wrote: > On 2014/03/21 13:08:21, Håvard Molland wrote: > > On 2014/03/20 23:40:24, mattm wrote: > > > Is this necessary? It's already part of the top level deps dict. > > I can try to remove it from a clean checkout. However, if this is not needed, > > why is the NSS dependency just above needed? > > Because on Win/Mac/iOS, we build all of NSS & NSPR, and thus have two checkouts: > third_party/nss (for all of NSPR + NSS - libssl) > net/third_party/nss (for all of libssl) > > On Linux, we only have one checkout > net/third_party/nss (for all of libssl) > > +1 to removing this as unneeded Done. https://codereview.chromium.org/206453002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode50 build/common.gypi:50: # Use OpenSSL instead of NSS. Under development: see http://crbug.com/62803 On 2014/03/20 21:14:56, wtc wrote: > > Please update this comment. At least, the "Under development: see > http://crbug.com/ 62803" part should be removed. It would be good to describe > the new, narrower meaning of use_openssl and the relation to use_openssl_certs. Done. https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode53 build/common.gypi:53: # Use OpenSSL for certificate handling. On 2014/03/21 19:47:51, Ryan Sleevi wrote: > I'm a little confused here, when reading both this code and the later code for > CertVerifier. > > When use_openssl_certs:1, X509Certificate::OSCertHandle will always be an X509*, > right? > When use_openssl_certs:1, certificate verification MAY happen with OpenSSL - or > it may not (eg: Android), right? > > Perhaps there's a better way to express that here than just saying "certificate > handling". The way it is currently implemented, use_openssl_certs means "use openssl for certificate verification", and is only on for linux. The naming is confusing, so it should either be renamed to something along "use_openssl_verification", or I should follow wtc's suggestion and let use_openssl_certs mean "X509Certificate::OSCertHandle is typedef'd as an X509*". I think I agree with WTC on this. https://codereview.chromium.org/206453002/diff/1/build/common.gypi#newcode551 build/common.gypi:551: 'use_openssl_certs%': 1, On 2014/03/20 21:14:56, wtc wrote: > > Should we also add an "else" block that set use_openssl_certs to 0? All the > neighboring code does that, so it would be good to be consistent even if it's > not necessary. Done. https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/1/crypto/crypto.gyp#newcode223 crypto/crypto.gyp:223: [ 'use_openssl == 0 and OS == "mac"', { On 2014/03/20 21:14:56, wtc wrote: > > I think these two blocks can be simply deleted. We should get the dependency on > nss and nspr from the export_dependent_settings of the "crypto" target. > > If not, I think at least this block can be deleted. Done. https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h File crypto/encryptor.h (right): https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h#newcode17 crypto/encryptor.h:17: (defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)) On 2014/03/21 19:47:51, Ryan Sleevi wrote: > On 2014/03/21 13:08:21, Håvard Molland wrote: > > On 2014/03/20 21:14:56, wtc wrote: > > > > > > This should be written like this: > > > > > > #if defined(USE_NSS) || \ > > > (!defined(USE_OPENSSL) && (defined(OS_WIN) || defined(OS_MACOSX))) > > > > > > Please make the same change to similar ifdefs in other files. > > That's what I had first actually, but I found it a bit complex. I think they > > logically are the same though, as long as USE_NSS and USE_OPENSSL are not both > > defined. > > > > I'll put that back your version if that is what you prefer. > > I think I agree with Wan-Teh, in that I found myself having to consult his > version more than yours when trying to understand under what situations this > would be used. With the && (as written), I always have to keep that mental state > in place, whereas with the || (as wan-teh wrote), I can do fewer backtracking > about the conditions. Hmm.. Looking at it again, I agree. Although it is a slightly more complex expression, it is easier to parse. https://codereview.chromium.org/206453002/diff/1/crypto/encryptor.h#newcode17 crypto/encryptor.h:17: (defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)) On 2014/03/20 21:14:56, wtc wrote: > > This should be written like this: > > #if defined(USE_NSS) || \ > (!defined(USE_OPENSSL) && (defined(OS_WIN) || defined(OS_MACOSX))) > > Please make the same change to similar ifdefs in other files. Done. https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc... net/cert/cert_verify_proc.cc:23: #elif defined(USE_OPENSSL_CERTS) On 2014/03/21 19:47:51, Ryan Sleevi wrote: > Shouldn't you still have a !defined(OS_ANDROID) here? Currently you are wrong. The way I set this up, USE_OPENSSL_CERTS is off on OS_ANDROID. I will change this though, as wtc suggested. After that change you are right :) https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc... net/cert/cert_verify_proc.cc:23: #elif defined(USE_OPENSSL_CERTS) On 2014/03/21 19:47:51, Ryan Sleevi wrote: > Shouldn't you still have a !defined(OS_ANDROID) here? Done. https://codereview.chromium.org/206453002/diff/1/net/cert/cert_verify_proc.cc... net/cert/cert_verify_proc.cc:170: #elif defined(USE_OPENSSL_CERTS) On 2014/03/21 19:47:51, Ryan Sleevi wrote: > ditto Done. https://codereview.chromium.org/206453002/diff/1/net/cert/test_root_certs.h File net/cert/test_root_certs.h (right): https://codereview.chromium.org/206453002/diff/1/net/cert/test_root_certs.h#n... net/cert/test_root_certs.h:15: #elif defined(USE_OPENSSL_CERTS) On 2014/03/21 19:47:51, Ryan Sleevi wrote: > I don't understand why no more OS_ANDROID check? Because USE_OPENSSL_CERTS meant use OpenSSL for certificate verification. Changed now, so it follows your understanding.
Patch set 2 LGTM. IMPORTANT: please update the CL's description to match the new meaning of USE_OPENSSL_CERTS. I think the entire paragraph about the Android special case can be deleted now. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. Nit: "protocol" should be changed to "implementation" or "provider". Add "the" before "underlying". Also, I would say "crypto and SSL" because this also affects crypto/. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode53 build/common.gypi:53: # uses OpenSSLL's struct X509 to represent certificates, Typo: "OpenSSLL" has an extra L. I think the "openssl also is used for certificate verification" part can be removed. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode57 build/common.gypi:57: # Typedef fX509Certificate::OSCertHandle to OpenSSL's struct X509*. Remove the "f" before "X509Certificate". https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... build/common.gypi:566: # When OpenSSL is used for SSL and crypto on unix like systems, use Nit: unix like => Unix-like https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... build/common.gypi:2591: }], It seems clearer to rewrite these two as follows: ['<(use_openssl)==1', { 'defines': ['USE_OPENSSL=1'], }], ['<(use_openssl_certs)==1', { 'defines': ['USE_OPENSSL_CERTS=1'], }], ['>(nacl_untrusted_build)==1', { 'defines': [ 'USE_OPENSSL=1', 'USE_OPENSSL_CERTS=1', ], }], https://codereview.chromium.org/206453002/diff/20001/net/cert/cert_verify_pro... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/206453002/diff/20001/net/cert/cert_verify_pro... net/cert/cert_verify_proc.cc:26: #include "net/cert/cert_verify_proc_android.h" Nit: this can be simplified by careful reordering of the ifdefs: #elif defined(OS_ANDROID) #include "net/cert/cert_verify_proc_android.h" #elif defined(USE_OPENSSL_CERTS) #include "net/cert/cert_verify_proc_openssl.h" If you like this, make the same change to the ifdefs inside CertVerifyProc::CreateDefault(). https://codereview.chromium.org/206453002/diff/20001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/206453002/diff/20001/net/net.gyp#newcode1464 net/net.gyp:1464: [ 'use_openssl == 1', { Merge this with the existing use_openssl==1 block at line 1374. https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... net/ssl/server_bound_cert_service_unittest.cc:772: Nit: Delete one blank line. https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:6951: // a certificate is EV or not and the system won't recognise our testing root. Nit: the first couple of words can be moved to the previous line.
On 2014/03/24 18:48:52, Håvard Molland wrote: > > When USE_OPENSSL_CERTS is defined, X509::OsCertHandle is now typedef'ed to > struct X509*. > > When USE_OPENSSL is defined, USE_OPENSSL_CERTS will now be defined for linux and > android, while being off for Mac and Windows. > > OpenSSL cert verifying code will only be compiled on linux. This is a great summary of the current code. You can use this in the CL's description and perhaps also copy this to the source code (build/common.gypi).
https://codereview.chromium.org/206453002/diff/20001/net/cert/cert_verify_pro... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/206453002/diff/20001/net/cert/cert_verify_pro... net/cert/cert_verify_proc.cc:26: #include "net/cert/cert_verify_proc_android.h" The current expression keeps the USE_FOOs, and OS_FOO's separated. The if-else blocks seems to be organized this way most places. On 2014/03/25 17:18:28, wtc wrote: > > Nit: this can be simplified by careful reordering of the ifdefs: > > #elif defined(OS_ANDROID) > #include "net/cert/cert_verify_proc_android.h" > #elif defined(USE_OPENSSL_CERTS) > #include "net/cert/cert_verify_proc_openssl.h" > > If you like this, make the same change to the ifdefs inside > CertVerifyProc::CreateDefault().
Ryan, does this also look OK for you also? https://codereview.chromium.org/206453002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. On 2014/03/25 17:18:28, wtc wrote: > > Nit: "protocol" should be changed to "implementation" or "provider". Add "the" > before "underlying". > > Also, I would say "crypto and SSL" because this also affects crypto/. Done. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. On 2014/03/25 17:18:28, wtc wrote: > > Nit: "protocol" should be changed to "implementation" or "provider". Add "the" > before "underlying". > > Also, I would say "crypto and SSL" because this also affects crypto/. Done. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode53 build/common.gypi:53: # uses OpenSSLL's struct X509 to represent certificates, On 2014/03/25 17:18:28, wtc wrote: > > Typo: "OpenSSLL" has an extra L. > > I think the "openssl also is used for certificate verification" part can be > removed. Done. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode57 build/common.gypi:57: # Typedef fX509Certificate::OSCertHandle to OpenSSL's struct X509*. On 2014/03/25 17:18:28, wtc wrote: > > Remove the "f" before "X509Certificate". Done. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... build/common.gypi:566: # When OpenSSL is used for SSL and crypto on unix like systems, use On 2014/03/25 17:18:28, wtc wrote: > > Nit: unix like => Unix-like Done. https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... build/common.gypi:2591: }], On 2014/03/25 17:18:28, wtc wrote: > > It seems clearer to rewrite these two as follows: > > ['<(use_openssl)==1', { > 'defines': ['USE_OPENSSL=1'], > }], > ['<(use_openssl_certs)==1', { > 'defines': ['USE_OPENSSL_CERTS=1'], > }], > ['>(nacl_untrusted_build)==1', { > 'defines': [ > 'USE_OPENSSL=1', > 'USE_OPENSSL_CERTS=1', > ], > }], Done. https://codereview.chromium.org/206453002/diff/20001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/206453002/diff/20001/net/net.gyp#newcode1464 net/net.gyp:1464: [ 'use_openssl == 1', { On 2014/03/25 17:18:28, wtc wrote: > > Merge this with the existing use_openssl==1 block at line 1374. Done. https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... net/ssl/server_bound_cert_service_unittest.cc:772: On 2014/03/25 17:18:28, wtc wrote: > > Nit: Delete one blank line. Done. https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:6951: // a certificate is EV or not and the system won't recognise our testing root. On 2014/03/25 17:18:28, wtc wrote: > > Nit: the first couple of words can be moved to the previous line. Done.
Since this patch affects many profiles, I did a test compile using "git cl try". As you see above some profiles fails. Especially mac_chromium_compile_dbg, and the windows profiles. The other failures looks like flukes or expected errors. The common error for Windows and Mac is that the linker fails to find the symbol for PR_ImplodeTime, which should be defined in src/base/third_party/nspr/prtime.cc. I don't see that any of my changes can cause this, and mac_chromium_compile_dbg (with USE_OPENSSL undefied) compiles here locally. Is this a trybot failure? On 2014/03/26 12:25:17, Håvard Molland wrote: > Ryan, does this also look OK for you also? > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 > build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. > On 2014/03/25 17:18:28, wtc wrote: > > > > Nit: "protocol" should be changed to "implementation" or "provider". Add "the" > > before "underlying". > > > > Also, I would say "crypto and SSL" because this also affects crypto/. > > Done. > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 > build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. > On 2014/03/25 17:18:28, wtc wrote: > > > > Nit: "protocol" should be changed to "implementation" or "provider". Add "the" > > before "underlying". > > > > Also, I would say "crypto and SSL" because this also affects crypto/. > > Done. > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode53 > build/common.gypi:53: # uses OpenSSLL's struct X509 to represent certificates, > On 2014/03/25 17:18:28, wtc wrote: > > > > Typo: "OpenSSLL" has an extra L. > > > > I think the "openssl also is used for certificate verification" part can be > > removed. > > Done. > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode57 > build/common.gypi:57: # Typedef fX509Certificate::OSCertHandle to OpenSSL's > struct X509*. > On 2014/03/25 17:18:28, wtc wrote: > > > > Remove the "f" before "X509Certificate". > > Done. > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... > build/common.gypi:566: # When OpenSSL is used for SSL and crypto on unix like > systems, use > On 2014/03/25 17:18:28, wtc wrote: > > > > Nit: unix like => Unix-like > > Done. > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... > build/common.gypi:2591: }], > On 2014/03/25 17:18:28, wtc wrote: > > > > It seems clearer to rewrite these two as follows: > > > > ['<(use_openssl)==1', { > > 'defines': ['USE_OPENSSL=1'], > > }], > > ['<(use_openssl_certs)==1', { > > 'defines': ['USE_OPENSSL_CERTS=1'], > > }], > > ['>(nacl_untrusted_build)==1', { > > 'defines': [ > > 'USE_OPENSSL=1', > > 'USE_OPENSSL_CERTS=1', > > ], > > }], > > Done. > > https://codereview.chromium.org/206453002/diff/20001/net/net.gyp > File net/net.gyp (right): > > https://codereview.chromium.org/206453002/diff/20001/net/net.gyp#newcode1464 > net/net.gyp:1464: [ 'use_openssl == 1', { > On 2014/03/25 17:18:28, wtc wrote: > > > > Merge this with the existing use_openssl==1 block at line 1374. > > Done. > > https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... > File net/ssl/server_bound_cert_service_unittest.cc (right): > > https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... > net/ssl/server_bound_cert_service_unittest.cc:772: > On 2014/03/25 17:18:28, wtc wrote: > > > > Nit: Delete one blank line. > > Done. > > https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... > net/url_request/url_request_unittest.cc:6951: // a certificate is EV or not and > the system won't recognise our testing root. > On 2014/03/25 17:18:28, wtc wrote: > > > > Nit: the first couple of words can be moved to the previous line. > > Done.
Since I couldn't reproduce the link error locally, I created a test CL at https://codereview.chromium.org/214523002/#ps40001 and ran the try bots. It shows that it compiles nicely when putting the nss and nspr dependencies back in for crypto_unittests (patch set 2), and fails when removing them (patch set 3). Not sure why that is. The missing function seems to normally be implemented at src/base/third_party/nspr/prtime.cc. Also the dependencies should be set by the "crypto" target as mentioned by wtc. I put the dependencies back in latest patch set in this CL. On 2014/03/26 15:32:35, Håvard Molland wrote: > Since this patch affects many profiles, I did a test compile using "git cl try". > As you see above some profiles fails. Especially mac_chromium_compile_dbg, and > the windows profiles. The other failures looks like flukes or expected errors. > > The common error for Windows and Mac is that the linker fails to find the symbol > for PR_ImplodeTime, which should be defined in > src/base/third_party/nspr/prtime.cc. I don't see that any of my changes can > cause this, and mac_chromium_compile_dbg (with USE_OPENSSL undefied) compiles > here locally. > > Is this a trybot failure? > > > > On 2014/03/26 12:25:17, Håvard Molland wrote: > > Ryan, does this also look OK for you also? > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi > > File build/common.gypi (right): > > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 > > build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Nit: "protocol" should be changed to "implementation" or "provider". Add > "the" > > > before "underlying". > > > > > > Also, I would say "crypto and SSL" because this also affects crypto/. > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode50 > > build/common.gypi:50: # Use OpenSSL instead of NSS as underlying SSL protocol. > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Nit: "protocol" should be changed to "implementation" or "provider". Add > "the" > > > before "underlying". > > > > > > Also, I would say "crypto and SSL" because this also affects crypto/. > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode53 > > build/common.gypi:53: # uses OpenSSLL's struct X509 to represent certificates, > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Typo: "OpenSSLL" has an extra L. > > > > > > I think the "openssl also is used for certificate verification" part can be > > > removed. > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcode57 > > build/common.gypi:57: # Typedef fX509Certificate::OSCertHandle to OpenSSL's > > struct X509*. > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Remove the "f" before "X509Certificate". > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... > > build/common.gypi:566: # When OpenSSL is used for SSL and crypto on unix like > > systems, use > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Nit: unix like => Unix-like > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/build/common.gypi#newcod... > > build/common.gypi:2591: }], > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > It seems clearer to rewrite these two as follows: > > > > > > ['<(use_openssl)==1', { > > > 'defines': ['USE_OPENSSL=1'], > > > }], > > > ['<(use_openssl_certs)==1', { > > > 'defines': ['USE_OPENSSL_CERTS=1'], > > > }], > > > ['>(nacl_untrusted_build)==1', { > > > 'defines': [ > > > 'USE_OPENSSL=1', > > > 'USE_OPENSSL_CERTS=1', > > > ], > > > }], > > > > Done. > > > > https://codereview.chromium.org/206453002/diff/20001/net/net.gyp > > File net/net.gyp (right): > > > > https://codereview.chromium.org/206453002/diff/20001/net/net.gyp#newcode1464 > > net/net.gyp:1464: [ 'use_openssl == 1', { > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Merge this with the existing use_openssl==1 block at line 1374. > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... > > File net/ssl/server_bound_cert_service_unittest.cc (right): > > > > > https://codereview.chromium.org/206453002/diff/20001/net/ssl/server_bound_cer... > > net/ssl/server_bound_cert_service_unittest.cc:772: > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Nit: Delete one blank line. > > > > Done. > > > > > https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... > > File net/url_request/url_request_unittest.cc (right): > > > > > https://codereview.chromium.org/206453002/diff/20001/net/url_request/url_requ... > > net/url_request/url_request_unittest.cc:6951: // a certificate is EV or not > and > > the system won't recognise our testing root. > > On 2014/03/25 17:18:28, wtc wrote: > > > > > > Nit: the first couple of words can be moved to the previous line. > > > > Done.
Patch set 4 LGTM. I carefully reviewed latest patch set today, so you can go ahead and commit this. I don't want this CL to drag on too long. Nit: the CL's description has a typo: "OsCertHandle" should be "OSCertHandle". https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp#newco... crypto/crypto.gyp:201: 'openpgp_symmetric_encryption_unittest.cc', I believe openpgp_symmetric_encryption_unittest.cc is incorrectly excluded in this build configuration. I will deal with this separately. https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp#newco... crypto/crypto.gyp:206: '../third_party/nss/nss.gyp:nss', I misunderstood what export_dependent_settings means. Sorry about the wrong suggestion. The unresolved symbol PR_ImplodeTime shows crypto_unittests (nss_util_unittest.cc) calls that NSPR function. So crypto_unittests needs to have a dependency on nss.gyp:nspr. The dependency on nss.gyp:nss doesn't seem necessary. So I suggest changing this line to '../third_party/nss/nss.gyp:nspr', and remove lines 209-213. https://codereview.chromium.org/206453002/diff/190001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/206453002/diff/190001/net/net.gyp#newcode1589 net/net.gyp:1589: }], Nit: I suggest indent by two, except for 'conditions', which we want to align with 'link_settings' on line 1591. It's too bad the indentation level is inconsistent in this file. https://codereview.chromium.org/206453002/diff/190001/net/net.gyp#newcode2275 net/net.gyp:2275: 'cert/x509_util_openssl_unittest.cc', I am wondering why this doesn't need to move to the use_openssl_certs == 0 block.
https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp#newco... crypto/crypto.gyp:201: 'openpgp_symmetric_encryption_unittest.cc', On 2014/03/27 17:40:18, wtc wrote: > > I believe openpgp_symmetric_encryption_unittest.cc is incorrectly excluded in > this build configuration. I will deal with this separately. I was wrong. openpgp_symmetric_encryption_unittest.cc needs to be excluded because openpgp_symmetric_encryption.h and openpgp_symmetric_encryption.cc are excluded in this build configuration. https://codereview.chromium.org/206453002/diff/190001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/206453002/diff/190001/net/net.gyp#newcode2275 net/net.gyp:2275: 'cert/x509_util_openssl_unittest.cc', On 2014/03/27 17:40:18, wtc wrote: > > I am wondering why this doesn't need to move to the use_openssl_certs == 0 > block. OK, this file doesn't test any function that operates on net::X509Certificate.
Thanks for the lgtm. When this is in, src/net and src/crypto will compile for mac with openssl on (when thirdpary/openssl also is patched). I think this patch also catches most of the issues for Windows+OpenSSL, but I leave that to someone working on Windows. https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp#newco... crypto/crypto.gyp:206: '../third_party/nss/nss.gyp:nss', On 2014/03/27 17:40:18, wtc wrote: > > I misunderstood what export_dependent_settings means. Sorry about the wrong > suggestion. > > The unresolved symbol PR_ImplodeTime shows crypto_unittests > (nss_util_unittest.cc) calls that NSPR function. So crypto_unittests needs to > have a dependency on nss.gyp:nspr. The dependency on nss.gyp:nss doesn't seem > necessary. > > So I suggest changing this line to > '../third_party/nss/nss.gyp:nspr', > > and remove lines 209-213. Done. It still confuses me why nspr is needed for the PR_ImplodeTime() as this function is implemented and (always?) compiled at src/base/third_party/nspr/prtime.cc. https://codereview.chromium.org/206453002/diff/190001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/206453002/diff/190001/net/net.gyp#newcode1589 net/net.gyp:1589: }], On 2014/03/27 17:40:18, wtc wrote: > > Nit: I suggest indent by two, except for 'conditions', which we want to align > with 'link_settings' on line 1591. It's too bad the indentation level is > inconsistent in this file. Done.
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/206453002/210001
Patch set 5 LGTM. https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp File crypto/crypto.gyp (right): https://codereview.chromium.org/206453002/diff/190001/crypto/crypto.gyp#newco... crypto/crypto.gyp:206: '../third_party/nss/nss.gyp:nss', On 2014/03/27 21:10:58, Håvard Molland wrote: > > It still confuses me why nspr is needed for the PR_ImplodeTime() as this > function is implemented and (always?) compiled at > src/base/third_party/nspr/prtime.cc. I forgot to answer that question. src/base/third_party/nspr/prtime.cc is a forked copy of the NSPR source file prtime.c. It is internal to the base component. nss_util_unittest.cc needs to get the PR_ImplodeTime function from the real NSPR.
The CQ bit was unchecked by haavardm@opera.com
The CQ bit was checked by haavardm@opera.com
The CQ bit was unchecked by haavardm@opera.com
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/206453002/210001
Message was sent while issue was closed.
Change committed as 260152
Message was sent while issue was closed.
On 2014/03/27 21:10:57, Håvard Molland wrote: > Thanks for the lgtm. When this is in, src/net and src/crypto will compile for > mac with openssl on (when thirdpary/openssl also is patched). You can have me review your third_party/openssl for Mac CL. I am good at porting. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
