Bug 197800 – Add a unit test for client certificate authentication

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alex Christensen]

Add a unit test for client certificate authentication

[Alex Christensen]

Created attachment 369609 [details]
Patch

[EWS Watchlist]

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.

[Alex Christensen]

Created attachment 369610 [details]
Patch

[Alex Christensen]

Created attachment 369630 [details]
Patch

[youenn fablet]

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

[Alex Christensen]

Created attachment 369849 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369849 [details]
Patch

Clearing flags on attachment: 369849

Committed r245281: <https://trac.webkit.org/changeset/245281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50768065>

[Aakash Jain]

This broke the API Test TestWebKitAPI.Challenge.ClientCertificate, as reported by EWS for API test.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 197887

[Alex Christensen]

Created attachment 370070 [details]
patch

[WebKit Commit Bot]

Comment on attachment 370070 [details]
patch

Clearing flags on attachment: 370070

Committed r245418: <https://trac.webkit.org/changeset/245418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 197989

[Shawn Roberts]

Rolled out in https://trac.webkit.org/changeset/245468/webkit 

Causing API crashes on testers

[Alex Christensen]

Relanded with the test disabled in http://trac.webkit.org/r246605

[Alexey Proskuryakov]

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

[Alex Christensen]

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.

[Alex Christensen]

http://trac.webkit.org/r246654

[Alex Christensen]

Enabling in https://bugs.webkit.org/show_bug.cgi?id=199735