[webkit-reviews] review denied: [Bug 200805] [GTK][WPE] Expose support for client certificate auth : [Attachment 419864] Rebased patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 200805: [GTK][WPE] Expose support for client certificate auth
https://bugs.webkit.org/show_bug.cgi?id=200805

Attachment 419864: Rebased patch

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




--- Comment #29 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 419864
  --> https://bugs.webkit.org/attachment.cgi?id=419864
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419864&action=review

I see several problems here and it seems like half of the patch belongs to
libsoup. But I'm not sure this really fits well in SouAuth API.

> Source/WebCore/platform/network/ProtectionSpaceBase.cpp:114
> +#if USE(GLIB)
> +    case ProtectionSpaceAuthenticationSchemeClientCertificatePINRequested:
> +#endif

So, pin requested is not actually a new protection space, no? it's
ClientCertificateRequested, but the client cert is password protected, right?
Is GTlsInteraction::ask_password always used for that particular case? or is it
supposed to be more generic?

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:48
> +WEBKIT_DEFINE_TYPE(WebKitSoupCertificateAuth, webkit_soup_certificate_auth,
SOUP_TYPE_AUTH)

The fact that this is a SoupAuth makes me think this should really be in
libsoup instead. But I'm not sure this really fits as a SoupAuth, that is
always thought to as for a user/password and it happens as part of HTTP (this
happens during the network connection).

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:57
> +	   SOUP_AUTH_IS_FOR_PROXY, FALSE,

Could it be the network connection to the proxy the one requiring a
certificate?

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:48
> +WEBKIT_DEFINE_TYPE(WebKitSoupCertificatePinAuth,
webkit_soup_certificate_pin_auth, SOUP_TYPE_AUTH)

This one fits better in the SoupAuth API because it's about asking for
user/password, but still happens differently. There's no auth associated to a
message, the auth cache not implemented, etc.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:58
> +    GRefPtr<SoupMessage> message =
static_cast<SoupMessage*>(g_object_get_data(G_OBJECT(self),
"wk-soup-message"));

We use a global interaction object but we assign a message to it. What happens
if two connections that require a client certificate are started in parallel?

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:59
> +    g_signal_emit_by_name(self->priv->session.get(), "authenticate",
message.get(), auth.get(), FALSE);

This is weird, this signal should always be emitted from libsoup I think. This
misses other SoupAuth features like retrying. What happens when the credentials
introduced are wrong? or when the dialog prompted is cancelled or closed? I
guess the load fails due to a connection error, and use has to manually reload
the page to try again.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:85
> +    GRefPtr<SoupMessage> message =
static_cast<SoupMessage*>(g_object_get_data(G_OBJECT(connection),
"wk-soup-message"));

Here the message is associated to the connection, so that's ok in case of
multiple connections.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:91
> +    g_object_set_data_full(G_OBJECT(interaction), "wk-soup-message",
g_object_ref(message.get()), g_object_unref);

But here we associate current message to the global interaction object. This
has the problem of multiple connections, but also that the message will be kept
alive until the session is destroyed, since the interaction object is owned by
the session.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:94
> +    GRefPtr<SoupAuth> auth =
adoptGRef(webkit_soup_certificate_auth_new(connection, message.get(),
task.get()));
> +    g_signal_emit_by_name(self->priv->session.get(), "authenticate",
message.get(), auth.get(), FALSE);

Since there isn't any cache, a new connection for the same host will ask for
the same certificate again, right?

> Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:175
> +#if GLIB_CHECK_VERSION(2, 68, 0)
> +    GError* innerError = nullptr;
> +    GRefPtr<GTlsCertificate> cert =
adoptGRef(g_tls_certificate_new_from_pkcs11_uris(certificateURI, privateKeyURI,
&innerError));
> +
> +    if (innerError) {
> +	   g_propagate_error(error, innerError);
> +	   return nullptr;
> +    }
> +
> +    return
webkitCredentialCreate(WebCore::Credential(CString(certificateURI),
CString(privateKeyURI)));
> +#else
> +    return nullptr;
> +#endif

Would it be possible to expose a single API that receives a GTlsCertiticate
instead?