Upstream certificate and mime Android implementation.

Upstream certificate and mime Android implementation.


BUG=
TEST=

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

[michaelbai, 2011-08-01 21:09:16]

Please help to review it

[wtc, 2011-08-09 18:47:22]

LGTM.  I reviewed Patch Set 3.

This CL is missing the net/net.gyp changes to add the new
files.

Please also make the changes below before you check this in.
Thanks.

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

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.cc:23: 
Delete one blank line.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.cc:42: ConvertUTF8ToJavaString(env, hostname));
"env" should be moved to this line.  Several other
function calls have the same formatting problem.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.cc:100: env, static_cast<jstring>(ret.obj()));
This can fit on the previous line.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library.h
File net/android/network_library.h (right):

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.h:9: #include <jni.h>
Add a blank line to separate the C and C++ system headers.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.h:23: };
Document this enum type.  In particular,
VERIFY_INVOCATION_ERROR should be documented.

"Expired certificate" and "revoked certificate" errors are
not in the VerifyResult enum.  Why?

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.h:46: bool RegisterNetworkLibrary(JNIEnv* env);
Document this function.

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
net/base/openssl_private_key_store_android.cc:7: #include
"net/base/openssl_private_key_store.h"
This header probably should be listed first.

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
net/base/openssl_private_key_store_android.cc:34: private_key, private_len);
Fix argument indentation.

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
net/base/openssl_private_key_store_android.cc:71: */
Please use
#if 0
...
#endif

to comment out a block of code.

Why do we want to define
OpenSSLPrivateKeyStore::GetInstance() in this file?
It seems that it should be defined in the same file where
other OpenSSLPrivateKeyStore methods are defined.

http://codereview.chromium.org/7538029/diff/3001/net/base/platform_mime_util_...
File net/base/platform_mime_util_linux.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/platform_mime_util_...
net/base/platform_mime_util_linux.cc:13: #endif
This should be done as follows:

#include "base/mime_util.h"
#include "build/build_config.h"

#if defined(OS_ANDROID)
#include "net/android/network_library.h"
#endif

See http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

Is it OK to include "base/mime_util.h" on Android?
If necessary, you can do it like this:

#include "build/build_config.h"

#if defined(OS_ANDROID)
#include "net/android/network_library.h"
#else
#include "base/mime_util.h"
#endif

http://codereview.chromium.org/7538029/diff/3001/net/base/platform_mime_util_...
net/base/platform_mime_util_linux.cc:55: #endif
Add a
  // defined(OS_ANDROID)
comment.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:300: // GetDEREncodedBytes below.
|chain| => |chain_bytes|

GetDEREncodedBytes => GetDEREncoded

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:508: #endif  // !OS_ANDROID
!OS_ANDROID => !defined(OS_ANDROID)

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl_android.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl_android.cc:26:
net::android::VerifyX509CertChain(cert_bytes, hostname, "RSA");
Remove the net:: prefixes because this is inside the net
namespace.

You verify the hostname twice, here and on line 18 above.
Is it possible for android::VerifyX509CertChain to NOT
verify the hostname?

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl_android.cc:50:
cert_handles.insert(cert_handles.begin(), cert_handle_);
The cert_handles[0] != cert_handle_ test is not necessary.
If cert_handles[0] == cert_handle_, that's a bug.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl_android.cc:58: reinterpret_cast<const
char*>(der_cache.data), der_cache.data_length);
Write this as
    std::string cert_bytes(
        reinterpret_cast<const char*>(der_cache.data), der_cache.data_length);

to avoid a potential temporary object.

[michaelbai, 2011-08-11 16:10:17]

I just landed the CL.

As to the gyp file, we still have a Java dependences issue need to resolve, I
will upstream it once done.

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

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.cc:23: 
On 2011/08/09 18:47:22, wtc wrote:
> Delete one blank line.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.cc:42: ConvertUTF8ToJavaString(env, hostname));
On 2011/08/09 18:47:22, wtc wrote:
> "env" should be moved to this line.  Several other
> function calls have the same formatting problem.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.cc:100: env, static_cast<jstring>(ret.obj()));
On 2011/08/09 18:47:22, wtc wrote:
> This can fit on the previous line.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library.h
File net/android/network_library.h (right):

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.h:9: #include <jni.h>
On 2011/08/09 18:47:22, wtc wrote:
> Add a blank line to separate the C and C++ system headers.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.h:23: };
On 2011/08/09 18:47:22, wtc wrote:
> Document this enum type.  In particular,
> VERIFY_INVOCATION_ERROR should be documented.
> 
> "Expired certificate" and "revoked certificate" errors are
> not in the VerifyResult enum.  Why?

Done.

http://codereview.chromium.org/7538029/diff/3001/net/android/network_library....
net/android/network_library.h:46: bool RegisterNetworkLibrary(JNIEnv* env);
On 2011/08/09 18:47:22, wtc wrote:
> Document this function.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
net/base/openssl_private_key_store_android.cc:7: #include
"net/base/openssl_private_key_store.h"
On 2011/08/09 18:47:22, wtc wrote:
> This header probably should be listed first.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
net/base/openssl_private_key_store_android.cc:34: private_key, private_len);
On 2011/08/09 18:47:22, wtc wrote:
> Fix argument indentation.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/openssl_private_key...
net/base/openssl_private_key_store_android.cc:71: */
On 2011/08/09 18:47:22, wtc wrote:
> Please use
> #if 0
> ...
> #endif
> 
> to comment out a block of code.
> 
> Why do we want to define
> OpenSSLPrivateKeyStore::GetInstance() in this file?
> It seems that it should be defined in the same file where
> other OpenSSLPrivateKeyStore methods are defined.

Fixed, This introduced in the merge and fixed in gyp file, we didn't use
openssl_memory_private_key_store.cc.

http://codereview.chromium.org/7538029/diff/3001/net/base/platform_mime_util_...
File net/base/platform_mime_util_linux.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/platform_mime_util_...
net/base/platform_mime_util_linux.cc:13: #endif
On 2011/08/09 18:47:22, wtc wrote:
> This should be done as follows:
> 
> #include "base/mime_util.h"
> #include "build/build_config.h"
> 
> #if defined(OS_ANDROID)
> #include "net/android/network_library.h"
> #endif
> 
> See http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code
> 
> Is it OK to include "base/mime_util.h" on Android?
> If necessary, you can do it like this:
> 
> #include "build/build_config.h"
> 
> #if defined(OS_ANDROID)
> #include "net/android/network_library.h"
> #else
> #include "base/mime_util.h"
> #endif

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/platform_mime_util_...
net/base/platform_mime_util_linux.cc:55: #endif
On 2011/08/09 18:47:22, wtc wrote:
> Add a
>   // defined(OS_ANDROID)
> comment.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:300: // GetDEREncodedBytes below.
On 2011/08/09 18:47:22, wtc wrote:
> |chain| => |chain_bytes|
> 
> GetDEREncodedBytes => GetDEREncoded

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:508: #endif  // !OS_ANDROID
On 2011/08/09 18:47:22, wtc wrote:
> !OS_ANDROID => !defined(OS_ANDROID)

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl_android.cc (right):

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl_android.cc:26:
net::android::VerifyX509CertChain(cert_bytes, hostname, "RSA");
On 2011/08/09 18:47:22, wtc wrote:
> Remove the net:: prefixes because this is inside the net
> namespace.
> 
> You verify the hostname twice, here and on line 18 above.
> Is it possible for android::VerifyX509CertChain to NOT
> verify the hostname?

There is a TODO in Java code to try to decide whether the hostname should be
verified again. Currently hostname isn't verified in Java code.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl_android.cc:50:
cert_handles.insert(cert_handles.begin(), cert_handle_);
On 2011/08/09 18:47:22, wtc wrote:
> The cert_handles[0] != cert_handle_ test is not necessary.
> If cert_handles[0] == cert_handle_, that's a bug.

Done.

http://codereview.chromium.org/7538029/diff/3001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl_android.cc:58: reinterpret_cast<const
char*>(der_cache.data), der_cache.data_length);
On 2011/08/09 18:47:22, wtc wrote:
> Write this as
>     std::string cert_bytes(
>         reinterpret_cast<const char*>(der_cache.data), der_cache.data_length);
> 
> to avoid a potential temporary object.

Done.