Update net/android/network_library.cc with fresher code.

Update net/android/network_library.cc with fresher code.

Also adds it to the build to ensure it doesn't suffer bit-rot again, and
include Java-side component.


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

[Yaron, 2012-04-20 19:52:16]

Follow-up from comment on http://codereview.chromium.org/10073024/

[John Grabowski, 2012-04-20 20:01:33]

LGTM

[Ryan Sleevi, 2012-04-20 20:55:39]

LGTM, with nits.

http://codereview.chromium.org/10171009/diff/1/base/android/java/org/chromium...
File base/android/java/org/chromium/base/CalledByNativeUnchecked.java (right):

http://codereview.chromium.org/10171009/diff/1/base/android/java/org/chromium...
base/android/java/org/chromium/base/CalledByNativeUnchecked.java:14: // such as
NPE, so the native code should differentiate these cases.
NPE?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
File net/android/java/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:37: // TODO(joth):
Use KeyChain API instead of hard-coding constants here: http://b/5859651
Public bug for public code?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:63: * certificates
format.
nit: The ASN.1 DER encoded bytes for certificates

I don't see where 'typically' makes sense?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:64: * @param
authType The authentication type for the cert chain
I don't know if this explanation really adds much. Is this the policy for which
the server certificate is being validated against (eg: TLS server vs... XMPP
server?)

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:73: throw new
IllegalArgumentException("bad certificate chain");
nit: We try to write meaning error messages

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:80:
(X509Certificate) sCertificateFactory.get().generateCertificate(
sCertificateFactory may be NULL here if ensureInitialized failed.

Should you be wrapping the larger code in a try / catch with a return false? Is
that antithetical to the Java way?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:100: private static
void ensureInitialized()
nit: Document, especially since this can throw?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:105:
sDefaultTrustManager.compareAndSet(null, createDefaultTrustManager());
If createDefaultTrustManager() returns null, then you'll needlessly be spinning
between this, createDefaultTrustManager, and findX509TrustManager, right?

Should you instead throw at this point?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:112: private static
X509TrustManager createDefaultTrustManager()
nit: Document, especially since this can throw?

http://codereview.chromium.org/10171009/diff/1/net/android/network_library.cc
File net/android/network_library.cc (right):

http://codereview.chromium.org/10171009/diff/1/net/android/network_library.cc...
net/android/network_library.cc:26: // TODO(joth): remove |hostname| as this
method no longer verifies it.
nit: File a cleanup bug?

[Yaron, 2012-04-23 22:21:28]

Comments addressed and I went ahead and just removed |hostname| and re-enabled
the caller. This caused one additional set of tests to fail due to lack of JNI,
but that should be coming online for net within 1-2 weeks.

rsleevi: Thanks, and PTAL

http://codereview.chromium.org/10171009/diff/1/base/android/java/org/chromium...
File base/android/java/org/chromium/base/CalledByNativeUnchecked.java (right):

http://codereview.chromium.org/10171009/diff/1/base/android/java/org/chromium...
base/android/java/org/chromium/base/CalledByNativeUnchecked.java:14: // such as
NPE, so the native code should differentiate these cases.
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> NPE?

Done.

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
File net/android/java/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:37: // TODO(joth):
Use KeyChain API instead of hard-coding constants here: http://b/5859651
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> Public bug for public code?

Done.

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:63: * certificates
format.
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> nit: The ASN.1 DER encoded bytes for certificates
> 
> I don't see where 'typically' makes sense?

Done.

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:64: * @param
authType The authentication type for the cert chain
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> I don't know if this explanation really adds much. Is this the policy for
which
> the server certificate is being validated against (eg: TLS server vs... XMPP
> server?) 

Done.

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:73: throw new
IllegalArgumentException("bad certificate chain");
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> nit: We try to write meaning error messages

Done.

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:80:
(X509Certificate) sCertificateFactory.get().generateCertificate(
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> sCertificateFactory may be NULL here if ensureInitialized failed.
> 
> Should you be wrapping the larger code in a try / catch with a return false?
Is
> that antithetical to the Java way?

How does ensureInitialized fail? If CertificateFactory.getInstance fails? Seems
like that's a bug that we'd deal with as a crash report instead of silently
failing later, doesn't it?

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:100: private static
void ensureInitialized()
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> nit: Document, especially since this can throw?

This is private but done. Also not really sure about a "meaningful" comment..

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:105:
sDefaultTrustManager.compareAndSet(null, createDefaultTrustManager());
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> If createDefaultTrustManager() returns null, then you'll needlessly be
spinning
> between this, createDefaultTrustManager, and findX509TrustManager, right?
> 
> Should you instead throw at this point?

I don't see that. It looks like verifyServerCertificates would crash when it
calls checkServerTrusted

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:112: private static
X509TrustManager createDefaultTrustManager()
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> nit: Document, especially since this can throw?

Done.

http://codereview.chromium.org/10171009/diff/1/net/android/network_library.cc
File net/android/network_library.cc (right):

http://codereview.chromium.org/10171009/diff/1/net/android/network_library.cc...
net/android/network_library.cc:26: // TODO(joth): remove |hostname| as this
method no longer verifies it.
On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> nit: File a cleanup bug?

Did it instead

[Ryan Sleevi, 2012-04-23 22:27:18]

lgtm

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
File net/android/java/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:80:
(X509Certificate) sCertificateFactory.get().generateCertificate(
On 2012/04/23 22:21:29, Yaron wrote:
> On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> > sCertificateFactory may be NULL here if ensureInitialized failed.
> > 
> > Should you be wrapping the larger code in a try / catch with a return false?
> Is
> > that antithetical to the Java way?
> 
> How does ensureInitialized fail? If CertificateFactory.getInstance fails?
Seems
> like that's a bug that we'd deal with as a crash report instead of silently
> failing later, doesn't it?

If the crash report is meaningful, sure. I just wanted to make sure to point out
the possibility that it might be NULL here, and make sure that was acceptable.

http://codereview.chromium.org/10171009/diff/8001/build/android/gtest_filter/...
File build/android/gtest_filter/net_unittests_disabled (right):

http://codereview.chromium.org/10171009/diff/8001/build/android/gtest_filter/...
build/android/gtest_filter/net_unittests_disabled:90: DiskCacheEntryTest.*
Really?

What test(s)? Just the mere act of Deserializing the cert is a JNI dependence?

This just really surprises me, and I almost wonder if it's a bug.

[Yaron, 2012-04-23 22:32:18]

http://codereview.chromium.org/10171009/diff/8001/build/android/gtest_filter/...
File build/android/gtest_filter/net_unittests_disabled (right):

http://codereview.chromium.org/10171009/diff/8001/build/android/gtest_filter/...
build/android/gtest_filter/net_unittests_disabled:90: DiskCacheEntryTest.*
On 2012/04/23 22:27:18, Ryan Sleevi wrote:
> Really?
> 
> What test(s)? Just the mere act of Deserializing the cert is a JNI dependence?
> 
> This just really surprises me, and I almost wonder if it's a bug.

It does seem kind of strange. Here's the logcat:

W/chromium( 3138):
[3138:3138:0423/221559:16724630589:WARNING:network_library.cc(31)] JNI
initialization failed
W/chromium( 3138):
[3138:3138:0423/221559:16724650353:WARNING:network_library.cc(31)] JNI
initialization failed
W/chromium( 3138):
[3138:3138:0423/221559:16724663617:WARNING:network_library.cc(31)] JNI
initialization failed
E/chromium( 3138):
[3138:3138:0423/221559:16724665793:ERROR:cert_verify_proc_openssl.cc(253)] X509
Verification error certificate signature failure : 7 : 1 : 128


So the call to CertVerifyProcOpenSSL::VerifyInternal is failing. Should it not
be called in this case? The first test I tried was
DiskCacheEntryTest.InternalSyncIO

[Ryan Sleevi, 2012-04-23 22:40:23]

On 2012/04/23 22:32:18, Yaron wrote:
>
http://codereview.chromium.org/10171009/diff/8001/build/android/gtest_filter/...
> File build/android/gtest_filter/net_unittests_disabled (right):
> 
>
http://codereview.chromium.org/10171009/diff/8001/build/android/gtest_filter/...
> build/android/gtest_filter/net_unittests_disabled:90: DiskCacheEntryTest.*
> On 2012/04/23 22:27:18, Ryan Sleevi wrote:
> > Really?
> > 
> > What test(s)? Just the mere act of Deserializing the cert is a JNI
dependence?
> > 
> > This just really surprises me, and I almost wonder if it's a bug.
> 
> It does seem kind of strange. Here's the logcat:
> 
> W/chromium( 3138):
> [3138:3138:0423/221559:16724630589:WARNING:network_library.cc(31)] JNI
> initialization failed
> W/chromium( 3138):
> [3138:3138:0423/221559:16724650353:WARNING:network_library.cc(31)] JNI
> initialization failed
> W/chromium( 3138):
> [3138:3138:0423/221559:16724663617:WARNING:network_library.cc(31)] JNI
> initialization failed
> E/chromium( 3138):
> [3138:3138:0423/221559:16724665793:ERROR:cert_verify_proc_openssl.cc(253)]
X509
> Verification error certificate signature failure : 7 : 1 : 128
> 
> 
> So the call to CertVerifyProcOpenSSL::VerifyInternal is failing. Should it not
> be called in this case? The first test I tried was
> DiskCacheEntryTest.InternalSyncIO

No, not as far as I understand it.

Short of DiskCacheBasedSSLHostInfo.*, there should be no calls to cert
verification. We use the verification status at the time of deserialization -
otherwise there could be all sorts of weirdness. Heck, I didn't even think the
disk cache took a CertVerifier - and if it did, it should be using a
MockCertVerifier, not the real one.

Not to hold up landing this CL on that, but definitely it seems like something
is wrong somewhere.

[commit-bot: I haz the power, 2012-04-23 22:42:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10171009/8001

[commit-bot: I haz the power, 2012-04-24 01:35:03]

Change committed as 133591

[joth, 2012-04-24 08:43:26]

Thanks for updating this Yaron! Sorry I only just got chance to look at it and
it reminded me of  a long standing niggle. I can address this in a follow up if
you like.

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
File net/android/java/org/chromium/net/AndroidNetworkLibrary.java (right):

http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/...
net/android/java/org/chromium/net/AndroidNetworkLibrary.java:105:
sDefaultTrustManager.compareAndSet(null, createDefaultTrustManager());
On 2012/04/23 22:21:29, Yaron wrote:
> On 2012/04/20 20:55:39, Ryan Sleevi wrote:
> > If createDefaultTrustManager() returns null, then you'll needlessly be
> spinning
> > between this, createDefaultTrustManager, and findX509TrustManager, right?
> > 
> > Should you instead throw at this point?
> 
> I don't see that. It looks like verifyServerCertificates would crash when it
> calls checkServerTrusted

FWIW... my understanding now is AtomicReference just adds needless confusion, as
(a) a plain old java reference is atomic anyway, and (b) using AtomicReference
doesn't avoid the risk that thread B gets a partially constructed TrustManager
due to instruction reorder. (c.f. perils of double checked locking).

Careful usage of volatile reference can solve this. Or using synchronization.
But I think http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
is the nicest pattern.

[joth, 2012-04-24 14:12:59]

On 24 April 2012 09:43, <joth@chromium.org> wrote:

> Thanks for updating this Yaron! Sorry I only just got chance to look at it
> and
> it reminded me of  a long standing niggle. I can address this in a follow
> up if
> you like.
>
>
>
>
>
> http://codereview.chromium.**org/10171009/diff/1/net/**
>
android/java/org/chromium/net/**AndroidNetworkLibrary.java<http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/net/AndroidNetworkLibrary.java>
> File net/android/java/org/chromium/**net/AndroidNetworkLibrary.java
> (right):
>
> http://codereview.chromium.**org/10171009/diff/1/net/**
>
android/java/org/chromium/net/**AndroidNetworkLibrary.java#**newcode105<http://codereview.chromium.org/10171009/diff/1/net/android/java/org/chromium/net/AndroidNetworkLibrary.java#newcode105>
> net/android/java/org/chromium/**net/AndroidNetworkLibrary.**java:105:
> sDefaultTrustManager.**compareAndSet(null, createDefaultTrustManager());
> On 2012/04/23 22:21:29, Yaron wrote:
>
>> On 2012/04/20 20:55:39, Ryan Sleevi wrote:
>> > If createDefaultTrustManager() returns null, then you'll needlessly
>>
> be
>
>> spinning
>> > between this, createDefaultTrustManager, and findX509TrustManager,
>>
> right?
>
>> >
>> > Should you instead throw at this point?
>>
>
>  I don't see that. It looks like verifyServerCertificates would crash
>>
> when it
>
>> calls checkServerTrusted
>>
>
> FWIW... my understanding now is AtomicReference just adds needless
> confusion, as (a) a plain old java reference is atomic anyway, and (b)
> using AtomicReference doesn't avoid the risk that thread B gets a
> partially constructed TrustManager due to instruction reorder. (c.f.
> perils of double checked locking).
>
> Careful usage of volatile reference can solve this. Or using
> synchronization. But I think
>
http://en.wikipedia.org/wiki/**Initialization-on-demand_**holder_idiom<http:/...
> the nicest pattern.
>
>
Actually lazy holder is not much good here due to the exceptions that can
get thrown.
Making ensureInitialized() synchronized would be far simpler.




>
http://codereview.chromium.**org/10171009/<http://codereview.chromium.org/101...
>