Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1883813002:
RTCCertificateGenerator added. (Closed)
DescriptionRTCCertificateGenerator added.
This is a new way of generating RTCCertificate objects that is meant
to replace DtlsIdentityStoreInterface and all of its implementations
(clean up work).
It is similar to the identity store in that it generates on the worker
thread and does callback on the signaling thread, but:
- It does not generate identities in the background that you did not
ask for (preemptive generation made more sense before certificates
were parameterized, not so much anymore, and ECDSA which will be most
common takes like <=2 ms to generate).
- As such this code is less complicated than the store's code.
- The API is different, it takes Optional<uint64_t> expires and it
returns RTCCertificates, not SSLIdentities.
- It supports a blocking version of GenerateCertificate that can be
called from any thread, necessary for Chrome which can generate
certificates before the signaling/worker threads have been
initialized as WebRTC-threads (Chrome can invoke this version on
the worker thread outside of WebRTC).
This CL does not remove the identity store, only adds the alternative.
Follow-up CLs will start using it, the store will be removed once it
is no longer used anywhere.
BUG=webrtc:5707, webrtc:5708
R=hta@webrtc.org, torbjorng@webrtc.org
Committed: https://crrev.com/da3a1da9b192e05153219745eb156d1ad469a495
Cr-Commit-Position: refs/heads/master@{#12381}
Patch Set 1 #Patch Set 2 : Updated comments and made it compile (using override everywhere, etc) #
Total comments: 20
Patch Set 3 : Addressed hta's comments #
Total comments: 6
Patch Set 4 : Addressed comments #
Messages
Total messages: 34 (17 generated)
The CQ bit was checked by hbos@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/1883813002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883813002/1
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/14161) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/10510) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2271)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by hbos@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/1883813002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883813002/40001
Patchset #2 (id:40001) has been deleted
Description was changed from ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG= ========== to ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 ==========
The CQ bit was checked by hbos@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/1883813002/30007 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883813002/30007
Description was changed from ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 ========== to ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 ==========
hbos@webrtc.org changed reviewers: + hta@webrtc.org, torbjorng@webrtc.org
Description was changed from ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 ========== to ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 ==========
Please take a look hta, torbjorng. The RTCCertificateGenerator is akin to RTCPeerConnection.generateCertificate in the WebRTC spec. Not made a static function of PeerConnection[Interface] because the number of lines of code makes me want to place it in a separate file and because it needs to know about the signaling and worker threads so it makes sense to construct it separately from Generate calls. (Though we could add a helper function in PeerConnection[Interface] that calls the generator to make it look more like the spec if you want?) I believe webrtc/base is part of the public API and nothing needs to move to webrtc/api?
https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator_unittest.cc (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:114: // but does not verify that this clock is relative to epoch. This approach was due to not finding a "get current time in milliseconds" function in webrtc that is relative to epoch and not being able to inject time on this layer. Time64() of timeutils.h returned a value that was less than a day. Generating a single certificate and looking at its absolute expires value directly would be nicer though. What do you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mainly looks good, but a number of language-ish comments. About Blockingly (my peeviest peevee): I assume we have other blocking functions in the APIs - what do we usually call them? https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.cc (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:30: // Using a separate helper class so that a generation request can outlive the Nit: Start the description with a description, not in the middle of the sentence. Something like: // Helper class for generating certificates. // We are using... https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:108: uint64_t expires_s = *expires_ms / 1000; If |expires_ms| is less than 1000, we will get an expired certificate, right? Is that OK? Consider returning an error (nullptr) in that case. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:113: expires_s = kYearInSeconds; This seems somewhat arbitrary. Consider returning an error when you don't like the value you get. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.h (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.h:35: // function |GenerateCertificateBlockingly| does the same thing but blockingly The suffix "Blockingly" grates on my English-language nerves. Can we use "AndWait" instead - GenerateCertificateAndWait? https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.h:46: // Must be called on the signaling thread. Is non-blocking; does certificate English: Start description with a description, and don't start a sentence with "Is". // Generates a certificate on the worker thread. // Must be called on the signaling thread. It is non-blocking; it does.... https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator_unittest.cc (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:23: // |RTCCertificateGeneratorCallback| is reference counted, and |testing::Test|s I think this comment just confuses people. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:25: class RTCCertificateGeneratorTester : public RTCCertificateGeneratorCallback { Naming suggestion: Call it a "fixture" rather than a "tester". It's something that the test uses. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:107: } Should you have a test that checks the timeout path - that is, something like start to generate RSA EXPECT_FALSE(tester_->certificate()) EXPECT_FALSE(tester_->GenerationCompleted(), 1) EXPECT_FALSE(tester_->certificate()) EXPECT_TRUE(tester->GenerationCompleted, kGenerationTimeoutMs) EXPECT_TRUE(tester_->certificate()) https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:122: EXPECT_TRUE(tester_->certificate()); Query: Why are you using the non-blocking version of the API here? It would seem that using the blocking version would make the test look simpler (no waiting).
Patchset #3 (id:70001) has been deleted
PTAL hta, torbjorng. Ah, I guess I made sentences more compact at the expense of not making them complete, I'll try to stop doing that. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.cc (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:30: // Using a separate helper class so that a generation request can outlive the On 2016/04/14 11:54:05, hta-webrtc wrote: > Nit: Start the description with a description, not in the middle of the > sentence. > Something like: > > // Helper class for generating certificates. > // We are using... Done. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:108: uint64_t expires_s = *expires_ms / 1000; On 2016/04/14 11:54:05, hta-webrtc wrote: > If |expires_ms| is less than 1000, we will get an expired certificate, right? > Is that OK? > > Consider returning an error (nullptr) in that case. You are right and this is on purpose. Even if you generate a certificate that expires in more than 1000 ms, say a second or two, it may expire by the time it is used or by the time the handshake has completed, where the cutoff point is is hard to define. I like the ability to generate expired certificates, it will help with writing test code that I have in mind for a different CL. It might also help developers test handling expired certificates. In either case, that it would fail to generate a certificate I would find surprising. I'm open to changing it to an error but I was purposefully hoping to generate expired certs for testing. Awaiting feedback, code not updated. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:113: expires_s = kYearInSeconds; On 2016/04/14 11:54:05, hta-webrtc wrote: > This seems somewhat arbitrary. Consider returning an error when you don't like > the value you get. Based on "a user agent may choose to limit the period over which an RTCCertificate is valid" (https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...) I think limiting it is preferred to failing. Do you disagree? If you disagree I will happily change it to returning an error, awaiting feedback. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.h (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.h:35: // function |GenerateCertificateBlockingly| does the same thing but blockingly On 2016/04/14 11:54:05, hta-webrtc wrote: > The suffix "Blockingly" grates on my English-language nerves. > Can we use "AndWait" instead - GenerateCertificateAndWait? > Renamed this to GenerateCertificate and the non-blocking variety to GenerateCertificateAsync. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.h:46: // Must be called on the signaling thread. Is non-blocking; does certificate On 2016/04/14 11:54:05, hta-webrtc wrote: > English: Start description with a description, and don't start a sentence with > "Is". > > // Generates a certificate on the worker thread. > // Must be called on the signaling thread. It is non-blocking; it does.... Done. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator_unittest.cc (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:23: // |RTCCertificateGeneratorCallback| is reference counted, and |testing::Test|s On 2016/04/14 11:54:05, hta-webrtc wrote: > I think this comment just confuses people. Removed it. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:25: class RTCCertificateGeneratorTester : public RTCCertificateGeneratorCallback { On 2016/04/14 11:54:05, hta-webrtc wrote: > Naming suggestion: Call it a "fixture" rather than a "tester". It's something > that the test uses. Done. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:107: } On 2016/04/14 11:54:05, hta-webrtc wrote: > Should you have a test that checks the timeout path - that is, something like > > start to generate RSA > EXPECT_FALSE(tester_->certificate()) > EXPECT_FALSE(tester_->GenerationCompleted(), 1) > EXPECT_FALSE(tester_->certificate()) > EXPECT_TRUE(tester->GenerationCompleted, kGenerationTimeoutMs) > EXPECT_TRUE(tester_->certificate()) > > > Done. I'm not sure I understand what you mean by "checks the timeout path". This essentially tests that a certificate is not delivered immediately (synchronously). Was that the idea, or am I not following you? If one is not aware of thread posting and processing, checking that GenerationCompleted() is false after GenerateCertificateAsync() might look like a race condition (even though it isn't). Hopefully my comment clarified why this is not the case. https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:122: EXPECT_TRUE(tester_->certificate()); On 2016/04/14 11:54:05, hta-webrtc wrote: > Query: Why are you using the non-blocking version of the API here? It would seem > that using the blocking version would make the test look simpler (no waiting). It is more complicated but it guarantees (with EXPECT_TRUE_WAIT) that no more than kGenerationTimeoutMs has passed per generation request, which is relevant for when comparing the time stamps below. But that ECDSA can be generated in this amount of time is already tested with a different test, so I think it makes sense to simplify this code by using the non-async version, yeah? Updated the code.
https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.cc (right): https://codereview.webrtc.org/1883813002/diff/30007/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:108: uint64_t expires_s = *expires_ms / 1000; On 2016/04/14 15:31:54, hbos wrote: > On 2016/04/14 11:54:05, hta-webrtc wrote: > > If |expires_ms| is less than 1000, we will get an expired certificate, right? > > Is that OK? > > > > Consider returning an error (nullptr) in that case. > > You are right and this is on purpose. Even if you generate a certificate that > expires in more than 1000 ms, say a second or two, it may expire by the time it > is used or by the time the handshake has completed, where the cutoff point is is > hard to define. > > I like the ability to generate expired certificates, it will help with writing > test code that I have in mind for a different CL. It might also help developers > test handling expired certificates. In either case, that it would fail to > generate a certificate I would find surprising. > > I'm open to changing it to an error but I was purposefully hoping to generate > expired certs for testing. Awaiting feedback, code not updated. For this CL: https://codereview.chromium.org/1644553002/. Generating an expired certificate for use in the RTCPeerConnection constructor should throw an exception.
lgtm https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator_unittest.cc (right): https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:132: EXPECT_GE(expires_diff, kExpiresMs - 2*kGenerationTimeoutMs - 1000); Since cert B is generated after cert A, and synchronously, we should be guaranteed that expires_diff >= kExpiresMs. Even the truncation to seconds shouldn't manage to change that. No problems with the amount of leeway granted in the next line.
lgtm https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.cc (right): https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:114: if (expires_s > kYearInSeconds) Consider using std::max. https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.h (right): https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.h:45: const Optional<uint64_t>& expires_ms); Please document what happens if this argument is not specified.
https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.cc (right): https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.cc:114: if (expires_s > kYearInSeconds) On 2016/04/15 12:17:48, torbjorng (webrtc) wrote: > Consider using std::max. Done. (std::min) https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator.h (right): https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator.h:45: const Optional<uint64_t>& expires_ms); On 2016/04/15 12:17:48, torbjorng (webrtc) wrote: > Please document what happens if this argument is not specified. Done. https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... File webrtc/base/rtccertificategenerator_unittest.cc (right): https://codereview.webrtc.org/1883813002/diff/90001/webrtc/base/rtccertificat... webrtc/base/rtccertificategenerator_unittest.cc:132: EXPECT_GE(expires_diff, kExpiresMs - 2*kGenerationTimeoutMs - 1000); On 2016/04/15 10:42:23, hta-webrtc wrote: > Since cert B is generated after cert A, and synchronously, we should be > guaranteed that expires_diff >= kExpiresMs. Even the truncation to seconds > shouldn't manage to change that. > > No problems with the amount of leeway granted in the next line. Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@webrtc.org, hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/1883813002/#ps110001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883813002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883813002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 ========== to ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 R=hta@webrtc.org, torbjorng@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/da3a1da9b192e05153219745e... ==========
Message was sent while issue was closed.
Description was changed from ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 R=hta@webrtc.org, torbjorng@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/da3a1da9b192e05153219745e... ========== to ========== RTCCertificateGenerator added. This is a new way of generating RTCCertificate objects that is meant to replace DtlsIdentityStoreInterface and all of its implementations (clean up work). It is similar to the identity store in that it generates on the worker thread and does callback on the signaling thread, but: - It does not generate identities in the background that you did not ask for (preemptive generation made more sense before certificates were parameterized, not so much anymore, and ECDSA which will be most common takes like <=2 ms to generate). - As such this code is less complicated than the store's code. - The API is different, it takes Optional<uint64_t> expires and it returns RTCCertificates, not SSLIdentities. - It supports a blocking version of GenerateCertificate that can be called from any thread, necessary for Chrome which can generate certificates before the signaling/worker threads have been initialized as WebRTC-threads (Chrome can invoke this version on the worker thread outside of WebRTC). This CL does not remove the identity store, only adds the alternative. Follow-up CLs will start using it, the store will be removed once it is no longer used anywhere. BUG=webrtc:5707, webrtc:5708 R=hta@webrtc.org, torbjorng@webrtc.org Committed: https://crrev.com/da3a1da9b192e05153219745eb156d1ad469a495 Cr-Commit-Position: refs/heads/master@{#12381} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:110001) manually as da3a1da9b192e05153219745eb156d1ad469a495 (presubmit successful).
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/da3a1da9b192e05153219745eb156d1ad469a495 Cr-Commit-Position: refs/heads/master@{#12381} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
