RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto

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

Index: third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
diff --git a/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp b/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
index 5dcf61e6ea105d298fd09c0997c1561b7d54c7fb..2504c4e03423f1b97b58b8fc7fcc10910442c209 100644
--- a/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
+++ b/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
@@ -44,6 +44,7 @@
 #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"
@@ -60,6 +61,7 @@
 #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"
@@ -122,6 +124,20 @@ private:
     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)
@@ -473,48 +489,52 @@ void RTCPeerConnection::updateIce(const Dictionary& rtcConfiguration, const Dict
         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);
@@ -523,7 +543,7 @@ ScriptPromise RTCPeerConnection::generateCertificate(ScriptState* scriptState, c
     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(),