Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1810153002:
Adding iOS OpenSSL Implementation (Closed)
|
Created:
4 years, 9 months ago by svaldez Modified:
4 years, 8 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, chromoting-reviews_chromium.org, kapishnikov, sdefresne+watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis adds the OpenSSL-specific implementations for iOS, using SecTrustEvaluate in order to determine the validity of the certificate chain.
BUG=591545
Committed: https://crrev.com/864f9468ae2a8d1ba95c64824ef2caf05b7121fc
Cr-Commit-Position: refs/heads/master@{#383297}
Patch Set 1 #Patch Set 2 : Fix upstream. #Patch Set 3 : Rebase. #Patch Set 4 : Fixing typoes. #Patch Set 5 : Merging in other CL and using ScopedX509. #Patch Set 6 : Remove unused accessor. #Patch Set 7 : Removing remoting_nacl #Patch Set 8 : Fixes. #Patch Set 9 : Fix chain 0. #Patch Set 10 : Fix build. #Patch Set 11 : Rebase on master. #
Total comments: 2
Patch Set 12 : Fixing build files. #Patch Set 13 : Fix dumb typo. #Patch Set 14 : Add missing test files. #Patch Set 15 : Fix duplicate. #Patch Set 16 : Simplifying logic for first iteration. #
Total comments: 20
Patch Set 17 : Fix removed dup. #Patch Set 18 : Fixing nits. #
Total comments: 24
Patch Set 19 : Fixing more comments. #Patch Set 20 : Fixing ref counting. #
Total comments: 14
Patch Set 21 : Fixing nits. #Patch Set 22 : Fix syntax. #
Total comments: 28
Patch Set 23 : Fixing more nits. #
Total comments: 2
Messages
Total messages: 39 (10 generated)
svaldez@chromium.org changed reviewers: + davidben@chromium.org
I'll fix the issue description tomorrow.
The CQ bit was checked by svaldez@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1808963004 Patch 120001). Please resolve the dependency and try again.
Description was changed from ========== Adding basic openssl_ios files BUG= ========== to ========== This adds the OpenSSL-specific implementations for iOS, using SecTrustEvaluate in order to determine the validity of the certificate chain. BUG=591545 ==========
Should be mostly the final version, modulo none of the tests working.
As a FYI, this should be the only CL you need to patch in now to get BoringSSL running on iOS. You'll need to set the use_openssl build flag to true, but then it should just work. net_unittests will still fail until I finish migrating the CertVerify tests over to the correct macros/results. Otherwise, let me know if you get any issues. I'll let you know once a finalized enough version makes it to trunk so you won't need to patch this in.
https://codereview.chromium.org/1810153002/diff/190001/remoting/remoting_nacl... File remoting/remoting_nacl.gyp (right): https://codereview.chromium.org/1810153002/diff/190001/remoting/remoting_nacl... remoting/remoting_nacl.gyp:16: 'use_nss_verifier': 0, This parameter is not used in this file and can be removed.
https://codereview.chromium.org/1810153002/diff/190001/remoting/remoting_nacl... File remoting/remoting_nacl.gyp (right): https://codereview.chromium.org/1810153002/diff/190001/remoting/remoting_nacl... remoting/remoting_nacl.gyp:16: 'use_nss_verifier': 0, On 2016/03/18 21:34:37, Sergey Ulanov wrote: > This parameter is not used in this file and can be removed. Done.
davidben@chromium.org changed reviewers: + rsleevi@chromium.org
This is probably best reviewed by +rsleevi, but I did a pass over it. https://codereview.chromium.org/1810153002/diff/280001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1810153002/diff/280001/net/BUILD.gn#newcode1451 net/BUILD.gn:1451: if (!is_ios || !use_nss_verifier) { Should this be if (use_openssl) { verify_certificate_chain_pkits_unittest.cc is #ifdefed on that near the top. https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:36: #include "net/cert/cert_verify_proc_openssl_ios.h" Optional: Since we'll eventually name it this anyway, should we name this CertVerifyProcIOS? The current iOS port uses CertVerifyProcNSS. Or would that be confusing because the other files are _openssl_ios? (I don't feel strongly.) Doesn't look like the file even depends on BoringSSL right now. https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. rsleevi is probably a better reviewer for this file. https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.h (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.h:12: // Performs certificate path construction and validation using IOS's iOS https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... File net/cert/x509_certificate_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:10: #include <openssl/x509v3.h> I think this also needs an <openssl/x509.h> https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:57: if (PKCS7_get_certificates(certs, &der_data)) { [Hrm. I really should get you a non-crypto/x509 version of this function.] https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:111: ScopedX509 cert = OSCertHandleToOpenSSL(os_cert); Check for errors here. iOS and OpenSSL may have different leniencies in parsing. Ditto elsewhere. https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:363: PublicKeyType* type) { I'm really puzzled why this can't be done from Apple's APIs, but I can't find anything. :-/ SecKeyGetBlockSize will apparently return the size of the RSA modulus (bytes? bits?), but I don't see a way to find the key type. https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:377: *size_bits = EVP_PKEY_size(key) * 8; I believe all of these want to be EVP_PKEY_bits(key). EVP_PKEY_size(key) is the maximum size of the signature. (The API names are terrible. Blame OpenSSL.) https://codereview.chromium.org/1810153002/diff/280001/net/net_common.gypi File net/net_common.gypi (right): https://codereview.chromium.org/1810153002/diff/280001/net/net_common.gypi#ne... net/net_common.gypi:308: 'cert/test_root_cert_mac.cc', Huh. Does test_root_cert_mac.cc normally get excluded from iOS by filename? Odd that OS_MAC matches but the suffix rule doesn't.
https://codereview.chromium.org/1810153002/diff/280001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1810153002/diff/280001/net/BUILD.gn#newcode1451 net/BUILD.gn:1451: if (!is_ios || !use_nss_verifier) { On 2016/03/21 19:46:47, davidben wrote: > Should this be > > if (use_openssl) { > > verify_certificate_chain_pkits_unittest.cc is #ifdefed on that near the top. Done. https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:36: #include "net/cert/cert_verify_proc_openssl_ios.h" On 2016/03/21 19:46:47, davidben wrote: > Optional: Since we'll eventually name it this anyway, should we name this > CertVerifyProcIOS? The current iOS port uses CertVerifyProcNSS. Or would that be > confusing because the other files are _openssl_ios? (I don't feel strongly.) > Doesn't look like the file even depends on BoringSSL right now. Done. https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/21 19:46:47, davidben wrote: > rsleevi is probably a better reviewer for this file. Acknowledged. https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.h (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.h:12: // Performs certificate path construction and validation using IOS's On 2016/03/21 19:46:47, davidben wrote: > iOS Done. https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... File net/cert/x509_certificate_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:10: #include <openssl/x509v3.h> On 2016/03/21 19:46:47, davidben wrote: > I think this also needs an <openssl/x509.h> Done. https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:57: if (PKCS7_get_certificates(certs, &der_data)) { On 2016/03/21 19:46:47, davidben wrote: > [Hrm. I really should get you a non-crypto/x509 version of this function.] Acknowledged. https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:111: ScopedX509 cert = OSCertHandleToOpenSSL(os_cert); On 2016/03/21 19:46:47, davidben wrote: > Check for errors here. iOS and OpenSSL may have different leniencies in parsing. > Ditto elsewhere. Done. https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:363: PublicKeyType* type) { On 2016/03/21 19:46:47, davidben wrote: > I'm really puzzled why this can't be done from Apple's APIs, but I can't find > anything. :-/ SecKeyGetBlockSize will apparently return the size of the RSA > modulus (bytes? bits?), but I don't see a way to find the key type. Because its iOS? https://codereview.chromium.org/1810153002/diff/280001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:377: *size_bits = EVP_PKEY_size(key) * 8; On 2016/03/21 19:46:47, davidben wrote: > I believe all of these want to be EVP_PKEY_bits(key). EVP_PKEY_size(key) is the > maximum size of the signature. (The API names are terrible. Blame OpenSSL.) Done. https://codereview.chromium.org/1810153002/diff/280001/net/net_common.gypi File net/net_common.gypi (right): https://codereview.chromium.org/1810153002/diff/280001/net/net_common.gypi#ne... net/net_common.gypi:308: 'cert/test_root_cert_mac.cc', On 2016/03/21 19:46:47, davidben wrote: > Huh. Does test_root_cert_mac.cc normally get excluded from iOS by filename? Odd > that OS_MAC matches but the suffix rule doesn't. Yup. Also had to move it due to ordering of platform exclusion rules.
https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:38: return ERR_ACCESS_DENIED; This doesn't seem right, because it means mapping a non-cert error code into the cert verification error space. Do we have precedent for this? https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:59: ssl_policy = SecPolicyCreateSSL(true, base::SysUTF8ToCFStringRef(hostname)); I'm torn as to whether or not we want to do this, because this means we'll be relying on iOS's hostname verification. I think we want to continue to use X509Certificate's (as we do on every other platform) https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:128: X509Certificate::DupOSCertHandle(reinterpret_cast<SecCertificateRef>( BUG: You end up leaking all of these certs. You shouldn't be DUPing them, because CreateFromHandle does so internally. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:194: bool CheckRevocationWithCRLSet(CFArrayRef chain, CRLSet* crl_set) { I don't think we want/need this behaviour, given that Android also doesn't support CRLSets. Feel free to leave this off and file a bug on me, because if we do add CRLSet behaviour to iOS, we want to make sure it does the right thing, and right now, it won't. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:289: switch (trust_result) { BUG: No expiration handling is done here, which was the one requirement from enamel (expired vs name mismatch vs everything else) https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:294: verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID; I don't think this this the right mapping code; a Deny error is the user has explicitly rejected the CA, not just that it's untrusted. I think REVOKED may be more appropriate? https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:301: verify_result->cert_status &= ~CERT_STATUS_COMMON_NAME_INVALID; This is never set (per above), so it's impossible to mask off. Instead, hostname mismatches will end up with CERT_STATUS_INVALID all the time. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:307: verify_result->cert_status &= ~CERT_STATUS_NO_REVOCATION_MECHANISM; This is never set. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:309: verify_result->is_issued_by_known_root = true; BUG: This isn't correct. We default to false, not true, when we don't know. (iOS doesn't expose to us the set of trust anchors, AFAIK) https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.h (right): https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.h:6: #define NET_CERT_CERT_VERIFY_PROC_OPENSSL_IOS_H_ This file name should match the class name - which means CERT_VERIFY_PROC_IOS_H_
https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:38: return ERR_ACCESS_DENIED; On 2016/03/21 20:49:55, Ryan Sleevi wrote: > This doesn't seem right, because it means mapping a non-cert error code into the > cert verification error space. Do we have precedent for this? This is the same thing we do for cert_verify_proc_mac. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:59: ssl_policy = SecPolicyCreateSSL(true, base::SysUTF8ToCFStringRef(hostname)); On 2016/03/21 20:49:55, Ryan Sleevi wrote: > I'm torn as to whether or not we want to do this, because this means we'll be > relying on iOS's hostname verification. I think we want to continue to use > X509Certificate's (as we do on every other platform) Will drop the hostname verification for now. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:128: X509Certificate::DupOSCertHandle(reinterpret_cast<SecCertificateRef>( On 2016/03/21 20:49:55, Ryan Sleevi wrote: > BUG: You end up leaking all of these certs. You shouldn't be DUPing them, > because CreateFromHandle does so internally. Is that the case, I had errors without this. The Retain from this Dup gets cancelled out by the destruction of verified_chain. Then there's another Retain/Release for the creation and destruction of the X509Certificate objects later. If we don't Retain here, then the destruction of the input cert_chain will result in the verified_chain references becoming invalid. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:194: bool CheckRevocationWithCRLSet(CFArrayRef chain, CRLSet* crl_set) { On 2016/03/21 20:49:55, Ryan Sleevi wrote: > I don't think we want/need this behaviour, given that Android also doesn't > support CRLSets. > > Feel free to leave this off and file a bug on me, because if we do add CRLSet > behaviour to iOS, we want to make sure it does the right thing, and right now, > it won't. Done. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:289: switch (trust_result) { On 2016/03/21 20:49:55, Ryan Sleevi wrote: > BUG: No expiration handling is done here, which was the one requirement from > enamel (expired vs name mismatch vs everything else) Added a TODO. As it stands, this is mostly focused on Cronet's use case, since there are still a bunch of edge cases we don't handle due to the low resolution of iOS' API. I think adding Expiration Handling and some more fine-grained status should be handled in a separate CL for making this usable enough for Chrome. There's a couple other things that got removed from here that would also need to be added back in. One thing, that I'm not sure if we want to keep, is the manual cert chain building that we do in Mac, to get around the system library possibly returning weak chains. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:294: verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID; On 2016/03/21 20:49:55, Ryan Sleevi wrote: > I don't think this this the right mapping code; a Deny error is the user has > explicitly rejected the CA, not just that it's untrusted. I think REVOKED may be > more appropriate? For Mac we've been using AUTHORITY_INVALID in this case. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:301: verify_result->cert_status &= ~CERT_STATUS_COMMON_NAME_INVALID; On 2016/03/21 20:49:55, Ryan Sleevi wrote: > This is never set (per above), so it's impossible to mask off. Instead, hostname > mismatches will end up with CERT_STATUS_INVALID all the time. Done. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:307: verify_result->cert_status &= ~CERT_STATUS_NO_REVOCATION_MECHANISM; On 2016/03/21 20:49:55, Ryan Sleevi wrote: > This is never set. Done. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:309: verify_result->is_issued_by_known_root = true; On 2016/03/21 20:49:55, Ryan Sleevi wrote: > BUG: This isn't correct. We default to false, not true, when we don't know. > > (iOS doesn't expose to us the set of trust anchors, AFAIK) Done. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.h (right): https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.h:6: #define NET_CERT_CERT_VERIFY_PROC_OPENSSL_IOS_H_ On 2016/03/21 20:49:56, Ryan Sleevi wrote: > This file name should match the class name - which means CERT_VERIFY_PROC_IOS_H_ Done.
https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:128: X509Certificate::DupOSCertHandle(reinterpret_cast<SecCertificateRef>( On 2016/03/21 21:36:26, svaldez wrote: > On 2016/03/21 20:49:55, Ryan Sleevi wrote: > > BUG: You end up leaking all of these certs. You shouldn't be DUPing them, > > because CreateFromHandle does so internally. > > Is that the case, I had errors without this. The Retain from this Dup gets > cancelled out by the destruction of verified_chain. You mean the member |verified_chain|? If so, that doesn't get cancelled - it's just an std::vector > Then there's another > Retain/Release for the creation and destruction of the X509Certificate objects > later. If we don't Retain here, then the destruction of the input cert_chain > will result in the verified_chain references becoming invalid. Is that a bug in CreateFromHandle? The semantics are that CreateFromHandle should CFRetain all of its input arguments (duping them). AFAICT, there are no CFRelease()s in this function - so you should have two refs. The only thing I can think of is some specialization of std::vector<> to CFRelease, but that surely doesn't sound right. Just from this function, though, it should be fully safe to create a new X509Certificate from the GetIntermediateCertificates() of another X509Certificate (which doesn't do any CFRetain before returning), which means it should be fully unnecessary to CFRetain here (via DupOSCertHandle) https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:289: switch (trust_result) { On 2016/03/21 21:36:26, svaldez wrote: > On 2016/03/21 20:49:55, Ryan Sleevi wrote: > > BUG: No expiration handling is done here, which was the one requirement from > > enamel (expired vs name mismatch vs everything else) > > Added a TODO. As it stands, this is mostly focused on Cronet's use case, since > there are still a bunch of edge cases we don't handle due to the low resolution > of iOS' API. I think adding Expiration Handling and some more fine-grained > status should be handled in a separate CL for making this usable enough for > Chrome. There's a couple other things that got removed from here that would also > need to be added back in. One thing, that I'm not sure if we want to keep, is > the manual cert chain building that we do in Mac, to get around the system > library possibly returning weak chains. Yeah, I debated a comment to that effect, but iOS uses the new chain building algorithm and new weighting, so I'm happy leaving off a lot of that logic for now. https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:294: verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID; On 2016/03/21 21:36:26, svaldez wrote: > On 2016/03/21 20:49:55, Ryan Sleevi wrote: > > I don't think this this the right mapping code; a Deny error is the user has > > explicitly rejected the CA, not just that it's untrusted. I think REVOKED may > be > > more appropriate? > > For Mac we've been using AUTHORITY_INVALID in this case. OK, fair enough.
https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:128: X509Certificate::DupOSCertHandle(reinterpret_cast<SecCertificateRef>( On 2016/03/21 21:44:14, Ryan Sleevi wrote: > On 2016/03/21 21:36:26, svaldez wrote: > > On 2016/03/21 20:49:55, Ryan Sleevi wrote: > > > BUG: You end up leaking all of these certs. You shouldn't be DUPing them, > > > because CreateFromHandle does so internally. > > > > Is that the case, I had errors without this. The Retain from this Dup gets > > cancelled out by the destruction of verified_chain. > > You mean the member |verified_chain|? If so, that doesn't get cancelled - it's > just an std::vector > > > Then there's another > > Retain/Release for the creation and destruction of the X509Certificate objects > > later. If we don't Retain here, then the destruction of the input cert_chain > > will result in the verified_chain references becoming invalid. > > Is that a bug in CreateFromHandle? The semantics are that CreateFromHandle > should CFRetain all of its input arguments (duping them). AFAICT, there are no > CFRelease()s in this function - so you should have two refs. > > The only thing I can think of is some specialization of std::vector<> to > CFRelease, but that surely doesn't sound right. Just from this function, though, > it should be fully safe to create a new X509Certificate from the > GetIntermediateCertificates() of another X509Certificate (which doesn't do any > CFRetain before returning), which means it should be fully unnecessary to > CFRetain here (via DupOSCertHandle) Fixed. There was an extra CFRelease I was doing up above.
On 2016/03/22 15:05:56, svaldez wrote: > https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... > File net/cert/cert_verify_proc_openssl_ios.cc (right): > > https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... > net/cert/cert_verify_proc_openssl_ios.cc:128: > X509Certificate::DupOSCertHandle(reinterpret_cast<SecCertificateRef>( > On 2016/03/21 21:44:14, Ryan Sleevi wrote: > > On 2016/03/21 21:36:26, svaldez wrote: > > > On 2016/03/21 20:49:55, Ryan Sleevi wrote: > > > > BUG: You end up leaking all of these certs. You shouldn't be DUPing them, > > > > because CreateFromHandle does so internally. > > > > > > Is that the case, I had errors without this. The Retain from this Dup gets > > > cancelled out by the destruction of verified_chain. > > > > You mean the member |verified_chain|? If so, that doesn't get cancelled - it's > > just an std::vector > > > > > Then there's another > > > Retain/Release for the creation and destruction of the X509Certificate > objects > > > later. If we don't Retain here, then the destruction of the input cert_chain > > > will result in the verified_chain references becoming invalid. > > > > Is that a bug in CreateFromHandle? The semantics are that CreateFromHandle > > should CFRetain all of its input arguments (duping them). AFAICT, there are no > > CFRelease()s in this function - so you should have two refs. > > > > The only thing I can think of is some specialization of std::vector<> to > > CFRelease, but that surely doesn't sound right. Just from this function, > though, > > it should be fully safe to create a new X509Certificate from the > > GetIntermediateCertificates() of another X509Certificate (which doesn't do any > > CFRetain before returning), which means it should be fully unnecessary to > > CFRetain here (via DupOSCertHandle) > > Fixed. There was an extra CFRelease I was doing up above. I've patched this CL into my iOS build, and basic cronet tests still work! Woo-hoo! To my surprise the binary size of executable is about 2mb smaller than with nss, is that expected?
On 2016/03/22 15:14:56, mef wrote: > On 2016/03/22 15:05:56, svaldez wrote: > > > https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... > > File net/cert/cert_verify_proc_openssl_ios.cc (right): > > > > > https://codereview.chromium.org/1810153002/diff/320001/net/cert/cert_verify_p... > > net/cert/cert_verify_proc_openssl_ios.cc:128: > > X509Certificate::DupOSCertHandle(reinterpret_cast<SecCertificateRef>( > > On 2016/03/21 21:44:14, Ryan Sleevi wrote: > > > On 2016/03/21 21:36:26, svaldez wrote: > > > > On 2016/03/21 20:49:55, Ryan Sleevi wrote: > > > > > BUG: You end up leaking all of these certs. You shouldn't be DUPing > them, > > > > > because CreateFromHandle does so internally. > > > > > > > > Is that the case, I had errors without this. The Retain from this Dup gets > > > > cancelled out by the destruction of verified_chain. > > > > > > You mean the member |verified_chain|? If so, that doesn't get cancelled - > it's > > > just an std::vector > > > > > > > Then there's another > > > > Retain/Release for the creation and destruction of the X509Certificate > > objects > > > > later. If we don't Retain here, then the destruction of the input > cert_chain > > > > will result in the verified_chain references becoming invalid. > > > > > > Is that a bug in CreateFromHandle? The semantics are that CreateFromHandle > > > should CFRetain all of its input arguments (duping them). AFAICT, there are > no > > > CFRelease()s in this function - so you should have two refs. > > > > > > The only thing I can think of is some specialization of std::vector<> to > > > CFRelease, but that surely doesn't sound right. Just from this function, > > though, > > > it should be fully safe to create a new X509Certificate from the > > > GetIntermediateCertificates() of another X509Certificate (which doesn't do > any > > > CFRetain before returning), which means it should be fully unnecessary to > > > CFRetain here (via DupOSCertHandle) > > > > Fixed. There was an extra CFRelease I was doing up above. > > I've patched this CL into my iOS build, and basic cronet tests still work! > Woo-hoo! > > To my surprise the binary size of executable is about 2mb smaller than with nss, > is that expected? I didn't realize it would be that drastic, but BoringSSL is much smaller than NSS, and much friendlier towards linkers dropping code. You can expect further savings when we lose the crypto/x509 dependency and that giant obj tables. :-) (That's a much more long-term thing though.)
> I've patched this CL into my iOS build, and basic cronet tests still work! > Woo-hoo! > > To my surprise the binary size of executable is about 2mb smaller than with nss, > is that expected? Yes. Because we lose about 1.5mb of root cert data.
On 2016/03/22 15:56:04, Ryan Sleevi wrote: > > I've patched this CL into my iOS build, and basic cronet tests still work! > > Woo-hoo! > > > > To my surprise the binary size of executable is about 2mb smaller than with > nss, > > is that expected? > > Yes. Because we lose about 1.5mb of root cert data. I think this is waiting on reviewers, and should be complete enough for the Chronet use case. Let me know if that's wrong.
Sorry, I didn't realize you were waiting for review feedback, I thought you were still working on stuff. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:49: OSStatus CreateTrustPolicies(const std::string& hostname, unused arg https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:59: ssl_policy = SecPolicyCreateSSL(true, NULL); Should this be nullptr? I'm guessing we can't use nil because we're .cc here (not .mm) https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:92: SecTrustResultType tmp_trust_result; naming wise, you don't need to keep the tmp_ name anymore, do you? You can just directly access/set the results, right? (Since there's no iterative looping) https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:169: void AppendPublicKeyHashes(CFArrayRef chain, HashValueVector* hashes) { It seems like it makes sense to fold this into GetCertChainInfo, which will reduce the amount of ASN.1 parsing overall that you have to do, and only iterate once, not twice. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:230: if (CFArrayGetCount(final_chain) > 0) { If the final_chain count == 0, we should be failing/erroring fast. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:248: if (!cert->VerifyNameMatch(hostname, For safety/robustness, even though it should be a noop, you should be using verify_result->verified_cert->VerifyNameMatch(...) https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.h (right): https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.h:6: #define NET_CERT_CERT_VERIFY_PROC_IOS_H_ Sorry that I wasn't clearer here - you'd need to rename all the files. That is, you want the class -> file name -> header declaration to be in sync. If there's a reason for the filename, then you should change the class name. Otherwise, all the files need to be updated. Concretely, I think these should all be CertVerifyProcIOS (cert_verify_proc_ios)
https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:49: OSStatus CreateTrustPolicies(const std::string& hostname, On 2016/03/24 18:50:24, Ryan Sleevi wrote: > unused arg Done. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:59: ssl_policy = SecPolicyCreateSSL(true, NULL); On 2016/03/24 18:50:24, Ryan Sleevi wrote: > Should this be nullptr? I'm guessing we can't use nil because we're .cc here > (not .mm) Done. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:92: SecTrustResultType tmp_trust_result; On 2016/03/24 18:50:24, Ryan Sleevi wrote: > naming wise, you don't need to keep the tmp_ name anymore, do you? You can just > directly access/set the results, right? (Since there's no iterative looping) We still need some temporary variables for this since we don't want to update the arguments unless we succeed to the end. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:169: void AppendPublicKeyHashes(CFArrayRef chain, HashValueVector* hashes) { On 2016/03/24 18:50:24, Ryan Sleevi wrote: > It seems like it makes sense to fold this into GetCertChainInfo, which will > reduce the amount of ASN.1 parsing overall that you have to do, and only iterate > once, not twice. Done. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:230: if (CFArrayGetCount(final_chain) > 0) { On 2016/03/24 18:50:24, Ryan Sleevi wrote: > If the final_chain count == 0, we should be failing/erroring fast. Done. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.cc:248: if (!cert->VerifyNameMatch(hostname, On 2016/03/24 18:50:24, Ryan Sleevi wrote: > For safety/robustness, even though it should be a noop, you should be using > verify_result->verified_cert->VerifyNameMatch(...) Done. https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_openssl_ios.h (right): https://codereview.chromium.org/1810153002/diff/360001/net/cert/cert_verify_p... net/cert/cert_verify_proc_openssl_ios.h:6: #define NET_CERT_CERT_VERIFY_PROC_IOS_H_ On 2016/03/24 18:50:24, Ryan Sleevi wrote: > Sorry that I wasn't clearer here - you'd need to rename all the files. > > That is, you want the class -> file name -> header declaration to be in sync. If > there's a reason for the filename, then you should change the class name. > Otherwise, all the files need to be updated. > > Concretely, I think these should all be CertVerifyProcIOS (cert_verify_proc_ios) Done.
LGTM % several nits https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios.cc (right): https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:12: #include "base/strings/sys_string_conversions.h" Unused? https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:17: #include "net/cert/crl_set.h" Unused/unneeded. https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:29: CFDictionaryRef*); Unused (it's for EV, no need to muck with that here anyways) https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:213: return ERR_ACCESS_DENIED; ERR_FAILED or ERR_CERT_INVALID seem more applicable here. https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:218: // TODO(svaldez): Add expiration handling. May be worth clarifying: // TODO(svaldez): Add specific error codes for expired/not-yet-valid certs. As it reads, it sounds like expiration isn't handled - it is, it just don't get a fancy status :) https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... File net/cert/x509_certificate_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:44: return sanity_check != NULL; nullptr? https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:164: return NULL; nullptr https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:179: if (!x509_cert.get()) You can drop the .get() [or should be able to, in a scoped_ptr world] https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:263: OSCertHandle cert_handle = SecCertificateCreateWithData(NULL, cert_data); nullptr https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:339: CFDataGetLength(a_data)) == 0; You don't need lines 334-339 for iOS. OS X < 10.6 had a bug with respect to CFEqual only comparing at the pointer type (that is, SecCertificateRef didn't have an entry in the CFTypeRef for overloading equality comparison). Since 10.6, CFEqual has implemented this logic - length comparison and then memcmp http://www.opensource.apple.com/source/Security/Security-55179.13/libsecurity... is what's relevant for iOS ( SecCertificateEqual ) So the whole function is DCHECK(a && b) return CFEqual(a, b); https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:372: if (!cert.get()) No .get() https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:441: if (!x509_cert.get()) No .get() https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:451: if (!intermediate_cert.get()) No .get() https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:475: if (!cert.get()) No .get()
https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios.cc (right): https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:12: #include "base/strings/sys_string_conversions.h" On 2016/03/24 20:17:42, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:17: #include "net/cert/crl_set.h" On 2016/03/24 20:17:42, Ryan Sleevi wrote: > Unused/unneeded. Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:29: CFDictionaryRef*); On 2016/03/24 20:17:42, Ryan Sleevi wrote: > Unused (it's for EV, no need to muck with that here anyways) Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:213: return ERR_ACCESS_DENIED; On 2016/03/24 20:17:42, Ryan Sleevi wrote: > ERR_FAILED or ERR_CERT_INVALID seem more applicable here. Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:218: // TODO(svaldez): Add expiration handling. On 2016/03/24 20:17:42, Ryan Sleevi wrote: > May be worth clarifying: > > // TODO(svaldez): Add specific error codes for expired/not-yet-valid certs. > > As it reads, it sounds like expiration isn't handled - it is, it just don't get > a fancy status :) Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... File net/cert/x509_certificate_openssl_ios.cc (right): https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:44: return sanity_check != NULL; On 2016/03/24 20:17:42, Ryan Sleevi wrote: > nullptr? Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:164: return NULL; On 2016/03/24 20:17:42, Ryan Sleevi wrote: > nullptr Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:179: if (!x509_cert.get()) On 2016/03/24 20:17:42, Ryan Sleevi wrote: > You can drop the .get() [or should be able to, in a scoped_ptr world] Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:263: OSCertHandle cert_handle = SecCertificateCreateWithData(NULL, cert_data); On 2016/03/24 20:17:42, Ryan Sleevi wrote: > nullptr Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:339: CFDataGetLength(a_data)) == 0; On 2016/03/24 20:17:42, Ryan Sleevi wrote: > You don't need lines 334-339 for iOS. > > OS X < 10.6 had a bug with respect to CFEqual only comparing at the pointer type > (that is, SecCertificateRef didn't have an entry in the CFTypeRef for > overloading equality comparison). Since 10.6, CFEqual has implemented this logic > - length comparison and then memcmp > > http://www.opensource.apple.com/source/Security/Security-55179.13/libsecurity... > is what's relevant for iOS ( SecCertificateEqual ) > > So the whole function is > > DCHECK(a && b) > return CFEqual(a, b); Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:372: if (!cert.get()) On 2016/03/24 20:17:42, Ryan Sleevi wrote: > No .get() Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:441: if (!x509_cert.get()) On 2016/03/24 20:17:42, Ryan Sleevi wrote: > No .get() Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:451: if (!intermediate_cert.get()) On 2016/03/24 20:17:42, Ryan Sleevi wrote: > No .get() Done. https://codereview.chromium.org/1810153002/diff/400001/net/cert/x509_certific... net/cert/x509_certificate_openssl_ios.cc:475: if (!cert.get()) On 2016/03/24 20:17:42, Ryan Sleevi wrote: > No .get() Done.
The CQ bit was checked by svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1810153002/#ps420001 (title: "Fixing more nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1810153002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810153002/420001
Message was sent while issue was closed.
Description was changed from ========== This adds the OpenSSL-specific implementations for iOS, using SecTrustEvaluate in order to determine the validity of the certificate chain. BUG=591545 ========== to ========== This adds the OpenSSL-specific implementations for iOS, using SecTrustEvaluate in order to determine the validity of the certificate chain. BUG=591545 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== This adds the OpenSSL-specific implementations for iOS, using SecTrustEvaluate in order to determine the validity of the certificate chain. BUG=591545 ========== to ========== This adds the OpenSSL-specific implementations for iOS, using SecTrustEvaluate in order to determine the validity of the certificate chain. BUG=591545 Committed: https://crrev.com/864f9468ae2a8d1ba95c64824ef2caf05b7121fc Cr-Commit-Position: refs/heads/master@{#383297} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/864f9468ae2a8d1ba95c64824ef2caf05b7121fc Cr-Commit-Position: refs/heads/master@{#383297}
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:420001) has been created in https://codereview.chromium.org/1838513002/ by rockot@chromium.org. The reason for reverting is: Broken iOS build: https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/44491/st... Let's try relanding with the potential fix from https://codereview.chromium.org/1834583006 merged in..
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:420001) has been created in https://codereview.chromium.org/1838523002/ by svaldez@chromium.org. The reason for reverting is: The tree breaks on iOS Device/Simulator due to included data files..
Message was sent while issue was closed.
On 2016/03/25 18:01:10, svaldez wrote: > A revert of this CL (patchset #23 id:420001) has been created in > https://codereview.chromium.org/1838523002/ by mailto:svaldez@chromium.org. > > The reason for reverting is: The tree breaks on iOS Device/Simulator due to > included data files.. Duplicated reverts, letting Ken's revert run.
Message was sent while issue was closed.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1810153002/diff/420001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1810153002/diff/420001/net/BUILD.gn#newcode1396 net/BUILD.gn:1396: "data/verify_name_match_unittest/names", You need to add the following here too if you want the tests to eventually work on iOS when building with gn (without the trailing "/", sorry about that, I have a bug to address this): "third_party/nist-pkits",
Message was sent while issue was closed.
https://codereview.chromium.org/1810153002/diff/420001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1810153002/diff/420001/net/BUILD.gn#newcode1396 net/BUILD.gn:1396: "data/verify_name_match_unittest/names", On 2016/03/25 21:46:36, sdefresne (OOO till 4th April) wrote: > You need to add the following here too if you want the tests to eventually work > on iOS when building with gn (without the trailing "/", sorry about that, I have > a bug to address this): > > "third_party/nist-pkits", Will fix that as well when re-adding the nist-pkits test files. For some reason gyp builds fail only on the Waterfall when changing those dependencies.
Message was sent while issue was closed.
On 2016/03/28 at 14:19:50, svaldez wrote: > https://codereview.chromium.org/1810153002/diff/420001/net/BUILD.gn > File net/BUILD.gn (right): > > https://codereview.chromium.org/1810153002/diff/420001/net/BUILD.gn#newcode1396 > net/BUILD.gn:1396: "data/verify_name_match_unittest/names", > On 2016/03/25 21:46:36, sdefresne (OOO till 4th April) wrote: > > You need to add the following here too if you want the tests to eventually work > > on iOS when building with gn (without the trailing "/", sorry about that, I have > > a bug to address this): > > > > "third_party/nist-pkits", > > Will fix that as well when re-adding the nist-pkits test files. For some reason gyp builds fail only on the Waterfall when changing those dependencies. This is probably because we do not run xcodebuild bots on the trybots (due to lack of resources) but still run them on the waterfall :-( |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
