Bug 190789 – [WPE][GTK] Pass full certificate chain in CertificateInfo coder

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Claudio Saavedra]

[WPE][GTK] Pass certificate issuer between processes if present in CertificateInfo

[Claudio Saavedra]

Created attachment 352882 [details]
Patch

[Claudio Saavedra]

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.

[Claudio Saavedra]

Additionally one could add a signed-certificate/issuer-certificate combo to the tests resources and a test that checks this.

[Michael Catanzaro]

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.

[Claudio Saavedra]

Created attachment 352962 [details]
Patch

[Claudio Saavedra]

Created attachment 352964 [details]
Patch

[Michael Catanzaro]

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.

[WebKit Commit Bot]

Comment on attachment 352964 [details]
Patch

Clearing flags on attachment: 352964

Committed r237351: <https://trac.webkit.org/changeset/237351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.