Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11316210:
Implement TestRootCerts for Android (Closed)
|
Created:
8 years ago by ppi Modified:
8 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, ilevy+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, darin-cc_chromium.org, peter+watch_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement TestRootCerts for Android
Currently we are unable to plant our test root certificate in Android trust store to run our certificate verification tests against it.
The patch adds custom test trust store to the java backend, allowing to run the certificate verification tests against custom set of root certificates.
The actual installation of our test root certificate is being done by TestRootCerts class, consistently with what we do on other platforms.
BUG=147786
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172488
Patch Set 1 : #
Total comments: 12
Patch Set 2 : address digit remarks #Patch Set 3 : fix indents/line widths/style following pliard advice #Patch Set 4 : Further style fixes. #
Total comments: 2
Patch Set 5 : address digit comment (thread safety) #
Total comments: 2
Patch Set 6 : address digit remark (thread-safety) #
Total comments: 2
Patch Set 7 : lock on a final separate lock object #Patch Set 8 : remove an obsolete findbugs suppression #
Total comments: 4
Patch Set 9 : address Ryan remarks #
Total comments: 8
Patch Set 10 : make test trust store reload on test key store modifications #Patch Set 11 : address Ryan's remark #Patch Set 12 : rebase + minor fix in comments #
Total comments: 3
Patch Set 13 : address Ryan's remarks #
Messages
Total messages: 25 (0 generated)
I have prepared a preliminary cl as we discussed. I would be happy to know what you think. Please note that it is still a draft - it compiles, but I did not test it yet. Tomorrow I will set up ssh tunneling and debug/check the patch.
https://codereview.chromium.org/11316210/diff/9001/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/9001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:127: try { I think it'd be preferable to separate the usage of the default and local trust manager. What I mean is that, during tests, we probably don't want to use the default trust manager at all. What about adding a static flag that is set when any of AddLocal.../ClearAllLocal.. is being called, and test it here to select the trust manager? https://codereview.chromium.org/11316210/diff/9001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:133: eInner.getMessage()); Doesn't this lose the exception message from the default trust manager? Is this important? https://codereview.chromium.org/11316210/diff/9001/net/android/network_library.h File net/android/network_library.h (right): https://codereview.chromium.org/11316210/diff/9001/net/android/network_librar... net/android/network_library.h:38: void AddLocalRootCertificate(const uint8* cert, size_t len); I think it'd prefer if we called this AddTestRootCertificate() to make it clear that this should only be used during testing. Same with ClearLocalRootCertificates -> ClearTestRootCertificates. https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs.h File net/base/test_root_certs.h (right): https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs.h... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) USE_OPENSSL is always defined for Android, so this should not be needed here. (If not, it's probably a bug in a .gyp/.gypi file). https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs_a... File net/base/test_root_certs_android.cc (right): https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs_a... net/base/test_root_certs_android.cc:18: net::android::AddLocalRootCertificate((const uint8*)cert_bytes.data(), I think the Chrome team prefers the use of C++ style casts, which would mean replacing this with: reinterpret_cast<const uint8*>(cert_bytes.data()) https://codereview.chromium.org/11316210/diff/9001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/11316210/diff/9001/net/net.gyp#newcode275 net/net.gyp:275: 'base/test_root_certs_android.cc', Adding this here is ok, but you also need a 'sources!' statement below to remove it from the sources list when not building for Android (.e.g in the OS != android section).
Thanks for the remarks, David. Patch set 2 addresses the issues you have pointed out. Please let me know if you have any further remarks. https://codereview.chromium.org/11316210/diff/9001/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/9001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:127: try { Sounds right, thanks. In patch set 2 I use additional reference pointing either to the local or to the system trust manager. Please let me know what do you think. On 2012/11/28 10:42:34, digit1 wrote: > I think it'd be preferable to separate the usage of the default and local trust > manager. What I mean is that, during tests, we probably don't want to use the > default trust manager at all. > > What about adding a static flag that is set when any of > AddLocal.../ClearAllLocal.. is being called, and test it here to select the > trust manager? https://codereview.chromium.org/11316210/diff/9001/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:133: eInner.getMessage()); Thanks, n/a in patch set 2. On 2012/11/28 10:42:34, digit1 wrote: > Doesn't this lose the exception message from the default trust manager? Is this > important? https://codereview.chromium.org/11316210/diff/9001/net/android/network_library.h File net/android/network_library.h (right): https://codereview.chromium.org/11316210/diff/9001/net/android/network_librar... net/android/network_library.h:38: void AddLocalRootCertificate(const uint8* cert, size_t len); Sure - done in patch set 2 (both in Java as well as C++ code). On 2012/11/28 10:42:34, digit1 wrote: > I think it'd prefer if we called this AddTestRootCertificate() to make it clear > that this should only be used during testing. Same with > ClearLocalRootCertificates -> ClearTestRootCertificates. https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs.h File net/base/test_root_certs.h (right): https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs.h... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) I wonder if relying on the USE_OPENSSL flag being defined on Android in this case is not unnecessary coupling (what if we drop OpenSSL on Android in the future?). Also, shouldn't it be possible to read the code of test_root_certs.h in the context of test_root_certs_android.cc without the knowledge about the USE_OPENSSL flag being defined on Android? Please let me know what do you think. On 2012/11/28 10:42:34, digit1 wrote: > USE_OPENSSL is always defined for Android, so this should not be needed here. > (If not, it's probably a bug in a .gyp/.gypi file). https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs_a... File net/base/test_root_certs_android.cc (right): https://codereview.chromium.org/11316210/diff/9001/net/base/test_root_certs_a... net/base/test_root_certs_android.cc:18: net::android::AddLocalRootCertificate((const uint8*)cert_bytes.data(), Thanks, fixed in patch set 2. On 2012/11/28 10:42:34, digit1 wrote: > I think the Chrome team prefers the use of C++ style casts, which would mean > replacing this with: > > reinterpret_cast<const uint8*>(cert_bytes.data()) https://codereview.chromium.org/11316210/diff/9001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/11316210/diff/9001/net/net.gyp#newcode275 net/net.gyp:275: 'base/test_root_certs_android.cc', As we have discussed, it should be ok without explicit suppresion on non-Android platforms. On 2012/11/28 10:42:34, digit1 wrote: > Adding this here is ok, but you also need a 'sources!' statement below to remove > it from the sources list when not building for Android (.e.g in the OS != > android section).
I have uploaded patch set 3, which fixes some style issues pointed out by Philippe (thanks!).
https://codereview.chromium.org/11316210/diff/1020/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/1020/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:104: sTrustManager = sLocalTrustManager; Ah, I should have noticed before that changes to the static variables should be protected by a lock. In the old code, ensureInitialized() is synchronized and the variables are never modified after initialization, so multi-thread usage is ok. To solve this, I'd suggest to use a common object to synchronize any calls that may change or use the static variables, since they are now mutable after initialization.
https://codereview.chromium.org/11316210/diff/1020/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/1020/net/android/java/src/org/c... net/android/java/src/org/chromium/net/X509Util.java:104: sTrustManager = sLocalTrustManager; Good catch, thanks! I have synchronized all modifications of the mutable state in patch set 5. Please let me know if you have any further remarks. On 2012/11/28 16:18:30, digit1 wrote: > Ah, I should have noticed before that changes to the static variables should be > protected by a lock. > > In the old code, ensureInitialized() is synchronized and the variables are never > modified after initialization, so multi-thread usage is ok. > > To solve this, I'd suggest to use a common object to synchronize any calls that > may change or use the static variables, since they are now mutable after > initialization.
https://codereview.chromium.org/11316210/diff/12003/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/12003/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:48: private static synchronized void ensureInitialized() throws CertificateException, You need to synchronize this function around the same lock/object. Otherwise one thread could call ensureInitialized() while another thread is inside a synchronized(sTrustManager), and both threads would end up using/modifying the same values. One way to do that it to make ensureInitialized() not synchronized at all, and call it inside a synchronized(sTrustManager) block in the callers.
https://codereview.chromium.org/11316210/diff/12003/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/12003/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:48: private static synchronized void ensureInitialized() throws CertificateException, Aw, my mistake, thanks! In patch set 6, I made ensureInitialized() synchronize on the same object as the other calls. I feel that the function should remain synchronized, so that it is thread-safe by itself, regardless of how it is called. Please let me know if anything still needs to be addressed. On 2012/11/29 13:28:31, digit1 wrote: > You need to synchronize this function around the same lock/object. Otherwise one > thread could call ensureInitialized() while another thread is inside a > synchronized(sTrustManager), and both threads would end up using/modifying the > same values. > > One way to do that it to make ensureInitialized() not synchronized at all, and > call it inside a synchronized(sTrustManager) block in the callers.
Following pliard remark, I have uploaded patch set 7, in which I lock on a separate final lock object.
https://chromiumcodereview.appspot.com/11316210/diff/10009/net/android/java/s... File net/android/java/src/org/chromium/net/X509Util.java (right): https://chromiumcodereview.appspot.com/11316210/diff/10009/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:50: synchronized(sTrustManager) { Ah, this will not work if sTrustManager is null, you need to be sure that the lock object is already created. There are several ways to do that: 1/ Use a statically initialized object as the lock: private static Object lock = new Object(); ... ... synchronized(lock) { } 2/ Use the class object itself (slower, but avoids slowing down class loading): synchronized (getClass()) { ... } 3/ Perform lazy initialization with a volatile lock object. private static volatile Object lock; Object getLock() { if (lock == null) { synchronized(getClass()) { lock = Object(); } } return lock; } ... synchronized(getLock()) { ... } I'd recommend using 1/, since it's the simpler way :)
https://chromiumcodereview.appspot.com/11316210/diff/10009/net/android/java/s... File net/android/java/src/org/chromium/net/X509Util.java (right): https://chromiumcodereview.appspot.com/11316210/diff/10009/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:50: synchronized(sTrustManager) { Absolutely, thanks - fixed (1/) in patch set 7. On 2012/11/29 14:35:53, digit1 wrote: > Ah, this will not work if sTrustManager is null, you need to be sure that the > lock object is already created. There are several ways to do that: > > 1/ Use a statically initialized object as the lock: > > private static Object lock = new Object(); > > ... > > ... synchronized(lock) { > } > > 2/ Use the class object itself (slower, but avoids slowing down class > loading): > > synchronized (getClass()) { > ... > } > > 3/ Perform lazy initialization with a volatile lock object. > > private static volatile Object lock; > > Object getLock() { > if (lock == null) { > synchronized(getClass()) { > lock = Object(); > } > } > return lock; > } > > ... synchronized(getLock()) { > ... > } > > I'd recommend using 1/, since it's the simpler way :)
lgtm. By the way, I've launched a try on the "linux_redux" bot to check that this doesn't break the Linux + openssl build too. I recommend doing so for any patch that touches code that uses OpenSSL. https://chromiumcodereview.appspot.com/11316210/diff/17/net/base/test_root_ce... File net/base/test_root_certs.h (right): https://chromiumcodereview.appspot.com/11316210/diff/17/net/base/test_root_ce... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) nit: I don't think this is necessary, since USE_OPENSSL is always defined on Android.
Mostly looks correct, but I have one design question below: https://codereview.chromium.org/11316210/diff/17/net/android/java/src/org/chr... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/17/net/android/java/src/org/chr... net/android/java/src/org/chromium/net/X509Util.java:43: private static X509TrustManager sTrustManager; Note: On all other platforms, TestRootCerts is additive to system trust - that is, system trust + local trust. As I understand this class to be implemented, you've done it exclusive - EITHER system trust OR app trust. This does not match the other platforms. https://codereview.chromium.org/11316210/diff/17/net/base/test_root_certs.h File net/base/test_root_certs.h (right): https://codereview.chromium.org/11316210/diff/17/net/base/test_root_certs.h#n... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) On 2012/11/30 10:05:25, digit1 wrote: > nit: I don't think this is necessary, since USE_OPENSSL is always defined on > Android. agreed
https://chromiumcodereview.appspot.com/11316210/diff/17/net/android/java/src/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://chromiumcodereview.appspot.com/11316210/diff/17/net/android/java/src/... net/android/java/src/org/chromium/net/X509Util.java:43: private static X509TrustManager sTrustManager; Ah, my bad, I was the one advising ppi to implement it this way. Hopefully should be easy to fix.
LGTM, mod the #ifdef nit below and provided that the answer to my question is "It's a non issue" https://codereview.chromium.org/11316210/diff/19001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/19001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:137: sTestTrustManager.checkServerTrusted(serverCertificates, authType); My ignorance in Java is showing here, but does the sTestKeyStore also consider/include system keys? Or is it an empty key store? The only way this would become an issue is if sDefaultTrustManager also had an intermediate certificate store that checkServerTrusted could use for path building, but not trust. If that's not the case, then I agree, this implementation should be fine. https://codereview.chromium.org/11316210/diff/19001/net/base/test_root_certs.h File net/base/test_root_certs.h (right): https://codereview.chromium.org/11316210/diff/19001/net/base/test_root_certs.... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) Isn't this redundant here - USE_OPENSSL is always true for OS_ANDROID
https://codereview.chromium.org/11316210/diff/19001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/X509Util.java (right): https://codereview.chromium.org/11316210/diff/19001/net/android/java/src/org/... net/android/java/src/org/chromium/net/X509Util.java:137: sTestTrustManager.checkServerTrusted(serverCertificates, authType); The Java and Android API documentation is not very clear, so I looked at the platform sources for details. A KeyStore is a pretty generic abstract interface, and several implementations exist, differentiated by a "type" string. KeyStore.getDefaultType() returns "jks", which is a file-backed KeyStore implementation, while the Android system keystore (which runs in a separate process) uses the "AndroidKeyStore" type. So in the end, the test keystore in this existing code is completely empty, and thus separated from the platform one. I'm curious why we'd want to use platform certificate during unit testing though? Isn't this bound to be sort of flaky? I understand that we might want to check that a really well-known public CA certificate is installed on the test device, but this would rather use a specialized test, rather than the net::TestRootCerts class. Or maybe there is a use case I don't envision?
Thanks for the remarks, Ryan and David! And I am sorry for the delay - I was loaded with orientation classes last week. I reply to issues pointed out by Ryan below. I also uploaded patch set 10, as it turned out that KeyStore underlying the TrustManager is >not< stored by reference, and the trust manager has to be reloaded after each modification. I have verified that this implementation works - it fixes 7 tests straight away, the remaining ones fail as they rely on details of verification results that are simply not available on Android. I do not enable any of newly fixed tests in this CL, as they need https://code.google.com/p/chromium/issues/detail?id=164180 to be resolved earlier. Please let me know if there are any remaining issues in patch set 10 that need to be addressed in this CL. https://chromiumcodereview.appspot.com/11316210/diff/19001/net/android/java/s... File net/android/java/src/org/chromium/net/X509Util.java (right): https://chromiumcodereview.appspot.com/11316210/diff/19001/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:137: sTestTrustManager.checkServerTrusted(serverCertificates, authType); On 2012/12/07 10:28:11, digit1 wrote: > The Java and Android API documentation is not very clear, so I looked at the > platform sources for details. A KeyStore is a pretty generic abstract interface, > and several implementations exist, differentiated by a "type" string. > > KeyStore.getDefaultType() returns "jks", which is a file-backed KeyStore > implementation, while the Android system keystore (which runs in a separate > process) uses the "AndroidKeyStore" type. > > So in the end, the test keystore in this existing code is completely empty, and > thus separated from the platform one. > > I'm curious why we'd want to use platform certificate during unit testing > though? Isn't this bound to be sort of flaky? I understand that we might want to > check that a really well-known public CA certificate is installed on the test > device, but this would rather use a specialized test, rather than the > net::TestRootCerts class. > > Or maybe there is a use case I don't envision? That's an interesting remark, Ryan. I agree with David that the Java docs are far from clear on these issues... TrustManagerFactory interface clearly does not expose any way of feeding intermediate cert store. Moreover, KeyStore itself stores (all) its certificates in instances of TrustedCertificateEntry - my working hypothesis is that that Java does not support intermediate cert stores (in the context of trust managers) at all - does it sound probable? I second David's concern that relying on system trust roots in tests seems controversial (I am not aware of any tests doing so). https://chromiumcodereview.appspot.com/11316210/diff/19001/net/base/test_root... File net/base/test_root_certs.h (right): https://chromiumcodereview.appspot.com/11316210/diff/19001/net/base/test_root... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) On 2012/12/07 03:25:26, Ryan Sleevi wrote: > Isn't this redundant here - USE_OPENSSL is always true for OS_ANDROID David already pointed this out earlier (so that's 2:1 in favor of removal). My rationale for introducing the condition is the following: "I wonder if relying on the USE_OPENSSL flag being defined on Android in this case is not unnecessary coupling (what if we drop OpenSSL on Android in the future?). Also, shouldn't it be possible to read the code of test_root_certs.h in the context of test_root_certs_android.cc without the knowledge about the USE_OPENSSL flag being defined on Android?" Please let me know if that seems convincing to you.
https://chromiumcodereview.appspot.com/11316210/diff/19001/net/android/java/s... File net/android/java/src/org/chromium/net/X509Util.java (right): https://chromiumcodereview.appspot.com/11316210/diff/19001/net/android/java/s... net/android/java/src/org/chromium/net/X509Util.java:137: sTestTrustManager.checkServerTrusted(serverCertificates, authType); On 2012/12/11 04:24:21, ppi wrote: > On 2012/12/07 10:28:11, digit1 wrote: > > The Java and Android API documentation is not very clear, so I looked at the > > platform sources for details. A KeyStore is a pretty generic abstract > interface, > > and several implementations exist, differentiated by a "type" string. > > > > KeyStore.getDefaultType() returns "jks", which is a file-backed KeyStore > > implementation, while the Android system keystore (which runs in a separate > > process) uses the "AndroidKeyStore" type. > > > > So in the end, the test keystore in this existing code is completely empty, > and > > thus separated from the platform one. > > > > I'm curious why we'd want to use platform certificate during unit testing > > though? Isn't this bound to be sort of flaky? I understand that we might want > to > > check that a really well-known public CA certificate is installed on the test > > device, but this would rather use a specialized test, rather than the > > net::TestRootCerts class. > > > > Or maybe there is a use case I don't envision? > > That's an interesting remark, Ryan. I agree with David that the Java docs are > far from clear on these issues... > > TrustManagerFactory interface clearly does not expose any way of feeding > intermediate cert store. Moreover, KeyStore itself stores (all) its certificates > in instances of TrustedCertificateEntry - my working hypothesis is that that > Java does not support intermediate cert stores (in the context of trust > managers) at all - does it sound probable? > > I second David's concern that relying on system trust roots in tests seems > controversial (I am not aware of any tests doing so). The goal here with TestRootCerts is to make sure that, as consistently as possible, the 'native' code path is being executed (in particular, chain building and configuration) It has less to do with trusting system keys (we don't want unit tests to depend on external configuration), but DOES have to do with making sure that we're exercising the same codepaths (that the test roots are not just some simple 'return true') The Java abstraction may separate out these concepts, but on the other platforms, it doesn't, hence why we intentionally "plug into" the system implementation rather than "replace". https://chromiumcodereview.appspot.com/11316210/diff/19001/net/base/test_root... File net/base/test_root_certs.h (right): https://chromiumcodereview.appspot.com/11316210/diff/19001/net/base/test_root... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) On 2012/12/11 04:24:21, ppi wrote: > On 2012/12/07 03:25:26, Ryan Sleevi wrote: > > Isn't this redundant here - USE_OPENSSL is always true for OS_ANDROID > > David already pointed this out earlier (so that's 2:1 in favor of removal). > > My rationale for introducing the condition is the following: > > "I wonder if relying on the USE_OPENSSL flag being defined on Android in this > case is not unnecessary coupling (what if we drop OpenSSL on Android in the > future?). Whenever such a comment is coupled with "more code", I tend to be resistant. "What ifs" can easily be used to justify a lot. > > Also, shouldn't it be possible to read the code of test_root_certs.h in the > context of test_root_certs_android.cc without the knowledge about the > USE_OPENSSL flag being defined on Android?" To date, we've hidden the fact that Android is using OpenSSL (except where it isn't) by simply using USE_OPENSSL as the #define, and using code substitution (via .gyp). In general, I'm in favour of exposing as few details as possible about the implementation within the code itself, and letting the .gyp rules hold as much as possible. So I'm not sure this #ifdef really adds much. > > Please let me know if that seems convincing to you.
https://codereview.chromium.org/11316210/diff/19001/net/base/test_root_certs.h File net/base/test_root_certs.h (right): https://codereview.chromium.org/11316210/diff/19001/net/base/test_root_certs.... net/base/test_root_certs.h:94: #if defined(OS_WIN) || defined(USE_OPENSSL) || defined(OS_ANDROID) On 2012/12/11 17:40:10, Ryan Sleevi wrote: > On 2012/12/11 04:24:21, ppi wrote: > > On 2012/12/07 03:25:26, Ryan Sleevi wrote: > > > Isn't this redundant here - USE_OPENSSL is always true for OS_ANDROID > > > > David already pointed this out earlier (so that's 2:1 in favor of removal). > > > > My rationale for introducing the condition is the following: > > > > "I wonder if relying on the USE_OPENSSL flag being defined on Android in this > > case is not unnecessary coupling (what if we drop OpenSSL on Android in the > > future?). > > Whenever such a comment is coupled with "more code", I tend to be resistant. > "What ifs" can easily be used to justify a lot. > > > > > Also, shouldn't it be possible to read the code of test_root_certs.h in the > > context of test_root_certs_android.cc without the knowledge about the > > USE_OPENSSL flag being defined on Android?" > > To date, we've hidden the fact that Android is using OpenSSL (except where it > isn't) by simply using USE_OPENSSL as the #define, and using code substitution > (via .gyp). > > In general, I'm in favour of exposing as few details as possible about the > implementation within the code itself, and letting the .gyp rules hold as much > as possible. > > So I'm not sure this #ifdef really adds much. > > > > > Please let me know if that seems convincing to you. > Removed the condition in patch set 11.
LGTM, mod some BUGs below https://codereview.chromium.org/11316210/diff/38002/net/android/network_libra... File net/android/network_library.h (right): https://codereview.chromium.org/11316210/diff/38002/net/android/network_libra... net/android/network_library.h:39: void AddTestRootCertificate(const uint8* cert, size_t len); I never understood why Android doesn't make more use of StringPiece here for these types. Not a big deal, and at least it's consistent with the rest of the file. https://codereview.chromium.org/11316210/diff/38002/net/base/test_root_certs_... File net/base/test_root_certs_android.cc (right): https://codereview.chromium.org/11316210/diff/38002/net/base/test_root_certs_... net/base/test_root_certs_android.cc:17: certificate->os_cert_handle(), &cert_bytes); BUG: Check return value here BUG: You've redundantly used X509Certificate:: twice STYLE: No need to use net:: prefix here (or line 27) if (!X509Certificate::GetDEREncoded(certificate()->os_cert_handle(), &cert_bytes)) return false; android::AddTestRootCertificate(...)
Thanks for the remarks, Ryan! I have addressed the bugs/style remarks and unless anybody points out further issues, I will proceed with commit later today. https://codereview.chromium.org/11316210/diff/38002/net/base/test_root_certs_... File net/base/test_root_certs_android.cc (right): https://codereview.chromium.org/11316210/diff/38002/net/base/test_root_certs_... net/base/test_root_certs_android.cc:17: certificate->os_cert_handle(), &cert_bytes); On 2012/12/11 20:18:38, Ryan Sleevi wrote: > BUG: Check return value here > BUG: You've redundantly used X509Certificate:: twice > STYLE: No need to use net:: prefix here (or line 27) > > if (!X509Certificate::GetDEREncoded(certificate()->os_cert_handle(), > &cert_bytes)) > return false; > android::AddTestRootCertificate(...) Thanks! Fixed in patch set 13.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ppi@chromium.org/11316210/33002
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ppi@chromium.org/11316210/33002
Message was sent while issue was closed.
Change committed as 172488 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
