Summary: | [WPE][GTK] Pass full certificate chain in CertificateInfo coder | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||||||
Component: | WebKitGTK | Assignee: | Claudio Saavedra <csaavedra> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, commit-queue, mcatanzaro | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Claudio Saavedra
2018-10-22 02:33:20 PDT
Created attachment 352882 [details]
Patch
This is a bit of a RFC/WIP, but it would be really useful for apps to have at least the issuer certificate in the GTlsCertificate that the WebKitWebView API provides. The idea of the patch is to not lose that information when passing it between process. It could be done more elegantly (or even continue with the whole chain but I think this is good enough for now. Additionally one could add a signed-certificate/issuer-certificate combo to the tests resources and a test that checks this. Comment on attachment 352882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352882&action=review This seems like a great idea! But I disagree that it's sufficient to pass just the immediate issuer: I'd consider it broken to pass only a single issuer and not the entire chain. The problem is that this will enable manual certificate verification and more complex certificate information dialogs... or it would seem to, and they'll work for most websites, but they will be broken for websites that send more than one intermediate certificate. So can you add a loop here? You'd just need to use a uint32_t instead of a bool to encode the total number of certificates in the chain before decoding the certificates themselves, so it should be an easy enhancement. > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:129 > + GRefPtr<GTlsCertificate> issuer = adoptGRef(G_TLS_CERTIFICATE(g_initable_new( > + g_tls_backend_get_certificate_type(backend), 0, 0, "certificate", issuerBytes.get(), nullptr))); > + > + certificate = adoptGRef(G_TLS_CERTIFICATE(g_initable_new( > + g_tls_backend_get_certificate_type(backend), 0, 0, "certificate", certificateBytes.get(), "issuer", issuer.get(), nullptr))); It will also be less-confusing if you do this in the opposite order: encode the server certificate first, then the issuer, then the issuer's issuer, etc. until the chain ends. > Tools/ChangeLog:17 > + (testSSL): Test that the self-signed certificatt has no bogus certificate > Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:77 > + // Self-signed certificate has a nullptr issuer. > + g_assert(!g_tls_certificate_get_issuer(test->m_certificate.get())); It doesn't really test the change, but it's good enough IMO. Setting up a more complex certificate, as would be needed to test this, is already tested elsewhere in our stack below the WebKit layer, and the likelihood of this particular WebKit code breaking in the future is relatively low, so I think it's OK. Created attachment 352962 [details]
Patch
Created attachment 352964 [details]
Patch
Comment on attachment 352964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352964&action=review Wow, it's a good do-while loop. Don't see those very often. > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:85 > + // Encode starting from the root certificate. > + for (uint32_t i = chainLength; i > 0; i--) { > + GRefPtr<GByteArray> certificate = adoptGRef(certificatesDataList[i - 1]); > + encoder.encodeVariableLengthByteArray(IPC::DataReference(certificate->data, certificate->len)); > } Eh, maybe it *was* nicer the other way around... whatever, either order is totally fine, as you prefer. Comment on attachment 352964 [details] Patch Clearing flags on attachment: 352964 Committed r237351: <https://trac.webkit.org/changeset/237351> All reviewed patches have been landed. Closing bug. |