RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto

third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 26 matching lines @@
 #include "bindings/core/v8/Nullable.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/modules/v8/V8RTCCertificate.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/frame/LocalFrame.h"
 #include "core/html/VoidCallback.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
+#include "modules/crypto/CryptoResultImpl.h"
 #include "modules/mediastream/MediaConstraintsImpl.h"
 #include "modules/mediastream/MediaStreamEvent.h"
 #include "modules/mediastream/RTCDTMFSender.h"
 #include "modules/mediastream/RTCDataChannel.h"
 #include "modules/mediastream/RTCDataChannelEvent.h"
 #include "modules/mediastream/RTCErrorCallback.h"
 #include "modules/mediastream/RTCIceCandidateEvent.h"
 #include "modules/mediastream/RTCSessionDescription.h"
 #include "modules/mediastream/RTCSessionDescriptionCallback.h"
 #include "modules/mediastream/RTCSessionDescriptionRequestImpl.h"
 #include "modules/mediastream/RTCStatsCallback.h"
 #include "modules/mediastream/RTCStatsRequestImpl.h"
 #include "modules/mediastream/RTCVoidRequestImpl.h"
 #include "platform/mediastream/RTCConfiguration.h"
 #include "platform/mediastream/RTCOfferOptions.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebCryptoAlgorithmParams.h"
 #include "public/platform/WebMediaStream.h"
 #include "public/platform/WebRTCCertificate.h"
 #include "public/platform/WebRTCCertificateGenerator.h"
 #include "public/platform/WebRTCConfiguration.h"
 #include "public/platform/WebRTCDataChannelHandler.h"
 #include "public/platform/WebRTCDataChannelInit.h"
 #include "public/platform/WebRTCICECandidate.h"
 #include "public/platform/WebRTCKeyParams.h"
 #include "public/platform/WebRTCOfferOptions.h"
 #include "public/platform/WebRTCSessionDescription.h"
@@ skipping 42 matching lines @@
 
     void onError() override
     {
         m_resolver->reject();
         delete this;
     }
 
     Persistent<ScriptPromiseResolver> m_resolver;
 };
 
+static bool uint8ArrayToUint32(const WebVector<uint8_t>& exponentBytes, uint32_t* out)
+{
+    if (exponentBytes.size() > 4) {
+        // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there

[eroman, 2015/10/22 17:38:34]

This implementation is not in line with the WebCrypto spec
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#big-integer

In particular:
The API SHALL accept values with any number of leading zero bits, including the
empty array, which represents zero.

The only assumption you can make about the normalized exponent returned by
WebCrypto's normalizeAlgorithm() is that it is non-empty (it will re-write an
empty input to simply 0).

Here is a more complete implementation:

https://code.google.com/p/chromium/codesearch#chromium/src/components/webcryp...

I suggest moving that implementation into shared code (will necessarily have to
be part of the Blink public API).  How about making it a method on
RsaHashedKeyGenParams ?

[hta - Chromium, 2015/10/23 06:55:00]

won't it have to be a change (at least a suggested change) to the WebCrypto spec
in order to be an extension of the Blink public API?

[hbos_chromium, 2015/10/23 09:59:05]

@eroman: Ah I misinterpreted that piece of WebCrypto spec about the leading
zeros. No need to reinvent the wheel, moving and using existing code.

By RsaHashedKeyGenParams I assume you mean WebCryptoRsaHashedKeyGenParams, i.e.
a publicExponentUint32(). I moved it there, but these are all header files and I
don't think I'm allowed to create .cc files. Is it OK to put "non-trivial"
methods like this in a header? (If it necessarily has to be part of Blink public
API I guess so?)

Also, while I was able to place it there, BigInteger->uint32 is a handy utility
method that you might want to use for other BigIntegers than RSA publicExponent.
May I suggest moving this to WebCrypto.h or create a new file, something like
WebCryptoUtils.h before landing? (It would have to be a static or inline
function if it's all in a header like this.)

@hta: (Does it really have to be part of the Blink public API? Can't I place it
in Source/modules/crypto? But looks like the other files here don't including
from that...)
If it's a (static) global utility function, not part of
WebCryptoRsaHashedKeyGenParams, that's just there as a stand-alone helper
function, would it still have to be part of the spec?

[eroman, 2015/10/23 19:13:03]

@hta: Does not require any spec changes, just adding a helper to implement
existing spec behaviors.

@hbos: Sure, a general BigInteger --> uint32 method sounds good. In the interest
of preventing more churn on this CL, I suggest you propose the generalization as
a separate CL, and we can discuss the details over there. (also requires API
OWNERs approval). In this one either keep it inline in the header, or
temporarily duplicate that code into your .cpp file.

[eroman, 2015/10/23 19:15:52]

Oh actually, I withdraw the request to do that refactor as a follow-up. Forgot
this is actually already a separate CL :)
Please go ahead and make the proposed change to move BigInteger --> uint32 to a
helper, and ensure that it is not defined inline.

[hbos_chromium, 2015/10/26 13:21:32]

Done.

+        // is no need to check for leading 0-bytes. Thus exponentBytes.size() > 4 values do not fit in uint32_t.
+        return false;
+    }
+    // Big-endian: exponentBytes[0] is the most significant byte.
+    *out = 0;
+    for (size_t i = exponentBytes.size(), shift = 0; i-- > 0; shift += 8)
+        *out += (exponentBytes[i] << shift);
+    return true;
+}
+
 } // namespace
 
 RTCConfiguration* RTCPeerConnection::parseConfiguration(const Dictionary& configuration, ExceptionState& exceptionState)
 {
     if (configuration.isUndefinedOrNull())
         return 0;
 
     RTCIceTransports iceTransports = RTCIceTransportsAll;
     String iceTransportsString;
     if (DictionaryHelper::get(configuration, "iceTransports", iceTransportsString)) {
@@ skipping 331 matching lines @@
 
     WebMediaConstraints constraints = MediaConstraintsImpl::create(mediaConstraints, exceptionState);
     if (exceptionState.hadException())
         return;
 
     bool valid = m_peerHandler->updateICE(configuration, constraints);
     if (!valid)
         exceptionState.throwDOMException(SyntaxError, "Could not update the ICE Agent with the given configuration.");
 }
 
-ScriptPromise RTCPeerConnection::generateCertificate(ScriptState* scriptState, const Dictionary& keygenAlgorithm, ExceptionState& exceptionState)
+ScriptPromise RTCPeerConnection::generateCertificate(ScriptState* scriptState, const AlgorithmIdentifier& keygenAlgorithm, ExceptionState& exceptionState)
 {
-    // Validate and interpret input |keygenAlgorithm|.
-    // TODO(hbos): Use WebCrypto normalization process to validate and interpret |keygenAlgorithm|.
-    // This may create a dependency between the Blink and WebCrypto modules? crbug.com/544917
+    // Normalize |keygenAlgorithm| with WebCrypto, making sure it is a recognized AlgorithmIdentifier.
+    WebCryptoAlgorithm cryptoAlgorithm;
+    AlgorithmError error;
+    if (!normalizeAlgorithm(keygenAlgorithm, WebCryptoOperationGenerateKey, cryptoAlgorithm, &error)) {
+        // Reject generateCertificate with the same error as was produced by WebCrypto.
+        // |result| is garbage collected, no need to delete.
+        CryptoResultImpl* result = CryptoResultImpl::create(scriptState);
+        ScriptPromise promise = result->promise();
+        result->completeWithError(error.errorType, error.errorDetails);
+        return promise;
+    }
+
+    // Convert from WebCrypto representation to recognized WebRTCKeyParams. WebRTC supports a small subset of what are valid AlgorithmIdentifiers.
     Nullable<WebRTCKeyParams> keyParams;
-    String name;
-    if (DictionaryHelper::get(keygenAlgorithm, "name", name)) {
-        if (name == "RSASSA-PKCS1-v1_5") {
-            // RSA - Supported |keygenAlgorithm|:
-            // { name: "RSASSA-PKCS1-v1_5", modulusLength: <int>, publicExponent: 65537 }
-            int modulusLength = -1;
-            int publicExponent = -1;
-            if (DictionaryHelper::get(keygenAlgorithm, "modulusLength", modulusLength)
-                && modulusLength >= 0
-                && DictionaryHelper::get(keygenAlgorithm, "publicExponent", publicExponent)
-                && publicExponent >= 0) {
-                keyParams.set(blink::WebRTCKeyParams::createRSA(modulusLength, publicExponent));
-            }
-        } else if (name == "ECDSA") {
-            // ECDSA - Supported |keygenAlgorithm|:
-            // { name: "ECDSA", namedCurve: "P-256" }
-            String namedCurve;
-            DictionaryHelper::get(keygenAlgorithm, "namedCurve", namedCurve);
-            if (namedCurve == "P-256") {
-                keyParams.set(blink::WebRTCKeyParams::createECDSA(WebRTCECCurveNistP256));
-            }
+    switch (cryptoAlgorithm.id()) {
+    case WebCryptoAlgorithmIdRsaSsaPkcs1v1_5:
+        // name: "RSASSA-PKCS1-v1_5"
+        uint32_t publicExponent;
+        // "publicExponent" must fit in a uint32_t.
+        // The only recognized "hash" is "SHA-256".
+        if (uint8ArrayToUint32(cryptoAlgorithm.rsaHashedKeyGenParams()->publicExponent(), &publicExponent)

[eroman, 2015/10/22 17:38:34]

nit: extract cryptoAlgorithm.rsaHashedKeyGenParams() to a temporary called
"params" or something (will make this more readable)

[hbos_chromium, 2015/10/23 09:59:05]

Acknowledged. With publicExponentToUint32 it's now more readable without a temp
var.

+            && cryptoAlgorithm.rsaHashedKeyGenParams()->hash().id() == WebCryptoAlgorithmIdSha256) {
+            unsigned modulusLength = cryptoAlgorithm.rsaHashedKeyGenParams()->modulusLengthBits();
+            keyParams.set(blink::WebRTCKeyParams::createRSA(modulusLength, publicExponent));
         }

[hta - Chromium, 2015/10/23 06:55:00]

More readable to reject the promise from the hidden "else" branch.

[hbos_chromium, 2015/10/23 09:59:05]

Done.

-    }
-    if (keyParams.isNull()) {
-        // Invalid argument.
-        return ScriptPromise::rejectWithDOMException(
-            scriptState, DOMException::create(InvalidAccessError, ExceptionMessages::argumentNullOrIncorrectType(1, "AlgorithmIdentifier")));
+        break;
+    case WebCryptoAlgorithmIdEcdsa:
+        // name: "ECDSA"
+        // The only recognized "namedCurve" is "P-256".
+        if (cryptoAlgorithm.ecKeyGenParams()->namedCurve() == WebCryptoNamedCurveP256) {
+            keyParams.set(blink::WebRTCKeyParams::createECDSA(blink::WebRTCECCurveNistP256));
+        }
+        break;
+    default:
+        break;

[hta - Chromium, 2015/10/23 06:55:00]

More readable to reject straight from here.

[hbos_chromium, 2015/10/23 09:59:05]

Done.

     }
 
     OwnPtr<WebRTCCertificateGenerator> certificateGenerator = adoptPtr(
         Platform::current()->createRTCCertificateGenerator());
 
-    // Check validity of |keyParams|.
-    if (!certificateGenerator->isValidKeyParams(keyParams.get())) {
+    // If |keyParams| is null the AlgorithmIdentifier is unrecognized by WebRTC, if isValidKeyParams it is recognized but the parameters unsupported or invalid.
+    if (keyParams.isNull() || !certificateGenerator->isValidKeyParams(keyParams.get())) {
         return ScriptPromise::rejectWithDOMException(
-            scriptState, DOMException::create(NotSupportedError, "The 1st argument provided is an AlgorithmIdentifier, but it has unsupported parameter values."));
+            scriptState, DOMException::create(NotSupportedError, "The 1st argument provided is an AlgorithmIdentifier, but the algorithm or parameters specified are not supported."));

[hta - Chromium, 2015/10/23 06:55:00]

You can do better (and more readable) by moving the handling for the
keyParams.isNull() case into the "default" statement of the case. That gives you
the opportunity to say "the algorithm is not supported" and "the parameters are
not supported" as two different error messages without any extra code.

There's a second case where it can be null - commented above.

[hbos_chromium, 2015/10/23 09:59:05]

Done.

     }
 
     ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
     ScriptPromise promise = resolver->promise();
 
     WebRTCCertificateObserver* certificateObserver = WebRTCCertificateObserver::create(resolver);
 
     // Generate certificate. The |certificateObserver| will resolve the promise asynchronously upon completion.
-    // The observer will manage its own destruction as well as the resolver's destruction.
+    // The observer will manage its own destruction as well as the resolver's destruction as well.

[hta - Chromium, 2015/10/23 06:55:00]

Grammar nit: Delete the last "as well".

[hbos_chromium, 2015/10/23 09:59:05]

Oops.

     certificateGenerator->generateCertificate(
         keyParams.get(),
         toDocument(scriptState->executionContext())->url(),
         toDocument(scriptState->executionContext())->firstPartyForCookies(),
         certificateObserver);
 
     return promise;
 }
 
 void RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, ExceptionState& exceptionState)
@@ skipping 432 matching lines @@
 {
     visitor->trace(m_localStreams);
     visitor->trace(m_remoteStreams);
     visitor->trace(m_dataChannels);
     visitor->trace(m_scheduledEvents);
     RefCountedGarbageCollectedEventTargetWithInlineData<RTCPeerConnection>::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink