Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1468273004:
Provide method for returning certificate expiration timestamp. (Closed)
|
Created:
5 years ago by torbjorng (webrtc) Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionProvide method for returning certificate expiration time stamp.
We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds.
Conversion via std::tm might might seem silly, but actually doesn't add any complexity.
One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem).
The ASN1 TIME parsing is limited to what is required by RFC 5280.
BUG=webrtc:5150
R=hbos@webrtc.org, nisse@webrtc.org, tommi@webrtc.org
Committed: https://crrev.com/46c9cc01901ecd3af0191872f9660b710d5fe757
Cr-Commit-Position: refs/heads/master@{#10854}
Patch Set 1 #Patch Set 2 : Misc fixes. #
Total comments: 4
Patch Set 3 : Add temporary debug logging #Patch Set 4 : More debug printing #Patch Set 5 : Restrict random range to avoid overflow in OpenSSL #Patch Set 6 : Misc CL cleanups #
Total comments: 21
Patch Set 7 : Address hbos' feedback comments. #Patch Set 8 : Remove an unused variable. #Patch Set 9 : Move most new code from opensslidentity.cc to timeutils.cc and sslidentity.cc #
Total comments: 2
Patch Set 10 : Address hbos' feedback comments #
Total comments: 11
Patch Set 11 : Address nisse's feedback comments #Patch Set 12 : Fix formatting #
Total comments: 19
Patch Set 13 : Adjust cut-over as per RFC 5280 #Patch Set 14 : Address tommi's feedback #
Total comments: 1
Patch Set 15 : Add yet more testing #Patch Set 16 : Fix type typo in last change #
Total comments: 1
Patch Set 17 : Windows fixes #
Messages
Total messages: 66 (28 generated)
Description was changed from ========== Provide method for returning certificate expiration timestamp. BUG=webrtc:5150 ========== to ========== Provide method for returning certificate expiration time stamp. BUG=webrtc:5150 We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. We may want to move some of the conversion code away from opensslidentity.cc to some more generic function at some point. ==========
torbjorng@webrtc.org changed reviewers: + hbos@webrtc.org
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/6167)
Description was changed from ========== Provide method for returning certificate expiration time stamp. BUG=webrtc:5150 We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. We may want to move some of the conversion code away from opensslidentity.cc to some more generic function at some point. ========== to ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. We may want to move some of the conversion code away from opensslidentity.cc to some more generic function at some point. BUG=webrtc:5150 ==========
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/10818)
First pass, happy debugging https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:11: #include <ctime> Move this to the other includes, everything else in this file is inside the #if https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:176: int64_t not_after; // Offset from current time or absolute time in seconds. Is this necessary? Now any code that use these have to understand two interpretations of the same variables. I see you changed from int to int64_t. Is this the correct type? Here's a usage case of not_before and not_after: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Which calls: ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj). long may be int32.
PTAL, hbos. https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:11: #include <ctime> On 2015/11/25 11:18:31, hbos wrote: > Move this to the other includes, everything else in this file is inside the #if Done. https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1468273004/diff/20001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:176: int64_t not_after; // Offset from current time or absolute time in seconds. On 2015/11/25 11:18:31, hbos wrote: > Is this necessary? Now any code that use these have to understand two > interpretations of the same variables. > > I see you changed from int to int64_t. Is this the correct type? > > Here's a usage case of not_before and not_after: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Which calls: ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj). > long may be int32. I changed the logic around this to always use time in epoch (i.e., "absolute" time.)
Minor things https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:13: #include <ctime> The first include should be opensslidentity.h. I would place it above <oppenssl/...> or above win32.h. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:101: time_t epoch_off = 0; // Time offset since epoch. nit/optional: Is it a style in this file or personal preference to define this variable at the top of the function? Personally I would define it where it's used. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:406: return ((((int64_t)year * 365 + day) * 24 + hour) * 60 + min) * 60 + sec; I think style guide says static_cast<int64_t> instead even for stuff like this? https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:420: } while (0) Macros are the devil. I think these should be functions instead (static functions which are typically placed at the top in an anonymous namespace, code style thingy). https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:429: const unsigned char* s = (const unsigned char*)time->data; Style guide: use static_cast. Wait, isn't this an unsigned char*? Then you don't need a cast, non-const -> const is implicit. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:430: size_t bytes_left = time->length; size_t is what should be used for stuff like this but I think time->length is int, so maybe make bytes_left and int? (Even though if its not unsigned and doesn't fit as size_t there must be a bug) https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:465: ASN1_READ2(tm.tm_sec, s, bytes_left); A short comment here and/or at the function definition would be nice. Same for the usage above. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:72: // Returns the time in milliseconds relative to epoch. Is it unambiguous what "epoch" means? Could you write out the datetime and zone in the comment? https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:175: int not_after; // Absolute time since epoch in seconds. What happens at the wrap-around date (code using this)? https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:310: params.not_after = rtc::CreateRandomId() % 0x80000000; Add a comment explaining the % https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslstreamada... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslstreamada... webrtc/base/sslstreamadapter_unittest.cc:297: time_t now = time(NULL); I would use nullptr even though NULL is used in most places of this file.
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/5855)
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/140001
torbjorng@webrtc.org changed reviewers: + juberti@webrtc.org
PTAL, juberti. If there is any chance you could take a look before the US holidays, that would be great! https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:13: #include <ctime> On 2015/11/25 15:21:44, hbos wrote: > The first include should be opensslidentity.h. I would place it above > <oppenssl/...> or above win32.h. Done. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:101: time_t epoch_off = 0; // Time offset since epoch. On 2015/11/25 15:21:44, hbos wrote: > nit/optional: Is it a style in this file or personal preference to define this > variable at the top of the function? Personally I would define it where it's > used. Since it sets a value and since the function is full of gotos, putting it in the usual place causes compilation failure. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:406: return ((((int64_t)year * 365 + day) * 24 + hour) * 60 + min) * 60 + sec; On 2015/11/25 15:21:44, hbos wrote: > I think style guide says static_cast<int64_t> instead even for stuff like this? Done. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:429: const unsigned char* s = (const unsigned char*)time->data; On 2015/11/25 15:21:44, hbos wrote: > Style guide: use static_cast. Wait, isn't this an unsigned char*? Then you don't > need a cast, non-const -> const is implicit. Done. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:430: size_t bytes_left = time->length; On 2015/11/25 15:21:44, hbos wrote: > size_t is what should be used for stuff like this but I think time->length is > int, so maybe make bytes_left and int? (Even though if its not unsigned and > doesn't fit as size_t there must be a bug) OpenSSL uses 'int' for sizes. I expect that to get fixed. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:465: ASN1_READ2(tm.tm_sec, s, bytes_left); On 2015/11/25 15:21:44, hbos wrote: > A short comment here and/or at the function definition would be nice. Same for > the usage above. Done. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:72: // Returns the time in milliseconds relative to epoch. On 2015/11/25 15:21:45, hbos wrote: > Is it unambiguous what "epoch" means? Could you write out the datetime and zone > in the comment? https://en.wikipedia.org/wiki/Unix_time https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:175: int not_after; // Absolute time since epoch in seconds. On 2015/11/25 15:21:45, hbos wrote: > What happens at the wrap-around date (code using this)? I'll change this to time_t, which is more kosher (but still has problems after 2038 on older systems). https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:310: params.not_after = rtc::CreateRandomId() % 0x80000000; On 2015/11/25 15:21:45, hbos wrote: > Add a comment explaining the % Done. https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslstreamada... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/100001/webrtc/base/sslstreamada... webrtc/base/sslstreamadapter_unittest.cc:297: time_t now = time(NULL); On 2015/11/25 15:21:45, hbos wrote: > I would use nullptr even though NULL is used in most places of this file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/160001
https://codereview.webrtc.org/1468273004/diff/160001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/160001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:326: TestExpireTime(500); If TestExpireTime(500) fails, will the effects of SetRandomTestMode(true) last for other tests that might run after it? I'm thinking maybe the SetRandomTestMode stuff should happen in the constructor/destructor?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
lgtm if you address my SetRandomTestMode comment. Also, update the CL description to reflect the fact that all the code is not in opensslidentity.cc anymore (last sentence).
Description was changed from ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. We may want to move some of the conversion code away from opensslidentity.cc to some more generic function at some point. BUG=webrtc:5150 ========== to ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. BUG=webrtc:5150 ==========
torbjorng@webrtc.org changed reviewers: + nisse@webrtc.org - juberti@webrtc.org
https://codereview.webrtc.org/1468273004/diff/160001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/160001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:133: This change from using the undocumented X509_gmtime_adj to using the likewise undocumented X509_time_adj is needed in order to set absolute time. The former function calls time() and adds its last parameter to it, the latter function adds its last two parameters instead. Ref: chromium/src/third_party/boringssl/src/crypto/x509/x509_vfy.c
PTAL, nisse. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/opensslident... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/opensslident... webrtc/base/opensslidentity.cc:133: This change, from using the undocumented X509_gmtime_adj to using the likewise undocumented X509_time_adj, is needed in order to set absolute time. The former function calls time() and adds its last parameter to it, the latter function adds its last two parameters instead. Ref: chromium/src/third_party/boringssl/src/crypto/x509/x509_vfy.c
https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.cc File webrtc/base/sslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:219: year += 100; It would be nice to return -1 for *all* invalid inputs. If you don't want lots of error handling clutter, you could do an upfront check that the string ends with Z, and use strnspn to check for any unexpected non-digits. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:226: int64_t ASN1TimeToSec(const unsigned char *s, size_t length, bool long_format); There's some confusion on whether this and CertificateExpirationTime returns seconds or milliseconds; the latter makes a tail call to this one and is documented as returning ms. Is there any reason why higher precision than seconds is needed? https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:328: }; I'd like to also see unit tests for TmToSeconds and ASN1TimeToSec, to be able to test behaviour with invalid inputs. It would also be good with a few of tests involving timestamps chosen close to a leap day. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/timeutils.cc... webrtc/base/timeutils.cc:218: day += cumul_days[month]; I think you get an out-of-bounds read for invalid month in the input. this functions needs to either handle out of range inputs properly, like month 15 or -15 spilling over to a different year, and similarly for day of month. Or return -1 for all out-of-range inputs (which has the side-effect of also having the only current caller of this function return -1 for invalid inputs. Special care is needed for validating February 29, of course...). https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/timeutils.cc... webrtc/base/timeutils.cc:227: day -= 1; This is a bit subtle... +1 for a leap year would be a bit more expected, I think it's correct because if the current year is a leap year, that year's leap day is already included in the leap day count a few lines up.
torbjorng@webrtc.org changed reviewers: + tommi@webrtc.org
Thanks for feedback. The ASN1 parsing code should now be fairly tight. It relies on TmToSeconds for out-of-range dates. Now, we test also invalid data and close-to-invalid, trying to determine that the envelope if exactly right. PTAL nisse, tommi. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.cc File webrtc/base/sslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:219: year += 100; On 2015/11/27 12:47:12, nisse wrote: > It would be nice to return -1 for *all* invalid inputs. > > If you don't want lots of error handling clutter, you could do an upfront check > that the string ends with Z, and use strnspn to check for any unexpected > non-digits. Done (using strspn). https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity.... webrtc/base/sslidentity.h:226: int64_t ASN1TimeToSec(const unsigned char *s, size_t length, bool long_format); On 2015/11/27 12:47:12, nisse wrote: > There's some confusion on whether this and CertificateExpirationTime returns > seconds or milliseconds; the latter makes a tail call to this one and is > documented as returning ms. > > Is there any reason why higher precision than seconds is needed? Fixed. Should be seconds everywhere. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:328: }; On 2015/11/27 12:47:12, nisse wrote: > I'd like to also see unit tests for TmToSeconds and ASN1TimeToSec, to be able to > test behaviour with invalid inputs. > > It would also be good with a few of tests involving timestamps chosen close to a > leap day. Done. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/timeutils.cc... webrtc/base/timeutils.cc:218: day += cumul_days[month]; On 2015/11/27 12:47:12, nisse wrote: > I think you get an out-of-bounds read for invalid month in the input. > > this functions needs to either handle out of range inputs properly, like month > 15 or -15 spilling over to a different year, and similarly for day of month. Or > return -1 for all out-of-range inputs (which has the side-effect of also having > the only current caller of this function return -1 for invalid inputs. Special > care is needed for validating February 29, of course...). Sure, this was intended as a low-level internal function for valid input... But validation never hurts (in not time-critical code) so I added it. https://codereview.webrtc.org/1468273004/diff/180001/webrtc/base/timeutils.cc... webrtc/base/timeutils.cc:227: day -= 1; On 2015/11/27 12:47:12, nisse wrote: > This is a bit subtle... +1 for a leap year would be a bit more expected, I think > it's correct because if the current year is a leap year, that year's leap day is > already included in the leap day count a few lines up. Tried to clarify.
lgtm https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.cc File webrtc/base/sslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:186: for (size_t i = 0; i < n; i++) { nit: no {} https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:195: std::tm tm; nit: define these variables where they're needed (smallest scope) https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:201: if (length == 0 || s[length - 1] != 'Z') Below, I thought you might have a problem if the string isn't zero terminated, but looks like 'Z' handles that here. You might want to add a note in the comment that the string doesn't need to be zero terminated just for future readers. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:364: {nullptr} // sentinel nit: you could sneak a case in here of a string that is not null terminated. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:368: for (int i = 0; data[i].string != nullptr; i++) { nit: ++i Actually, you don't need to use nullptr to terminate the array. You could remove the last element of the array and do this instead: for (const auto& entry : asn_example data) { // use |entry| instead of data[i] } https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:377: for (int i = 0; data[i].string != nullptr; i++) { same here https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:398: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); can GenerateForTest return a scoped_ptr? https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/timeutils_un... File webrtc/base/timeutils_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/timeutils_un... webrtc/base/timeutils_unittest.cc:184: std::tm tm; declare where used https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/timeutils_un... webrtc/base/timeutils_unittest.cc:192: if (year % 100 == 0) TIL!
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.cc File webrtc/base/sslidentity.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:186: for (size_t i = 0; i < n; i++) { On 2015/11/30 15:53:55, tommi (webrtc) wrote: > nit: no {} Done. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:195: std::tm tm; On 2015/11/30 15:53:55, tommi (webrtc) wrote: > nit: define these variables where they're needed (smallest scope) Done. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity.... webrtc/base/sslidentity.cc:201: if (length == 0 || s[length - 1] != 'Z') On 2015/11/30 15:53:55, tommi (webrtc) wrote: > Below, I thought you might have a problem if the string isn't zero terminated, > but looks like 'Z' handles that here. You might want to add a note in the > comment that the string doesn't need to be zero terminated just for future > readers. This is noted in the declaration of this method. (The comment there was a bit ambiguous, now fixed.) https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:364: {nullptr} // sentinel On 2015/11/30 15:53:56, tommi (webrtc) wrote: > nit: you could sneak a case in here of a string that is not null terminated. Good idea. Made'em all garbage terminated. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:368: for (int i = 0; data[i].string != nullptr; i++) { On 2015/11/30 15:53:56, tommi (webrtc) wrote: > nit: ++i > > Actually, you don't need to use nullptr to terminate the array. > > You could remove the last element of the array and do this instead: > > for (const auto& entry : asn_example data) { > // use |entry| instead of data[i] > } Done. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:377: for (int i = 0; data[i].string != nullptr; i++) { On 2015/11/30 15:53:55, tommi (webrtc) wrote: > same here Done. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:398: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); On 2015/11/30 15:53:55, tommi (webrtc) wrote: > can GenerateForTest return a scoped_ptr? I don't understand. Are you suggesting that we make GenerateForTest return a scoped_ptr? That would be possible, but that seems like a separate CL. Or are you suggesting that I use a scoped_ptr for the test itself and avoid the delete? https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/timeutils_un... File webrtc/base/timeutils_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/timeutils_un... webrtc/base/timeutils_unittest.cc:184: std::tm tm; On 2015/11/30 15:53:56, tommi (webrtc) wrote: > declare where used Done. https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/timeutils_un... webrtc/base/timeutils_unittest.cc:192: if (year % 100 == 0) On 2015/11/30 15:53:56, tommi (webrtc) wrote: > TIL! As you can see, this code is carefully designed to work past year 2400. :-)
lgtm++ https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/220001/webrtc/base/sslidentity_... webrtc/base/sslidentity_unittest.cc:398: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); On 2015/11/30 21:05:45, torbjorng (webrtc) wrote: > On 2015/11/30 15:53:55, tommi (webrtc) wrote: > > can GenerateForTest return a scoped_ptr? > > I don't understand. > > Are you suggesting that we make GenerateForTest return a scoped_ptr? That would > be possible, but that seems like a separate CL. > > Or are you suggesting that I use a scoped_ptr for the test itself and avoid the > delete? Either. Fine as is too.
https://codereview.webrtc.org/1468273004/diff/260001/webrtc/base/timeutils_un... File webrtc/base/timeutils_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/260001/webrtc/base/timeutils_un... webrtc/base/timeutils_unittest.cc:185: // First generate something correct and check that TmToSeconds is happy. If you don't do that elsewhere, I think it would make sense to produce valid inputs using gmtime, and check that TmToSeconds is the inverse. Like, time_t t = random_something(); // Possibly 32-bit only std::tm tm; EXPECT_TRUE(gmtime_r(&t, &tm)); EXPECT_TRUE(rtc::TmToSeconds(tm) == t); BTW, I guess all time formats involved here are blissfully ignorant of leap seconds? (Which could be represented in a struct tm, but gmtime inherits ignorance from the definition of time_t values. To be clear, I see *no* practical problem with treating an expiration time with seconds == 60 as always invalid, and seconds == 59 as always valid, but I'm a bit curious as to what the spec says. Most likely, it says nothing...).
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/280001
I added the testing you suggested, but it too failed to debunk my code. PTAL nisse.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/11124)
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...)
lgtm https://codereview.webrtc.org/1468273004/diff/300001/webrtc/base/timeutils_un... File webrtc/base/timeutils_unittest.cc (right): https://codereview.webrtc.org/1468273004/diff/300001/webrtc/base/timeutils_un... webrtc/base/timeutils_unittest.cc:245: for (int i = 0; i < times; i++) { lgtm. nit: Should be gmtime_r also in the comment.
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1468273004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1468273004/320001
Description was changed from ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. BUG=webrtc:5150 ========== to ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. BUG=webrtc:5150 R=hbos@webrtc.org, nisse@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/46c9cc01901ecd3af0191872f... ==========
Message was sent while issue was closed.
Description was changed from ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. BUG=webrtc:5150 R=hbos@webrtc.org, nisse@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/46c9cc01901ecd3af0191872f... ========== to ========== Provide method for returning certificate expiration time stamp. We convert ASN1 time via std::tm to int64_t representing milliseconds-since-epoch. We do not use time_t since that cannot store milliseconds, and expires for 32-bit platforms in 2038 also for seconds. Conversion via std::tm might might seem silly, but actually doesn't add any complexity. One would expect tm -> seconds-since-epoch to already exist on the standard library. There is mktime, but it uses localtime (and sets an environment variable, and has the 2038 problem). The ASN1 TIME parsing is limited to what is required by RFC 5280. BUG=webrtc:5150 R=hbos@webrtc.org, nisse@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/46c9cc01901ecd3af0191872f9660b710d5fe757 Cr-Commit-Position: refs/heads/master@{#10854} ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) manually as 46c9cc01901ecd3af0191872f9660b710d5fe757 (presubmit successful).
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/46c9cc01901ecd3af0191872f9660b710d5fe757 Cr-Commit-Position: refs/heads/master@{#10854} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
