Closed Bug 1172785 Opened 9 years ago Closed 9 years ago

WebRTC certificate management

Categories

(Core :: WebRTC: Signaling, defect, P3)

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: mt, Assigned: mt)

References

(Blocks 3 open bugs,
URL
)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files)

MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
(deleted), text/x-review-board-request
peterv
: review+
Details
MozReview Request: Bug 1172785 - RTCCertificate interfaces
(deleted), text/x-review-board-request
peterv
: review+
Details
MozReview Request: Bug 1172785 - RTCCertificate implementation
(deleted), text/x-review-board-request
rbarnes
: review+
Details
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC
(deleted), text/x-review-board-request
ekr
: review+
Details
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr
(deleted), text/x-review-board-request
Details 
As per the specification, which has a terrible URL because the editors refuse to keep a public version updated.
Note that certificate persistence for ECDSA certificates won't work until bug 1133698 is fixed.  Also, we will have to have a different factory method for this until bug 863952.  The ECDSA thing should sort itself out without changes to this code, but the latter will require some degree of coordination.
Depends on: 863952
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Bug 1172785 - RTCCertificate implementation
Bug 1172785 - Using RTCCertificate for WebRTC
Blocks: 798862
Assignee: nobody → martin.thomson
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate implementation
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - Using RTCCertificate for WebRTC
Bug 1172785 - Switch to ECDSA for MTI suites
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Attachment #8617113 - Attachment description: MozReview Request: Bug 1172785 - RTCCertificate interfaces → MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Attachment #8617113 - Flags: review?(peterv)
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Attachment #8617114 - Attachment description: MozReview Request: Bug 1172785 - RTCCertificate implementation → MozReview Request: Bug 1172785 - RTCCertificate interfaces
Attachment #8617114 - Flags: review?(peterv)
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Attachment #8617115 - Attachment description: MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC → MozReview Request: Bug 1172785 - RTCCertificate implementation
Attachment #8617115 - Flags: review?(rlb)
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Attachment #8620689 - Attachment description: MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites → MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC
Attachment #8620689 - Flags: review?(ekr)
Bug 1172785 - Switch to ECDSA for MTI suites
Attachment #8624495 - Flags: review?(ekr)
Unblocked thanks to bz.
URL: https://github.com/w3c/webrtc-pc/blob... → https://w3c.github.io/webrtc-pc/#cert...
No longer depends on: 863952
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

https://reviewboard.mozilla.org/r/10803/#review10243

::: media/mtransport/dtlsidentity.h:11
(Diff revision 2)
>  #include "nsISupportsImpl.h"
> +#include "sslt.h"
>  #include "ScopedNSSTypes.h"
> +#include "mozilla/RefPtr.h"

Why this change?

::: media/mtransport/dtlsidentity.h:23
(Diff revision 2)
> -  // Generate an identity with a random name.
> +  DtlsIdentity(SECKEYPrivateKey *privkey,

Comment about whether we take ownership (I assume yes)

::: media/mtransport/dtlsidentity.h:42
(Diff revision 2)
> +  // TODO: move these to RTCCertificate

Please add bug #

::: media/mtransport/dtlsidentity.h:45
(Diff revision 2)
>                                std::size_t size,
> -                              std::size_t *digest_length);
> +                              std::size_t *digest_length) const;

juberti, but these don't ned std:: right?

::: media/mtransport/dtlsidentity.h:47
(Diff revision 2)
>    static nsresult ComputeFingerprint(const CERTCertificate *cert,
>                                       const std::string algorithm,
> -                                     unsigned char *digest,
> +                                     uint8_t *digest,
>                                       std::size_t size,
>                                       std::size_t *digest_length);

Isn't this also const?

::: media/mtransport/dtlsidentity.h:62
(Diff revision 2)
> -      : privkey_(privkey), cert_(cert) {}
> -  DISALLOW_COPY_ASSIGN(DtlsIdentity);
> +  void operator=(const DtlsIdentity&) = delete;
> +  DtlsIdentity(const DtlsIdentity&) = delete;

Why did you remove the macro here?

::: media/mtransport/dtlsidentity.cpp
(Diff revision 2)
> -DtlsIdentity::~DtlsIdentity() {
> -  // XXX: make cert_ a smart pointer to avoid this, after we figure
> -  // out the linking problem.
> -  if (cert_)
> -    CERT_DestroyCertificate(cert_);
> -}
> -

You seem to hve just moved this. Why?

::: media/mtransport/dtlsidentity.cpp:48
(Diff revision 2)
> -  rsaparams.pe = 65537; // We are too paranoid to use 3 as the exponent.
> +  if (!oidData || oidData->oid.len > sizeof(paramBuf) - 2) {

Parens please.

::: media/mtransport/dtlsidentity.cpp:162
(Diff revision 2)
>                                            unsigned char *digest,

This is uint8_t in the signature.

::: media/mtransport/dtlsidentity.cpp:173
(Diff revision 2)
>                                            unsigned char *digest,

uint8_t

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
(Diff revision 2)
> -                                 public nsNSSShutDownObject,
>                                   public DOMMediaStream::PrincipalChangeObserver,

Note to self: review shutdown cycle.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:352
(Diff revision 2)
> +  // This is a hack to support external linkage.
> +  TemporaryRef<DtlsIdentity> Identity() const;
> +

Should it be on the other side of an #ifdef?

::: dom/media/PeerConnection.js:661
(Diff revision 2)
>        return this._chain(() => {
> -        let p = new this._win.Promise((resolve, reject) => {
> +        let p = this._certificateReady.then(
> +          () => new this._win.Promise((resolve, reject) => {
> -          this._onCreateOfferSuccess = resolve;
> +            this._onCreateOfferSuccess = resolve;

This repeated certificateReady() idiom is pretty gross. Is there some way to refactor that out?

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:24
(Diff revision 2)
> +      badCertificate({
> +        name: "RSASSA-PKCS1-v1_5",
> +        hash: "SHA-256",
> +        modulusLength: 1024,
> +        publicExponent: new Uint8Array([1, 0, 1])
> +      }, "1024-bit is too small to succeed"),

Do we really want to prohibit 1024-bit? I note that we presently generate that as our default, so this seems a little odd.

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:49
(Diff revision 2)
> +    openDB.onupgradeneeded = e => {
> +      var db = e.target.result;

Wow, this makes me want to kill myself.

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:65
(Diff revision 2)
> +      write.onsuccess = () => resolve();

I'm not an expert on indexeddb, but don't oyu need onerror?

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:76
(Diff revision 2)
> +      read.onsuccess = e => resolve(e.target.result);

onerror?

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:94
(Diff revision 2)
> +    return checkBadParameters()
> +      .then(() => Promise.all([
> +        mozRTCPeerConnection.generateCertificate({ name: "ECDSA", namedCurve: "P-256" }).then(c => storeAndRetrieve(c)),
> +        // Note: we can't store and retrieve ECDSA keys, so we don't test that
> +        // See Bug 1133698

This is confusing, since it seems you are testing storing and retrieving ECDSA but not RSA, despite this comment

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:118
(Diff revision 2)
> +      });

This test should verify which key is used.

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:120
(Diff revision 2)
> +</script>
> +</pre>
> +</body>

Please add tests for:
1. Attempts to set after creation failing.
2. Expiry (if possible)

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:16
(Diff revision 2)
> +      .then(() => ok(false, message), () => ok(true, message));

check that the error is InvalidAccessError

ceck that expires exists.

::: dom/media/PeerConnection.js:400
(Diff revision 2)
> +  _initCertificate: function(certificates) {
> +    let certPromise;
> +    if (certificates && certificates.length > 0) {
> +      let cert = certificates.find(c => c.expires.getTime() > Date.now());
> +      if (!cert) {
> +        throw new this._win.DOMException(
> +          "Unable to create RTCPeerConnection with an expired certificate",
> +          "InvalidParameterError");
> +      }
> +      certPromise = Promise.resolve(cert);
> +    } else {
> +      certPromise = this._win.mozRTCPeerConnection.generateCertificate({
> +        name: "ECDSA", namedCurve: "P-256"
> +      });
> +    }
> +    this._certificateReady = certPromise
> +      .then(cert => this._impl.certificate = cert);
> +  },
> +

This code does not seem to satisfy the implied API invariant, which is to use *all* the certificates. Rather, it allows only the first one. At minimum, it seems like this requires some sort of API warning if the user supplies > 1 valid certificate, and a bug # to fix it.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:800
(Diff revision 2)
> +  if (mCertificate) {

Is there any way in which this can be nonzero given that you just took the address of a reference?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:801
(Diff revision 2)
> +    const std::string& fpAlg = DtlsIdentity::DEFAULT_HASH_ALGORITHM;

Isn't this already a const std::string?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:803
(Diff revision 2)
> +    nsresult rv = CalculateFingerprint(fpAlg, fingerprint);

This should be a pointer to fingerprint, because it's non-const. I know this was in the previous code, but since we're here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:810
(Diff revision 2)
> +    rv = mJsepSession->AddDtlsFingerprint(fpAlg, fingerprint);
> +    if (NS_FAILED(rv)) {
> +      CSFLogError(logTag, "%s: Couldn't set DTLS credentials, rv=%u",
> +                  __FUNCTION__, static_cast<unsigned>(rv));
> +      mCertificate = nullptr;
> +      return;
> +    }

This doesnt do the right thing, since we *add* the fingerprint, not replace it, so multiple calls to SetCertificate() produce an invalid state.

Is there a reason not to instead MOZ_ASSERT(!mCertificate) at the top?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:807
(Diff revision 2)
> +      mCertificate = nullptr;
> +      return;

Wouldn't it make more sense to set mCertificate at the end of this, not the beginning, since then you don't blow away the existing cert if you are in an invalid state. You can ignore this if you assert as above.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:820
(Diff revision 2)
> +already_AddRefed<mozilla::dom::RTCCertificate>

Why are you returning already_AddRefed() instead of const RefPtr&?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:829
(Diff revision 2)
> +TemporaryRef<DtlsIdentity>

Kind of sad about the TemporaryRef(), why not const RefPtr<>&

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
(Diff revision 2)
> -#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> -// If NSS is shutting down, then we need to get rid of the DTLS
> -// identity right now; otherwise, we'll cause wreckage when we do
> -// finally deallocate it in our destructor.
> -void
> -PeerConnectionImpl::virtualDestroyNSSReference()
> -{
> -  destructorSafeDestroyNSSReference();
> -}
> -
> -void
> -PeerConnectionImpl::destructorSafeDestroyNSSReference()
> -{
> -  MOZ_ASSERT(NS_IsMainThread());
> -  CSFLogDebug(logTag, "%s: NSS shutting down; freeing our DtlsIdentity.", __FUNCTION__);
> -  mIdentity = nullptr;
> -}
> -#endif
> -

Is the reason that this is now safe that the JS is supposedly holding a strong reference to the certificate and hence the identity? If not, then why is it safe?
Attachment #8620689 - Flags: review?(ekr)
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

https://reviewboard.mozilla.org/r/11707/#review10247

::: media/mtransport/transportlayerdtls.cpp:653
(Diff revision 1)
> -  TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
> -  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
> +  TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
> +  TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA

Does this mean that the RSA suites are now not on? If the situation is that they are on in Firefox prefs, you should make a comment here so it's clear that we're not disabling them.
Attachment #8624495 - Flags: review?(ekr)
Blocks: 1176518
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

https://reviewboard.mozilla.org/r/10549/#review10445

::: dom/bindings/Codegen.py:6754
(Diff revision 3)
> -        elif descriptor.interface.isJSImplemented():
> +        elif descriptor.interface.isJSImplemented() and not idlNode.isStatic():

I think this should be

        elif descriptor.interface.isJSImplemented():
            if not idlNode.isStatic():
                ...

::: dom/bindings/parser/WebIDL.py:4304
(Diff revision 3)
> +              identifier == "StaticClassOverride"):

I think validate() should check that this attribute is only used on JS implemented things.

::: dom/bindings/Codegen.py:13695
(Diff revision 3)
> -        CGBindingImplClass.__init__(self, descriptor, CGJSImplMethod, CGJSImplGetter, CGJSImplSetter)
> +        CGBindingImplClass.__init__(self, descriptor, CGJSImplMethod, CGJSImplGetter, CGJSImplSetter, skipStaticMethods=True)

This doesn't look to be used? Remove it if so.

::: dom/bindings/Codegen.py:13191
(Diff revision 3)
> -    def __init__(self, descriptor, cgMethod, cgGetter, cgSetter, wantGetParent=True, wrapMethodName="WrapObject"):
> +    def __init__(self, descriptor, cgMethod, cgGetter, cgSetter, wantGetParent=True, wrapMethodName="WrapObject", skipStaticMethods=False):

This doesn't look to be used? Remove it if so.
Attachment #8617113 - Flags: review?(peterv)
(In reply to Peter Van der Beken [:peterv] from comment #18)

BTW, I can't defend the architectural decisions here; bz wrote the patch, I applied it.

> I think validate() should check that this attribute is only used on JS
> implemented things.

I hope that I got this part right.  I've tested that it works, but there may be a more elegant way.

> ::: dom/bindings/Codegen.py:13191
> (Diff revision 3)
> > +    def __init__(self, descriptor, cgMethod, cgGetter, cgSetter, wantGetParent=True, wrapMethodName="WrapObject", skipStaticMethods=False):
> 
> This doesn't look to be used? Remove it if so.

This one is used.
BTW, both skipStaticMethods are used.
Blocks: 1177919
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Attachment #8617113 - Flags: review?(peterv)
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Attachment #8620689 - Flags: review?(ekr)
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

Bug 1172785 - Switch to ECDSA for MTI suites
Attachment #8624495 - Flags: review?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #16)
> >    static nsresult method(arguments...);
> 
> Isn't this also const?

Well, yes, but static methods always are.

> Note to self: review shutdown cycle.

Don't forget...  Hopefully the note I added will help.

> > +  TemporaryRef<DtlsIdentity> Identity() const;
> 
> Should it be on the other side of an #ifdef?

No, this is the unconditional bit so that users of the class have a uniform interface and they don't have to worry about their own conditionals.

> ::: dom/media/PeerConnection.js:661
> This repeated certificateReady() idiom is pretty gross. Is there some way to
> refactor that out?

Yeah, I considered having every chained function call depend on having the certificate generated.  This is adequate; and analogous to the targetted GMP initialization stuff that happens in C++.

> Do we really want to prohibit 1024-bit? I note that we presently generate
> that as our default, so this seems a little odd.

I know that the analysis support 1024 being OK, for now, but I'd prefer to be consistent in our message about what we think is acceptable.

> This is confusing, since it seems you are testing storing and retrieving
> ECDSA but not RSA, despite this comment

Ahh, that was something I forgot to fix up.

> ::: dom/media/tests/mochitest/test_peerConnection_certificates.html:118
> (Diff revision 2)
> > +      });
> 
> This test should verify which key is used.

I can get the configuration, if we supported it, but I can't otherwise tell.  I'm happy to expand the API even more, but I'd rather do that later: bug 1177919.

> 1. Attempts to set after creation failing.

That doesn't make sense.  If you can't get a certificate, then you can't even try to set it.

> 2. Expiry (if possible)

Quite tricky, but done.

> > +TemporaryRef<DtlsIdentity>
> 
> Kind of sad about the TemporaryRef(), why not const RefPtr<>&

This is your code...

The trick here is that in one path the code makes a new instance and on the other, it

> > -PeerConnectionImpl::destructorSafeDestroyNSSReference()
> 
> Is the reason that this is now safe that the JS is supposedly holding a
> strong reference to the certificate and hence the identity? If not, then why
> is it safe?

Now, RTCCertificate has a fully independent lifecycle and therefore it needs this stuff.  Previously, the use of NSS could track RTCPeerConnection because RTCPeerConnection strictly owned DtlsIdentity.

BTW, factoring this so that DtlsIdentity could be used in tests makes this quite a lot uglier than I'd like.  I'm actually creating new instances of DtlsIdentity for each transport.  I'll put in a note explaining how this whole thing is designed; in particular, the lifecycle for the crypto objects is complicated by having to support external linkage.
(In reply to Martin Thomson [:mt:] from comment #30)
> (In reply to Eric Rescorla (:ekr) from comment #16)
> > >    static nsresult method(arguments...);
> > 
> > Isn't this also const?
> 
> Well, yes, but static methods always are.
> 
> > Note to self: review shutdown cycle.
> 
> Don't forget...  Hopefully the note I added will help.
> 
> > > +  TemporaryRef<DtlsIdentity> Identity() const;
> > 
> > Should it be on the other side of an #ifdef?
> 
> No, this is the unconditional bit so that users of the class have a uniform
> interface and they don't have to worry about their own conditionals.
> 
> > ::: dom/media/PeerConnection.js:661
> > This repeated certificateReady() idiom is pretty gross. Is there some way to
> > refactor that out?
> 
> Yeah, I considered having every chained function call depend on having the
> certificate generated.  This is adequate; and analogous to the targetted GMP
> initialization stuff that happens in C++.

Why not just push it onto the chain?



> 
> > ::: dom/media/tests/mochitest/test_peerConnection_certificates.html:118
> > (Diff revision 2)
> > > +      });
> > 
> > This test should verify which key is used.
> 
> I can get the configuration, if we supported it, but I can't otherwise tell.
> I'm happy to expand the API even more, but I'd rather do that later: bug
> 1177919.
> 
> > 1. Attempts to set after creation failing.
> 
> That doesn't make sense.  If you can't get a certificate, then you can't
> even try to set it.

Sorry, attempts to set the certificate on the RTCPeerConnection after it
was created (using setConfiguration)
(In reply to Eric Rescorla (:ekr) from comment #31)> 
> Why not just push it onto the chain?

Because that breaks encapsulation in ways that (I think) are worse than this.

> > > 1. Attempts to set after creation failing.
> > 
> > That doesn't make sense.  If you can't get a certificate, then you can't
> > even try to set it.
> 
> Sorry, attempts to set the certificate on the RTCPeerConnection after it
> was created (using setConfiguration)

Oh, we don't support that.  One thing I noticed on the plane was that fact.  updateIce throws UnsupportedError, setConfiguration doesn't exist, and getConfiguration throws an internal error (because we declare it in WebIDL and don't have an implementation for the function).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52940269958d because (for some reason) the push-to-try button disappeared from reviewboard.
https://reviewboard.mozilla.org/r/10803/#review10243

> check that the error is InvalidAccessError
> 
> ceck that expires exists.

This does not seem to be fixed.
https://reviewboard.mozilla.org/r/10803/#review10243

> This does not seem to be fixed.

I've added checks in other places: lines 100 and 108 check different aspects of the expiration.
Yes, but you're not checking that the error is the correct error.
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Awaiting new patch
Attachment #8620689 - Flags: review?(ekr)
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

awaiting new patch
Attachment #8624495 - Flags: review?(ekr)
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

https://reviewboard.mozilla.org/r/10553/#review10753

::: dom/base/nsJSEnvironment.cpp:2595
(Diff revision 4)
> +  RTCCertificate* cert;

Please add a comment here in parallel with the above.

::: dom/crypto/WebCryptoTask.h:208
(Diff revision 4)
> +class GenerateAsymmetricKeyTask : public WebCryptoTask

If you're going to re-structure one of the WebCryptoTask implementation objects, please do them all (or file a follow-on).

::: dom/media/webrtc/RTCCertificate.h:59
(Diff revision 4)
> +  int64_t Expires() const { return mExpires / PR_USEC_PER_MSEC; }

I assume it's documented somewhere that in the WebIDL-to-C++ mapping, Date corresponds to an int64 of usecs since epoch?  It would be useful to note that here.

::: dom/media/webrtc/RTCCertificate.h:60
(Diff revision 4)
> +

It would be helpful to have a comment here to distinguish the WebIDL implementation methods from other public methods (e.g., dom/crypto/CryptoKey.h:103)

::: dom/media/webrtc/RTCCertificate.h:77
(Diff revision 4)
> +  void operator=(const RTCCertificate&) = delete;

Is the "= delete" on these methods reflected in JS?  That is, if I have a JS variable X whose value is an RTCCertificate, what happens on "let Y = X"?

::: dom/media/webrtc/RTCCertificate.h:90
(Diff revision 4)
> +

Nit: Extra blank line between member variables.

::: dom/media/webrtc/RTCCertificate.cpp:52
(Diff revision 4)
> +    mExpires = ONE_DAY * 30;

Please make the default lifetime a named constant / #define, or ideally a pref.

::: dom/media/webrtc/RTCCertificate.cpp:53
(Diff revision 4)
> +    if (!aAlgorithm.IsObject()) {

Just checking that the algorithm is an object seems kind of odd.  I guess it makes some sense, given that none of the acceptable algorithms can be specified by name alone.  But GenerateAsymmetricKeyTask() already does much more thorough checking on the algorithm value.  It seems like the thing to do here is check mEarlyRv, as the various Import*KeyTask classes do:
https://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoTask.cpp?from=GenerateAsymmetricKeyTask#1819

::: dom/media/webrtc/RTCCertificate.cpp:59
(Diff revision 4)
> +    bool ok = JS_GetProperty(aCx, jsval, "expires", &exp);

This seems to imply that the algorithm field may have an "expires" field containing the desired expiration date.

The most obvious issue with this behavior is that it doesn't appear to be in the spec.

If we do want to accept this attribute, we should probably place an upper bound on it, say 1 year.  (Certainly no more than 39 months, the CABF limit on server certs.)

I am assuming that the JS type for this attribute would be Date and that JS::ToInt64() handles the conversion properly.

::: dom/media/webrtc/RTCCertificate.cpp:76
(Diff revision 4)
> +    uint8_t randomName[16];

This would be another good thing to #define.

::: dom/media/webrtc/RTCCertificate.cpp:36
(Diff revision 4)
> +class RTCCertificateTask : public GenerateAsymmetricKeyTask

GenerateRTCCertificateTask?

::: dom/media/webrtc/RTCCertificate.cpp:144
(Diff revision 4)
> +  nsresult SignCertificate(SECOidTag aMethodOid) {

The distinction between GenerateCertificate and SignCertificate here seems kind of artificial.  Could also lead to bugs, since if someone does Generate without Sign, they end up with an (unexpectedly) unsigned cert.  Unless you're planning to use them separately, let's just combine them.

In fact, having these be separate from DoCrypto has some of the same risk, since both methods will fail badly if they are not called after a successful run of GenerateAsymmetricKeyTask::DoCrypto().

::: dom/media/webrtc/RTCCertificate.cpp:168
(Diff revision 4)
> +    ScopedSECKEYPrivateKey privateKey(mKeyPair.mPrivateKey.get()->GetPrivateKey());

You could just use mPrivateKey here.  mKeyPair is only used for promise resolution.

Note that if this is not called after a successful run of GenerateAsymmetricKeyTask::DoCrypto(), then either flavor of the private key will be null, and the call to SEC_DerSignData will crash.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/cryptohi/secsign.c?from=SEC_DerSignData#316

::: dom/media/webrtc/RTCCertificate.cpp:219
(Diff revision 4)
> +    // information necessary for export; mPrivateKey is not always good.

This doesn't really seem like a kludge, just picking the right thing.  mPrivateKey is a ScopedSECKEYPrivateKey, so it *can't* be fixed up with export data.

::: dom/media/webrtc/RTCCertificate.cpp:351
(Diff revision 4)
> +  return JS_WriteBytes(aWriter, certs->certs[0].data, certs->certs[0].len);

If you're going to use ReadBuffer below, you should use WriteBuffer here.  It requires a gratuitous copy to a CryptoBuffer, but it saves the risk of breakage if someone changes WriteBuffer / ReadBuffer.  You could also add a WriteBuffer variant that takes (uint8_t\* data, uint32_t len) directly to avoid the gratuitous copy.

::: dom/media/webrtc/RTCCertificate.cpp:181
(Diff revision 4)
> +    if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1) ||

This checking seems like it would be better done in the constructor.

::: dom/media/webrtc/RTCCertificate.cpp:182
(Diff revision 4)
> +        mAlgName.EqualsLiteral(WEBCRYPTO_ALG_RSA_OAEP)) {

OAEP is not relevant here.

::: dom/media/webrtc/RTCCertificate.cpp:184
(Diff revision 4)
> +      if (mRsaParams.keySizeInBits < 1024) {

Replace literal with #define or pref.
Attachment #8617115 - Flags: review?(rlb)
https://reviewboard.mozilla.org/r/10553/#review10753

> Please add a comment here in parallel with the above.

You and I have very different sensibilities when it comes to redundant comments.

> If you're going to re-structure one of the WebCryptoTask implementation objects, please do them all (or file a follow-on).

Again, it seems like we disagree on the consistency/goodness tradeoff.  I'm happier with a small amount of inconsistency than I am with having all this stuff split and having to deal with the horrors of C++ class declarations.  The other classes don't need to be exported, so don't export them.

> Is the "= delete" on these methods reflected in JS?  That is, if I have a JS variable X whose value is an RTCCertificate, what happens on "let Y = X"?

X = Y works because it is equivalent to pointer assignment; value copying isn't a thing in JS (aside from structured clone).

> Please make the default lifetime a named constant / #define, or ideally a pref.

A pref isn't necessary since we support an expires attribute.

> Just checking that the algorithm is an object seems kind of odd.  I guess it makes some sense, given that none of the acceptable algorithms can be specified by name alone.  But GenerateAsymmetricKeyTask() already does much more thorough checking on the algorithm value.  It seems like the thing to do here is check mEarlyRv, as the various Import*KeyTask classes do:
> https://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoTask.cpp?from=GenerateAsymmetricKeyTask#1819

I think that the next bit goes to why this is the case.  I'm not checking that this is an object for fun, or because I somehow don't trust GenerateAsymmetricKeyTask.

> This seems to imply that the algorithm field may have an "expires" field containing the desired expiration date.
> 
> The most obvious issue with this behavior is that it doesn't appear to be in the spec.
> 
> If we do want to accept this attribute, we should probably place an upper bound on it, say 1 year.  (Certainly no more than 39 months, the CABF limit on server certs.)
> 
> I am assuming that the JS type for this attribute would be Date and that JS::ToInt64() handles the conversion properly.

I've added an upper bound.  And a comment explaining why this exists.  We go off spec here to facilitate testing only.  It's already turned out to be valuable.

> The distinction between GenerateCertificate and SignCertificate here seems kind of artificial.  Could also lead to bugs, since if someone does Generate without Sign, they end up with an (unexpectedly) unsigned cert.  Unless you're planning to use them separately, let's just combine them.
> 
> In fact, having these be separate from DoCrypto has some of the same risk, since both methods will fail badly if they are not called after a successful run of GenerateAsymmetricKeyTask::DoCrypto().

Oh, so even though you've "heard" of functional decomposition, you just don't believe in it.  This would be a bad idea.  They are private methods; it's not like I'm leaving land mines lying around here.

> You could just use mPrivateKey here.  mKeyPair is only used for promise resolution.
> 
> Note that if this is not called after a successful run of GenerateAsymmetricKeyTask::DoCrypto(), then either flavor of the private key will be null, and the call to SEC_DerSignData will crash.
> 
> https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/cryptohi/secsign.c?from=SEC_DerSignData#316

I'm exercising variable-use discipline.  I have to pretend that mPrivateKey doesn't exist here because it is dangerous to use it later.  The value in mKeyPair is the only one that gets fixed up so that it can be exported.  In fact, I'm going to mark the private and public keys private on GenerateAsymmetricKeyTask.

> This checking seems like it would be better done in the constructor.

Absolutely not.  I will move it to BeforeCrypto though.
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Attachment #8617115 - Flags: review?(rlb)
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Attachment #8620689 - Flags: review?(ekr)
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

Bug 1172785 - Switch to ECDSA for MTI suites
Attachment #8624495 - Flags: review?(ekr)
Attachment #8624495 - Flags: review?(ekr) → review+
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

https://reviewboard.mozilla.org/r/11707/#review10771

Ship It!
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Attachment #8624495 - Flags: review+ → review?(ekr)
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

Bug 1172785 - Switch to ECDSA for MTI suites
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

https://reviewboard.mozilla.org/r/10803/#review10773

::: dom/media/tests/mochitest/test_peerConnection_certificates.html:21
(Diff revisions 3 - 4)
> -  // of the checking is done by the WebCrypto code underpinning this, but a
> -  // sanity check is still in order.
> +  // of the checking is done by the WebCrypto code underpinning this, hence the
> +  // baffling error codes, but a sanity check is still in order.
>    function checkBadParameters() {
>      return Promise.all([
>        badCertificate({
>          name: "RSASSA-PKCS1-v1_5",
>          hash: "SHA-256",
>          modulusLength: 1023,
>          publicExponent: new Uint8Array([1, 0, 1])
> -      }, "1023-bit is too small to succeed"),
> +      }, "NotSupportedError", "1023-bit is too small to succeed"),
>  
>        badCertificate({
>          name: "ECDH",
>          namedCurve: "P-256"
> -      }, "valid ECDH isn't OK"),
> +      }, "DataError", "otherwise valid ECDH config is rejected"),
>  
>        badCertificate({
>          name: "not a valid algorithm"
> -      }, "not a valid algorithm"),
> +      }, "SyntaxError", "not a valid algorithm"),
> +
> +      badCertificate("ECDSA", "SyntaxError", "a bare name is not enough"),
>  
>        badCertificate({
>          name: "ECDSA",
>          namedCurve: "not a curve"
> -      }, "not a curve")
> +      }, "NotSupportedError", "ECDSA with an unknown curve")

This doesn't seem spec conformant. The spec says:

"A user agent must reject a call to generateCertificate() with a DOMError of type "InvalidAccessError" if the keygenAlgorithm parameter identifies an algorithm that the user agent cannot or will not use to generate a certificate for RTCPeerConnection."
Attachment #8620689 - Flags: review?(ekr)
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Attachment #8620689 - Flags: review?(ekr)
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

Bug 1172785 - Switch to ECDSA for MTI suites
Attachment #8624495 - Flags: review?(ekr)
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

Bug 1172785 - RTCCertificate interfaces
Comment on attachment 8617115 [details]
MozReview Request: Bug 1172785 - RTCCertificate implementation

Bug 1172785 - RTCCertificate implementation
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

Bug 1172785 - Using RTCCertificate for WebRTC
Comment on attachment 8624495 [details]
MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr

Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr
Attachment #8624495 - Attachment description: MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites → MozReview Request: Bug 1172785 - Switch to ECDSA for MTI suites, r=ekr
Attachment #8624495 - Flags: review?(ekr)
A bunch of needless churn because of the change to s/TemporaryRef/already_Addrefed...
Comment on attachment 8620689 [details]
MozReview Request: Bug 1172785 - Using RTCCertificate for WebRTC

https://reviewboard.mozilla.org/r/10803/#review10817

Ship It!
Attachment #8620689 - Flags: review?(ekr) → review+
Comment on attachment 8617113 [details]
MozReview Request: Bug 1172785 - Adding StaticClassOverride routing for JS implemented WebIDL

https://reviewboard.mozilla.org/r/10549/#review10861

Ship It!
Attachment #8617113 - Flags: review?(peterv) → review+
Comment on attachment 8617114 [details]
MozReview Request: Bug 1172785 - RTCCertificate interfaces

https://reviewboard.mozilla.org/r/10551/#review10863

Ship It!
Attachment #8617114 - Flags: review?(peterv) → review+
https://reviewboard.mozilla.org/r/10553/#review10753

> Again, it seems like we disagree on the consistency/goodness tradeoff.  I'm happier with a small amount of inconsistency than I am with having all this stuff split and having to deal with the horrors of C++ class declarations.  The other classes don't need to be exported, so don't export them.

That's fine.  How about a comment, something like:

> // XXX This class is split differently from the others to enable reuse by WebRTC.

> You and I have very different sensibilities when it comes to redundant comments.

Consistency!
https://reviewboard.mozilla.org/r/10553/#review10753

> Consistency!

Ahh, but that's the problem: comments aren't tested, so maintaining consistency is hard.  The code on the other hand, can never lie.

> That's fine.  How about a comment, something like:
> 
> > // XXX This class is split differently from the others to enable reuse by WebRTC.

Ack, done.
Attachment #8617115 - Flags: review?(rlb) → review+
Attachment #8624495 - Flags: review?(ekr)
Depends on: 1181262
Depends on: 1215519
Keywords: dev-doc-needed
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: