[Remoting Host] Select Latest Valid Cert

[Remoting Host] Select Latest Valid Cert

Currently TokenValidatorBase will always select the first certificate for
third-party authentication that matches the issuer but sometimes an incorrect
certificate will be selected.

This CL tries to improve this by:
* Not selecting certificate that is obviously expired (now > valid_expiry).
* Selecting the certificate with latest |valid_start| time.
* Selecting the certifiacte with latest |valid_expiry| time when |valid_start|
  is the same.

BUG=646944
Committed: https://crrev.com/e4807badb80cef2a24ffd3fdb3b8c28859c65a5c
Cr-Commit-Position: refs/heads/master@{#422001}

[Yuwei, 2016-09-26 23:25:16]

Patchset #3 (id:40001) has been deleted

[Yuwei, 2016-09-26 23:46:17]

Description was changed from

==========
[Remoting Linux] Select Latest Valid Cert

BUG=
==========

to

==========
[Remoting Host] Select Latest Valid Cert

Currently TokenValidatorBase will always select the first certificate for
third-party authentication that matches the issuer but sometimes an incorrect
certificate will be selected.

This CL tries to improve this by:
* Not selecting certificate that is obviously expired (now > valid_expiry).
* Selecting the certificate with latest |valid_start| time.
* Selecting the certifiacte with latest |valid_expiry| time when |valid_start|
  is the same.

BUG=646944
==========

[Yuwei, 2016-09-27 00:09:37]

yuweih@chromium.org changed reviewers:
+ jamiewalch@chromium.org, lambroslambrou@chromium.org

[Yuwei, 2016-09-27 00:09:37]

PTAL

This should theoretically work but I still couldn't figure out how to do a real
scenario test or write unit test that mocks net::X509Certificate...

[Lambros, 2016-09-27 00:23:33]

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:48: net::X509Certificate*
GetBestCertificate(net::X509Certificate* c1,
I wonder if this could be written as a Predicate:
bool betterThan(cert* c1, cert* c2) {..}
?
And apply it with std::max_element over the list?

[Sergey Ulanov, 2016-09-27 00:40:06]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-09-27 00:40:06]

I don't think you really need to mock X509Certificate to test this logic. The
test could use net::CreateKeyAndSelfSignedCert() to generate X509 certs.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:46: // Returns the best certificate to
use. The returned value will only be null if
maybe explain what "best certificate" means.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:53: if (!c2) {
I think this code would be simpler if it didn't have to deal with null
parameters. c2 is never null here. Also the case when c1 is null can be handled
in GetBestCertificate().

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:56: if (c1->valid_start() !=
c2->valid_start()) {
remove  {} for consistency with other single-line if statements in this file

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:57: return c1->valid_start() >
c2->valid_start() ? c1 : c2;
Should we also verify that valid_start() is not in future?

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:193: if (request_) {
if (!request)
  return;

[Yuwei, 2016-09-27 21:37:42]

PTAL

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:46: // Returns the best certificate to
use. The returned value will only be null if
On 2016/09/27 00:40:06, Sergey Ulanov wrote:
> maybe explain what "best certificate" means.

Done.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:48: net::X509Certificate*
GetBestCertificate(net::X509Certificate* c1,
On 2016/09/27 00:23:33, Lambros wrote:
> I wonder if this could be written as a Predicate:
> bool betterThan(cert* c1, cert* c2) {..}
> ?
> And apply it with std::max_element over the list?

Done.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:53: if (!c2) {
On 2016/09/27 00:40:06, Sergey Ulanov wrote:
> I think this code would be simpler if it didn't have to deal with null
> parameters. c2 is never null here. Also the case when c1 is null can be
handled
> in GetBestCertificate().

Obsolete. Now c1 and c2 will never be null.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:56: if (c1->valid_start() !=
c2->valid_start()) {
On 2016/09/27 00:40:06, Sergey Ulanov wrote:
> remove  {} for consistency with other single-line if statements in this file

Done.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:57: return c1->valid_start() >
c2->valid_start() ? c1 : c2;
On 2016/09/27 00:40:06, Sergey Ulanov wrote:
> Should we also verify that valid_start() is not in future?

Yep. Forgot that... Added the check.

https://codereview.chromium.org/2369193002/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:193: if (request_) {
On 2016/09/27 00:40:06, Sergey Ulanov wrote:
> if (!request)
>   return;

Done.

https://codereview.chromium.org/2369193002/diff/80001/remoting/host/token_val...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2369193002/diff/80001/remoting/host/token_val...
remoting/host/token_validator_base.cc:63: // 1. An invalid certificate is always
worse than a valid certificate.
Ideally if there is something like a filtering iterator adapter then WorseThan
won't need to care whether a certificate is valid. Doesn't look like STL has
such thing...

[Lambros, 2016-09-27 22:13:58]

lgtm, thanks!

https://codereview.chromium.org/2369193002/diff/120001/remoting/host/token_va...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2369193002/diff/120001/remoting/host/token_va...
remoting/host/token_validator_base.cc:79: if (!c2_valid)
Optional: If this 'if' were moved before the first 'if', the first 'if' could be
simplified, because c2_valid would be true.

[Yuwei, 2016-09-27 22:21:46]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 22:22:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuwei, 2016-09-27 22:22:50]

@lambroslambrou thank you and @sergeyu PTAL :)

https://codereview.chromium.org/2369193002/diff/120001/remoting/host/token_va...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2369193002/diff/120001/remoting/host/token_va...
remoting/host/token_validator_base.cc:79: if (!c2_valid)
On 2016/09/27 22:13:58, Lambros wrote:
> Optional: If this 'if' were moved before the first 'if', the first 'if' could
be
> simplified, because c2_valid would be true.

Done. It can be simplified a lot actually :)

[Sergey Ulanov, 2016-09-29 18:20:28]

lgtm

[Yuwei, 2016-09-29 18:43:26]

Thanks!

BTW I just got a reply from Wan-Teh:

```
This definition shows |notAfter| is a required field of |Validity|,
and |validity| (which is of the type |Validity|) is a required field
of the certificate. So the comment "or the certificate lacks the date"
in net/cert/x509_certificate.h is wrong.

The comment "|valid_expiry| can be be null() if the date is failed to
parse" may be correct, but if anything in the certificate is
incorrectly encoded, I think Chrome won't return an X509Certificate
object at all. It is unlikely that Chrome will return an
X509Certificate object to represent a certificate that it cannot
parse.

I think this is why the HasExpired() implementation simply checks
whether Now() is greater than |valid_expiry|.

> I think my question is, given that |valid_expiry| is null, should I just
> treat the validity status of the certificate as unknown and still give it a
> try?

I believe |valid_expiry| cannot be null in any X509Certificate object
created by Chrome. If |valid_expiry| is null, I think that is a Chrome
bug. You should file a bug report. If for some reason |valid_expiry|
may be null, you should reject that certificate because it is
incorrectly encoded.
```

It sounds like a certificate without |valid_expiry| is just broken and Chrome
may not even return you such a certificate. I think I can remove the
`valid_expiry.is_null()->[valid_start, +inf)` logic from the validity check.

[Yuwei, 2016-09-29 19:07:03]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:07:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 20:20:17]

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

[commit-bot: I haz the power, 2016-09-29 20:20:18]

Dry run: This issue passed the CQ dry run.

[Yuwei, 2016-09-30 00:50:31]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-09-30 00:50:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2369193002/#ps170001
(title: "Remove the [valid_start,  inf) case ")

[commit-bot: I haz the power, 2016-09-30 00:51:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 00:57:20]

Description was changed from

==========
[Remoting Host] Select Latest Valid Cert

Currently TokenValidatorBase will always select the first certificate for
third-party authentication that matches the issuer but sometimes an incorrect
certificate will be selected.

This CL tries to improve this by:
* Not selecting certificate that is obviously expired (now > valid_expiry).
* Selecting the certificate with latest |valid_start| time.
* Selecting the certifiacte with latest |valid_expiry| time when |valid_start|
  is the same.

BUG=646944
==========

to

==========
[Remoting Host] Select Latest Valid Cert

Currently TokenValidatorBase will always select the first certificate for
third-party authentication that matches the issuer but sometimes an incorrect
certificate will be selected.

This CL tries to improve this by:
* Not selecting certificate that is obviously expired (now > valid_expiry).
* Selecting the certificate with latest |valid_start| time.
* Selecting the certifiacte with latest |valid_expiry| time when |valid_start|
  is the same.

BUG=646944
==========

[commit-bot: I haz the power, 2016-09-30 00:57:23]

Committed patchset #9 (id:170001)

[commit-bot: I haz the power, 2016-09-30 01:01:20]

Description was changed from

==========
[Remoting Host] Select Latest Valid Cert

Currently TokenValidatorBase will always select the first certificate for
third-party authentication that matches the issuer but sometimes an incorrect
certificate will be selected.

This CL tries to improve this by:
* Not selecting certificate that is obviously expired (now > valid_expiry).
* Selecting the certificate with latest |valid_start| time.
* Selecting the certifiacte with latest |valid_expiry| time when |valid_start|
  is the same.

BUG=646944
==========

to

==========
[Remoting Host] Select Latest Valid Cert

Currently TokenValidatorBase will always select the first certificate for
third-party authentication that matches the issuer but sometimes an incorrect
certificate will be selected.

This CL tries to improve this by:
* Not selecting certificate that is obviously expired (now > valid_expiry).
* Selecting the certificate with latest |valid_start| time.
* Selecting the certifiacte with latest |valid_expiry| time when |valid_start|
  is the same.

BUG=646944

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

[commit-bot: I haz the power, 2016-09-30 01:01:22]

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