Bug 172856

Created attachment 311822 [details] WIP

Attachment 311822 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:203: CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:231: CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 311822 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review Now that Jiewen is a reviewer, I'll let him give the final r+. Just a couple nits: > Source/WTF/wtf/Platform.h:1274 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || PLATFORM(GTK) || PLATFORM(WPE) Shouldn't this be || USE(GCRYPT)? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:31 > +#if ENABLE(SUBTLE_CRYPTO) && HAVE(RSA_PSS) This is confusing, since it implies that HAVE(RSA_PSS) can be false when compiling this file. I would remove it. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:198 > + // a failure in any other case. OperationError should not be returned at this point. Same as in the last patch, you should explain why.

Comment on attachment 311822 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review Looks good to me, and please address comments in gcryptVerify. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:43 > +static std::optional<PAL::CryptoDigest::Algorithm> hashCryptoDigestAlgorithm(CryptoAlgorithmIdentifier identifier) Remember to move this into a utility file. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:61 > +static std::optional<const char*> hashAlgorithmName(CryptoAlgorithmIdentifier identifier) Ditto. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:79 > +static std::optional<Vector<uint8_t>> mpiData(gcry_sexp_t paramSexp) Ditto. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:198 >> + // a failure in any other case. OperationError should not be returned at this point. > > Same as in the last patch, you should explain why. I start wondering maybe we shouldn't return any std::nullopt before as that will cause OperationError.

Comment on attachment 311822 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review >>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:198 >>> + // a failure in any other case. OperationError should not be returned at this point. >> >> Same as in the last patch, you should explain why. > > I start wondering maybe we shouldn't return any std::nullopt before as that will cause OperationError. Maybe we shouldn't even pass exceptionCallback to platformVerify as there won't be any exceptions till this level. I will address that in Bug 171983.

Comment on attachment 311822 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=311822&action=review >> Source/WTF/wtf/Platform.h:1274 >> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000) || PLATFORM(GTK) || PLATFORM(WPE) > > Shouldn't this be || USE(GCRYPT)? Could be, doesn't exist yet though. Should be simple to expose through CMake. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:31 >> +#if ENABLE(SUBTLE_CRYPTO) && HAVE(RSA_PSS) > > This is confusing, since it implies that HAVE(RSA_PSS) can be false when compiling this file. I would remove it. The cross-platform code still uses HAVE(RSA_PSS). I'll add a preprocessor error that's triggered if HAVE(RSA_PSS) is false here, which it shouldn't be because USE(GCRYPT) is at the moment definitely true if this file is being compiled.

Created attachment 311954 [details] Patch

Attachment 311954 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:145: CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:173: CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 311954 [details] Patch wpe port seems not happy.

Created attachment 312058 [details] Patch

Attachment 312058 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:145: CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:173: CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 312058 [details] Patch r=me. Thanks.

Created attachment 312172 [details] Patch for landing

Attachment 312172 [details] did not pass style-queue: ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:145: CryptoAlgorithmRSA_PSS::platformSign is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/crypto/gcrypt/CryptoAlgorithmRSA_PSSGCrypt.cpp:173: CryptoAlgorithmRSA_PSS::platformVerify is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 312172 [details] Patch for landing Clearing flags on attachment: 312172 Committed r217877: <http://trac.webkit.org/changeset/217877>

All reviewed patches have been landed. Closing bug.