Summary: | Add a unit test for client certificate authentication | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||
Component: | Tools / Tests | Assignee: | Alex Christensen <achristensen> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aakash_jain, ap, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, sroberts, tsavell, webkit-bot-watchers-bugzilla, webkit-bug-importer, wilander, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 197887, 197989 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Alex Christensen
2019-05-10 15:04:05 PDT
Created attachment 369609 [details]
Patch
Attachment 369609 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/TCPServer.cpp:44: Declaration has space between type name and * in X509 *cert [whitespace/declaration] [3]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 369610 [details]
Patch
Created attachment 369630 [details]
Patch
Comment on attachment 369630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369630&action=review > Tools/TestWebKitAPI/TCPServer.cpp:282 > + EXPECT_TRUE(static_cast<size_t>(bytesRead) < sizeof(buffer)); These tests seem a bit strange since they are not test per-se but test infra. It looks like it should be ASSERT(...) > Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:43 > + RetainPtr<SecCertificateRef> certificate = adoptCF(SecCertificateCreateWithData(nullptr, (__bridge CFDataRef)[NSData dataWithBytes:certificateBytes.data() length:certificateBytes.size()])); auto > Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:45 > + Vector<uint8_t> privateKeyBytes = TestWebKitAPI::TCPServer::testPrivateKey(); auto here and below. > Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:58 > + RetainPtr<SecIdentityRef> identity = adoptCF(SecIdentityCreate(kCFAllocatorDefault, certificate.get(), privateKey.get())); auto > Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:157 > + EXPECT_EQ([delegate challengeCount], 2ull); We could check that we first receive a server trust challenge and then a client certificate challenge. > Tools/TestWebKitAPI/Tests/WebKitCocoa/PDFLinkReferrer.mm:79 > + Vector<uint8_t> request = TCPServer::read(socket); auto > Tools/TestWebKitAPI/Tests/WebKitCocoa/PDFLinkReferrer.mm:82 > + const char* currentLine = reinterpret_cast<char*>(request.data()); auto Created attachment 369849 [details]
Patch
Comment on attachment 369849 [details] Patch Clearing flags on attachment: 369849 Committed r245281: <https://trac.webkit.org/changeset/245281> All reviewed patches have been landed. Closing bug. This broke the API Test TestWebKitAPI.Challenge.ClientCertificate, as reported by EWS for API test. Re-opened since this is blocked by bug 197887 Created attachment 370070 [details]
patch
Comment on attachment 370070 [details] patch Clearing flags on attachment: 370070 Committed r245418: <https://trac.webkit.org/changeset/245418> All reviewed patches have been landed. Closing bug. It looks like the changes in https://trac.webkit.org/changeset/245418/webkit has caused API test TestWebKitAPI.Challenge.ClientCertificate to crash on Mojave. Build: https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4171 Re-opened since this is blocked by bug 197989 Rolled out in https://trac.webkit.org/changeset/245468/webkit Causing API crashes on testers Relanded with the test disabled in http://trac.webkit.org/r246605 > Relanded with the test disabled in http://trac.webkit.org/r246605
Can you elaborate? Generally, this is grounds for immediate rollback, and that is even more so in a bug whose title is "Add a unit test for client certificate authentication".
Comment on attachment 370070 [details]
patch
I needed to use the infrastructure that was developed for this test for another test, so I landed the code. Since the test itself crashed for an unknown reason on the bots the other times I tried to commit this, I disabled it while I look into the reason. If you would like I could remove the Challenge.ClientCertificate API test, but I need the other cleanups for other things.
The needed code changes were moving utility functions to TCPServer.cpp, and moving declarations to SecuritySPI.h is also a good cleanup.
Enabling in https://bugs.webkit.org/show_bug.cgi?id=199735 |