Fix certificate and keychain installation on Android.

Fix certificate and keychain installation on Android.

This patch is necessary to allow Chrome on Android to properly
install CA certificates and PKCS#12 keychains. This feature is
not supported on other platforms, but necessary on mobile.

It does modify the content client API to deal with the new
file types, i.e. the AddNewCertificate() method is renamed
AddCryptoFile(), and its signature changed to receive the
file data directly (along with a file type enum).

It is now the reponsability of the browser / content embedder
to perform certificate verification.

More specifically:

- Modify net/base/mime_util.h to provide two new functions:

    * IsSupportedCertificateMimeType(), which returns true iff
      a mime type corresponds to a supported crypto file
      (only "application/x-x509-user-cert" is supported,
      except on Android, which adds ".../x-x509-ca-cert" and
      ".../x-pkcs12").

    * GetCertificateMimeTypeForMimeType() which translates a
      mime type string into an enum value that is also
      understood from Java (see below), describing the
      type of file.

  Note that "net/base/mime_util_certificate_list.h" is used to hold
  the list of certificate mime type constants, both for C++ and Java
  (i.e. it is used to auto-generate org.chromium.net.CertificateMimeType.java
  at build time, under out/$BUILDTYPE/gen/template/).

- Rename X509UserCertResourceHandler to CertificateResourceHandler
  under content/browser/loader/ in order to deal with
  all certificate mime types. Modify buffered_resource_handler.cc
  appropriately.

- Add net::android::StoreCertificate(), and the Java
  org.chromium.net.AndroidNetworkLibrary.storeCertificate()
  method to send the certificate data for installation through
  the system's CertInstaller activity.

- Add chrome::SSLAddCertificate() to implement the
   platform-specific code that used to be in
   content::ContentBrowserClient::AddNewCertificate().

- Rename content::ContentBrowserClient::AddNewCertificate()
  to ::AddCertificate(), and change its signature to accept
  resource file bytes directly and a net::CertificateMimeType
  (was an X509Certificate pointer).

This change shall not modify the behaviour of Chromium on other
platforms.

BUG=149306
TEST=Manual test with ChromiumTestShell, see internal b/6668254 for details.



Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172350

[digit1, 2012-10-24 13:00:48]

This is a rewrite of the patch that was found at
https://chromiumcodereview.appspot.com/11031043/, except that:

- the keygen public/private key fix is moved to
https://codereview.chromium.org/11260015/ instead.

- this implements a cleaner way to implementation the installation of crypto
files, following a recent conversation I had with Ryan.

[Ryan Sleevi, 2012-10-24 22:01:25]

NACK on the "CryptoFile" naming scheme (see below).

I think Darin and/or John Abd-El-Malek's feedback regarding the overall content/
design will be important here, so please make sure to engage them directly if
you don't hear on this CL.

If this change works as an interim step for them, I think the implementation
looks good (and was more or less already reviewed), and it's just a matter of
working out the naming.

https://codereview.chromium.org/11266008/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/crypto_file_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/crypto_file_resource_handler.cc:113: // Note that
it's up to the browser to handle verification.
It's not verification - it's parsing. The browser is already responsible today
with handling verification (hence the IsExpired checks and such)

https://codereview.chromium.org/11266008/diff/1/content/public/browser/conten...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11266008/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:332: virtual void
AddNewCryptoFile(
I would prefer the name "AddCertificates" be used here. I find "Crypto" is too
generic a term to be meaningful here, and I find "File" to be too misleading,
since it's not actually passing a base::File. The "new" is dropped because it
implies this was freshly generated for the user - but it may not be (in the case
of intermediates / roots)

This also entails renaming IsSupportedCrypto to
net::IsSupportedCertificateMimeType(...);, and net::CertificateMimeType enum (or
possibly just the string mime type)

typo: x509 -> X.509
nit: s/user certificate/client certificate/
nit: "If file_size is 0, |file_data| will be NULL" (if-then rather than then-if)

Alternatively, and this depends on how John feels about the API, it may make
more sense to have some sort of CertificateInstallerClient part of the public
API which is used (similar to how the downloads are handled), rather than
forcing the methods directly on the ContentBrowserClient.

https://codereview.chromium.org/11266008/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/11266008/diff/1/net/base/mime_util.cc#newcode355
net/base/mime_util.cc:355: { "application/x-x509-ca-cert",  
CRYPTO_FILE_TYPE_X509_CA_CERT },
Does Android not also need to handle application/x-x509-root-cert?

From the perspective of Keychain's Intents, ca- and root- should be the same
(that is, they install trusted certs). For other OSes, ca-cert installs
intermediates, *but they are not automagically trusted* (although transitively,
they do chain to a trusted root). Consider them "supplementary" certs.

https://codereview.chromium.org/11266008/diff/1/net/base/mime_util.cc#newcode567
net/base/mime_util.cc:567: static const char* const legal_top_level_types[] = {
unrelated change? (but good change none the less, just not for this CL)

[digit1, 2012-10-25 13:57:38]

https://codereview.chromium.org/11266008/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/crypto_file_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/crypto_file_resource_handler.cc:113: // Note that
it's up to the browser to handle verification.
Sure, will fix.

https://codereview.chromium.org/11266008/diff/1/content/public/browser/conten...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11266008/diff/1/content/public/browser/conten...
content/public/browser/content_browser_client.h:332: virtual void
AddNewCryptoFile(
yes, I wasn't a big fan of "CryptoFile", but since this is used to install both
certificates and private key chains, it sounded a bit better.

I'll rename this to AddCertificates, and the class will be
CertificateResourceHandler (instead of CryptoFileResourceHandler).

I'm not sure where a CertificateInstallerClient would go. I'd appreciate if you
could clarify, if this becomes necessary.

https://codereview.chromium.org/11266008/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/11266008/diff/1/net/base/mime_util.cc#newcode355
net/base/mime_util.cc:355: { "application/x-x509-ca-cert",  
CRYPTO_FILE_TYPE_X509_CA_CERT },
I've asked the Android folks responsible for this.

https://codereview.chromium.org/11266008/diff/1/net/base/mime_util.cc#newcode567
net/base/mime_util.cc:567: static const char* const legal_top_level_types[] = {
Sure, I've moved this to https://codereview.chromium.org/11263039/

[darin (slow to review), 2012-10-26 07:34:05]

renaming to CertificateResourceHandler is fine, but see my note.  i don't think
any of this code belongs here.

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/buffered_resource_handler.cc:306: if
(net::IsSupportedCertificateMimeType(mime_type)) {
I'm pretty sure the existing code is wrongly integrated here.  Please confirm,
but I don't think <img src="certificate"> is supposed to trigger the
CertificateResourceHandler, yet it does.  I think certificate handling should be
integrated into the download system much in the same way as CRX handling is.

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/certificate_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/certificate_resource_handler.cc:111: content_bytes
= resource_buffer_->data();
nit: indentation

[digit1, 2012-10-26 11:38:10]

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/buffered_resource_handler.cc:306: if
(net::IsSupportedCertificateMimeType(mime_type)) {
In his original comment at https://codereview.chromium.org/11031043/, Ryan
agrees with you. He describes a short-term fix as well as a long-term one, where
the latter would move all this certificate download code to a better location.

This current patch corresponds to the short-term fix only. If you think this is
important, I can try to handle the long-term fix too (with an intermediate patch
before another revision of this one), but in this case, I would appreciate a few
pointers / clarifications about which layers / source files would likely be
impacted.

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/certificate_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/certificate_resource_handler.cc:111: content_bytes
= resource_buffer_->data();
Sure, will fix.

[darin (slow to review), 2012-10-26 17:37:02]

On Fri, Oct 26, 2012 at 4:38 AM, <digit@chromium.org> wrote:

>
> https://codereview.chromium.**org/11266008/diff/4001/**
>
content/browser/renderer_host/**buffered_resource_handler.cc<https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_host/buffered_resource_handler.cc>
> File content/browser/renderer_host/**buffered_resource_handler.cc (right):
>
> https://codereview.chromium.**org/11266008/diff/4001/**
>
content/browser/renderer_host/**buffered_resource_handler.cc#**newcode306<https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_host/buffered_resource_handler.cc#newcode306>
> content/browser/renderer_host/**buffered_resource_handler.cc:**306: if
> (net::**IsSupportedCertificateMimeType**(mime_type)) {
> In his original comment at
https://codereview.chromium.**org/11031043/<https://codereview.chromium.org/1...
> ,
> Ryan agrees with you. He describes a short-term fix as well as a
> long-term one, where the latter would move all this certificate download
> code to a better location.
>
> This current patch corresponds to the short-term fix only. If you think
> this is important, I can try to handle the long-term fix too (with an
> intermediate patch before another revision of this one), but in this
> case, I would appreciate a few pointers / clarifications about which
> layers / source files would likely be impacted.
>
>
Definitely OK to stage work like this.  Sorry, I have just been meaning
to raise the issue!  This code is just so out of place.


>
> https://codereview.chromium.**org/11266008/diff/4001/**
>
content/browser/renderer_host/**certificate_resource_handler.**cc<https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_host/certificate_resource_handler.cc>
> File content/browser/renderer_host/**certificate_resource_handler.**cc
> (right):
>
> https://codereview.chromium.**org/11266008/diff/4001/**
> content/browser/renderer_host/**certificate_resource_handler.**
>
cc#newcode111<https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_host/certificate_resource_handler.cc#newcode111>
> content/browser/renderer_host/**certificate_resource_handler.**cc:111:
> content_bytes = resource_buffer_->data();
> Sure, will fix.
>
>
https://codereview.chromium.**org/11266008/<https://codereview.chromium.org/1...
>

[Ryan Sleevi, 2012-11-13 19:37:50]

https://codereview.chromium.org/11266008/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11266008/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1286: #ifdef OS_ANDROID
nit: #if defined(OS_ANDROID)

https://codereview.chromium.org/11266008/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1299: // - The Android
CertInstaller activity will check the crypto files
nit: "crypto files"

https://codereview.chromium.org/11266008/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1301: //   not valid, so the UI
stuff performed by SSLAddCertHandler would
nit: s/stuff//

https://codereview.chromium.org/11266008/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1311: (void)render_view_id;
I believe we have an ALLOW_UNUSED macro for this. FWIW

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/buffered_resource_handler.cc:307: // Install
crypto file.
nit: crypto file

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/certificate_resource_handler.h (right):

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/certificate_resource_handler.h:6: #define
CONTENT_BROWSER_RENDERER_HOST_CRYPTO_FILE_RESOURCE_HANDLER_H_
nit: headers

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/certificate_resource_handler.h:27: // This class
handles the cryptographics mime-types supported by
nit: s/cryptographics/cryptographic/

https://codereview.chromium.org/11266008/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/certificate_resource_handler.h:30: // X509 CA
certificates and PKCS#12 keychains.
nit: s/X509/X.509/ (throughout the CL)

https://codereview.chromium.org/11266008/diff/4001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11266008/diff/4001/content/public/browser/con...
content/public/browser/content_browser_client.h:327: // Adds a new installable
certificate or keychain. This is a downloaded
I'm hestitant to use the term "keychain", which is already overloaded on
Android, iOS, and OS X.

// Adds a new installable certificate or private key. ...

might be better

https://codereview.chromium.org/11266008/diff/4001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/11266008/diff/4001/net/base/mime_util.cc#newc...
net/base/mime_util.cc:352: static const CertificateTypeInfo 
supported_certificate_types[] = {
nit: whitespace ("  " should be " ")

https://codereview.chromium.org/11266008/diff/4001/net/base/mime_util.cc#newc...
net/base/mime_util.cc:946: return net::supported_certificate_types[i].cert_type;
nit: indentation

https://codereview.chromium.org/11266008/diff/4001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/11266008/diff/4001/net/base/mime_util.h#newco...
net/base/mime_util.h:125: CERTIFICATE_TYPE_PKCS12_KEYCHAIN = 3,
nit: I would replace PKCS12_KEYCHAIN with PKCS12_ARCHIVE throughout

[digit1, 2012-11-15 17:42:13]

https://chromiumcodereview.appspot.com/11266008/diff/4001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/4001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1286: #ifdef OS_ANDROID
On 2012/11/13 19:37:50, Ryan Sleevi wrote:
> nit: #if defined(OS_ANDROID)

Done.

https://chromiumcodereview.appspot.com/11266008/diff/4001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1299: // - The Android
CertInstaller activity will check the crypto files
On 2012/11/13 19:37:50, Ryan Sleevi wrote:
> nit: "crypto files"

Done.

https://chromiumcodereview.appspot.com/11266008/diff/4001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1301: //   not valid, so the UI
stuff performed by SSLAddCertHandler would
On 2012/11/13 19:37:50, Ryan Sleevi wrote:
> nit: s/stuff//

Done.

https://chromiumcodereview.appspot.com/11266008/diff/4001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:1311: (void)render_view_id;
On 2012/11/13 19:37:50, Ryan Sleevi wrote:
> I believe we have an ALLOW_UNUSED macro for this. FWIW

I believe it only works for local variable declarations. I've separated this in
two different function definitions instead.

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/browser/ren...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/browser/ren...
content/browser/renderer_host/buffered_resource_handler.cc:307: // Install
crypto file.
On 2012/11/13 19:37:50, Ryan Sleevi wrote:
> nit: crypto file

Done.

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/browser/ren...
File content/browser/renderer_host/certificate_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/browser/ren...
content/browser/renderer_host/certificate_resource_handler.cc:111: content_bytes
= resource_buffer_->data();
On 2012/10/26 11:38:10, digit1 wrote:
> Sure, will fix.

Done.

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/browser/ren...
File content/browser/renderer_host/certificate_resource_handler.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/browser/ren...
content/browser/renderer_host/certificate_resource_handler.h:30: // X509 CA
certificates and PKCS#12 keychains.
On 2012/11/13 19:37:50, Ryan Sleevi wrote:
> nit: s/X509/X.509/ (throughout the CL)

Done.

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/public/brow...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/4001/content/public/brow...
content/public/browser/content_browser_client.h:327: // Adds a new installable
certificate or keychain. This is a downloaded
I've changed everything to say "PKCS#12 archive" instead then.

[Ryan Sleevi, 2012-11-15 20:05:59]

LGTM, mod a few nits. No need for re-review.

If Darin is happy with this (and like was mentioned previous - this is a only a
short-term solution for Android upstreaming, until cert handling is moved out
entirely), go forth and commit.

https://chromiumcodereview.appspot.com/11266008/diff/17001/content/browser/re...
File content/browser/renderer_host/certificate_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/17001/content/browser/re...
content/browser/renderer_host/certificate_resource_handler.cc:113: // Note that
it's up to the browser to handle verification.
small nit: "verification of the certificate data" or just "verification of the
data"

Just to avoid ambiguity between "certificate validation" (eg: checking chains)
and format validation (eg: that it parses).

Your documentation in AddCertificates() is clearer in this regard ("it's up to
the embedder to parse the data")

https://chromiumcodereview.appspot.com/11266008/diff/17001/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/17001/net/base/mime_util...
net/base/mime_util.h:116: // A list of supported crypto-related mime types.
nit: certificate-related

https://chromiumcodereview.appspot.com/11266008/diff/17001/net/base/mime_util...
net/base/mime_util.h:125: CERTIFICATE_TYPE_PKCS12_ARCHIVE = 3,
nit: whitespace

[darin (slow to review), 2012-11-15 20:47:04]

https://chromiumcodereview.appspot.com/11266008/diff/17001/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/17001/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:1333: void
ChromeContentBrowserClient::AddCertificates(
nit: It seems like it would be good to move all of this code out into a helper
function somewhere, perhaps under chrome/browser/ssl/ that just provides a
AddCertificates function.  That way we avoid mucking up this file with platform
#ifdefs.

https://chromiumcodereview.appspot.com/11266008/diff/17001/content/browser/re...
File content/browser/renderer_host/certificate_resource_handler.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/17001/content/browser/re...
content/browser/renderer_host/certificate_resource_handler.h:28: //
net::IsSupportedCertificateMimeType(mime_type). These typically
nit: Be careful about adding references to functions in the code.  What happens
if someone renames this function?  We don't have any mechanism to ensure that
this comment gets fixed.  Also, this class doesn't internally enforce that it
only gets used to handle the mime types that that function indicates.

https://chromiumcodereview.appspot.com/11266008/diff/17001/content/browser/re...
content/browser/renderer_host/certificate_resource_handler.h:32: 
nit: kill extra whitespace here?

https://chromiumcodereview.appspot.com/11266008/diff/17001/content/browser/re...
content/browser/renderer_host/certificate_resource_handler.h:36: int
render_process_host_id,
nit: indentation

[digit1, 2012-11-21 09:33:13]

Thanks Darin, the latest patch fixes the issues you noticed and moved some code
to chrome/browser/ssl/ssl_add_certificate.*

Note that I also renamed one method from AddCertificates() to AddCertificate()
on purpose, given that it can only be used to install a single certificate at a
time, I found it less confusing.

[darin (slow to review), 2012-11-26 19:53:42]

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
File chrome/browser/ssl/ssl_add_certificate.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate.cc:10: void chrome::SSLAddCertificate(
nit: use "namespace chrome {" instead.

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate.cc:20: if (cert_type ==
net::CERTIFICATE_TYPE_X509_USER_CERT) {
nit: how about returning early when cert_type is not X509?  that way you reduce
indentation for the rest of the function.  less indentation is good for
readability.

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate.cc:23: cert =
net::X509Certificate::CreateFromBytes(
nit: indentation is wrong here

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
File chrome/browser/ssl/ssl_add_certificate_android.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate_android.cc:25: void
chrome::SSLAddCertificate(
nit: ditto about namespace

https://chromiumcodereview.appspot.com/11266008/diff/23002/content/browser/re...
File content/browser/renderer_host/certificate_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/content/browser/re...
content/browser/renderer_host/certificate_resource_handler.cc:116: request_,
cert_type_, content_bytes, content_length_,
nit: you are already in the content namespace, so no need for content::

https://chromiumcodereview.appspot.com/11266008/diff/23002/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/net/base/mime_util...
net/base/mime_util.h:122: CERTIFICATE_TYPE_UNKNOWN = 0,
nit: indentation

it seems a bit unexpected that a data type named CertificateType would be
defined in mime_util.h.  i see we have net/base/x509_cert_types.h, and i guess
that is particular to x509 certs.  it seems like you could invent a
net/base/certificate_type.h unless there is a better place for this new enum. 
hmm, but there's already net/base/cert_type.h.  how does this compare to that? 
very confusing to have CertificateType and CertType!!

[digit1, 2012-11-26 21:41:34]

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
File chrome/browser/ssl/ssl_add_certificate.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate.cc:10: void chrome::SSLAddCertificate(
On 2012/11/26 19:53:43, darin wrote:
> nit: use "namespace chrome {" instead.

Done.

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate.cc:20: if (cert_type ==
net::CERTIFICATE_TYPE_X509_USER_CERT) {
On 2012/11/26 19:53:43, darin wrote:
> nit: how about returning early when cert_type is not X509?  that way you
reduce
> indentation for the rest of the function.  less indentation is good for
> readability.

Done.

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate.cc:23: cert =
net::X509Certificate::CreateFromBytes(
On 2012/11/26 19:53:43, darin wrote:
> nit: indentation is wrong here

Done.

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
File chrome/browser/ssl/ssl_add_certificate_android.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/chrome/browser/ssl...
chrome/browser/ssl/ssl_add_certificate_android.cc:25: void
chrome::SSLAddCertificate(
On 2012/11/26 19:53:43, darin wrote:
> nit: ditto about namespace

Done.

https://chromiumcodereview.appspot.com/11266008/diff/23002/content/browser/re...
File content/browser/renderer_host/certificate_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/content/browser/re...
content/browser/renderer_host/certificate_resource_handler.cc:116: request_,
cert_type_, content_bytes, content_length_,
On 2012/11/26 19:53:43, darin wrote:
> nit: you are already in the content namespace, so no need for content::

Done.

https://chromiumcodereview.appspot.com/11266008/diff/23002/net/base/cert_data...
File net/base/cert_database_openssl.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/net/base/cert_data...
net/base/cert_database_openssl.cc:34: NOTIMPLEMENTED();
It turns out that this change will probably break the net unit tests on the
"linux_redux" build, so I've modified this to only return ERR_NOT_IMPLEMENTED on
OS_ANDROID.

https://chromiumcodereview.appspot.com/11266008/diff/23002/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/23002/net/base/mime_util...
net/base/mime_util.h:122: CERTIFICATE_TYPE_UNKNOWN = 0,
Fixed indentation. On the other hand, I've renamed the type to
CertificateMimeType to avoid ambiguities. I really thing that mime_util.h is the
right location here, since it directly maps to a MIME TYPE.

For the record, in case you'd ask, the reason that the code uses CONSTANT_NAMES
instead of kConstantNames is because that's the convention that the Chrome on
Android code uses for constants that are to be shared with Java (which is the
case here). Hope this isn't a problem here.

[darin (slow to review), 2012-11-27 05:24:34]

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util...
net/base/mime_util.cc:355: { "application/x-x509-user-cert",
nit: indentation is wrong

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util...
net/base/mime_util.h:119: // 
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java
Instead of writing this comment, is it possible to construct some kind of
assertion that runs at build time?  It seems very fragile to rely on comments
for this sort of thing.  Has anyone considered some kind of auto-gen solution
for keeping these constants in sync?

It also looks like you have a bug.  The values of the constants don't match up!

[digit1, 2012-11-27 10:19:47]

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util...
net/base/mime_util.cc:355: { "application/x-x509-user-cert",
I treated this a a line-continuation indent. Do you mean you prefer something
like:

  { "application/x-x509-user-cert",
    CERTIFICATE_MIME_TYPE_X509_USER_CERT },

The coding style isn't very  specific about this.

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util.h
File net/base/mime_util.h (right):

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util...
net/base/mime_util.h:119: // 
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java
This has been discussed on chromium-dev and actually implemented as a special
case in https://chromiumcodereview.appspot.com/10912136 for
net/base/net_error_list.h, but there is no general solution for this yet. I'll
try to cook something in a different patch first then.

Good catch otherwise :) Didn't see it during manual testing because the platform
doesn't distinguish between user and CA certs at the moment. :/

[digit1, 2012-11-27 16:52:33]

fwiw, I've uploaded https://chromiumcodereview.appspot.com/11415152/ which
addresses the Java template issue. I'll update this patch to use it as soon as
it's submitted.

[darin (slow to review), 2012-11-27 21:27:53]

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util...
net/base/mime_util.cc:355: { "application/x-x509-user-cert",
On 2012/11/27 10:19:48, digit1 wrote:
> I treated this a a line-continuation indent. Do you mean you prefer something
> like:
> 
>   { "application/x-x509-user-cert",
>     CERTIFICATE_MIME_TYPE_X509_USER_CERT },
> 
> The coding style isn't very  specific about this.
> 

It's an array, much like supported_non_image_types above, so the opening bracket
should be indented by two spaces.

[digit1, 2012-11-27 22:33:04]

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/28003/net/base/mime_util...
net/base/mime_util.cc:355: { "application/x-x509-user-cert",
Oh snap. Of course, I wasn't looking at the right indentation. Sorry about that.
Done

[digit1, 2012-12-10 20:43:52]

Finally I could upload a new version of this patch. Note that the Java constants
are now auto-generated from net/base/mime_util_certificate_list.h, and that the
code has been updated to match the code that was moved into
content/browser/loader/ (was in content/browser/renderer_host/, IRC)

I'll update the commit message to include the correct BUG= and TEST= entries.

[digit1, 2012-12-10 21:10:30]

https://chromiumcodereview.appspot.com/11266008/diff/32001/net/android/java/s...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://chromiumcodereview.appspot.com/11266008/diff/32001/net/android/java/s...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:35: private
static final int CERTIFICATE_MIME_TYPE_UNKNOWN = 0;
these values are obsolete now, I'll remove them.

[digit1, 2012-12-10 21:52:05]

https://chromiumcodereview.appspot.com/11266008/diff/32001/net/android/java/s...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://chromiumcodereview.appspot.com/11266008/diff/32001/net/android/java/s...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:35: private
static final int CERTIFICATE_MIME_TYPE_UNKNOWN = 0;
On 2012/12/10 21:10:31, digit1 wrote:
> these values are obsolete now, I'll remove them.

Done.

[darin (slow to review), 2012-12-11 00:00:08]

LGTM

https://chromiumcodereview.appspot.com/11266008/diff/42001/net/base/cert_data...
File net/base/cert_database_openssl.cc (right):

https://chromiumcodereview.appspot.com/11266008/diff/42001/net/base/cert_data...
net/base/cert_database_openssl.cc:25: #if defined(OS_ANDROID)
nit: Would it make more sense to create a CertDatabaseAndroid instead?  It seems
like CertDatabase does nothing under OS_ANDROID.

[digit1, 2012-12-11 13:34:59]

On 2012/12/11 00:00:08, darin wrote:
https://chromiumcodereview.appspot.com/11266008/diff/42001/net/base/cert_data...
> net/base/cert_database_openssl.cc:25: #if defined(OS_ANDROID)
> nit: Would it make more sense to create a CertDatabaseAndroid instead?  It
seems
> like CertDatabase does nothing under OS_ANDROID.

Yes, good idea, I did just that in patch set 11.

[commit-bot: I haz the power, 2012-12-11 16:17:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11266008/46001

[darin (slow to review), 2012-12-11 17:13:09]

On 2012/12/11 13:34:59, digit1 wrote:
> On 2012/12/11 00:00:08, darin wrote:
>
https://chromiumcodereview.appspot.com/11266008/diff/42001/net/base/cert_data...
> > net/base/cert_database_openssl.cc:25: #if defined(OS_ANDROID)
> > nit: Would it make more sense to create a CertDatabaseAndroid instead?  It
> seems
> > like CertDatabase does nothing under OS_ANDROID.
> 
> Yes, good idea, I did just that in patch set 11.

OK, cool.

[commit-bot: I haz the power, 2012-12-11 18:23:00]

Change committed as 172350