Implement coder for CTAP ClientPIN requests and responses.
<rdar://problem/58034395>
Created attachment 385945 [details] Patch
Created attachment 385972 [details] Patch
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
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.
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.
Created attachment 386172 [details] Patch
Comment on attachment 386172 [details] Patch Thank you for incorporating my suggestions. R=me
(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.
Comment on attachment 386172 [details] Patch Clearing flags on attachment: 386172 Committed r253811: <https://trac.webkit.org/changeset/253811>
All reviewed patches have been landed. Closing bug.