[webkit-reviews] review denied: [Bug 190789] [WPE][GTK] Pass certificate issuer between processes if present in CertificateInfo : [Attachment 352882] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 190789: [WPE][GTK] Pass certificate issuer between processes if present in
CertificateInfo
https://bugs.webkit.org/show_bug.cgi?id=190789

Attachment 352882: Patch

https://bugs.webkit.org/attachment.cgi?id=352882&action=review




--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 352882
  --> https://bugs.webkit.org/attachment.cgi?id=352882
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.