Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1032093002:
Certificate Transparency: Correct month calculation. (Closed)
|
Created:
5 years, 9 months ago by Eran Messeri Modified:
5 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Correct month calculation.
The number of SCTs required for certificates with a validity period of over 14 months (but under 15) was incorrect. Fixed by not counting partial months.
BUG=470169
Committed: https://crrev.com/3d509763116223016fe56931f6d11cf7bc435a2e
Cr-Commit-Position: refs/heads/master@{#322501}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressing review comments #
Total comments: 21
Patch Set 3 : 2nd round of review comments #
Total comments: 4
Patch Set 4 : Addressing davidben's comments #Messages
Total messages: 15 (3 generated)
eranm@chromium.org changed reviewers: + davidben@chromium.org, rsleevi@chromium.org
https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer.cc:45: Maybe add a comment here that this is the number of months, rounding down, since "approximate" doesn't tell you exactly what it measures. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer.cc:53: (exploded_expiry.month - exploded_start.month); This still isn't quite right, is it? A certificate valid from 2015-03-31 to 2015-04-01 would give 1 here, but, rounding down, it's 0. (Also, doesn't really matter since code elsewhere will hopefully reject, but this computation will underflow if end < start. Probably worth having this method return 0.) https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... File net/cert/cert_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:77: for (size_t j = 0; j < required_scts - 1; ++j) { Nit: j -> i https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:171: const size_t validity_period[] = {417 /* 14 months */, time_t? (Though see comment below.) https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:181: base::Time fixed_start(base::Time::FromTimeT(1427283904)); Since this is all test code, it might be more understandable if you used base::Time::FromString or base::Time::FromUTCExploded. And likewise for the validity_period bits above. Maybe also wrap it in a struct to pair the validity_period and the needed_scts together. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { BUG: 3 -> arraysize(validity_period) (It looks like the old one got this wrong too...) https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { While you're here, maybe put a SCOPED_TRACE(i) so you know which one failed.
Yeah, David's pointed out the buggy path month_diff = year_diff * 12 + (end_date.month - start_date.month); if end_date.day > start_date.day, do nothing if end_date.day < start_date.day, subtract 1 I think that's right? I don't know, run some tests :) https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... File net/cert/cert_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:181: base::Time fixed_start(base::Time::FromTimeT(1427283904)); On 2015/03/25 15:00:54, David Benjamin wrote: > Since this is all test code, it might be more understandable if you used > base::Time::FromString or base::Time::FromUTCExploded. And likewise for the > validity_period bits above. Maybe also wrap it in a struct to pair the > validity_period and the needed_scts together. +1 Easier to just make a set of base::Time::FromUTCExploded in a set of test structures const struct TestData { base::Time validity_start; base::Time validity_end; int scts_present; bool expected_result; } kTestData[] = { { base::Time::FromUTCExploded({2012, 1, 1, ... }), base::Time::FromUTCExploded({2014, 1, 1, ... }), 3, false }, { base::Time.... }; If you make the definition of the TestData (perhaps better named) struct outside of this function, you can also declare struct BetterName { // stuff }; void PrintTo(const BetterName& object, ::std::ostream* os) { *os << "Validity Start: " << object.validity_start << "Validity End: " << object.validity_end << ... } TEST_F(CertPolicyEnforcerTest, Conforms....) { const BetterName test_data[] = { { ... } }; } https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { s/int/size_t/ https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { On 2015/03/25 15:00:54, David Benjamin wrote: > While you're here, maybe put a SCOPED_TRACE(i) so you know which one failed. +1
Thanks for the quick review, everyone. I believe I've addressed all comments. Note the explanation for the somewhat non-elegant policy checking code in cert_policy_enforcer.cc https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer.cc:45: On 2015/03/25 15:00:54, David Benjamin wrote: > Maybe add a comment here that this is the number of months, rounding down, since > "approximate" doesn't tell you exactly what it measures. Done. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer.cc:53: (exploded_expiry.month - exploded_start.month); On 2015/03/25 15:00:54, David Benjamin wrote: > This still isn't quite right, is it? A certificate valid from 2015-03-31 to > 2015-04-01 would give 1 here, but, rounding down, it's 0. > > (Also, doesn't really matter since code elsewhere will hopefully reject, but > this computation will underflow if end < start. Probably worth having this > method return 0.) This is trickier than it seems, since the policy, as stated in month, does not allow for consistent rounding up/rounding down of months - i.e., the code has to know whether the validity period is exactly 15 months (or more) , so rounding up is wrong in the case of 14 and a half months (as the issue pointed out). But the code also has to know whether the validity period is exactly 27 months (or less) have passed, so rounding down is wrong in the case of 27 and a half months. I've resorted to returning, in addition to the number of rounded-down months, an indicator of whether the last month counted is a full month. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... File net/cert/cert_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:77: for (size_t j = 0; j < required_scts - 1; ++j) { On 2015/03/25 15:00:54, David Benjamin wrote: > Nit: j -> i Done. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:171: const size_t validity_period[] = {417 /* 14 months */, On 2015/03/25 15:00:54, David Benjamin wrote: > time_t? (Though see comment below.) Left at size_t but grouped the number of required scts with this array. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:181: base::Time fixed_start(base::Time::FromTimeT(1427283904)); On 2015/03/25 16:54:23, Ryan Sleevi wrote: > On 2015/03/25 15:00:54, David Benjamin wrote: > > Since this is all test code, it might be more understandable if you used > > base::Time::FromString or base::Time::FromUTCExploded. And likewise for the > > validity_period bits above. Maybe also wrap it in a struct to pair the > > validity_period and the needed_scts together. > > +1 > > Easier to just make a set of base::Time::FromUTCExploded in a set of test > structures > > const struct TestData { > base::Time validity_start; > base::Time validity_end; > int scts_present; > bool expected_result; > } kTestData[] = { > { base::Time::FromUTCExploded({2012, 1, 1, ... }), > base::Time::FromUTCExploded({2014, 1, 1, ... }), > 3, > false }, > { base::Time.... > > }; > > If you make the definition of the TestData (perhaps better named) struct outside > of this function, you can also declare > > struct BetterName { > // stuff > }; > > void PrintTo(const BetterName& object, ::std::ostream* os) { > *os << "Validity Start: " << object.validity_start > << "Validity End: " << object.validity_end > << ... > } > > TEST_F(CertPolicyEnforcerTest, > Conforms....) { > const BetterName test_data[] = { > { ... } > }; > } Done as you suggested - since the TestData structure is only used in one test method I've defined it there. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { On 2015/03/25 15:00:54, David Benjamin wrote: > BUG: 3 -> arraysize(validity_period) > > (It looks like the old one got this wrong too...) Done. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { On 2015/03/25 15:00:54, David Benjamin wrote: > While you're here, maybe put a SCOPED_TRACE(i) so you know which one failed. I do - the detail about the number of days is included in the EXPECT_TRUE/EXPECT_FALSE printout. https://codereview.chromium.org/1032093002/diff/1/net/cert/cert_policy_enforc... net/cert/cert_policy_enforcer_unittest.cc:182: for (int i = 0; i < 3; ++i) { On 2015/03/25 16:54:23, Ryan Sleevi wrote: > s/int/size_t/ Done.
https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:47: MonthsDiff() : num_months(0), last_month_is_full(false) {} Per https://chromium-cpp.appspot.com/, these can be inline initializers. So just struct MonthsDiff { size_t num_months = 0; bool last_month_is_full = false; }; https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:49: bool last_month_is_full; last_month_is_full to me reads like you're rounding up and then reporting whether the last month in num_months was full or partial. Whereas you're rounding down and then reporting whether or not there was *additional* time after adding num_months. Perhaps has_extra or has_partial_month? (Note: that would invert the meaning of the boolean.) https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:52: /* Use // C++-style comments rather than /* C-style */. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:65: return MonthsDiff(); Not that it matters, but I think this actually wants to return (0, true) by your current meaning, not (0, false). Though see comment above; I think inverting the meaning of the boolean makes it clearer anyway. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:74: ret.last_month_is_full = true; It occurs to me this also doesn't account for hours and seconds and such. I don't know how much we care since that's kind of tedious. (Both the < and the == comparisons would need to be modified. That or you unexplode the time, but then you run into problems of what the unexplode function does if you pass in February 31st.) https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:102: MonthsDiff exp = exp -> lifetime? I first read that as exponentiation and was very confused. :-) It's also not really the expiration, right? It's the total lifetime of the certificate. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:170: // Over 39 months This list in the comment seems to be slightly out of date now. Perhaps just remove it? https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:175: } kTestData[] = {{// 14 months, need 2 Oh clang-format. :-/ I don't know how to convince it to indent that better though, so I suppose we can leave it. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:179: {// exactly 15 months, need 3 Nit: They're not really complete sentences, but better to capitalize the first letter anyway. I'd also end with a period. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:204: for (size_t i = 0; i < arraysize(kTestData); ++i) { Still would prefer adding the SCOPED_TRACE. It's in the EXPECT_* output, but this way you don't need to go back and do arithmetic and search for which one it was.
https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:47: MonthsDiff() : num_months(0), last_month_is_full(false) {} On 2015/03/25 19:10:11, David Benjamin wrote: > Per https://chromium-cpp.appspot.com/, these can be inline initializers. So just > > struct MonthsDiff { > size_t num_months = 0; > bool last_month_is_full = false; > }; That said, please just use two out variables; don't encapsulate this in a struct. (Yes, I'm generally anti-tuple)
Addressed all comment. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:47: MonthsDiff() : num_months(0), last_month_is_full(false) {} On 2015/03/25 19:16:07, Ryan Sleevi wrote: > On 2015/03/25 19:10:11, David Benjamin wrote: > > Per https://chromium-cpp.appspot.com/, these can be inline initializers. So > just > > > > struct MonthsDiff { > > size_t num_months = 0; > > bool last_month_is_full = false; > > }; > > That said, please just use two out variables; don't encapsulate this in a > struct. > > (Yes, I'm generally anti-tuple) Done - as Ryan suggested. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:49: bool last_month_is_full; On 2015/03/25 19:10:11, David Benjamin wrote: > last_month_is_full to me reads like you're rounding up and then reporting > whether the last month in num_months was full or partial. > > Whereas you're rounding down and then reporting whether or not there was > *additional* time after adding num_months. Perhaps has_extra or > has_partial_month? (Note: that would invert the meaning of the boolean.) Renamed to has_partial_month as suggested. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:52: /* On 2015/03/25 19:10:11, David Benjamin wrote: > Use // C++-style comments rather than /* C-style */. Done. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:65: return MonthsDiff(); On 2015/03/25 19:10:11, David Benjamin wrote: > Not that it matters, but I think this actually wants to return (0, true) by your > current meaning, not (0, false). Though see comment above; I think inverting the > meaning of the boolean makes it clearer anyway. Done. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:74: ret.last_month_is_full = true; On 2015/03/25 19:10:11, David Benjamin wrote: > It occurs to me this also doesn't account for hours and seconds and such. I > don't know how much we care since that's kind of tedious. (Both the < and the == > comparisons would need to be modified. That or you unexplode the time, but then > you run into problems of what the unexplode function does if you pass in > February 31st.) No benefit to being that tedious, it'd be extra code for no good reason. The whole point behind this policy is to guarantee that long-lived certificates have enough SCTs, nitpicking on the time of day will not matter much. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:102: MonthsDiff exp = On 2015/03/25 19:10:11, David Benjamin wrote: > exp -> lifetime? I first read that as exponentiation and was very confused. :-) > It's also not really the expiration, right? It's the total lifetime of the > certificate. Done - rename to lifetime. You're right, it's not the expiration. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:170: // Over 39 months On 2015/03/25 19:10:11, David Benjamin wrote: > This list in the comment seems to be slightly out of date now. Perhaps just > remove it? Done. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:175: } kTestData[] = {{// 14 months, need 2 On 2015/03/25 19:10:11, David Benjamin wrote: > Oh clang-format. :-/ I don't know how to convince it to indent that better > though, so I suppose we can leave it. Yes, clang-format. While I don't like it either myself, consistency wins in this case. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:179: {// exactly 15 months, need 3 On 2015/03/25 19:10:11, David Benjamin wrote: > Nit: They're not really complete sentences, but better to capitalize the first > letter anyway. I'd also end with a period. Done. https://codereview.chromium.org/1032093002/diff/20001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer_unittest.cc:204: for (size_t i = 0; i < arraysize(kTestData); ++i) { On 2015/03/25 19:10:11, David Benjamin wrote: > Still would prefer adding the SCOPED_TRACE. It's in the EXPECT_* output, but > this way you don't need to go back and do arithmetic and search for which one it > was. Done.
lgtm with two comments. https://codereview.chromium.org/1032093002/diff/40001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/40001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:50: }; This struct should go away now. https://codereview.chromium.org/1032093002/diff/40001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:113: if ((lifetime > 39) || (lifetime == 39 && has_partial_month)) { Nit: parens around (lifetime > 39) are unnecessary. Ditto for line 115.
Addressed both comments. Ryan, I intend to submit this patchset, I'll be happy to address any post-commit comments you may have in a follow-up change (I did not identify any blocking issue in your comments). https://codereview.chromium.org/1032093002/diff/40001/net/cert/cert_policy_en... File net/cert/cert_policy_enforcer.cc (right): https://codereview.chromium.org/1032093002/diff/40001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:50: }; On 2015/03/26 15:05:06, David Benjamin wrote: > This struct should go away now. Oops, Done. https://codereview.chromium.org/1032093002/diff/40001/net/cert/cert_policy_en... net/cert/cert_policy_enforcer.cc:113: if ((lifetime > 39) || (lifetime == 39 && has_partial_month)) { On 2015/03/26 15:05:06, David Benjamin wrote: > Nit: parens around (lifetime > 39) are unnecessary. Ditto for line 115. Done.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1032093002/#ps60001 (title: "Addressing davidben's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1032093002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3d509763116223016fe56931f6d11cf7bc435a2e Cr-Commit-Position: refs/heads/master@{#322501} |
