Bug 197800 - Add a unit test for client certificate authentication
Summary: Add a unit test for client certificate authentication
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on: 197887 197989
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-10 15:04 PDT by Alex Christensen
Modified: 2019-07-11 16:40 PDT (History)
15 users (show)

See Also:


Attachments
Patch (42.07 KB, patch)
2019-05-10 16:02 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (41.75 KB, patch)
2019-05-10 16:06 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (41.68 KB, patch)
2019-05-10 18:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (42.45 KB, patch)
2019-05-14 08:16 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
patch (42.52 KB, patch)
2019-05-16 14:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-05-10 15:04:05 PDT
Add a unit test for client certificate authentication
Comment 1 Alex Christensen 2019-05-10 16:02:46 PDT
Created attachment 369609 [details]
Patch
Comment 2 Build Bot 2019-05-10 16:04:03 PDT
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.
Comment 3 Alex Christensen 2019-05-10 16:06:28 PDT
Created attachment 369610 [details]
Patch
Comment 4 Alex Christensen 2019-05-10 18:23:05 PDT
Created attachment 369630 [details]
Patch
Comment 5 youenn fablet 2019-05-13 16:55:59 PDT
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
Comment 6 Alex Christensen 2019-05-14 08:16:13 PDT
Created attachment 369849 [details]
Patch
Comment 7 WebKit Commit Bot 2019-05-14 08:49:41 PDT
Comment on attachment 369849 [details]
Patch

Clearing flags on attachment: 369849

Committed r245281: <https://trac.webkit.org/changeset/245281>
Comment 8 WebKit Commit Bot 2019-05-14 08:49:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-05-14 08:50:19 PDT
<rdar://problem/50768065>
Comment 10 Aakash Jain 2019-05-14 10:09:37 PDT
This broke the API Test TestWebKitAPI.Challenge.ClientCertificate, as reported by EWS for API test.
Comment 11 WebKit Commit Bot 2019-05-14 11:06:34 PDT
Re-opened since this is blocked by bug 197887
Comment 12 Alex Christensen 2019-05-16 14:29:06 PDT
Created attachment 370070 [details]
patch
Comment 13 WebKit Commit Bot 2019-05-16 16:08:53 PDT
Comment on attachment 370070 [details]
patch

Clearing flags on attachment: 370070

Committed r245418: <https://trac.webkit.org/changeset/245418>
Comment 14 WebKit Commit Bot 2019-05-16 16:08:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Truitt Savell 2019-05-17 08:19:50 PDT
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
Comment 16 WebKit Commit Bot 2019-05-17 10:19:34 PDT
Re-opened since this is blocked by bug 197989
Comment 17 Shawn Roberts 2019-05-17 11:25:14 PDT
Rolled out in https://trac.webkit.org/changeset/245468/webkit 

Causing API crashes on testers
Comment 18 Alex Christensen 2019-06-19 12:29:14 PDT
Relanded with the test disabled in http://trac.webkit.org/r246605
Comment 19 Alexey Proskuryakov 2019-06-20 00:35:15 PDT
> 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 20 Alex Christensen 2019-06-20 11:54:13 PDT
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.
Comment 21 Alex Christensen 2019-06-20 15:00:14 PDT
http://trac.webkit.org/r246654
Comment 22 Alex Christensen 2019-07-11 16:40:04 PDT
Enabling in https://bugs.webkit.org/show_bug.cgi?id=199735