Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463
Committed: https://crrev.com/a758ec2ba845713d654a75c2c0251d460244c58d
Cr-Commit-Position: refs/heads/master@{#400396}

[ryanchung, 2016-06-16 21:53:49]

Description was changed from

==========
Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463
==========

to

==========
Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463
==========

[ryanchung, 2016-06-16 21:53:50]

ryanchung@chromium.org changed reviewers:
+ eroman@chromium.org, mattm@chromium.org

[mattm, 2016-06-16 22:08:46]

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:501: *trusted_chain_out =
full_chain;
Does
*trusted_chain_out = std::move(full_chain)
work?

[eroman, 2016-06-16 22:11:36]

LGTM after adding some testing.

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:501: *trusted_chain_out =
full_chain;
On 2016/06/16 22:08:46, mattm wrote:
> Does
> *trusted_chain_out = std::move(full_chain)
> work?

+1

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.h (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.h:58: //     ignored.
Please additionally mention the order of the vector -- namely
trusted_chain_out->front() is the target certificate, and
trusted_chain_out->back() is the trust anchor.

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain_unittest.cc (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:117: bool result =
VerifyCertificateChain(input_chain, trust_store,
Please add some form of verification of the out parameter.

Basically it should be one longer than |input_chain|, match on all those
elements, and then the last element should be the trusted certificate in
trust_store.

Can check the last one by using
TrustStore::IsTrustedCertificate(built_chain.back()) or something like that.

[ryanchung, 2016-06-16 22:47:09]

Thanks.

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:501: *trusted_chain_out =
full_chain;
On 2016/06/16 22:11:36, eroman wrote:
> On 2016/06/16 22:08:46, mattm wrote:
> > Does
> > *trusted_chain_out = std::move(full_chain)
> > work?
> 
> +1

Done.

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.h (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.h:58: //     ignored.
On 2016/06/16 22:11:36, eroman wrote:
> Please additionally mention the order of the vector -- namely
> trusted_chain_out->front() is the target certificate, and
> trusted_chain_out->back() is the trust anchor.

Done.

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain_unittest.cc (right):

https://codereview.chromium.org/2078653002/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:117: bool result =
VerifyCertificateChain(input_chain, trust_store,
On 2016/06/16 22:11:36, eroman wrote:
> Please add some form of verification of the out parameter.
> 
> Basically it should be one longer than |input_chain|, match on all those
> elements, and then the last element should be the trusted certificate in
> trust_store.
> 
> Can check the last one by using
> TrustStore::IsTrustedCertificate(built_chain.back()) or something like that.

Done.

[eroman, 2016-06-16 22:55:01]

lgtm

https://codereview.chromium.org/2078653002/diff/40001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain_unittest.cc (right):

https://codereview.chromium.org/2078653002/diff/40001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:121:
ASSERT_EQ(trusted_chain.size(), input_chain.size() + 1);
This is true for most if not all the tests.
I guess run the tests and see if this works :)

It is possible we also have a test where the input chain contained the trust
anchor -- I don't remember if I added that as part of these unit tests or
elsewhere...

If that is the case you may need to relax this to allow for both possibilities.

https://codereview.chromium.org/2078653002/diff/40001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:123: input_chain.begin()
+ input_chain.size(),
nit: input_chain.end()

[ryanchung, 2016-06-16 23:13:09]

https://codereview.chromium.org/2078653002/diff/40001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain_unittest.cc (right):

https://codereview.chromium.org/2078653002/diff/40001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:121:
ASSERT_EQ(trusted_chain.size(), input_chain.size() + 1);
On 2016/06/16 22:55:01, eroman wrote:
> This is true for most if not all the tests.
> I guess run the tests and see if this works :)
> 
> It is possible we also have a test where the input chain contained the trust
> anchor -- I don't remember if I added that as part of these unit tests or
> elsewhere...
> 
> If that is the case you may need to relax this to allow for both
possibilities.

All tests passed locally. Will let try bot confirm.

https://codereview.chromium.org/2078653002/diff/40001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:123: input_chain.begin()
+ input_chain.size(),
On 2016/06/16 22:55:01, eroman wrote:
> nit: input_chain.end()

Done.

[ryanchung, 2016-06-16 23:21:37]

ryanchung@chromium.org changed reviewers:
+ sheretov@chromium.org

[ryanchung, 2016-06-16 23:21:38]

+sheretov for components/cast_certificate

Thanks.

[sheretov, 2016-06-17 01:00:50]

On 2016/06/16 23:21:38, ryanchung wrote:
> +sheretov for components/cast_certificate
> 
> Thanks.

components/cast_certificate lgtm

[ryanchung, 2016-06-17 01:02:21]

The CQ bit was checked by ryanchung@chromium.org

[ryanchung, 2016-06-17 01:02:22]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2078653002/#ps60001
(title: "Fixed nit")

[commit-bot: I haz the power, 2016-06-17 01:03:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078653002/60001

[commit-bot: I haz the power, 2016-06-17 02:38:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-17 02:38:15]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ryanchung, 2016-06-17 08:19:01]

The CQ bit was checked by ryanchung@chromium.org

[commit-bot: I haz the power, 2016-06-17 08:19:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078653002/60001

[commit-bot: I haz the power, 2016-06-17 08:23:21]

Description was changed from

==========
Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463
==========

to

==========
Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463
==========

[commit-bot: I haz the power, 2016-06-17 08:23:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-17 08:25:09]

Description was changed from

==========
Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463
==========

to

==========
Add an out parameter to VerifyCertificateChain for retrieving the trusted chain.

Until certificate path building is available, this will be temporarily used
to obtain the full chain built by VerifyCertificateChain.

BUG=618463

Committed: https://crrev.com/a758ec2ba845713d654a75c2c0251d460244c58d
Cr-Commit-Position: refs/heads/master@{#400396}
==========

[commit-bot: I haz the power, 2016-06-17 08:25:10]

Patchset 4 (id:??) landed as
https://crrev.com/a758ec2ba845713d654a75c2c0251d460244c58d
Cr-Commit-Position: refs/heads/master@{#400396}