Upstream: Build net_unittests for Android.

net/base/x509_certificate_openssl.cc

Index: net/base/x509_certificate_openssl.cc
diff --git a/net/base/x509_certificate_openssl.cc b/net/base/x509_certificate_openssl.cc
index 5612d61ab49445ec1b3b9acf646be1f5e2ddbd02..1c95fab59cc384767d6149ca3397931c67ff5f2e 100644
--- a/net/base/x509_certificate_openssl.cc
+++ b/net/base/x509_certificate_openssl.cc
@@ -25,6 +25,11 @@
 #include "net/base/net_errors.h"
 #include "net/base/x509_util_openssl.h"
 
+#if defined(OS_ANDROID)
+#include "base/logging.h"
+#include "net/android/network_library.h"
+#endif
+
 namespace net {
 
 namespace {
@@ -512,8 +517,44 @@ X509_STORE* X509Certificate::cert_store() {
   return X509InitSingleton::GetInstance()->store();
 }
 
-#if !defined(OS_ANDROID)
+#if defined(OS_ANDROID)
+int X509Certificate::VerifyInternal(const std::string& hostname,
+                                    int flags,
+                                    CRLSet* crl_set,
+                                    CertVerifyResult* verify_result) const {
+  if (!VerifyNameMatch(hostname))
+    verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
+
+  std::vector<std::string> cert_bytes;
+  GetChainDEREncodedBytes(&cert_bytes);
 
+  // TODO(joth): Fetch the authentication type from SSL rather than hardcode.
+  // TODO(jingzhao): Recover the original implementation once we support JNI.
+#if 0
+  android::VerifyResult result =
+      android::VerifyX509CertChain(cert_bytes, hostname, "RSA");
+#else
+  android::VerifyResult result = android::VERIFY_INVOCATION_ERROR;
+  NOTIMPLEMENTED();
+#endif
+  switch (result) {
+    case android::VERIFY_OK:
+      return OK;

[Ryan Sleevi, 2011/11/06 02:39:48]

random drive by:

What does it mean if android::VerifyX509CertChain returns VERIFY_OK, but the
VerifyNameMatch (on line 525/526) fails? Right now, that error would be
suppressed.

As/if/when Chrom[e/ium] implements more stringent checks, this could potentially
be an issue.

One lesser note (perhaps a TODO, or perhaps never for the android impl.) is to
have the android::VerifyX509CertChain return the constructed certification path.
The reason for this is purely presentation (being able to display the
certificate chain).

It'd be a low priority, as the display code is not yet wired up, but some of the
unit tests are testing that unrelated, untrusted intermediates are pruned from
the certification path. X509Certificate::Verify() has a default implementation
setting the verified_chain = this, but the expectation is that VerifyInternal()
will update the chain as/when appropriate.

[Jing Zhao, 2011/11/07 03:42:49]

I think Joth is the original author of this function. Joth, what do you think
about Ryan's comments (including the other two comments below)?

[joth, 2011/11/07 10:42:20]

oh yes, good spot.
To be consistent with the !ANDROID version below, we should just break here, and
then at the end of the function:-
  if (IsCertStatusError(verify_result->cert_status))
    return MapCertStatusToNetError(verify_result->cert_status);

  return OK;
}
I've got a task to do that as a follow up (http://b/4290005 for the record, but
afraid that tracker is not public)
And I think we'll need it for GetCertChainInfo and AppendPublicKeyHashes too.
And set is_issued_by_known_root....
I think this thread shows we need some continuous builders and  tests running
first though 

[jingzhao, 2011/11/08 02:02:17]

Fixed as you said.
Yes, thank you for the record.

+    case android::VERIFY_BAD_HOSTNAME:
+      verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
+      break;
+    case android::VERIFY_NO_TRUSTED_ROOT:
+      verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
+      break;
+    case android::VERIFY_INVOCATION_ERROR:
+    default:
+      verify_result->cert_status |= ERR_CERT_INVALID;
+      break;
+  }
+  return MapCertStatusToNetError(verify_result->cert_status);
+}
+
+#else
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
                                     CRLSet* crl_set,
@@ -565,7 +606,7 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
   return OK;
 }
 
-#endif  // !defined(OS_ANDROID)
+#endif  // defined(OS_ANDROID)
 
 // static
 bool X509Certificate::GetDEREncoded(X509Certificate::OSCertHandle cert_handle,
@@ -620,4 +661,25 @@ bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
       der_cache.data_length);
 }
 
+#if defined(OS_ANDROID)
+void X509Certificate::GetChainDEREncodedBytes(
+    std::vector<std::string>* chain_bytes) const {
+  OSCertHandles cert_handles(intermediate_ca_certs_);

[Ryan Sleevi, 2011/11/06 02:39:48]

Two things:

1) Why not implement this in terms of GetDEREncoded (line 612), to avoid
diverging DER-accessing implementations? Since you end up copying
|der_cache.data| to a string anyways, there is no extra copy overhead here.

[joth, 2011/11/07 10:42:20]

I think it's just GetDEREncoded didn't exist when the first version of this was
written. I'll do a follow up to refactor this, to avoid iterating it more in
this CL

[jingzhao, 2011/11/08 02:02:17]

This change seems small so I made it.. Please take a look if it's what you want.

[joth, 2011/11/08 10:05:23]

Thanks, this looks OK.

For my own record, when we have test coverage running for this, I would like to
go further:

void X509Certificate::GetChainDEREncodedBytes(..)
  chain_bytes->resize(intermediate_ca_certs_.size() + 1);
  if (!GetDEREncoded(cert_handle_, &chain_bytes->at(0)))
    return <ERROR>
  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
    GetDEREncoded(intermediate_ca_certs_[i], &chain_bytes->at(i + 1));
  }
}

Also the whole function could be inlined at the call-site, avoiding the need for
this extra #ifdef in the .h and .cc files.

[Ryan Sleevi, 2011/11/08 15:03:23]

Even if not unlined to the callsite, you can move this to the anonymous
namespace since GetDEREncoded is public.

If you do further tweak this, just use &cert_handles[i], not
cert_handles->at(i). I get dinged for using it in style reviews too ;-)

+  // Make sure the peer's own cert is the first in the chain, if it's not
+  // already there.
+  if (cert_handles.empty())
+    cert_handles.insert(cert_handles.begin(), cert_handle_);

[Ryan Sleevi, 2011/11/06 02:39:48]

2) This isn't correct how X509Certificate stores its chain, so I'm surprised
this works.

|intermediate_ca_certs_| contains only the intermediates supplied by the server
- it never contains the end-entity cert (|cert_handle_|). For servers who supply
a certificate chain > 1 (ie: most?), this would fail to consider the EE
certificate.

[joth, 2011/11/07 10:42:20]

Good spot, thank you. This was broken when it was landed in
http://codereview.chromium.org/7538029 (which I never saw to review) (which is
also where it was moved to the _android.cc file where it couldn't compile...)

In my version it said:
  if (cert_handles.empty() || cert_handles[0] != cert_handle_)
    cert_handles.insert(cert_handles.begin(), cert_handle_);

As you say intermediate_ca_certs_ never contains cert_handle_ so this guard is
redundant, but it does do the right thing at least.

[jingzhao, 2011/11/08 02:02:17]

Fixed as you said.

+
+  chain_bytes->reserve(cert_handles.size());
+  for (OSCertHandles::const_iterator it = cert_handles.begin();
+       it != cert_handles.end(); ++it) {
+    DERCache der_cache = {0};
+    GetDERAndCacheIfNeeded(*it, &der_cache);
+    std::string cert_bytes (
+        reinterpret_cast<const char*>(der_cache.data), der_cache.data_length);
+    chain_bytes->push_back(cert_bytes);
+  }
+}
+#endif
+
 }  // namespace net