[webkit-reviews] review canceled: [Bug 200805] [GTK][WPE] Expose support for client certificate auth : [Attachment 378296] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has canceled 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 378296: Patch

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




--- Comment #10 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 378296
  --> https://bugs.webkit.org/attachment.cgi?id=378296
Patch

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

Overall it looks excellent -- all my comments are nits -- except it really
needs an API test. You can modify
Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp to require client
auth, and add a new test in TestSSL.cpp or a new test file. It won't be quick,
but it's important work to ensure this doesn't break in the future.

> Source/WebCore/platform/network/ProtectionSpaceBase.h:58
> +#if USE(GLIB)
> +    ProtectionSpaceAuthenticationSchemeClientCertificatePINRequested = 10,
> +#endif

This isn't public, so you can safely put it after
ProtectionSpaceAuthenticationSchemeClientCertificateRequested.

The #if is OK, but I'd favor avoiding it here since it's not really needed.
Having an extra enum value shouldn't cause problems for ports that don't
support it. You'll just want to avoid introducing -Wswitch or -Wswitch-enum
warnings.

> Source/WebCore/platform/network/soup/CredentialSoup.cpp:2
> + * Copyright (C) 2019 Igalia S.L. All rights reserved.

"All rights reserved" is nonsense above a license that disclaims all rights.
Don't cargo-cult it into new places.

> Source/WebCore/platform/network/soup/CredentialSoup.cpp:39
> +    , certificatePEM(certificatePEM)

Missing WTFMove()

> Source/WebCore/platform/network/soup/CredentialSoup.cpp:45
> +    , certificateURI(certificateURI)

Missing WTFMove()

> Source/WebCore/platform/network/soup/CredentialSoup.cpp:46
> +    , privateKeyURI(privateKeyURI)

Missing WTFMove()

> Source/WebCore/platform/network/soup/CredentialSoup.cpp:90
> +    else if (certificateURI) // TODO: Hide behind version check once landed
in glib

Land this in glib before setting r? since it's not ready for final review if
the build is failing.

> Source/WebCore/platform/network/soup/CredentialSoup.cpp:94
> +    if (!certificate && error.get())
> +	   g_warning("Failed to create certificate: %s", error->message);

Pedantic, but: I think you should crash dereferencing error->message if
!certificate is true and error is unset, rather than fail to write the error
message. It would be a GIO bug to fail to set error if returning null.

> Source/WebCore/platform/network/soup/CredentialSoup.h:53
> +    WEBCORE_EXPORT explicit Credential(CString&& certificatePEM);
> +
> +    WEBCORE_EXPORT explicit Credential(CString&& certificateURI, CString&&
privateKeyURI);
> +
> +    WEBCORE_EXPORT bool isEmpty() const;

WEBCORE_EXPORT shouldn't be needed because this is soup-specific and there is
no shared library boundary between WebCore and higher layers for soup ports.

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:34
> +	   if (!this->taskCompleted)

if (!taskCompleted)

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:35
> +	       g_task_return_new_error(this->task.get(), G_IO_ERROR,
G_IO_ERROR_CANCELLED, "Cancelled");

task.get()

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:54
> +    ASSERT(connection);
> +    ASSERT(message);
> +    ASSERT(task);

Might as well assert that they're valid (G_IS_TLS_CONNECTION(connection), etc.)
if you're going to assert that they're nonnull? These will be compiled out of
release builds anyway.

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

You did this properly. Just wanted to point out how important it is to be
careful when passing boolean properties to g_object_new(). If you accidentally
write false (one byte) instead of FALSE (four bytes), you'd have a disaster.
I've been waiting for you to make this mistake in a patch, but you haven't, so
I guess I'd better stop waiting and just warn you about it. :P

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:75
> +    g_return_if_fail(auth);

Hm, I know I told you to do this in the previous review, but I wonder: is this
really exposed to API users?

 * We're pretty deep in WebCore to be using g_return_if_fail(). If this class
is not somehow exposed to API users such that applications using WebKit can
cause this check to fail, it should be plain ASSERT().
 * Either way, there's no guard in the next vfunc down, is_authenticated().
Please be consistent!

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:34
> +	   if (!this->taskCompleted)
> +	       g_task_return_new_error(this->task.get(), G_IO_ERROR,
G_IO_ERROR_CANCELLED, "Cancelled");

Really there's no point in writing "this" except when really needed to
disambiguate.

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:53
> +    ASSERT(task);
> +    ASSERT(password);

ASSERT(G_IS_TASK);
ASSERT(G_IS_TLS_PASSWORD);

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:80
> +    return g_tls_password_get_value(self->priv->password.get(), nullptr) !=
nullptr;

Explicit comparisons to nullptr aren't allowed in WebKit. When the return value
is bool, it's just redundant. But since the return value is gboolean rather
than bool, you should use !! instead.

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

You don't need ownership here. No need to use GRefPtr.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:67
> +    const auto ret =
static_cast<GTlsInteractionResult>(g_task_propagate_int(G_TASK(result),
&innerError));

const seems weird for a local variable. There's no need to prevent three lines
of code from mutating the ret if it wanted to do that for some reason.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:83
> +    UNUSED_PARAM(flags); // Soup doesn't use this value

Comments should be complete sentences. But I think it's OK to omit the comment,
since there are currently no flags anyway.

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

Again, you don't need ownership here, no need for GRefPtr. Right?

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:541
> +		       g_critical("Certificate set for non-certificate auth");

Should it be an ASSERT?

> Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:148
> + * @error: (nullable): A #GError in case PKCS \#11 is unsupported

Looking over https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations,
I don't think error parameters are supposed to be annotated (nullable). It's
probably implicit.

> Source/WebKit/UIProcess/API/gtk/WebKitCredential.h:65
> +WEBKIT_API WebKitCredential *
> +webkit_credential_new_for_certificate_data (GBytes		      
*certificate_data);

If you don't want to reindent the header, then start the parameter list on the
next line, ( under the a in data, to keep it aligned.