Fix handling of user and CA certificates on Android.

Fix handling of user and CA certificates on Android.

This patch fixes several things in the Android Chromium build:

- Fix two bugs that prevented the proper installation of keygen-ed
  (public,private) key pairs on the system. The key data format was
  invalid, due to the use of openssl's i2d_PrivateKey() and
  i2d_PublicKey(), and the keys were incorrectly swapped in
  the intent extras.

- Allow Chromium to install CA certificates as well as PKCS#12
  keychains. The code only supported user certificates. This is
  needed to match feature parity on Android with the native
  browser. The reason why this is *not* enabled on other
  platforms is to avoid denial-of-service issues, which are
  not present on Android, because the CertInstaller will
  always show a UI Dialog asking the user to name the
  installed certificate.

- Fix the code used to install certificates on the system,
  i.e. directly launch the CertInstaller activity, passing it
  the appropriate bytes through an Intent.

  The old code used SSLAddCertHandler, which forced Chromium
  to perform minimal checks on the certificate's data validity.

  These checks always failed due to CertDatabase::CheckUserCert()
  failing in net/base/cert_database_openssl.cc due to the
  fact that OpenSSLKeyStoreAndroid::FetchPrivateKeyStore()
  is not implemented (and probably never will, since the
  platform doesn't provide APIs to retrieve stored private
  keys).

  Instead, the CertInstaller is entirely in charge of verifying
  the validity of certificates and display a UI toast (i.e. a
  tiny fading non-interactive dialog) to indicate installation
  success / failure.

Note: It is not possible to test this with the upstream Chromium
      build for Android (neither the ContentShell or
      ChromiumTestShell support the necessary features to do it).

      The patch was tested by applying to the internal Chrome
      for Android tree, and checking manually that everything
      works as expected.

BUG=154006

[digit1, 2012-10-04 14:29:34]

this is a first attempt at upstreaming the fix that was performed last week on
the internal Chrome for Android M18 branch.

[Ryan Sleevi, 2012-10-04 14:37:44]

Quick note:

For upstreaming, I don't think we want to modify the content client,
or at least, not by adding this alternate path for a single platform.

I'm not going to be able to take a deep dive on this CL before Monday,
but I think we need to find a cleaner solution for long-term, in a way
that both desktop and Android can stay on the same path for as long as
possible.

Given the existing desktop bugs, I suspect this probably means passing
along the entire data and the associated mime type (or logical "this
is the type of install being requested") through to content layer,
then having the SSLAddCertHandler handle the parsing into
X509Certificate* (or passing to intents)

I'll check with a few other people today, but that's what I think is
the right (long term) approach.

On Thu, Oct 4, 2012 at 7:29 AM,  <digit@chromium.org> wrote:
> Reviewers: Ryan Sleevi, joth, Matt Menke, creis, Satish,
>
> Message:
> this is a first attempt at upstreaming the fix that was performed last week
> on
> the internal Chrome for Android M18 branch.
>
>
> Description:
> Fix handling of user and CA certificates on Android.
>
> This patch fixes several things in the Android Chromium build:
>
> - Fix two bugs that prevented the proper installation of keygen-ed
>   (public,private) key pairs on the system. The key data format was
>   invalid, due to the use of openssl's i2d_PrivateKey() and
>   i2d_PublicKey(), and the keys were incorrectly swapped in
>   the intent extras.
>
> - Allow Chromium to install CA certificates as well as PKCS#12
>   keychains. The code only supported user certificates. This is
>   needed to match feature parity on Android with the native
>   browser. The reason why this is *not* enabled on other
>   platforms is to avoid denial-of-service issues, which are
>   not present on Android, because the CertInstaller will
>   always show a UI Dialog asking the user to name the
>   installed certificate.
>
> - Fix the code used to install certificates on the system,
>   i.e. directly launch the CertInstaller activity, passing it
>   the appropriate bytes through an Intent.
>
>   The old code used SSLAddCertHandler, which forced Chromium
>   to perform minimal checks on the certificate's data validity.
>
>   These checks always failed due to CertDatabase::CheckUserCert()
>   failing in net/base/cert_database_openssl.cc due to the
>   fact that OpenSSLKeyStoreAndroid::FetchPrivateKeyStore()
>   is not implemented (and probably never will, since the
>   platform doesn't provide APIs to retrieve stored private
>   keys).
>
>   Instead, the CertInstaller is entirely in charge of verifying
>   the validity of certificates and display a UI toast (i.e. a
>   tiny fading non-interactive dialog) to indicate installation
>   success / failure.
>
> Note: It is not possible to test this with the upstream Chromium
>       build for Android (neither the ContentShell or
>       ChromiumTestShell support the necessary features to do it).
>
>       The patch was tested by applying to the internal Chrome
>       for Android tree, and checking manually that everything
>       works as expected.
>
> BUG=154006
>
>
> Please review this at https://chromiumcodereview.appspot.com/11031043/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>   M chrome/browser/chrome_content_browser_client.h
>   M chrome/browser/chrome_content_browser_client.cc
>   M content/browser/renderer_host/buffered_resource_handler.cc
>   M content/browser/renderer_host/x509_user_cert_resource_handler.h
>   M content/browser/renderer_host/x509_user_cert_resource_handler.cc
>   M content/public/browser/content_browser_client.h
>   M net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java
>   M net/android/network_library.h
>   M net/android/network_library.cc
>   M net/base/cert_database_openssl.cc
>   M net/base/mime_util.cc
>   M net/base/openssl_private_key_store_android.cc
>
>

[Ryan Sleevi, 2012-10-11 01:06:09]

On 2012/10/04 14:37:44, Ryan Sleevi wrote:
> Quick note:
> 
> For upstreaming, I don't think we want to modify the content client,
> or at least, not by adding this alternate path for a single platform.
> 
> I'm not going to be able to take a deep dive on this CL before Monday,
> but I think we need to find a cleaner solution for long-term, in a way
> that both desktop and Android can stay on the same path for as long as
> possible.
> 
> Given the existing desktop bugs, I suspect this probably means passing
> along the entire data and the associated mime type (or logical "this
> is the type of install being requested") through to content layer,
> then having the SSLAddCertHandler handle the parsing into
> X509Certificate* (or passing to intents)
> 
> I'll check with a few other people today, but that's what I think is
> the right (long term) approach.

Short term - we should just pass the string (and mime type) on all platforms for
now, and move the parsing of X509Certificate into the ContentClient. That's a
Content API change, so we'll want to confer with the Content owners. My
suggestion is to encapsulate such a request into a structure, and pass that
structure.

Long term - After discussing with rdsmith@ about the downloads API, we have two
options:
 - Change the structure to keep a number of net/ internal details alive (like
the URLRequest) so that if the user chooses, it can redirect the
X509UserCertResourceHandler to pass to the Downloads layer
 - Have Chrome implement a downloads-independent way of saving a file (in which
case, we continue just passing a string and mime type)

But I definitely don't think we should split the ContentClient upstream - it was
simply a temporary solution for the M18 / ChromeView.

[Ryan Sleevi, 2012-11-13 19:29:07]

Ping?

Should this be closed in favor of https://codereview.chromium.org/11266008/ ?

[digit1, 2012-11-14 15:33:00]

On 2012/11/13 19:29:07, Ryan Sleevi wrote:
> Ping?
> 
> Should this be closed in favor of https://codereview.chromium.org/11266008/ ?

Yes absolutely, I'll close this.