Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1683193003:
Implement certificate lifetime parameter as required by WebRTC RFC. (Closed)
|
Created:
4 years, 10 months ago by torbjorng (webrtc) Modified:
4 years, 8 months 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. |
DescriptionImplement certificate lifetime parameter as required by WebRTC RFC.
BUG=chromium:569005
Committed: https://crrev.com/e8dc081c35b5fbac58d786bee89035902c578284
Cr-Commit-Position: refs/heads/master@{#11629}
Patch Set 1 #Patch Set 2 : Adjust test code #
Total comments: 8
Patch Set 3 : Address hbos' feedback #
Total comments: 11
Patch Set 4 : Address feedback #
Total comments: 14
Messages
Total messages: 30 (8 generated)
Description was changed from ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG= ========== to ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG= ==========
torbjorng@webrtc.org changed reviewers: + hbos@webrtc.org
https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (left): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:400: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); Is GenerateForTest still needed after this CL or can we nuke it? https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:396: time_t lifetime = rtc::CreateRandomId() % (0x80000000 - now); What about negative lifetimes? Generating an already expired certificate.
https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (left): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:400: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); On 2016/02/11 11:25:17, hbos wrote: > Is GenerateForTest still needed after this CL or can we nuke it? It is still used elsewhere (in sslstreamadapter_unittest.cc and rtccertificate_unittests.cc). https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:396: time_t lifetime = rtc::CreateRandomId() % (0x80000000 - now); On 2016/02/11 11:25:17, hbos wrote: > What about negative lifetimes? Generating an already expired certificate. That's an odd thing to do, and I don't think enforcing any particular behaviour for that case is important. Note also that the interface will then get notBefore > notAfter, something which could rightfully upset the low-level code.
https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (left): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:400: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); On 2016/02/11 13:41:48, torbjorng (webrtc) wrote: > On 2016/02/11 11:25:17, hbos wrote: > > Is GenerateForTest still needed after this CL or can we nuke it? > > It is still used elsewhere (in sslstreamadapter_unittest.cc and > rtccertificate_unittests.cc). Maybe have a TODO to clean it up some time in the future? :) https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:396: time_t lifetime = rtc::CreateRandomId() % (0x80000000 - now); On 2016/02/11 13:41:48, torbjorng (webrtc) wrote: > On 2016/02/11 11:25:17, hbos wrote: > > What about negative lifetimes? Generating an already expired certificate. > > That's an odd thing to do, and I don't think enforcing any particular behaviour > for that case is important. Note also that the interface will then get notBefore > > notAfter, something which could rightfully upset the low-level code. I think it could be fine to require it to be positive, but right now it is possible to provide a negative one, and its not clear what happens if notBefore > notAfter. I think we should either support it or DCHECK.
torbjorng@webrtc.org changed reviewers: + tommi@webrtc.org
PTAL, tommi. https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (left): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:400: SSLIdentity* identity = rtc::SSLIdentity::GenerateForTest(params); On 2016/02/11 14:56:45, hbos wrote: > On 2016/02/11 13:41:48, torbjorng (webrtc) wrote: > > On 2016/02/11 11:25:17, hbos wrote: > > > Is GenerateForTest still needed after this CL or can we nuke it? > > > > It is still used elsewhere (in sslstreamadapter_unittest.cc and > > rtccertificate_unittests.cc). > > Maybe have a TODO to clean it up some time in the future? :) Done. https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... File webrtc/base/sslidentity_unittest.cc (right): https://codereview.webrtc.org/1683193003/diff/20001/webrtc/base/sslidentity_u... webrtc/base/sslidentity_unittest.cc:396: time_t lifetime = rtc::CreateRandomId() % (0x80000000 - now); On 2016/02/11 14:56:45, hbos wrote: > On 2016/02/11 13:41:48, torbjorng (webrtc) wrote: > > On 2016/02/11 11:25:17, hbos wrote: > > > What about negative lifetimes? Generating an already expired certificate. > > > > That's an odd thing to do, and I don't think enforcing any particular > behaviour > > for that case is important. Note also that the interface will then get > notBefore > > > notAfter, something which could rightfully upset the low-level code. > > I think it could be fine to require it to be positive, but right now it is > possible to provide a negative one, and its not clear what happens if notBefore > > notAfter. I think we should either support it or DCHECK. Done.
Description was changed from ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG= ========== to ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG=chromium:569005 ==========
lgtm
P.S. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:197: static SSLIdentity* Generate(const std::string& common_name, Add a comment about |certificate_lifetime| and say that it should be >= 0. (Technically it would still work if it was >= CERTIFICATE_WINDOW but that's an implementation detail that I don't think should be documented in the API)
https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:412: time_t certificate_lifetime) { does it make sense to DCHECK the validity of the certificate_lifetime value? https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:419: RTC_DCHECK(params.not_before < params.not_after); ah, perhaps this is enough... unless now is bogus. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:129: static const int CERTIFICATE_LIFETIME = 60*60*24*30; // 30 days, arbitrarily spaces around operators. Also, this should be kCertificateLifetime https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: static const int CERTIFICATE_WINDOW = -60*60*24; same thing here https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:197: static SSLIdentity* Generate(const std::string& common_name, On 2016/02/11 16:37:04, hbos wrote: > Add a comment about |certificate_lifetime| and say that it should be >= 0. > (Technically it would still work if it was >= CERTIFICATE_WINDOW but that's an > implementation detail that I don't think should be documented in the API) having dchecks for the expected valid values, helps
PTAL hbos, tommi. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:412: time_t certificate_lifetime) { On 2016/02/11 16:59:30, tommi-webrtc wrote: > does it make sense to DCHECK the validity of the certificate_lifetime value? Reply below. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:419: RTC_DCHECK(params.not_before < params.not_after); On 2016/02/11 16:59:30, tommi-webrtc wrote: > ah, perhaps this is enough... unless now is bogus. This is an intentionally somewhat week assertion, intended to avoid problems in the low-level crypto library. We set the not_before field (which is part of every certificate) to now - 1 day, a behaviour which might be debatable but I retain. This CL allows explicit setting of not_after (but not not_before), supplanting the previously hardwired 30 days. Setting not_after to a (relative to now) negative value makes little sense except for testing some predicate functions. It will cause peer rejection, which is a benign failure. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:129: static const int CERTIFICATE_LIFETIME = 60*60*24*30; // 30 days, arbitrarily On 2016/02/11 16:59:30, tommi-webrtc wrote: > spaces around operators. > Also, this should be kCertificateLifetime Done. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: static const int CERTIFICATE_WINDOW = -60*60*24; On 2016/02/11 16:59:30, tommi-webrtc wrote: > same thing here Done. https://codereview.webrtc.org/1683193003/diff/40001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:197: static SSLIdentity* Generate(const std::string& common_name, On 2016/02/11 16:37:04, hbos wrote: > Add a comment about |certificate_lifetime| and say that it should be >= 0. > (Technically it would still work if it was >= CERTIFICATE_WINDOW but that's an > implementation detail that I don't think should be documented in the API) Done.
Do you think I should rely on "expires in 0 seconds" to generate an expired certificate, GenerateForTest or that a new function should be added? Or allow negative expires on this level even though the java api is supposed to be unsigned. https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.cc File webrtc/base/sslidentity.cc (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.c... webrtc/base/sslidentity.cc:164: return OpenSSLIdentity::Generate(common_name, key_params, I think DCHECKing that |certificate_lifetime| is non-negative makes sense here.
On 2016/02/12 11:40:12, hbos wrote: > Do you think I should rely on "expires in 0 seconds" to generate an expired > certificate, GenerateForTest or that a new function should be added? Or allow > negative expires on this level even though the java api is supposed to be > unsigned. > > https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.cc > File webrtc/base/sslidentity.cc (right): > > https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.c... > webrtc/base/sslidentity.cc:164: return OpenSSLIdentity::Generate(common_name, > key_params, > I think DCHECKing that |certificate_lifetime| is non-negative makes sense here. Because of the way the javascript layer is I would prefer not to inject time up there (would be cumbersome I think), it would be best I think if I can test with a certificate that has actually expired.
Your two comments would seem to contradict each other. Either we leave the possibility of creating an expired certificate, or we don't. It might seem obvious that we should tread carefully in this code and assert parameters very strictly. I agree that we should do that when a security issue would open without such checks. But that's not the case here; strange lifetime for a certificate would simply lead to rejection of a certificate by a connection peer. I only avoid sending clearly silly data down to the low-level library which it could reasonably stumble over. An example of that is to let a certificate expire before it becomes valid. But I can certainly enforce non-negative expiration_lifetime. Then the test code you want to do from the javascript side would need to somehow find its way to the deprecated GenerateForTest() while live code uses Generate(). https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.cc File webrtc/base/sslidentity.cc (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.c... webrtc/base/sslidentity.cc:164: return OpenSSLIdentity::Generate(common_name, key_params, On 2016/02/12 11:40:12, hbos wrote: > I think DCHECKing that |certificate_lifetime| is non-negative makes sense here. Which means test code cannot generate an expired certificate except by using GenerateForTest() which you agreed should go away.
On 2016/02/12 12:58:00, torbjorng (webrtc) wrote: > Your two comments would seem to contradict each other. Either we leave the > possibility of creating an expired certificate, or we don't. Yes, we need to make a decision, I just wanted to point out my desire to be able to generate something that has in fact expired as opposed to pretending something has expired by injecting future "now" values. And then listed some possibilities, some contradictory. > > It might seem obvious that we should tread carefully in this code and assert > parameters very strictly. I agree that we should do that when a security issue > would open without such checks. But that's not the case here; strange lifetime > for a certificate would simply lead to rejection of a certificate by a > connection peer. > > I only avoid sending clearly silly data down to the low-level library which it > could reasonably stumble over. An example of that is to let a certificate expire > before it becomes valid. When the SSLIdenity::Generate API says it expects non-negative I think it makes sense to DCHECK that it is non-negative, whether or not -5 would work due to the kCertificateWindow implementation detail, it is currently not supported by our API. (That is unless we decide to make negative expires values supported.) > > But I can certainly enforce non-negative expiration_lifetime. Then the test code > you want to do from the javascript side would need to somehow find its way to > the deprecated GenerateForTest() while live code uses Generate(). > > https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.cc > File webrtc/base/sslidentity.cc (right): > > https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.c... > webrtc/base/sslidentity.cc:164: return OpenSSLIdentity::Generate(common_name, > key_params, > On 2016/02/12 11:40:12, hbos wrote: > > I think DCHECKing that |certificate_lifetime| is non-negative makes sense > here. > > Which means test code cannot generate an expired certificate except by using > GenerateForTest() which you agreed should go away. Indeed, so, either we support negative values and then I think we should remove GenerateForTest, or we don't in which case we may or may not still have a need for GenerateForTest. I'm OK with either decision. As long as I can get an expired certificate somehow. I'd be happy to make expires=0 count as expired (like the code we saw) in which case I don't need any special function on any layer.
lgtm
On 2016/02/15 16:14:13, tommi-webrtc wrote: > lgtm And my lgtm still hold.
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1683193003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1683193003/60001
Message was sent while issue was closed.
Description was changed from ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG=chromium:569005 ========== to ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG=chromium:569005 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG=chromium:569005 ========== to ========== Implement certificate lifetime parameter as required by WebRTC RFC. BUG=chromium:569005 Committed: https://crrev.com/e8dc081c35b5fbac58d786bee89035902c578284 Cr-Commit-Position: refs/heads/master@{#11629} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e8dc081c35b5fbac58d786bee89035902c578284 Cr-Commit-Position: refs/heads/master@{#11629}
Message was sent while issue was closed.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Message was sent while issue was closed.
Drive-by after https://codereview.chromium.org/1740993002/ https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:419: RTC_DCHECK(params.not_before < params.not_after); This is not a good DCHECK, because it's not a programmer error - you're talking input that's ultimately user supplied (e.g. in https://codereview.chromium.org/1740993002/ ) https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.h:106: time_t certificate_lifetime); I would strongly advise against using time_t in a public API, as opposed to a more clearer expression (base::Time or seconds), due to platform interoperability w/r/t time_t https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: static const int kCertificateWindow = -60 * 60 * 24; Explain the units this is in (seconds?) Explain why it's negative in the constant (unclear) Explain how the value was derived (empirically or just gut? Because our user metrics indicate this is a bad assumption that they're only off a day) https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:200: static SSLIdentity* Generate(const std::string& common_name, Per Google style guide on overloading, this would be better called GenerateWithExpiration
Message was sent while issue was closed.
https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.cc (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.cc:419: RTC_DCHECK(params.not_before < params.not_after); On 2016/03/08 17:04:43, Ryan Sleevi wrote: > This is not a good DCHECK, because it's not a programmer error - you're talking > input that's ultimately user supplied (e.g. in > https://codereview.chromium.org/1740993002/ ) I am fixing this in a follow-up CL. https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.h:106: time_t certificate_lifetime); On 2016/03/08 17:04:43, Ryan Sleevi wrote: > I would strongly advise against using time_t in a public API, as opposed to a > more clearer expression (base::Time or seconds), due to platform > interoperability w/r/t time_t Note that Open/BoringSSL uses time_t, and this is an interface to it. We need to convert someplace from some other type to time_t. (Besides, base::Time is not available in WebRTC.) Incidentally, we're working on a new C++ API for these things. It won't be using time_t, for sure. https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: static const int kCertificateWindow = -60 * 60 * 24; On 2016/03/08 17:04:43, Ryan Sleevi wrote: > Explain the units this is in (seconds?) > Explain why it's negative in the constant (unclear) > Explain how the value was derived (empirically or just gut? Because our user > metrics indicate this is a bad assumption that they're only off a day) Like all things in x509, this is seconds. I agree the comments could be improved. I cannot tell you how the value of one day was derived; this code was just moved in this CL and not semantically changed. My guess is that localtime/UTC confusion on some systems and the fact that times on the planet are up to 24 h from UTC might be the root of this value. https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:200: static SSLIdentity* Generate(const std::string& common_name, On 2016/03/08 17:04:43, Ryan Sleevi wrote: > Per Google style guide on overloading, this would be better called > GenerateWithExpiration I'm fixing this in a follow-up CL.
Message was sent while issue was closed.
https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.h:106: time_t certificate_lifetime); On 2016/03/30 14:00:29, torbjorng (webrtc) wrote: > On 2016/03/08 17:04:43, Ryan Sleevi wrote: > > I would strongly advise against using time_t in a public API, as opposed to a > > more clearer expression (base::Time or seconds), due to platform > > interoperability w/r/t time_t > > Note that Open/BoringSSL uses time_t, and this is an interface to it. We need to > convert someplace from some other type to time_t. (Besides, base::Time is not > available in WebRTC.) > > Incidentally, we're working on a new C++ API for these things. It won't be using > time_t, for sure. OpenSSL's API is actually designed around the ASN1_TIME interface. The time_t bits are part of the legacy interface, and precisely because of issues with time_t sizing - such as the Y2038 problem. The 'right' thing to use is the ASN1_TIME interfaces (ASN1_TIME_DIFF, ASN1_GENERALIZEDTIME_set_string, ASN1_UTCTIME_set_string, etc) https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: static const int kCertificateWindow = -60 * 60 * 24; On 2016/03/30 14:00:29, torbjorng (webrtc) wrote: > Like all things in x509, this is seconds. I'm not sure what you meant, but X.509 is based on fractional microseconds. It's... hairy. However, including the units in constants like this substantially improves readability. > I cannot tell you how the value of one day was derived; this code was just moved > in this CL and not semantically changed. My guess is that localtime/UTC > confusion on some systems and the fact that times on the planet are up to 24 h > from UTC might be the root of this value. That seems like it would be a bug in the WebRTC code, then, perhaps setting the certificate date (which is always in UTC/Z with respect to RFC 5280 certificates) based on local time, rather than the adjustment.
Message was sent while issue was closed.
Thanks for feedback, simple things addressed in https://codereview.webrtc.org/1844313002, the rest will be addressed as per https://bugs.chromium.org/p/webrtc/issues/detail?id=5720 . https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... File webrtc/base/opensslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/opensslidenti... webrtc/base/opensslidentity.h:106: time_t certificate_lifetime); > OpenSSL's API is actually designed around the ASN1_TIME interface. The time_t > bits are part of the legacy interface, and precisely because of issues with > time_t sizing - such as the Y2038 problem. The 'right' thing to use is the > ASN1_TIME interfaces (ASN1_TIME_DIFF, ASN1_GENERALIZEDTIME_set_string, > ASN1_UTCTIME_set_string, etc) Right, I wasn't aware of that one set of interfaces were considered obsolete. It is also hard to guess which interfaces are intended as part of the external API. I created https://bugs.chromium.org/p/webrtc/issues/detail?id=5720 about cleaning up this. https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h File webrtc/base/sslidentity.h (right): https://codereview.webrtc.org/1683193003/diff/60001/webrtc/base/sslidentity.h... webrtc/base/sslidentity.h:132: static const int kCertificateWindow = -60 * 60 * 24; > I'm not sure what you meant, but X.509 is based on fractional microseconds. > It's... hairy. Really? The ANS1_TIME type used therein explicitly forbids fractional seconds. > However, including the units in constants like this substantially improves > readability. Fixing this is a separate CL. > That seems like it would be a bug in the WebRTC code, then, perhaps setting the > certificate date (which is always in UTC/Z with respect to RFC 5280 > certificates) based on local time, rather than the adjustment. I am not aware if that this would happen in WebRTC except if people have incorrectly set system clocks. We never mess with localtime anywhere near any certificate code... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
