Fix a glitch in disentanglement of CertVerifyProc(OpenSSL/Android)

Fix a glitch in disentanglement of CertVerifyProc(OpenSSL/Android)

This refers to https://chromiumcodereview.appspot.com/11549033/, which separated OpenSSL/Android certificate verification routines.

As joth@chromium.org pointed out, the above erroneously omits hard-setting |issued_by_trusted_root| field of the result on Android. This patch fixes this omission.    

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

[ppi, 2012-12-14 04:13:56]

A simple fix, could you guys have a quick look?

[joth, 2012-12-14 05:58:55]

Lgtm


On Thursday, 13 December 2012,  <ppi@chromium.org> wrote:
> Reviewers: joth, Ryan Sleevi,
>
> Message:
> A simple fix, could you guys have a quick look?
>
> Description:
> Fix a glitch in disentanglement of CertVerifyProc(OpenSSL/Android)
>
> This refers to https://chromiumcodereview.appspot.com/11549033/, which
separated
> OpenSSL/Android certificate verification routines.
>
> As joth@chromium.org pointed out, the above erroneously omits hard-setting
> |issued_by_trusted_root| field of the result on Android. This patch fixes
this
> omission.
>
> BUG=147786
>
> Please review this at https://codereview.chromium.org/11570019/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>   M net/base/cert_verify_proc_android.cc
>
>
> Index: net/base/cert_verify_proc_android.cc
> diff --git a/net/base/cert_verify_proc_android.cc
b/net/base/cert_verify_proc_android.cc
> index
1bdbcef87b22eb71937be686f02be26d6d194943..fc46251f09475862e92eef96d4d790a0774b4ba1
100644
> --- a/net/base/cert_verify_proc_android.cc
> +++ b/net/base/cert_verify_proc_android.cc
> @@ -88,6 +88,14 @@ int
CertVerifyProcAndroid::VerifyInternal(X509Certificate* cert,
>    if (IsCertStatusError(verify_result->cert_status))
>      return MapCertStatusToNetError(verify_result->cert_status);
>
> +  // Android call does not provide information if the trust root at the
end of
> +  // the constructed chain was standard or user-added CA, so we mark all
> +  // correctly verified certificates as issued by a known root.
> +  verify_result->is_issued_by_known_root = true;
> +
> +  // TODO(ppi): Implement missing functionality of certificate
verification:
> +  // providing the constructed trust chain (along with public key hashes
of its
> +  // certificates). See also: crbug.com/116838
>    return OK;
>  }
>
>
>
>

[Ryan Sleevi, 2012-12-14 18:22:07]

LGTM, mod nit.

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

https://codereview.chromium.org/11570019/diff/4001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:94: verify_result->is_issued_by_known_root
= true;
blaaarghh

This will need to be a system API feature request, otherwise getting the
verified hashes back won't help much for pinning.

https://codereview.chromium.org/11570019/diff/4001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:98: // certificates). See also:
crbug.com/116838
nit: http://-ify the link

[ppi, 2012-12-14 20:27:35]

Thanks joth and Ryan!

I have uploaded patch set 2 addressing Ryan's remarks - unless anybody suggests
further changes, I will proceed with committing later today.

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

https://codereview.chromium.org/11570019/diff/4001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:94: verify_result->is_issued_by_known_root
= true;
On 2012/12/14 18:22:07, Ryan Sleevi wrote:
> blaaarghh
> 
> This will need to be a system API feature request, otherwise getting the
> verified hashes back won't help much for pinning.

I edited the comment to mention the need of such support.

https://codereview.chromium.org/11570019/diff/4001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:98: // certificates). See also:
crbug.com/116838
On 2012/12/14 18:22:07, Ryan Sleevi wrote:
> nit: http://-ify the link

Thanks, fixed.

[joth, 2012-12-14 20:30:01]

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

https://codereview.chromium.org/11570019/diff/4001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_android.cc:94: verify_result->is_issued_by_known_root
= true;
On 2012/12/14 20:27:35, ppi wrote:
> On 2012/12/14 18:22:07, Ryan Sleevi wrote:
> > blaaarghh
> > 
> > This will need to be a system API feature request, otherwise getting the
> > verified hashes back won't help much for pinning.
> 
> I edited the comment to mention the need of such support.

A solution was discussed in b/5826113 (linked from the bug you reference below)

[commit-bot: I haz the power, 2012-12-14 21:41:53]

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

[commit-bot: I haz the power, 2012-12-15 02:15:16]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 02:21:25]

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

[commit-bot: I haz the power, 2012-12-15 05:59:13]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-17 10:21:37]

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

[commit-bot: I haz the power, 2012-12-17 15:01:54]

Change committed as 173458