Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091103002:
Add CheckOCSPDateValid() to net/cert/internal (Closed)
DescriptionAdd CheckOCSPDateValid() to net/cert/internal
Intended to be used internally by certificate validation as part of OCSP
validity checks and Expect-Staple.
BUG=598021
Committed: https://crrev.com/687cf9fffda1a7eb45c80a596fa4f9e3e524f0e4
Cr-Commit-Position: refs/heads/master@{#404708}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Add tests for ConvertBaseUTCTime #
Total comments: 29
Patch Set 4 : Address comments from rsleevi@ #
Total comments: 8
Patch Set 5 : Readable unit tests #Patch Set 6 : EncodeGeneralizedTime can fail #Patch Set 7 : Return a bool #Patch Set 8 : Add tests for Windows edge cases #Patch Set 9 : Fix compilation #
Total comments: 6
Patch Set 10 : All hail the style guide. #Patch Set 11 : Fix compilation error. #
Total comments: 6
Patch Set 12 : Nits. #Patch Set 13 : Remove extra include. #
Messages
Total messages: 43 (10 generated)
dadrian@google.com changed reviewers: + estark@chromium.org, rsleevi@chromium.org, svaldez@chromium.org
rsleevi@: I'm starting to pull apart https://codereview.chromium.org/2040513003/ per your suggestion in chat, and will rebase and update that CL as others land. This CL pulls out the date checking logic into net/cert/internal.
Happy to let Steven drive this review https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.cc:541: der::GeneralizedTime verify_time_der = der::ConvertBaseUTCTime(verify_time); The purpose of this function is just so you don't have to decode this_update and next_update? https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.cc:554: } STYLE: Keep brace style consistent with local file & directory ( https://google.github.io/styleguide/cppguide.html#Conditionals "In general" allows it, and we value consistency) https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:11: #include "base/time/time.h" Forward declare https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:129: // Converts a base::Time assuming the time is in UTC. Comment nit: This isn't that descriptive // Encodes |time|, a UTC-based time, to DER, for comparing against // other times.
https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocsp.h File net/cert/internal/parse_ocsp.h (right): https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.h:12: #include "base/time/time.h" should be able to forward-declare this https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.h:283: // thisUpdate + max_age. Maybe also document what happens if there is no |nextUpdate|. https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp_unittest.cc:185: const base::TimeDelta kOCSPAgeOneWeek = base::TimeDelta::FromDays(7); this could go in the anonymous namespace
https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.cc:541: der::GeneralizedTime verify_time_der = der::ConvertBaseUTCTime(verify_time); On 2016/06/23 21:27:35, Ryan Sleevi wrote: > The purpose of this function is just so you don't have to decode this_update and > next_update? As far as I know there's no function to convert a der::GeneralizedTime to a base::Time. The goal is just be able to compare timestamps. https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.cc:554: } On 2016/06/23 21:27:35, Ryan Sleevi wrote: > STYLE: Keep brace style consistent with local file & directory ( > https://google.github.io/styleguide/cppguide.html#Conditionals "In general" > allows it, and we value consistency) Done. Sigh, old habit, my bad. https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocsp.h File net/cert/internal/parse_ocsp.h (right): https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.h:12: #include "base/time/time.h" On 2016/06/24 00:27:35, estark wrote: > should be able to forward-declare this Done. https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp.h:283: // thisUpdate + max_age. On 2016/06/24 00:27:35, estark wrote: > Maybe also document what happens if there is no |nextUpdate|. Done. https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/1/net/cert/internal/parse_ocs... net/cert/internal/parse_ocsp_unittest.cc:185: const base::TimeDelta kOCSPAgeOneWeek = base::TimeDelta::FromDays(7); On 2016/06/24 00:27:35, estark wrote: > this could go in the anonymous namespace Done. https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:11: #include "base/time/time.h" On 2016/06/23 21:27:35, Ryan Sleevi wrote: > Forward declare Done. https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:129: // Converts a base::Time assuming the time is in UTC. On 2016/06/23 21:27:35, Ryan Sleevi wrote: > Comment nit: This isn't that descriptive > > // Encodes |time|, a UTC-based time, to DER, for comparing against > // other times. Done.
https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2091103002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:386: der::GeneralizedTime ConvertBaseUTCTime(const base::Time& time) { Might need a basic unittest. https://codereview.chromium.org/2091103002/diff/20001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/20001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:545: if (response.has_next_update && (response.next_update <= verify_time_der)) Possibly move this to the bottom, to match the ordering in the header comment. https://codereview.chromium.org/2091103002/diff/20001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:551: if (response.this_update < lower_bound) Use '<=' to match the same upper bound semantic as the next_update <= verify_time check. https://codereview.chromium.org/2091103002/diff/20001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2091103002/diff/20001/net/der/parse_values.cc... net/der/parse_values.cc:398: return result; Might need a unittest, or at the very least throw in a ValidateGeneralizedTime somewhere here.
https://codereview.chromium.org/2091103002/diff/20001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/20001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:545: if (response.has_next_update && (response.next_update <= verify_time_der)) On 2016/06/24 13:40:46, svaldez wrote: > Possibly move this to the bottom, to match the ordering in the header comment. Done. https://codereview.chromium.org/2091103002/diff/20001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:551: if (response.this_update < lower_bound) On 2016/06/24 13:40:46, svaldez wrote: > Use '<=' to match the same upper bound semantic as the next_update <= > verify_time check. Done. https://codereview.chromium.org/2091103002/diff/20001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2091103002/diff/20001/net/der/parse_values.cc... net/der/parse_values.cc:398: return result; On 2016/06/24 13:40:46, svaldez wrote: > Might need a unittest, or at the very least throw in a ValidateGeneralizedTime > somewhere here. Done.
lgtm
On 2016/06/27 13:41:33, svaldez wrote: > lgtm rsleevi: Does this look good to you as well?
https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:537: // Enforce thisUpdate <= |verify_time|. Unnecessary comment https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:538: if (response.this_update > verify_time_der) Why use > when the comment describes it as <=? https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:541: // Enforce |verify_time| < thisUpdate + |max_age|. Unnecessary comment https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:544: if (response.this_update <= lower_bound) 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. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:553: return false; 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; https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.h (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.h:287: // that |verify_time| < nextUpdate. COMMENT: "Checks" doesn't really describe what the bool result would be // Returns true if |response|, a valid OCSP response with a thisUpdate // field and potentially a nextUpdate field, is valid at |verify_time| // and not older than |max_age|. // Expressed differently, returns true if // response.thisUpdate <= verify_time < min(response.thisUpdate + max_age, response.nextUpdate) https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:226: TEST(OCSPDateTest, Backwards) { What is backwards? https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:239: TEST(OCSPDateTest, Long) { What is long? https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:398: DCHECK(ValidateGeneralizedTime(result)); Why DCHECK here? A DCHECK signals that there's a caller error here - so what is the caller supposed to be checking? If they have something that base::Time accepted, why are they at fault? If it's something that time.UTCExplode(...) can't handle (e.g. Y2038), why are they at fault? The core of this should be explaining "How are you supposed to hold this API correctly" - and I'm not sure what the caller is supposed to do, either for 'malicious' inputs or for benign one. If the user's clock is wonky, is that an API misuse error? Or is that a condition we should be handling - gracefully? https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h#... net/der/parse_values.h:134: NET_EXPORT der::GeneralizedTime ConvertBaseUTCTime(const base::Time& time); NAMING: This naming doesn't really help understand what this code does... "ConvertBaseUTCTime" - is it converting timezones? What's it converting to? This is really: EncodeTimeAsGeneralizedTime(...) right? Does this really belong in parse_values? Isn't this an encoding property? https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values_un... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values_un... net/der/parse_values_unittest.cc:372: base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(1466787894); What about the edge cases? What about 1600? (< Windows FILETIME start, < unix epoch) What about 2039? (> Linux 32-bit time_t max)
https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:538: if (response.this_update > verify_time_der) On 2016/06/28 17:33:29, Ryan Sleevi wrote: > Why use > when the comment describes it as <=? In addressing previous comments from svaldez, I changed every condition remove the leading negative, e.g. change if (!(x <= y) to if (x > y). https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:544: if (response.this_update <= lower_bound) On 2016/06/28 17:33:30, Ryan Sleevi wrote: > 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. It's written this way because addition is not defined on der::GeneralizedTime (and probably should not be). https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:553: return false; On 2016/06/28 17:33:29, Ryan Sleevi wrote: > DESIGN: Why structure the conditionals like this? > 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. > 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? 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. > > if (response.next_update <= verify_time_der) > return false; https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:398: DCHECK(ValidateGeneralizedTime(result)); On 2016/06/28 17:33:30, Ryan Sleevi wrote: > Why DCHECK here? A DCHECK signals that there's a caller error here - so what is > the caller supposed to be checking? > > If they have something that base::Time accepted, why are they at fault? > If it's something that time.UTCExplode(...) can't handle (e.g. Y2038), why are > they at fault? > > The core of this should be explaining "How are you supposed to hold this API > correctly" - and I'm not sure what the caller is supposed to do, either for > 'malicious' inputs or for benign one. If the user's clock is wonky, is that an > API misuse error? Or is that a condition we should be handling - gracefully? Presumably we should handle this the same way explode does, although perhaps a better idea is to convert der::GeneralizedTime to base::Time, with an API that can fail similar to Time::FromExploded(), and compare base::Time objects instead. https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h#... net/der/parse_values.h:134: NET_EXPORT der::GeneralizedTime ConvertBaseUTCTime(const base::Time& time); On 2016/06/28 17:33:30, Ryan Sleevi wrote: > NAMING: This naming doesn't really help understand what this code does... > "ConvertBaseUTCTime" - is it converting timezones? What's it converting to? > > This is really: > > EncodeTimeAsGeneralizedTime(...) > > right? > > Does this really belong in parse_values? Isn't this an encoding property? I'm happy to move this function somewhere else, but offhand I don't know a better location.
https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:544: if (response.this_update <= lower_bound) On 2016/06/28 19:15:27, dadrian wrote: > It's written this way because addition is not defined on der::GeneralizedTime > (and probably should not be). 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) https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:553: return false; On 2016/06/28 19:15:27, dadrian wrote: > On 2016/06/28 17:33:29, Ryan Sleevi wrote: > > DESIGN: Why structure the conditionals like this? > > > 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 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 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. I'm not sure why you draw this distinction. We enforce other 5280-sanity checks during parsing/validation. > That being said, this check is technically covered by the union of > enforcing thisUpdate <= |verify_time| and |verify_time| < nextUpdate. Yes, I'm aware, but I don't think this is the place to encode that check. https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h#... net/der/parse_values.h:134: NET_EXPORT der::GeneralizedTime ConvertBaseUTCTime(const base::Time& time); On 2016/06/28 19:15:27, dadrian wrote: > On 2016/06/28 17:33:30, Ryan Sleevi wrote: > > NAMING: This naming doesn't really help understand what this code does... > > "ConvertBaseUTCTime" - is it converting timezones? What's it converting to? > > > > This is really: > > > > EncodeTimeAsGeneralizedTime(...) > > > > right? > > > > Does this really belong in parse_values? Isn't this an encoding property? > > I'm happy to move this function somewhere else, but offhand I don't know a > better location. net/der/encode_values.h ?
https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:537: // Enforce thisUpdate <= |verify_time|. On 2016/06/28 17:33:30, Ryan Sleevi wrote: > Unnecessary comment Done. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:541: // Enforce |verify_time| < thisUpdate + |max_age|. On 2016/06/28 17:33:29, Ryan Sleevi wrote: > Unnecessary comment Done. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:544: if (response.this_update <= lower_bound) On 2016/06/28 19:26:44, Ryan Sleevi wrote: > On 2016/06/28 19:15:27, dadrian wrote: > > It's written this way because addition is not defined on der::GeneralizedTime > > (and probably should not be). > > 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) Done. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.cc:553: return false; On 2016/06/28 19:26:44, Ryan Sleevi wrote: > On 2016/06/28 19:15:27, dadrian wrote: > > On 2016/06/28 17:33:29, Ryan Sleevi wrote: > > > DESIGN: Why structure the conditionals like this? > > > > > 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 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 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. > > I'm not sure why you draw this distinction. We enforce other 5280-sanity checks > during parsing/validation. > > > That being said, this check is technically covered by the union of > > enforcing thisUpdate <= |verify_time| and |verify_time| < nextUpdate. > > Yes, I'm aware, but I don't think this is the place to encode that check. Done. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp.h (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp.h:287: // that |verify_time| < nextUpdate. On 2016/06/28 17:33:30, Ryan Sleevi wrote: > COMMENT: "Checks" doesn't really describe what the bool result would be > > // Returns true if |response|, a valid OCSP response with a thisUpdate > // field and potentially a nextUpdate field, is valid at |verify_time| > // and not older than |max_age|. > // Expressed differently, returns true if > // response.thisUpdate <= verify_time < min(response.thisUpdate + max_age, > response.nextUpdate) Done. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:226: TEST(OCSPDateTest, Backwards) { On 2016/06/28 17:33:30, Ryan Sleevi wrote: > What is backwards? Done. https://codereview.chromium.org/2091103002/diff/40001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:239: TEST(OCSPDateTest, Long) { On 2016/06/28 17:33:30, Ryan Sleevi wrote: > What is long? Done. https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:398: DCHECK(ValidateGeneralizedTime(result)); On 2016/06/28 19:15:27, dadrian wrote: > On 2016/06/28 17:33:30, Ryan Sleevi wrote: > > Why DCHECK here? A DCHECK signals that there's a caller error here - so what > is > > the caller supposed to be checking? > > > > If they have something that base::Time accepted, why are they at fault? > > If it's something that time.UTCExplode(...) can't handle (e.g. Y2038), why are > > they at fault? > > > > The core of this should be explaining "How are you supposed to hold this API > > correctly" - and I'm not sure what the caller is supposed to do, either for > > 'malicious' inputs or for benign one. If the user's clock is wonky, is that an > > API misuse error? Or is that a condition we should be handling - gracefully? > > Presumably we should handle this the same way explode does, although perhaps a > better idea is to convert der::GeneralizedTime to base::Time, with an API that > can fail similar to Time::FromExploded(), and compare base::Time objects > instead. Nvm, just removed the DCHECK(). https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values.h#... net/der/parse_values.h:134: NET_EXPORT der::GeneralizedTime ConvertBaseUTCTime(const base::Time& time); On 2016/06/28 19:26:44, Ryan Sleevi wrote: > On 2016/06/28 19:15:27, dadrian wrote: > > On 2016/06/28 17:33:30, Ryan Sleevi wrote: > > > NAMING: This naming doesn't really help understand what this code does... > > > "ConvertBaseUTCTime" - is it converting timezones? What's it converting to? > > > > > > This is really: > > > > > > EncodeTimeAsGeneralizedTime(...) > > > > > > right? > > > > > > Does this really belong in parse_values? Isn't this an encoding property? > > > > I'm happy to move this function somewhere else, but offhand I don't know a > > better location. > > net/der/encode_values.h ? Done. https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values_un... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/40001/net/der/parse_values_un... net/der/parse_values_unittest.cc:372: base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(1466787894); On 2016/06/28 17:33:30, Ryan Sleevi wrote: > What about the edge cases? > > What about 1600? (< Windows FILETIME start, < unix epoch) > What about 2039? (> Linux 32-bit time_t max) Done.
https://codereview.chromium.org/2091103002/diff/60001/net/cert/internal/parse... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/60001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:198: EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek)); Reading each of these tests, I find it visually cluttered to try to figure out what's being tested and the core logic and checking that the expectations match what's claimed (and the claim is in the description) Concrete structural suggestions: - Declare your variables close to the first usage ( see https://google.github.io/styleguide/cppguide.html#Local_Variables ) In particular, next_update isn't used until 197, so it's declaration ideally would be before 196 or before 197 - Make use of vertical whitespace ( https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace ) There's a tension here between min/maxing whitespace, but the goal should be readability. When I look at these tests in the codereview screen, they're same font, same color - assume no syntax-highlighting. So are they all one big test? Well, no, they tend to be testing in two lumps - without NextUpdate and with NextUpdate. To me, this seems to suggest that there should be a line-break between 195 and 196 (and 208/209, 221/222, etc) https://codereview.chromium.org/2091103002/diff/60001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:234: EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek)); When visually scanning, this tripped me up. That's because you've swapped your declaration order and dependency relationship, and thus the pattern matching I acquired on the above lines is now broken. That's because I *thought* your |this_update| was now - base::TimeDelta::FromHours(1) - but no, you swapped their definitions (because you made them relative to eachother) You then swap back in the next test (243-244) My concrete suggestion would be: base::Time now = base::Time::Now(); base::Time this_update = now + base::TimeDelta::FromDays(7); response.this_update = ... response.has_next_update = false; EXPECT_FALSE(...); base::Time next_update = this_update - base::TimeDelta::FromHours(1); response ... ... EXPECT_FALSE(...) https://codereview.chromium.org/2091103002/diff/60001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:234: EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek)); This seems like it's testing something other than what the description says - is that the case? Why should this be false? AFAICT, you're testing ThisUpdateOlderThanMaxAge here - shouldn't this be a valid this_update, but an earlier next_update, in line with the description, since you have the ThisUpdate test on line 240? https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.cc File net/der/encode_values.cc (right): https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.c... net/der/encode_values.cc:14: time.UTCExplode(&exploded); DESIGN: Check HasValidValues()? API suggestion: bool EncodeTimeAsGeneralizedTime(const base::Time& time, der::GeneralizedTime* generalized_time) { if (!time.UTCExplode(&exploded)) return false; ... } (In looking at this code, it's not clear to me why we wouldn't just use base::Time::Exploded, but that's for another day)
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.cc File net/der/encode_values.cc (right): https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.c... net/der/encode_values.cc:14: time.UTCExplode(&exploded); On 2016/07/01 00:49:53, Ryan Sleevi wrote: > DESIGN: Check HasValidValues()? > > API suggestion: > bool EncodeTimeAsGeneralizedTime(const base::Time& time, der::GeneralizedTime* > generalized_time) { > if (!time.UTCExplode(&exploded)) UTCExplode is void...what error condition are you referring to here? > return false; > ... > } > > (In looking at this code, it's not clear to me why we wouldn't just use > base::Time::Exploded, but that's for another day)
https://codereview.chromium.org/2091103002/diff/60001/net/cert/internal/parse... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/60001/net/cert/internal/parse... net/cert/internal/parse_ocsp_unittest.cc:234: EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek)); On 2016/07/01 00:49:53, Ryan Sleevi wrote: > This seems like it's testing something other than what the description says - is > that the case? Why should this be false? > > AFAICT, you're testing ThisUpdateOlderThanMaxAge here - shouldn't this be a > valid this_update, but an earlier next_update, in line with the description, > since you have the ThisUpdate test on line 240? Done.
https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.cc File net/der/encode_values.cc (right): https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.c... net/der/encode_values.cc:14: time.UTCExplode(&exploded); On 2016/07/01 17:13:43, dadrian wrote: > On 2016/07/01 00:49:53, Ryan Sleevi wrote: > > DESIGN: Check HasValidValues()? > > > > API suggestion: > > bool EncodeTimeAsGeneralizedTime(const base::Time& time, der::GeneralizedTime* > > generalized_time) { > > if (!time.UTCExplode(&exploded)) > UTCExplode is void...what error condition are you referring to here? time.UTCExplode(&exploded); if (!exploded.HasValidValues()) return false;
https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.cc File net/der/encode_values.cc (right): https://codereview.chromium.org/2091103002/diff/60001/net/der/encode_values.c... net/der/encode_values.cc:14: time.UTCExplode(&exploded); On 2016/07/01 17:58:07, Ryan Sleevi wrote: > On 2016/07/01 17:13:43, dadrian wrote: > > On 2016/07/01 00:49:53, Ryan Sleevi wrote: > > > DESIGN: Check HasValidValues()? > > > > > > API suggestion: > > > bool EncodeTimeAsGeneralizedTime(const base::Time& time, > der::GeneralizedTime* > > > generalized_time) { > > > if (!time.UTCExplode(&exploded)) > > UTCExplode is void...what error condition are you referring to here? > > time.UTCExplode(&exploded); > if (!exploded.HasValidValues()) > return false; Ah. Done. Will this fix the Windows case / thoughts on how to fix it in tests?
Sorry about the delay; hopefully this helps progress? https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... net/cert/internal/parse_ocsp_unittest.cc:23: base::TimeDelta::FromSeconds(INT64_C(11644473600)); No need for the INT64_C now that we're C++11 (which added the support for int64 constants w/o the size specifier) That said, base::Time(0) is equivalent to the Windows Epoch (it's part of base::Time's API contract - https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=7 ) so this should be unnecessary https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... net/cert/internal/parse_ocsp_unittest.cc:286: base::Time verify_time = kWindowsEpoch - base::TimeDelta::FromDays(1); I think my concerns about Windows Epoch may have been confused. That is, I don't ever expect we'll send a junk verify_time in to these routines (and if we do, always failing is a perfectly reasonable response). My concern about Windows Epochs was related to this_update and next_update fields, since those are the 'untrusted' data that comes from CAs. That is, imagine a CA is compromised and issues an OCSP response that, for whatever reason, says 1600 (or 2039 on 32-bit, but let's use Windows as the example) My hope and expectation is that we reject it. |verify_time| I'm not worried about (because we should always be programatically checking that, and so DCHECK'ing on junk is fine, and base::Time::Now() shouldn't give us junk), and while the max age has the theoretical risk of causing overflow, that's also a developer screw up. https://codereview.chromium.org/2091103002/diff/160001/net/der/encode_values.cc File net/der/encode_values.cc (right): https://codereview.chromium.org/2091103002/diff/160001/net/der/encode_values.... net/der/encode_values.cc:16: if (!exploded.HasValidValues()) { No brace ;)
https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... net/cert/internal/parse_ocsp_unittest.cc:286: base::Time verify_time = kWindowsEpoch - base::TimeDelta::FromDays(1); On 2016/07/08 02:21:02, Ryan Sleevi (extremely slow) wrote: > I think my concerns about Windows Epoch may have been confused. That is, I don't > ever expect we'll send a junk verify_time in to these routines (and if we do, > always failing is a perfectly reasonable response). My concern about Windows > Epochs was related to this_update and next_update fields, since those are the > 'untrusted' data that comes from CAs. > > That is, imagine a CA is compromised and issues an OCSP response that, for > whatever reason, says 1600 (or 2039 on 32-bit, but let's use Windows as the > example) > > My hope and expectation is that we reject it. > > |verify_time| I'm not worried about (because we should always be programatically > checking that, and so DCHECK'ing on junk is fine, and base::Time::Now() > shouldn't give us junk), and while the max age has the theoretical risk of > causing overflow, that's also a developer screw up. Since GeneralizedTime handles times from before the Windows epoch, and we convert _to_ generalized time, this is the only possible error condition we can force here. This test just checks that we don't accidentally fail open.
https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/160001/net/cert/internal/pars... net/cert/internal/parse_ocsp_unittest.cc:23: base::TimeDelta::FromSeconds(INT64_C(11644473600)); On 2016/07/08 02:21:02, Ryan Sleevi (extremely slow) wrote: > No need for the INT64_C now that we're C++11 (which added the support for int64 > constants w/o the size specifier) > > That said, base::Time(0) is equivalent to the Windows Epoch (it's part of > base::Time's API contract - > https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=7 ) so this should > be unnecessary Done. https://codereview.chromium.org/2091103002/diff/160001/net/der/encode_values.cc File net/der/encode_values.cc (right): https://codereview.chromium.org/2091103002/diff/160001/net/der/encode_values.... net/der/encode_values.cc:16: if (!exploded.HasValidValues()) { On 2016/07/08 02:21:02, Ryan Sleevi (extremely slow) wrote: > No brace ;) Done.
lgtm https://codereview.chromium.org/2091103002/diff/200001/net/cert/internal/pars... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/200001/net/cert/internal/pars... net/cert/internal/parse_ocsp_unittest.cc:21: const base::Time kWindowsEpoch; Style guide: Class-level statics aren't allowed - https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables (Modified C++11 'unless constexpr', which TimeDelta is) Plus its totes unnecessary; just use a base::Time in the relevant test https://codereview.chromium.org/2091103002/diff/200001/net/der/encode_values_... File net/der/encode_values_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/200001/net/der/encode_values_... net/der/encode_values_unittest.cc:32: #ifdef OS_WIN style guide pedantry: #if defined(OS_WIN) https://codereview.chromium.org/2091103002/diff/200001/net/der/parse_values_u... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/200001/net/der/parse_values_u... net/der/parse_values_unittest.cc:8: #include "base/time/time.h" unneeded I believe
https://codereview.chromium.org/2091103002/diff/200001/net/cert/internal/pars... File net/cert/internal/parse_ocsp_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/200001/net/cert/internal/pars... net/cert/internal/parse_ocsp_unittest.cc:21: const base::Time kWindowsEpoch; On 2016/07/08 23:37:27, Ryan Sleevi (extremely slow) wrote: > Style guide: Class-level statics aren't allowed - > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > (Modified C++11 'unless constexpr', which TimeDelta is) > > Plus its totes unnecessary; just use a base::Time in the relevant test Done. https://codereview.chromium.org/2091103002/diff/200001/net/der/encode_values_... File net/der/encode_values_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/200001/net/der/encode_values_... net/der/encode_values_unittest.cc:32: #ifdef OS_WIN On 2016/07/08 23:37:27, Ryan Sleevi (extremely slow) wrote: > style guide pedantry: > #if defined(OS_WIN) Done. https://codereview.chromium.org/2091103002/diff/200001/net/der/parse_values_u... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/2091103002/diff/200001/net/der/parse_values_u... net/der/parse_values_unittest.cc:8: #include "base/time/time.h" On 2016/07/08 23:37:27, Ryan Sleevi (extremely slow) wrote: > unneeded I believe Done.
The CQ bit was checked by dadrian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2091103002/#ps240001 (title: "Remove extra include.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dadrian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dadrian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add CheckOCSPDateValid() to net/cert/internal Intended to be used internally by certificate validation as part of OCSP validity checks and Expect-Staple. BUG=598021 ========== to ========== Add CheckOCSPDateValid() to net/cert/internal Intended to be used internally by certificate validation as part of OCSP validity checks and Expect-Staple. BUG=598021 Committed: https://crrev.com/687cf9fffda1a7eb45c80a596fa4f9e3e524f0e4 Cr-Commit-Position: refs/heads/master@{#404708} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/687cf9fffda1a7eb45c80a596fa4f9e3e524f0e4 Cr-Commit-Position: refs/heads/master@{#404708}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2138413002/ by engedy@chromium.org. The reason for reverting is: Causes reliable test failures on Linux Tests (dbg)(1)(32): EncodeValuesTest.EncodeTimeAfterTimeTMax EncodeValuesTest.EncodeTimeFromBeforeWindows . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
