Separate CertVerifyProcAndroid from CertVerifyProcOpenSSL

Separate CertVerifyProcAndroid from CertVerifyProcOpenSSL


Since we no longer rely on OpenSSL to perform certificate verification on
Android, Android-specific logic can be moved to a separate class.

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

[ppi, 2012-12-13 01:59:56]

What do you guys think?

[Ryan Sleevi, 2012-12-13 02:16:36]

Some minor cleanup while you're here

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc.cc
File net/base/cert_verify_proc.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc....
net/base/cert_verify_proc.cc:57: return new CertVerifyProcAndroid();
In "most" places, we try to follow the order of

if USE_X
elif USE Y
elif OS_A
elif OS_B
elif OS_C

so that USE_ directives always take precedence. Mostly due to how OS_ variables
can clash (eg: OS_IOS)

perhaps

#elif defined(USE_OPENSSL) && !defined(USE_ANDROID)

I'm not strictly wedded to this, but I like this style because I think it makes
it clearer the relationship between these two variables then the positional form
(ie: what you have)

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_android.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:25: // TODO(joth): Fetch the authentication
type from SSL rather than hardcode.
Reference the crbug DIGIT filed

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:60:
X509Certificate::X509Certificate::GetDEREncoded(*it, &cert_bytes);
[existing?] BUG: Check the ret value of GetDEREncoded

This method should also be a bool so that you can fail validation on invalid
certs.

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_android.h (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.h:12: // Performs platform-based certificate
verification on Android.
This comment is not as clear as what we've tried for the other platforms.

You should mention JNI here

// Performs certificate verification on Android by using JNI to [whatever the
API/framework is called]

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_openssl.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_openssl.cc:193: CHECK_EQ(1, rv);
[existing?] BUG: I don't think this should be a CHECK - both
|cert->os_Cert_handle()| and |intermediates.get()| represent potentially
'attacker' (read: user) input.

[ppi, 2012-12-13 04:49:08]

Thanks for the remarks, Ryan! I have uploaded patch set 2, please find my
replies to individual issues below.

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc.cc
File net/base/cert_verify_proc.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc....
net/base/cert_verify_proc.cc:57: return new CertVerifyProcAndroid();
On 2012/12/13 02:16:36, Ryan Sleevi wrote:
> In "most" places, we try to follow the order of
> 
> if USE_X
> elif USE Y
> elif OS_A
> elif OS_B
> elif OS_C
> 
> so that USE_ directives always take precedence. Mostly due to how OS_
variables
> can clash (eg: OS_IOS)
> 
> perhaps
> 
> #elif defined(USE_OPENSSL) && !defined(USE_ANDROID)
> 
> I'm not strictly wedded to this, but I like this style because I think it
makes
> it clearer the relationship between these two variables then the positional
form
> (ie: what you have)

Thanks, I agree that your suggested form is clearer. Addressed in patch set 2.

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_android.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:25: // TODO(joth): Fetch the authentication
type from SSL rather than hardcode.
On 2012/12/13 02:16:36, Ryan Sleevi wrote:
> Reference the crbug DIGIT filed

My understanding is that David's bug
(https://code.google.com/p/chromium/issues/detail?id=165446) refers: (a) to
client rather than server certificate verification (b) to OpenSSL functionality,
which should not be within the concern of this file. 

Do you think that the bug should be nevertheless referenced?

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:60:
X509Certificate::X509Certificate::GetDEREncoded(*it, &cert_bytes);
On 2012/12/13 02:16:36, Ryan Sleevi wrote:
> [existing?] BUG: Check the ret value of GetDEREncoded
> 
> This method should also be a bool so that you can fail validation on invalid
> certs.

Thanks, addressed in patch set 2.

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_android.h (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.h:12: // Performs platform-based certificate
verification on Android.
On 2012/12/13 02:16:36, Ryan Sleevi wrote:
> This comment is not as clear as what we've tried for the other platforms.
> 
> You should mention JNI here
> 
> // Performs certificate verification on Android by using JNI to [whatever the
> API/framework is called]

Absolutely, addressed in patch set 2.

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_openssl.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_openssl.cc:193: CHECK_EQ(1, rv);
On 2012/12/13 02:16:36, Ryan Sleevi wrote:
> [existing?] BUG: I don't think this should be a CHECK - both
> |cert->os_Cert_handle()| and |intermediates.get()| represent potentially
> 'attacker' (read: user) input.

Nice catch! I will address this separately: crbug.com/165858 .

[digit1, 2012-12-13 12:14:24]

lgtm (but I'm not an owner :-))

[digit1, 2012-12-13 12:14:29]

https://chromiumcodereview.appspot.com/11549033/diff/2001/net/base/cert_verif...
File net/base/cert_verify_proc_android.cc (right):

https://chromiumcodereview.appspot.com/11549033/diff/2001/net/base/cert_verif...
net/base/cert_verify_proc_android.cc:25: // TODO(joth): Fetch the authentication
type from SSL rather than hardcode.
It's really a different bug. On the other hand, this can be fixed relatively
easily here by using a function like X509Certificate::GetPublicKeyInfo() to
retrieve the key type and translate that into a string. I'd recommend doing this
in a different patch though.

[Ryan Sleevi, 2012-12-13 19:50:50]

lgtm

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
File net/base/cert_verify_proc_android.cc (right):

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
net/base/cert_verify_proc_android.cc:53: if (cert_handles.empty() ||
cert_handles[0] != cert_handle)
This second condition should always be false. It's a bug if cert_handles[0] ever
== cert_handle.

For further cleanup though.

[ppi, 2012-12-13 21:06:39]

https://codereview.chromium.org/11549033/diff/10001/net/base/cert_verify_proc...
File net/base/cert_verify_proc_android.cc (right):

https://codereview.chromium.org/11549033/diff/10001/net/base/cert_verify_proc...
net/base/cert_verify_proc_android.cc:53: if (cert_handles.empty() ||
cert_handles[0] != cert_handle)
On 2012/12/13 19:50:50, Ryan Sleevi wrote:
> This second condition should always be false. It's a bug if cert_handles[0]
ever
> == cert_handle.
> 
> For further cleanup though.

Thanks! If you don't mind I will address this separately.

[ppi, 2012-12-13 21:09:52]

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_android.cc (right):

https://codereview.chromium.org/11549033/diff/2001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:25: // TODO(joth): Fetch the authentication
type from SSL rather than hardcode.
On 2012/12/13 12:14:29, digit1 wrote:
> It's really a different bug. On the other hand, this can be fixed relatively
> easily here by using a function like X509Certificate::GetPublicKeyInfo() to
> retrieve the key type and translate that into a string. I'd recommend doing
this
> in a different patch though.

Sounds good, I will take care of this. Thanks!

[commit-bot: I haz the power, 2012-12-13 21:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/11549033/10001

[commit-bot: I haz the power, 2012-12-14 00:17:21]

Change committed as 173016

[joth, 2012-12-14 00:45:01]

Nice! great to see these two split apart at last.

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
File net/base/cert_verify_proc_android.cc (right):

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
net/base/cert_verify_proc_android.cc:89: return
MapCertStatusToNetError(verify_result->cert_status);
Bug?
I think prior to this patch even on OS_ANDROID we'd be setting
verify_result->is_issued_by_known_root = true here.

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
net/base/cert_verify_proc_android.cc:90: 
should there be a TODO / bug link here to implement the other missing bits?
- GetCertChainInfo(ctx.get(), verify_result);
- AppendPublicKeyHashes()


they depend on  crbug.com/116838 IIRC.

[ppi, 2012-12-14 04:02:32]

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
File net/base/cert_verify_proc_android.cc (right):

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
net/base/cert_verify_proc_android.cc:89: return
MapCertStatusToNetError(verify_result->cert_status);
On 2012/12/14 00:45:01, joth wrote:
> Bug?
> I think prior to this patch even on OS_ANDROID we'd be setting
> verify_result->is_issued_by_known_root = true here.
> 

My bad, thanks for catching that! I am fixing this in
https://codereview.chromium.org/11570019/ .

https://chromiumcodereview.appspot.com/11549033/diff/10001/net/base/cert_veri...
net/base/cert_verify_proc_android.cc:90: 
On 2012/12/14 00:45:01, joth wrote:
> should there be a TODO / bug link here to implement the other missing bits?
> - GetCertChainInfo(ctx.get(), verify_result);
> - AppendPublicKeyHashes()
> 
> 
> they depend on  crbug.com/116838 IIRC.

Good idea, thanks. Being addressed in https://codereview.chromium.org/11570019/.