Add CheckOCSPDateValid() to net/cert/internal

net/cert/internal/parse_ocsp.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 
 #include "base/sha1.h"
 #include "crypto/sha2.h"
 #include "net/cert/internal/parse_ocsp.h"
 
@@ skipping 511 matching lines @@
       }
     }
   }
 
   if (!found)
     out->status = OCSPCertStatus::Status::UNKNOWN;
 
   return found;
 }
 
+bool CheckOCSPDateValid(const OCSPSingleResponse& response,
+                        const base::Time& verify_time,
+                        const base::TimeDelta& max_age) {
+  der::GeneralizedTime verify_time_der = der::ConvertBaseUTCTime(verify_time);
+
+  // Enforce thisUpdate <= |verify_time|.

[Ryan Sleevi, 2016/06/28 17:33:30]

Unnecessary comment

[dadrian, 2016/06/29 22:54:02]

Done.

+  if (response.this_update > verify_time_der)

[Ryan Sleevi, 2016/06/28 17:33:29]

Why use > when the comment describes it as <=?

[dadrian, 2016/06/28 19:15:27]

In addressing previous comments from svaldez, I changed every condition remove
the leading negative, e.g. change
   if (!(x <= y)
to
   if (x > y).

+    return false;
+
+  // Enforce |verify_time| < thisUpdate + |max_age|.

[Ryan Sleevi, 2016/06/28 17:33:29]

Unnecessary comment

[dadrian, 2016/06/29 22:54:02]

Done.

+  der::GeneralizedTime lower_bound =
+      der::ConvertBaseUTCTime(verify_time - max_age);
+  if (response.this_update <= lower_bound)

[Ryan Sleevi, 2016/06/28 17:33:30]

DESIGN: This code is quite confusing with the comment - why take verify_time -
max_age? Why not take this_update + max_age?

The implications for over/underflow are not clear here, so your design choice is
.. subtle and confusing.

[dadrian, 2016/06/28 19:15:27]

It's written this way because addition is not defined on der::GeneralizedTime
(and probably should not be).

[Ryan Sleevi, 2016/06/28 19:26:44]

If I understand your response, you're arguing that you did this math (on
base::Time), because you don't want math to be defined for der::GeneralizedTime
- only comparisons (since </=/> can all be implemented in terms of ASCII
comparisons). Is that correct?

If so, I think this code definitely needs restructuring, both in comments and
structure, to make it obvious why it is you're doing what you're doing
(understandably, readability isn't helped when we're talking base::Time,
base::TimeDelta, and der::GeneralizedTime all using the same noun of time)

[dadrian, 2016/06/29 22:54:02]

Done.

+    return false;
+
+  // Enforce |verify_time| < nextUpdate, if present.
+  if (response.has_next_update &&
+      (response.next_update <= response.this_update)) {
+    return false;
+  }
+  if (response.has_next_update && (response.next_update <= verify_time_der))
+    return false;

[Ryan Sleevi, 2016/06/28 17:33:29]

DESIGN: Why structure the conditionals like this?

if (!response.has_next_update)
  return true;

if (response.next_update <= response.this_update)
  return false;  // Why? Isn't this more of a parge/logic bug? Should this be
captured during input processing?

if (response.next_update <= verify_time_der)
  return false;

[dadrian, 2016/06/28 19:15:27]

I was trying to avoid the possible future bug, where a conditional is added to
the function, but not executed, because the function returned |true| early
erroneously.
I disagree; while it may be reasonable to have parsing verify that thisUpdate
and nextUpdate are well-formed GeneralizedTime's (e.g. not month 13), and do the
remaining date validation here, rather than needlessly splitting it up into two
places. That being said, this check is technically covered by the union of
enforcing thisUpdate <= |verify_time| and |verify_time| < nextUpdate.

[Ryan Sleevi, 2016/06/28 19:26:44]

I don't think the readability sacrifice is worth that hypothetical. 

Concretely: Please restructure this code

I'm fine if you want to do:
if (response.has_next_update) {
  if (response.next_update <= response.this_update)
    return false;  // Explain why
  if (response.next_update <= verify_time_der)
    return false;  // Explain why
}
return true;

But I think the early return is perfectly acceptable. Your hypothetical involves
both the author and the reviewer not reading the code, which I don't think for a
function as small as this, if properly documented, is a realistic concern. We
should take the fast path and reduce conditionals.
I'm not sure why you draw this distinction. We enforce other 5280-sanity checks
during parsing/validation.
Yes, I'm aware, but I don't think this is the place to encode that check.

[dadrian, 2016/06/29 22:54:02]

Done.

+
+  return true;
+}
+
 }  // namespace net