Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1299223002:
Adds RTCCertificate, a reference counted object indirectly owning an SSLCertificate (Closed)
DescriptionAdds RTCCertificate, a reference counted object indirectly owning an SSLCertificate (by owning the SSLIdentity).
BUG=webrtc:4927
R=tommi@chromium.org, tommi@webrtc.org, torbjorng@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/41b3a384f446985bdd7e2b9a8830aa90264fef29
Patch Set 1 : #
Total comments: 7
Patch Set 2 : #
Total comments: 7
Patch Set 3 : ~RTCCertificate() {} to fix compile error on trybots #
Messages
Total messages: 21 (8 generated)
Patchset #1 (id:1) has been deleted
hbos@webrtc.org changed reviewers: + tommi@webrtc.org, torbjorng@webrtc.org
PTAL, tiny CL this time. I will create an issue and reference it with BUG= for all upcoming related CLs. https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificate.h File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.h:20: namespace rtc { Note that I placed this in webrtc/base under the rtc namespace and in the rtc_base build target. https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.h:37: SSLIdentity* identity() const { return identity_.get(); } The SSLIdentity owns both the key pair and the SSLCertificate. Un-entangling "certificate" from "key pair" would require a lot of clean up work that's probably not worth doing now... But theoretically a certificate should be separate from a key pair. (Not even the WebRTC specs seems to distinguish generating keys from generating certificates?) I imagine we want to replace SSLIdentity* with scoped_refptr<RTCCertificate> in a lot of places though, and for now having access to the underlying SSLIdentity* might be necessary in some parts of the code, even though ideally a certificate should just be expires_ns & ssl_certificate. https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.h:40: explicit RTCCertificate(SSLIdentity* identity); Reason for not using scoped_ptr in constructor: The constructor of RefCountedObject<RTCCertificate> does not realize it has to do .Pass() with scoped_ptr parameters. The static Create function substitutes a public constructor.
Two minor comments, C++ subtleties not properly scrutinized. :-) https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:28: uint64 RTCCertificate::expires_ns() const { Is "ns" here for nanoseconds? I believe a saved expires field ought to be a time stamp, not a field of remaining time. https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:33: const SSLCertificate& RTCCertificate::ssl_certificate() const { I suspect (but haven't checked) that we'd need access to the keypair too at this abstraction level (although that is of course not part of the cert, but it might anyway for ownership reasons be easiest to do it here).
https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:28: uint64 RTCCertificate::expires_ns() const { On 2015/08/19 15:23:19, torbjorng1 wrote: > Is "ns" here for nanoseconds? I believe a saved expires field ought to be a time > stamp, not a field of remaining time. Yes ns for nanoseconds, but meant as a timestamp. I looked at timeutils.h and saw Time(), TimeMicros() and TimeNanos(); these are the "current time in milli/micro/nanoseconds". Maybe there is a date format that could be used instead, but I'm assuming these are time since 1970. Time() returns uint32 so I'm guessing that has wrap around problems if you care about the date. The other two Time...() are uint64 and should be fine. And I'm hoping a timestamp like this is easily convertable to a blink/JavaScript type of date. I'll make the name clearer though to avoid this confusion. (tommi might be more familiar with date and time in webrtc/blink?) https://codereview.webrtc.org/1299223002/diff/20001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:33: const SSLCertificate& RTCCertificate::ssl_certificate() const { On 2015/08/19 15:23:19, torbjorng1 wrote: > I suspect (but haven't checked) that we'd need access to the keypair too at this > abstraction level (although that is of course not part of the cert, but it might > anyway for ownership reasons be easiest to do it here). Agreed. The keypair would be an implementation-specific thing, so whatever needs to know about the keypair should get the SSLIdentity*, not some abstract "key pair" class, correct? (Otherwise I could add getters for that and still hide the raw SSLIdentity*).
tommi@chromium.org changed reviewers: + tommi@chromium.org
Great stuff, much easier for a slow reviewer :) Still ~100 lines, so I wouldn't call it tiny ;) https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:20: return new RefCountedObject<RTCCertificate>(identity.release()); use Pass() https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:23: RTCCertificate::RTCCertificate(SSLIdentity* identity) change param to scoped_ptr<SSLIdentity> identity https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:24: : identity_(identity) { and here do identity_(identity.Pass()) https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificate.h File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.h:47: scoped_ptr<SSLIdentity> identity_; I'm a bit confused here, so I'll ask tomorrow in person. I thought the identity was going to be used to generate the certificate, but now it looks like I got that backwards?
All is relative :P https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:24: : identity_(identity) { On 2015/08/19 19:42:38, tommi wrote: > and here do identity_(identity.Pass()) Did you see my previous comment? "Reason for not using scoped_ptr in constructor: The constructor of RefCountedObject<RTCCertificate> does not realize it has to do .Pass() with scoped_ptr parameters. The static Create function substitutes a public constructor." I don't think what you're asking is possible with scoped_ptr? I tried what you asked and it didn't compile because the RefCountedObject constructor tries to call the RTCCertificate constructor without using .Pass(). https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificate.h File webrtc/base/rtccertificate.h (right): https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.h:47: scoped_ptr<SSLIdentity> identity_; On 2015/08/19 19:42:38, tommi wrote: > I'm a bit confused here, so I'll ask tomorrow in person. I thought the identity > was going to be used to generate the certificate, but now it looks like I got > that backwards? Please correct me if I'm wrong torbjorng, but here's how I see it: If everything made sense what we would Generate is the key pair alone, and this would be the expensive part, and then from the key pair we would very quickly be able to generate any number of certificates. The "identity" is a WebRTC concept which is a key pair AND a certificate. SSLIdentity::Generate goes to OpenSSLIdentity::Generate which generates both. SSLIdentity* is used all over the place and splitting up key pairs and certificates would probably be large clean up work that if performed now would block this more important work. The following I am less confident about, but I'm not sure if RSA/ECDSA/etc is relevant when generating a certificate? Only relevant when generating key pairs... or is this wrong? The WebRTC specs don't talk about generating keys separately from generating certificates though... It's akin to "generate RSA cert with x bits" or "generate ECDSA cert". We can talk more offline.
https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... File webrtc/base/rtccertificate.cc (right): https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... webrtc/base/rtccertificate.cc:24: : identity_(identity) { On 2015/08/20 08:01:39, hbos wrote: > On 2015/08/19 19:42:38, tommi wrote: > > and here do identity_(identity.Pass()) > > Did you see my previous comment? > "Reason for not using scoped_ptr in constructor: The constructor of > RefCountedObject<RTCCertificate> does not realize it has to do .Pass() with > scoped_ptr parameters. > The static Create function substitutes a public constructor." > > I don't think what you're asking is possible with scoped_ptr? I tried what you > asked and it didn't compile because the RefCountedObject constructor tries to > call the RTCCertificate constructor without using .Pass(). I probably saw the comment but didn't understand it. I get it now and the issue is with this line: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... OK, as is then.
On 2015/08/20 08:49:04, tommi wrote: > https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... > File webrtc/base/rtccertificate.cc (right): > > https://codereview.webrtc.org/1299223002/diff/40001/webrtc/base/rtccertificat... > webrtc/base/rtccertificate.cc:24: : identity_(identity) { > On 2015/08/20 08:01:39, hbos wrote: > > On 2015/08/19 19:42:38, tommi wrote: > > > and here do identity_(identity.Pass()) > > > > Did you see my previous comment? > > "Reason for not using scoped_ptr in constructor: The constructor of > > RefCountedObject<RTCCertificate> does not realize it has to do .Pass() with > > scoped_ptr parameters. > > The static Create function substitutes a public constructor." > > > > I don't think what you're asking is possible with scoped_ptr? I tried what you > > asked and it didn't compile because the RefCountedObject constructor tries to > > call the RTCCertificate constructor without using .Pass(). > > I probably saw the comment but didn't understand it. I get it now and the issue > is with this line: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > OK, as is then. oh and lgtm.
LGTM
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.webrtc.org/1299223002/#ps60001 (title: "~RTCCertificate() {} to fix compile error on trybots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1299223002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1299223002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/431)
darn it... lgtm!
The CQ bit was checked by hbos@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1299223002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1299223002/60001
The CQ bit was unchecked by hbos@webrtc.org
Message was sent while issue was closed.
Committed patchset #3 (id:60001) manually as 41b3a384f446985bdd7e2b9a8830aa90264fef29 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
