Bug 205376 – [WebAuthn] Implement coders for CTAP ClientPIN requests and responses

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Implement coder for CTAP ClientPIN requests and responses.

[Radar WebKit Bug Importer]

<rdar://problem/58034395>

[Jiewen Tan]

Created attachment 385945 [details]
Patch

[Jiewen Tan]

Created attachment 385972 [details]
Patch

[Brent Fulgham]

Comment on attachment 385945 [details]
Patch

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

> 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:139
> +// static

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

> Source/WebCore/Modules/webauthn/fido/Pin.cpp:171
> +    })) {

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/Modules/webauthn/fido/Pin.h:74
> +enum class ResponseKey : int {

Ditto.

> 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);

Ditto.

> Tools/TestWebKitAPI/Tests/WebCore/CtapPinTest.cpp:35
> +#include <WebCOre/CryptoAlgorithmAesCbcCfbParams.h>

Oops! Not good for case sensitive file systems! :-)

fatal error: 'WebCOre/CryptoAlgorithmAesCbcCfbParams.h' file not found

[Brent Fulgham]

Comment on attachment 385972 [details]
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.

[Jiewen Tan]

Comment on attachment 385972 [details]
Patch

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

Thanks Brent for reviewing this patch.

>> 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."

Fixed.

>> 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."

Fixed.

>> Source/WebCore/Modules/webauthn/fido/Pin.cpp:111
>> +// static
> 
> I don't think these comments are helpful. I suggest removing them.

Removed.

>> 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)"

This part was from the original Chromium patch. If I were to write this part, I won't write it like this as well. Maybe we should keep it as it is but replace the integer with the constants that I defined in WebAuthenticationConstants.h.

>> 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).

Good catch. I just keep this without second thought.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:204
>> +        return WTFMove(result);
> 
> Is this WTFMove() necessary? I would expect the return value optimization to handle this.

True, I did something earlier, and forgot to undo this part.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:211
>> +    return WTFMove(result);
> 
> Is this WTFMove() necessary? I would expect the return value optimization to handle this.

Ditto.

[Jiewen Tan]

Created attachment 386172 [details]
Patch

[Brent Fulgham]

Comment on attachment 386172 [details]
Patch

Thank you for incorporating my suggestions. R=me

[Jiewen Tan]

(In reply to Brent Fulgham from comment #8)
> Comment on attachment 386172 [details]
> Patch
> 
> Thank you for incorporating my suggestions. R=me

Thanks for r+ the patch.

[WebKit Commit Bot]

Comment on attachment 386172 [details]
Patch

Clearing flags on attachment: 386172

Committed r253811: <https://trac.webkit.org/changeset/253811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.