[webkit-reviews] review denied: [Bug 205376] [WebAuthn] Implement coders for CTAP ClientPIN requests and responses : [Attachment 385972] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 205376: [WebAuthn] Implement coders for CTAP ClientPIN requests and
responses
https://bugs.webkit.org/show_bug.cgi?id=205376

Attachment 385972: Patch

https://bugs.webkit.org/attachment.cgi?id=385972&action=review




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 385972
  --> https://bugs.webkit.org/attachment.cgi?id=385972
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385972&action=review

Looks like your API tests are failing on some systems.

> Source/WebCore/ChangeLog:24
> +	   of API tests.

I suggest rephrasing this as: "The authenticatorClientPIN subCommands are based
on a Chromium patch:
https://chromium-review.googlesource.com/c/chromium/src/+/1457004 Specifically,
it adopts the interfaces from that patch, but rewrites the BoringSSL-based
crypto features using WebCore's WebCrypto implementation. This allows us to
focus on high level crypto interfaces, and lets WebCrypto handle the underlying
crypto library. Also, the original Chromium patch lacks tests. We introduce a
large set of API tests to confirm proper function."

> Source/WebCore/ChangeLog:29
> +	   made headers private for TestWebKitAPI.

Maybe: "This patch also makes the AES CBC, EDCH, and HMAC platform*
implementations public, so that these implementations can be shared by
WebAuthentication and test infrastructure."

> Source/WebCore/Modules/webauthn/fido/Pin.cpp:111
> +// static

I don't think these comments are helpful. I suggest removing them.

> Source/WebCore/Modules/webauthn/fido/Pin.cpp:170
> +	   {-1 /* curve */, 1 /* P-256 */},

I think this Vector<std::pair<int, int>> would be better as a constexpr called
'P256Point' or something, and this loop as a helper function: "static bool
coseKeyIsP256Point(const CBORValue::MapValue& coseKey)"

> Source/WebCore/Modules/webauthn/fido/Pin.h:63
> +enum class RequestKey : int {

Does this need to be an 'int' by spec? All values are positive, and would fit
in a byte (could be uint8_t).

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:204
> +	   return WTFMove(result);

Is this WTFMove() necessary? I would expect the return value optimization to
handle this.

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:211
> +    return WTFMove(result);

Is this WTFMove() necessary? I would expect the return value optimization to
handle this.