Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1418113002:
RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto (Closed)
|
Created:
5 years, 2 months ago by hbos_chromium Modified:
5 years, 1 month ago CC:
blink-reviews, chromium-reviews, hta - Chromium, tommyw+watchlist_chromium.org, torbjorng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto
Latest WebRTC draft @ generateCertificate:
http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-generateCertificate-Promise-RTCCertificate--AlgorithmIdentifier-keygenAlgorithm
Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different:
https://github.com/w3c/webrtc-pc/blob/master/webrtc.html
(More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/webrtc.html)
This implementation is using WebCrypto and supports the AlgorithmIdentifier values required by the github version of the WebRTC spec:
{ name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" }
{ name: "ECDSA", namedCurve: "P-256" }
NOPRESUBMIT=true
BUG=544917
Committed: https://crrev.com/fcf6eb958079b14f71e4279fcd67a4c5e4099165
Cr-Commit-Position: refs/heads/master@{#357076}
Patch Set 1 : Hello, WebCrypto #
Total comments: 20
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Addressed comments #
Total comments: 3
Patch Set 4 : Moved implementation to .cpp #
Total comments: 6
Patch Set 5 : Addressed comments #
Total comments: 3
Patch Set 6 : Addressed nit #Patch Set 7 : Reupload #Patch Set 8 : Rebase with master #Messages
Total messages: 62 (22 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto Latest WebRTC draft @ generateCertificate: http://w3c.github.io/webrtc-pc/archives/20150611/webrtc.html#widl-RTCPeerConn... Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different: https://github.com/w3c/webrtc-pc/blob/master/webrtc.html (More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/we...) BUG=544917 ========== to ========== RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto Latest WebRTC draft @ generateCertificate: http://w3c.github.io/webrtc-pc/archives/20150611/webrtc.html#widl-RTCPeerConn... Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different: https://github.com/w3c/webrtc-pc/blob/master/webrtc.html (More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/we...) This implementation is using WebCrypto and supports the AlgorithmIdentifier values required by the github version of the WebRTC spec: { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" } { name: "ECDSA", namedCurve: "P-256" } BUG=544917 ==========
hbos@chromium.org changed reviewers: + eroman@chromium.org, tommi@chromium.org
Please take a look eroman and tommi.
Description was changed from ========== RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto Latest WebRTC draft @ generateCertificate: http://w3c.github.io/webrtc-pc/archives/20150611/webrtc.html#widl-RTCPeerConn... Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different: https://github.com/w3c/webrtc-pc/blob/master/webrtc.html (More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/we...) This implementation is using WebCrypto and supports the AlgorithmIdentifier values required by the github version of the WebRTC spec: { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" } { name: "ECDSA", namedCurve: "P-256" } BUG=544917 ========== to ========== RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto Latest WebRTC draft @ generateCertificate: http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-generateCertificate-Pr... Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different: https://github.com/w3c/webrtc-pc/blob/master/webrtc.html (More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/we...) This implementation is using WebCrypto and supports the AlgorithmIdentifier values required by the github version of the WebRTC spec: { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" } { name: "ECDSA", namedCurve: "P-256" } BUG=544917 ==========
https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:130: // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there 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 ? https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:514: if (uint8ArrayToUint32(cryptoAlgorithm.rsaHashedKeyGenParams()->publicExponent(), &publicExponent) nit: extract cryptoAlgorithm.rsaHashedKeyGenParams() to a temporary called "params" or something (will make this more readable)
hta@chromium.org changed reviewers: + hta@chromium.org
Drive-by review. Suggesting some changes to the structure of error handling only (use early return). + nits as always https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection.html (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection.html:72: function testCertificates1RSA() Nit: Blank lines above function declaration for readability. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:130: // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there On 2015/10/22 17:38:34, eroman wrote: > 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 ? 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? https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:518: } More readable to reject the promise from the hidden "else" branch. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528: break; More readable to reject straight from here. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:537: scriptState, DOMException::create(NotSupportedError, "The 1st argument provided is an AlgorithmIdentifier, but the algorithm or parameters specified are not supported.")); 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. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:546: // The observer will manage its own destruction as well as the resolver's destruction as well. Grammar nit: Delete the last "as well". https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:85: // Certificate Management Nit: Don't uppercase the second word in a name. It's not a title.
PTAL eroman, hta. Addressed comments and wondering about if ..ToUint32 should be placed where it is. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection.html (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection.html:72: function testCertificates1RSA() On 2015/10/23 06:55:00, hta - Chromium wrote: > Nit: Blank lines above function declaration for readability. Done. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:130: // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there On 2015/10/23 06:55:00, hta - Chromium wrote: > On 2015/10/22 17:38:34, eroman wrote: > > 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 ? > > 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? @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? https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:514: if (uint8ArrayToUint32(cryptoAlgorithm.rsaHashedKeyGenParams()->publicExponent(), &publicExponent) On 2015/10/22 17:38:34, eroman wrote: > nit: extract cryptoAlgorithm.rsaHashedKeyGenParams() to a temporary called > "params" or something (will make this more readable) Acknowledged. With publicExponentToUint32 it's now more readable without a temp var. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:518: } On 2015/10/23 06:55:00, hta - Chromium wrote: > More readable to reject the promise from the hidden "else" branch. Done. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528: break; On 2015/10/23 06:55:00, hta - Chromium wrote: > More readable to reject straight from here. Done. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:537: scriptState, DOMException::create(NotSupportedError, "The 1st argument provided is an AlgorithmIdentifier, but the algorithm or parameters specified are not supported.")); On 2015/10/23 06:55:00, hta - Chromium wrote: > 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. Done. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:546: // The observer will manage its own destruction as well as the resolver's destruction as well. On 2015/10/23 06:55:00, hta - Chromium wrote: > Grammar nit: Delete the last "as well". Oops. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:85: // Certificate Management On 2015/10/23 06:55:00, hta - Chromium wrote: > Nit: Don't uppercase the second word in a name. It's not a title. Done.
https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:130: // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there On 2015/10/23 09:59:05, hbos_chromium wrote: > On 2015/10/23 06:55:00, hta - Chromium wrote: > > On 2015/10/22 17:38:34, eroman wrote: > > > 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 ? > > > > 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? > > @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? @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. https://codereview.chromium.org/1418113002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h (right): https://codereview.chromium.org/1418113002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h:231: bool publicExponentToUint32(unsigned* result) const Blink style favors the use of references over out pointers: unsigned& result https://codereview.chromium.org/1418113002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h:233: if (m_publicExponent.size() == 0) While you are editing things, can you delete these two lines? (Rejecting empty BigIntegers was an older behavior of WebCrypto, and the spec now defines empty as meaning zero. In practice this was getting re-written to 0 at a higher layer so it wasn't actually reachable.)
https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:130: // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there On 2015/10/23 19:13:03, eroman wrote: > On 2015/10/23 09:59:05, hbos_chromium wrote: > > On 2015/10/23 06:55:00, hta - Chromium wrote: > > > On 2015/10/22 17:38:34, eroman wrote: > > > > 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 ? > > > > > > 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? > > > > @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? > > @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. 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.
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
PTAL eroman, tommi Addressed comments. Made the helper function not inline but global static function due to headerfiles. https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:130: // |exponentBytes| is of minimal typed array length (at most 7 leading zero bits for non-zero values) so there On 2015/10/23 19:15:52, eroman wrote: > On 2015/10/23 19:13:03, eroman wrote: > > On 2015/10/23 09:59:05, hbos_chromium wrote: > > > On 2015/10/23 06:55:00, hta - Chromium wrote: > > > > On 2015/10/22 17:38:34, eroman wrote: > > > > > 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 ? > > > > > > > > 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? > > > > > > @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? > > > > @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. > > 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. Done. https://codereview.chromium.org/1418113002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h (right): https://codereview.chromium.org/1418113002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h:231: bool publicExponentToUint32(unsigned* result) const On 2015/10/23 19:13:04, eroman wrote: > Blink style favors the use of references over out pointers: > unsigned& result Done. https://codereview.chromium.org/1418113002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h:233: if (m_publicExponent.size() == 0) On 2015/10/23 19:13:04, eroman wrote: > While you are editing things, can you delete these two lines? > > (Rejecting empty BigIntegers was an older behavior of WebCrypto, and the spec > now defines empty as meaning zero. In practice this was getting re-written to 0 > at a higher layer so it wasn't actually reachable.) Done.
https://codereview.chromium.org/1418113002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoUtil.h (right): https://codereview.chromium.org/1418113002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoUtil.h:14: static bool bigIntegerToUint(const WebVector<unsigned char>& bigInteger, unsigned& result) static method in a header file - won't this cause linker errors if more than one object files use it?
https://codereview.chromium.org/1418113002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoUtil.h (right): https://codereview.chromium.org/1418113002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoUtil.h:14: static bool bigIntegerToUint(const WebVector<unsigned char>& bigInteger, unsigned& result) On 2015/10/26 17:18:03, tommi wrote: > static method in a header file - won't this cause linker errors if more than one > object files use it? Regardless whether this compiles or not, it is undesirable as it defines an inline function. Instead put the function declaration here, mark it as BLINK_PLATFORM_EXPORT, and then provide the definition in a corresponding C++ file (which will be in the exported subdirectory). See other files for an example.
PTAL eroman, tommi. I moved it to .cpp. https://codereview.chromium.org/1418113002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoUtil.h (right): https://codereview.chromium.org/1418113002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoUtil.h:14: static bool bigIntegerToUint(const WebVector<unsigned char>& bigInteger, unsigned& result) On 2015/10/26 18:02:10, eroman wrote: > On 2015/10/26 17:18:03, tommi wrote: > > static method in a header file - won't this cause linker errors if more than > one > > object files use it? > > Regardless whether this compiles or not, it is undesirable as it defines an > inline function. > > Instead put the function declaration here, mark it as BLINK_PLATFORM_EXPORT, and > then provide the definition in a corresponding C++ file (which will be in the > exported subdirectory). > > See other files for an example. I thought I could not use .cc files because everything in the folder was header files and thought this was a good compromise, my bad, there are plenty of examples of .cc's in exported/. I should of course do the same! But FYI, this would not have caused linker errors and it would not have been inlined. @tommi It's a global function, not a method. If it was not static we would get linker errors, but static in the context of global functions (this bad boy keyword means very different things in different contexts) means it shall not be exposed to other compilation units. Thus it is safe, however, each compilation unit (.cc) that uses the function will get its own copy of the function code. @eroman While it is true that member functions implicitly get inlined when defined in the header (or rather, in the class definition, if you want to be picky) this is not true for normal functions. This would be like any helper function in a .cc file. Moving it to .cc file as suggested is the best approach. Then each compilation unit will not get a copy of the function and the next person who stumbles upon my code will not run into the same confusion as above.
https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:505: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError, unsupportedParamsString)); Is this the error that the WebRTC spec specifies? In the case of WebCrypto, if the publicExponent is too large (the only way in which a failure will be reached here), then the promise is rejected with an OperationError, whereas here it is rejected with a NotSupportedError. https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:527: if (!certificateGenerator->isValidKeyParams(keyParams.get())) { Based on the comment, sounds like perhaps isSupportedKeyParams() is a better name? https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h (right): https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h:232: bool publicExponentToUint(unsigned& result) const I suggest removing this function, and just using WebCryptoUtil.h directly. For instance: #include ...WebCryptoUtil.h ... blink::bigIntegerToUint(params->publicExponent(), result); At the callsites.
PTAL tommi, eroman https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:505: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(NotSupportedError, unsupportedParamsString)); On 2015/10/27 18:24:11, eroman wrote: > Is this the error that the WebRTC spec specifies? > > In the case of WebCrypto, if the publicExponent is too large (the only way in > which a failure will be reached here), then the promise is rejected with an > OperationError, whereas here it is rejected with a NotSupportedError. Yeah, it's correct. If WebCrypto returns an error we return that same error, in all other cases we return NotSupportedError. Too large for us but not WebCrypto is a case of "the user agent cannot or will not" use that parameter. (From spec: "If the algorithm normalization process produces an error, the call to generateCertificate MUST be rejected with that error. [...] The user agent MUST reject a call to generateCertificate() with a DOMError of type NotSupportedError if the keygenAlgorithm parameter identifies an algorithm that the user agent cannot or will not use to generate a certificate for RTCPeerConnection.") (A failure can also be reached here if other hash is used than SHA-256.) https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:527: if (!certificateGenerator->isValidKeyParams(keyParams.get())) { On 2015/10/27 18:24:10, eroman wrote: > Based on the comment, sounds like perhaps isSupportedKeyParams() is a better > name? Done. (The corresponding function in WebRTC is called IsValid() but "supported" makes more sense here.) https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h (right): https://codereview.chromium.org/1418113002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h:232: bool publicExponentToUint(unsigned& result) const On 2015/10/27 18:24:11, eroman wrote: > I suggest removing this function, and just using WebCryptoUtil.h directly. > > For instance: > > #include ...WebCryptoUtil.h > ... > blink::bigIntegerToUint(params->publicExponent(), result); > > > At the callsites. Done.
lgtm https://codereview.chromium.org/1418113002/diff/100001/components/webcrypto/a... File components/webcrypto/algorithms/rsa.cc (right): https://codereview.chromium.org/1418113002/diff/100001/components/webcrypto/a... components/webcrypto/algorithms/rsa.cc:282: if (!blink::bigIntegerToUint(params->publicExponent(), public_exponent)) { nit: remove the curly brackets (to match style elsewhere in this file with respect to single-line if statements)
PTAL tommi
lgtm https://codereview.chromium.org/1418113002/diff/100001/components/webcrypto/a... File components/webcrypto/algorithms/rsa.cc (right): https://codereview.chromium.org/1418113002/diff/100001/components/webcrypto/a... components/webcrypto/algorithms/rsa.cc:282: if (!blink::bigIntegerToUint(params->publicExponent(), public_exponent)) { On 2015/10/28 19:12:41, eroman wrote: > nit: remove the curly brackets (to match style elsewhere in this file with > respect to single-line if statements) ping
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1418113002/#ps120001 (title: "Addressed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Addressed nit after lgtm and presubmit fails. Do you need to lgtm again? https://codereview.chromium.org/1418113002/diff/100001/components/webcrypto/a... File components/webcrypto/algorithms/rsa.cc (right): https://codereview.chromium.org/1418113002/diff/100001/components/webcrypto/a... components/webcrypto/algorithms/rsa.cc:282: if (!blink::bigIntegerToUint(params->publicExponent(), public_exponent)) { On 2015/10/29 09:01:17, tommi wrote: > On 2015/10/28 19:12:41, eroman wrote: > > nit: remove the curly brackets (to match style elsewhere in this file with > > respect to single-line if statements) > > ping Done.
hbos@chromium.org changed reviewers: + jochen@chromium.org
Oh, I'm missing an LGTM for third_party/WebKit/Source/platform third_party/WebKit/public jochen, can you take a look (or stamp)?
lgtm
The CQ bit was checked by hbos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/10/29 14:43:42, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Tommi why does the presubmit fail due to DEPS? You're a mediastream owner and you lgtm'd. Might it be due to uploading a PS after lgtm? Can you help me commit?
On 2015/10/29 14:47:09, hbos_chromium wrote: > On 2015/10/29 14:43:42, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Tommi why does the presubmit fail due to DEPS? You're a mediastream owner and > you lgtm'd. Might it be due to uploading a PS after lgtm? > Can you help me commit? Or the owners of the added dependency, modules/crypto, but eroman's an owner of that...
On 2015/10/29 14:49:41, hbos_chromium wrote: > On 2015/10/29 14:47:09, hbos_chromium wrote: > > On 2015/10/29 14:43:42, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > Tommi why does the presubmit fail due to DEPS? You're a mediastream owner and > > you lgtm'd. Might it be due to uploading a PS after lgtm? > > Can you help me commit? > > Or the owners of the added dependency, modules/crypto, but eroman's an owner of > that... Looks like there's a bug in the presubmit script. Me, jochen (and eroman?) are owners, so that error doesn't make sense.
The CQ bit was checked by tommi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/10/29 15:47:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) There are at least two strange things going on. The first is that presubmit.py isn't realizing that the changes have been approved and the other thing is that the win_chromium_rel_ng bot is apparently trying to compile code that doesn't exist in the patch set :-/
The CQ bit was checked by tommi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/140001
Think I found at least one of the problems:
DEBUG subprocess2( 215): git apply --index -3 -p1 --verbose;
cwd=E:\b\build\slave\win\build\src
content/renderer/media/rtc_certificate_generator.cc
Checking patch content/renderer/media/rtc_certificate_generator.cc...
error: while searching for:
identity_observer->RequestIdentity(store.get(), key_params, observer);
}
bool RTCCertificateGenerator::isValidKeyParams(
const blink::WebRTCKeyParams& key_params) {
return WebRTCKeyParamsToKeyParams(key_params).IsValid();
}
error: patch failed: content/renderer/media/rtc_certificate_generator.cc:112
Falling back to three-way merge...
Applied patch to 'content/renderer/media/rtc_certificate_generator.cc'
cleanly.
Applied patch content/renderer/media/rtc_certificate_generator.cc cleanly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, eroman@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1418113002/#ps160001 (title: "Rebase with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/160001
Looks like presubmit is still failing and it looks wrong to me. Could there be an issue since the CL has changes in both Blink and Chromium? (third_party issue in the presubmit script?) In any case, it's OK with me to land with bypassing the presubmit step, but I'd like other approvers to agree/disagree first. jochen, eroman - wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/10/29 16:18:23, tommi wrote: > Looks like presubmit is still failing and it looks wrong to me. Could there be > an issue since the CL has changes in both Blink and Chromium? (third_party issue > in the presubmit script?) > > In any case, it's OK with me to land with bypassing the presubmit step, but I'd > like other approvers to agree/disagree first. jochen, eroman - wdyt? There's a bug filed for this already: https://code.google.com/p/chromium/issues/detail?id=545323 jochen said it is OK for him for us to land without presubmit checks.
Description was changed from ========== RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto Latest WebRTC draft @ generateCertificate: http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-generateCertificate-Pr... Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different: https://github.com/w3c/webrtc-pc/blob/master/webrtc.html (More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/we...) This implementation is using WebCrypto and supports the AlgorithmIdentifier values required by the github version of the WebRTC spec: { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" } { name: "ECDSA", namedCurve: "P-256" } BUG=544917 ========== to ========== RTCPeerConnection.generateCertificate taking AlgorithmIdentifier and using WebCrypto Latest WebRTC draft @ generateCertificate: http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-generateCertificate-Pr... Note the corrections that has since been made resolving WebRTC & WebCrypto spec discrepencies (https://github.com/w3c/webrtc-pc/issues/310, https://github.com/w3c/webrtc-pc/issues/322), the unstable github latest version looks slightly different: https://github.com/w3c/webrtc-pc/blob/master/webrtc.html (More readable version: http://htmlpreview.github.io/?https://github.com/w3c/webrtc-pc/blob/master/we...) This implementation is using WebCrypto and supports the AlgorithmIdentifier values required by the github version of the WebRTC spec: { name: "RSASSA-PKCS1-v1_5", modulusLength: 2048, publicExponent: new Uint8Array([1, 0, 1]), hash: "SHA-256" } { name: "ECDSA", namedCurve: "P-256" } NOPRESUBMIT=true BUG=544917 ==========
The CQ bit was checked by hbos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418113002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418113002/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fcf6eb958079b14f71e4279fcd67a4c5e4099165 Cr-Commit-Position: refs/heads/master@{#357076} |
