Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1740993002:
RTCPeerConnection.generateCertificate: Optionally specify expiration. (Closed)
|
Created:
4 years, 10 months ago by hbos_chromium Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, tommyw+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRTCPeerConnection.generateCertificate: Optionally specify expiration.
|keygenAlgorithm| of RTCPeerConnection.generateCertificate(
AlgorithmIdentifier keygenAlgorithm) gets the optional attribute
DOMTimeStamp which, if present, is used to specify when the
generated certificate will expires (expressed in number of
milliseconds relative to now).
BUG=chromium:569005
Committed: https://crrev.com/751894c78f4aab168ed17a5704fd6ce306e3781f
Cr-Commit-Position: refs/heads/master@{#388183}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rebase with master (DtlsIdentityStoreInterface of webrtc updated) #Patch Set 3 : Updated RequestIdentity signature #Patch Set 4 : ...and generateCertificateExpires signature #
Total comments: 14
Patch Set 5 : Rebase with master after some time #Patch Set 6 : Addressed comments #
Total comments: 1
Messages
Total messages: 26 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
hbos@chromium.org changed reviewers: + jochen@chromium.org, tommi@chromium.org
Please take a look, tommi and jochen. https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.cc:152: // ensures that the value is not too large for time_t. Spec: "a user agent may choose to limit the period over which an RTCCertificate is valid" https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_identity_store.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.h:34: override; The webrtc interface is updating to take |expires_ms|, after this CL the old version will be removed. During this transition period both versions exists with default implementation to call the other implementation, so that only one has to be implemented. https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_certificate_generator.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.h:35: blink::WebCallbacks<blink::WebRTCCertificate*, void>* observer) override; I would have preferred something equivalent to rtc::Optional for |expires_ms| to avoid two function signatures, but I couldn't find anything like that that works accross blink and content (this being the content implementation of a blink interface). Instead I'm using rtc::Optional in a private helper function called by both of these.
https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.cc:120: rtc::Optional<uint64_t> expires_ms, Is there a reason why the arguments need to be passed by value? https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.cc:152: // ensures that the value is not too large for time_t. On 2016/03/04 09:26:31, hbos_chromium wrote: > Spec: "a user agent may choose to limit the period over which an RTCCertificate > is valid" Was there a discussion on what is reasonable or is a year arbitrarily chosen? (I'm not disagreeing with it, just curious) https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_identity_store.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.h:30: void RequestIdentity( can you make this private? https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_certificate_generator.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.h:35: blink::WebCallbacks<blink::WebRTCCertificate*, void>* observer) override; On 2016/03/04 09:26:31, hbos_chromium wrote: > I would have preferred something equivalent to rtc::Optional for |expires_ms| to > avoid two function signatures, but I couldn't find anything like that that works > accross blink and content (this being the content implementation of a blink > interface). > > Instead I'm using rtc::Optional in a private helper function called by both of > these. Makes sense to me. I think we'll move from rtc::Optional to std::optional soon enough btw. https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.h:43: rtc::Optional<uint64_t> expires_ms, const &?
https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.cc:120: rtc::Optional<uint64_t> expires_ms, On 2016/03/04 09:55:40, tommi-chromium wrote: > Is there a reason why the arguments need to be passed by value? No, will update webrtc interface to use const&, roll and then update this. https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.cc:152: // ensures that the value is not too large for time_t. On 2016/03/04 09:55:40, tommi-chromium wrote: > On 2016/03/04 09:26:31, hbos_chromium wrote: > > Spec: "a user agent may choose to limit the period over which an > RTCCertificate > > is valid" > > Was there a discussion on what is reasonable or is a year arbitrarily chosen? > (I'm not disagreeing with it, just curious) Rather arbitrarily, could have chosen something else. (We need some sort of check here though since uint64_t could be larger than time_t.) https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_certificate_generator.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.h:43: rtc::Optional<uint64_t> expires_ms, On 2016/03/04 09:55:40, tommi-chromium wrote: > const &? Yes! Will fix after webrtc land + roll.
PTAL tommi, jochen. Using the latest RequestIdentity signature from webrtc. https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/peer_connection_identity_store.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/peer_connection_identity_store.h:30: void RequestIdentity( On 2016/03/04 09:55:40, tommi-chromium wrote: > can you make this private? This has been updated to be the implementation of the public interface. It shall not be private because it could be called by webrtc in another context. https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_certificate_generator.h (right): https://codereview.chromium.org/1740993002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.h:35: blink::WebCallbacks<blink::WebRTCCertificate*, void>* observer) override; On 2016/03/04 09:55:40, tommi-chromium wrote: > On 2016/03/04 09:26:31, hbos_chromium wrote: > > I would have preferred something equivalent to rtc::Optional for |expires_ms| > to > > avoid two function signatures, but I couldn't find anything like that that > works > > accross blink and content (this being the content implementation of a blink > > interface). > > > > Instead I'm using rtc::Optional in a private helper function called by both of > > these. > > Makes sense to me. I think we'll move from rtc::Optional to std::optional soon > enough btw. Acknowledged. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/rtc_certificate_generator.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/rtc_certificate_generator.cc:68: this, url, first_party_for_cookies, expires_ms)); (I'm purposefully using the copy constructor here since it's posting.)
jochen@chromium.org changed reviewers: + rsleevi@chromium.org
certificate stuff? -> rsleevi
Nothing really does certificate stuff here, so jochen@ as OWNERS review can decide whether or not my comments are something to consider. I've also left comments on https://codereview.chromium.org/1740993002/ , which did the meat of the work. I've left remarks here about the seeming inconsistency with Chromium and Google style guides, but it's up to jochen@ to approve/agree or not. This LGTM % those remarks. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.cc:156: kIdentityName, key_params, static_cast<time_t>(expires_s))); BUG: You cannot do this sort of cast. There's no guarantee that time_t is expressed like this. Explicitly convert (e.g. using base::Time) https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.h (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.h:32: const rtc::Optional<uint64_t>& expires_ms, This seems like a clear violation, through creative templating, of the Chromium (and Google) style guide with respect to Overloads. Is there a reason WebRTC isn't following the Google style guide in letter and in spirit? https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/rtc_certificate_generator.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/rtc_certificate_generator.cc:158: void RTCCertificateGenerator::generateCertificateExpires( generateCertificateWithExpiration https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/rtc_certificate_generator.h (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/rtc_certificate_generator.h:12: #include "third_party/webrtc/base/optional.h" This is unnecessary https://codereview.chromium.org/1740993002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebRTCCertificateGenerator.h (right): https://codereview.chromium.org/1740993002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCCertificateGenerator.h:53: virtual void generateCertificate( Per Chromium style guide with respect to overloads, shouldn't this be generateCertificateWithExpiration?
Heya! Late response since I've been on vacation. We will update webrtc stuff before returning to this CL, so it will be a little while longer before comments are addressed.
Patchset #5 (id:140001) has been deleted
PTAL tommi, jochen. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.cc:156: kIdentityName, key_params, static_cast<time_t>(expires_s))); On 2016/03/08 17:06:10, Ryan Sleevi wrote: > BUG: You cannot do this sort of cast. There's no guarantee that time_t is > expressed like this. > > Explicitly convert (e.g. using base::Time) From JavaScript we get a uint64_t timestamp. We want to use this all the way down but lower layer stuff uses time_t, we have no choice but to convert in at least one of the layers, preferrably we only convert once, thus uint64_t -> time_t. Introducing more conversion steps, say by using base::Time, would only increase the risk of errors and not add anything since the parameter is just passed around. You correctly pointed out that in a different CL (https://codereview.webrtc.org/1683193003/) that OpenSSL has other API which means we ultimately don't have to use time_t. TODOs and a bug (https://bugs.chromium.org/p/webrtc/issues/detail?id=5720) has been created to update SSLIdentity::Generate not to use time_t, in the meantime it is safe to assume that time_t can hold up to a year's worth of seconds (and the if-statement before the static_cast ensures that |expres_s| is not larger than this). I agree that we should stop using time_t and I have added a TODO with the bug in the comment in this CL, but I don't think it should block this CL. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.h (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.h:32: const rtc::Optional<uint64_t>& expires_ms, On 2016/03/08 17:06:10, Ryan Sleevi wrote: > This seems like a clear violation, through creative templating, of the Chromium > (and Google) style guide with respect to Overloads. > > Is there a reason WebRTC isn't following the Google style guide in letter and in > spirit? You're referring to two RequestIdentity signatures, same name but different parameters, with default implementation to call the other signature. There is the "old" signature and the "new" signature. There was only ever meant to be one signature, but because webrtc and chromium are different repos, both signatures exist for a transition period (as to not break chromium by landing a webrtc change that changes the signature). Incidentally this work was put on hold but it's being picked up again, moving forward there will only be this version of RequestIdentity, but the other version can't be removed until this change lands. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/rtc_certificate_generator.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/rtc_certificate_generator.cc:158: void RTCCertificateGenerator::generateCertificateExpires( On 2016/03/08 17:06:11, Ryan Sleevi wrote: > generateCertificateWithExpiration Done. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/rtc_certificate_generator.h (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/rtc_certificate_generator.h:12: #include "third_party/webrtc/base/optional.h" On 2016/03/08 17:06:11, Ryan Sleevi wrote: > This is unnecessary It's needed because only the private helper function uses Optional, the interface being implemented uses different signatures for if expiration is used or not (since that is a blink interface which does not have access to opetional.h). https://codereview.chromium.org/1740993002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebRTCCertificateGenerator.h (right): https://codereview.chromium.org/1740993002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCCertificateGenerator.h:53: virtual void generateCertificate( On 2016/03/08 17:06:11, Ryan Sleevi wrote: > Per Chromium style guide with respect to overloads, shouldn't this be > generateCertificateWithExpiration? Done. https://codereview.chromium.org/1740993002/diff/180001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.h (right): https://codereview.chromium.org/1740993002/diff/180001/content/renderer/media... content/renderer/media/peer_connection_identity_store.h:12: #include "third_party/webrtc/base/optional.h" This can be removed though! I can do so before landing.
https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.cc:156: kIdentityName, key_params, static_cast<time_t>(expires_s))); On 2016/04/13 17:03:08, hbos_chromium wrote: > On 2016/03/08 17:06:10, Ryan Sleevi wrote: > > BUG: You cannot do this sort of cast. There's no guarantee that time_t is > > expressed like this. > > > > Explicitly convert (e.g. using base::Time) > > From JavaScript we get a uint64_t timestamp. We want to use this all the way > down but lower layer stuff uses time_t, we have no choice but to convert in at > least one of the layers, preferrably we only convert once, thus uint64_t -> > time_t. Introducing more conversion steps, say by using base::Time, would only > increase the risk of errors and not add anything since the parameter is just > passed around. > > You correctly pointed out that in a different CL > (https://codereview.webrtc.org/1683193003/) that OpenSSL has other API which > means we ultimately don't have to use time_t. TODOs and a bug > (https://bugs.chromium.org/p/webrtc/issues/detail?id=5720) has been created to > update SSLIdentity::Generate not to use time_t, in the meantime it is safe to > assume that time_t can hold up to a year's worth of seconds (and the > if-statement before the static_cast ensures that |expres_s| is not larger than > this). > > I agree that we should stop using time_t and I have added a TODO with the bug in > the comment in this CL, but I don't think it should block this CL. PS I have a CL in the works that will use uint64_t for its API to be called from here instead of SSLIdentity::Generate, so that the |expires_ms| parameter can be passed around all the way down to the webrtc repo code. (Though until webrtc:5720 is fixed a static_cast is inevitable, but it then occurs at a lower layer and in the webrtc repo.)
https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.cc:156: kIdentityName, key_params, static_cast<time_t>(expires_s))); On 2016/04/13 17:09:39, hbos_chromium wrote: > On 2016/04/13 17:03:08, hbos_chromium wrote: > > On 2016/03/08 17:06:10, Ryan Sleevi wrote: > > > BUG: You cannot do this sort of cast. There's no guarantee that time_t is > > > expressed like this. > > > > > > Explicitly convert (e.g. using base::Time) > > > > From JavaScript we get a uint64_t timestamp. We want to use this all the way > > down but lower layer stuff uses time_t, we have no choice but to convert in at > > least one of the layers, preferrably we only convert once, thus uint64_t -> > > time_t. Introducing more conversion steps, say by using base::Time, would only > > increase the risk of errors and not add anything since the parameter is just > > passed around. > > > > You correctly pointed out that in a different CL > > (https://codereview.webrtc.org/1683193003/) that OpenSSL has other API which > > means we ultimately don't have to use time_t. TODOs and a bug > > (https://bugs.chromium.org/p/webrtc/issues/detail?id=5720) has been created to > > update SSLIdentity::Generate not to use time_t, in the meantime it is safe to > > assume that time_t can hold up to a year's worth of seconds (and the > > if-statement before the static_cast ensures that |expres_s| is not larger than > > this). > > > > I agree that we should stop using time_t and I have added a TODO with the bug > in > > the comment in this CL, but I don't think it should block this CL. > > PS I have a CL in the works that will use uint64_t for its API to be called from > here instead of SSLIdentity::Generate, so that the |expires_ms| parameter can be > passed around all the way down to the webrtc repo code. (Though until > webrtc:5720 is fixed a static_cast is inevitable, but it then occurs at a lower > layer and in the webrtc repo.) Hang on, I'll patch to use this now before asking to PTAL again.
Please take a look tommi and jochen. https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... File content/renderer/media/peer_connection_identity_store.cc (right): https://codereview.chromium.org/1740993002/diff/120001/content/renderer/media... content/renderer/media/peer_connection_identity_store.cc:156: kIdentityName, key_params, static_cast<time_t>(expires_s))); On 2016/04/18 10:04:27, hbos_chromium wrote: > On 2016/04/13 17:09:39, hbos_chromium wrote: > > On 2016/04/13 17:03:08, hbos_chromium wrote: > > > On 2016/03/08 17:06:10, Ryan Sleevi wrote: > > > > BUG: You cannot do this sort of cast. There's no guarantee that time_t is > > > > expressed like this. > > > > > > > > Explicitly convert (e.g. using base::Time) > > > > > > From JavaScript we get a uint64_t timestamp. We want to use this all the way > > > down but lower layer stuff uses time_t, we have no choice but to convert in > at > > > least one of the layers, preferrably we only convert once, thus uint64_t -> > > > time_t. Introducing more conversion steps, say by using base::Time, would > only > > > increase the risk of errors and not add anything since the parameter is just > > > passed around. > > > > > > You correctly pointed out that in a different CL > > > (https://codereview.webrtc.org/1683193003/) that OpenSSL has other API which > > > means we ultimately don't have to use time_t. TODOs and a bug > > > (https://bugs.chromium.org/p/webrtc/issues/detail?id=5720) has been created > to > > > update SSLIdentity::Generate not to use time_t, in the meantime it is safe > to > > > assume that time_t can hold up to a year's worth of seconds (and the > > > if-statement before the static_cast ensures that |expres_s| is not larger > than > > > this). > > > > > > I agree that we should stop using time_t and I have added a TODO with the > bug > > in > > > the comment in this CL, but I don't think it should block this CL. > > > > PS I have a CL in the works that will use uint64_t for its API to be called > from > > here instead of SSLIdentity::Generate, so that the |expires_ms| parameter can > be > > passed around all the way down to the webrtc repo code. (Though until > > webrtc:5720 is fixed a static_cast is inevitable, but it then occurs at a > lower > > layer and in the webrtc repo.) > > Hang on, I'll patch to use this now before asking to PTAL again. Nevermind, using the new API should be done separately, starting to use it before future cleanup work is done is like jumping through an extra hoop since another roll would be necessary. Please review as-is.
nice. lgtm
PTAL jochen
lgtm
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1740993002/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1740993002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1740993002/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== RTCPeerConnection.generateCertificate: Optionally specify expiration. |keygenAlgorithm| of RTCPeerConnection.generateCertificate( AlgorithmIdentifier keygenAlgorithm) gets the optional attribute DOMTimeStamp which, if present, is used to specify when the generated certificate will expires (expressed in number of milliseconds relative to now). BUG=chromium:569005 ========== to ========== RTCPeerConnection.generateCertificate: Optionally specify expiration. |keygenAlgorithm| of RTCPeerConnection.generateCertificate( AlgorithmIdentifier keygenAlgorithm) gets the optional attribute DOMTimeStamp which, if present, is used to specify when the generated certificate will expires (expressed in number of milliseconds relative to now). BUG=chromium:569005 Committed: https://crrev.com/751894c78f4aab168ed17a5704fd6ce306e3781f Cr-Commit-Position: refs/heads/master@{#388183} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/751894c78f4aab168ed17a5704fd6ce306e3781f Cr-Commit-Position: refs/heads/master@{#388183} |
