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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 16 13:43:44 PDT 2019


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

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




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

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

Please include ChangeLog entries whenever using r?, since we review those too.

I understand it's not going to be possible to do automatic tests for
smartcards. What level of testing is possible here?

> Source/WebCore/platform/network/soup/AuthenticationChallengeSoup.cpp:55
> +    if (WEBKIT_IS_SOUP_CERTIFICATE_AUTH(soupAuth))
> +	   scheme =
ProtectionSpaceAuthenticationSchemeClientCertificateRequested;
> +    else if (WEBKIT_IS_SOUP_CERTIFICATE_PIN_AUTH(soupAuth))

So WebKitSoupCertificatePinAuth is not a WebKitSoupCertificateAuth? I'd reverse
the conditions just in case that changes in the future.

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

2019

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

It will crash if error is nullptr, which could happen in release builds if
!certificatePEM && !certificateURI.

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

2019

> Source/WebCore/platform/network/soup/CredentialSoup.h:49
> +    WEBCORE_EXPORT explicit Credential(const CString certificatePEM);

Should be either const CString& or CString&&. Probably CString&&. Will comment
on this again later.

> Source/WebCore/platform/network/soup/CredentialSoup.h:51
> +    WEBCORE_EXPORT explicit Credential(const CString certificateURI, const
CString privateKeyURI);

It has multiple parameters, so you should remove explicit.

Also: const CString& or CString&&

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:146
> +    GRefPtr<GTlsInteraction> interaction =
static_cast<GTlsInteraction*>(g_object_new(webkit_tls_interaction_get_type(),
nullptr));

adoptGRef() or it leaks! After this line, the refcount is 2, one ref owned by
the GRefPtr and the other leaked.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:148
> +    g_object_set(m_soupSession.get(), SOUP_SESSION_TLS_INTERACTION,
interaction.get(), nullptr);

(Now the refcount is 3 since m_soupSession will own a ref.)

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:23
> +#include <wtf/glib/GRefPtr.h>

Also #include <wtf/glib/WTFGType.h>

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:26
> +struct _WebKitSoupCertificateAuth {
> +    SoupAuth parentInstance;

WTFGType uses old-style priv members, so:

struct _WebKitSoupCertificateAuthPrivate {

and drop the parentInstance member.

Yes, this means you'll need to use priv-> everywhere, so it's a little
less-convenient to write, but consistency is valuable.

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

WEBKIT_DEFINE_TYPE

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:43
> +    g_return_val_if_fail(connection, nullptr);
> +    g_return_val_if_fail(message, nullptr);
> +    g_return_val_if_fail(task, nullptr);

Use ASSERT(), since this isn't public API.

(If it was public API, then I would check GObjects for validity: e.g.
g_return_val_if_fail(G_IS_TLS_CONNECTION(connection), nullptr);

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:59
> +static void webkit_soup_certificate_auth_finalize(GObject *object)

GObject* object, but WEBKIT_DEFINE_TYPE will define a finalize for you that
calls ~_WebKitSoupCertificateAuth. So:

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:64
> +    if (!self->taskCompleted)
> +	   g_task_return_int(self->task.get(), G_TLS_INTERACTION_FAILED);

Give the priv struct a destructor, and put this there.

Are you sure this is right, though? This doesn't need g_task_return_error()?
I'm a bit confused because you can't return G_TLS_INTERACTION_FAILED without
setting error, but if you use g_task_return_error() then you can't return
G_TLS_INTERACTION_FAILED?

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:68
> +    self->task = nullptr;
> +    self->certificate = nullptr;
> +    self->connection = nullptr;

With a destructor, you don't need any of this unless you need to ensure they're
destroyed in a particular order (unlikely).

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:73
> +static void webkit_soup_certificate_auth_authenticate(SoupAuth* auth, const
char* username, const char* password)

Since this implements a public vfunc: g_return_val_if_fail(auth, FALSE);

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.cpp:86
> +    WebKitSoupCertificateAuth* self =
reinterpret_cast<WebKitSoupCertificateAuth*>(auth);

You're switching between WEBKIT_SOUP_CERTIFICATE_AUTH() and
reinterpret_cast<WebKitSoupCertificateAuth*>. Either way is fine so long as
you're consistent, but I'd prefer the GObject style for the extra safety since
none of this will be performance-critical. I see you prefer reinterpret_cast in
most of the rest of this patch, so I'll stop commenting on this.

Also, since this implements a public vfunc: g_return_val_if_fail(auth, FALSE);

> Source/WebCore/platform/network/soup/WebKitSoupCertificateAuth.h:37
> +GType webkit_soup_certificate_auth_get_type(void) G_GNUC_CONST;

Meh, I see we do it in a couple other places and I know it can be a measurable
performance improvement, but get_type() functions necessarily have side-effects
on the first call (type initialization), so this is undefined behavior.
Eventually some future GCC will optimize away type initialization and we'll
have explosions. I don't approve.

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

WEBKIT_DEFINE_TYPE() again.

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:46
> +    g_return_val_if_fail(task, nullptr);
> +    g_return_val_if_fail(password, nullptr);

ASSERT()

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:55
> +static void webkit_soup_certificate_pin_auth_finalize(GObject *object)

Goodbye.

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:60
> +	   g_task_return_int(self->priv->task.get(),
G_TLS_INTERACTION_UNHANDLED);

Same question regarding g_task_return_int() vs. g_task_return_error().

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

g_return_if_fail(auth);
g_return_if_fail(password);

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:73
> +    g_tls_password_set_value(self->priv->password.get(), (guchar*)password,
strlen(password));

static_cast<const guchar*>(password)

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.cpp:81
> +    WebKitSoupCertificatePinAuth* self =
reinterpret_cast<WebKitSoupCertificatePinAuth*>(auth);

g_return_if_fail(auth)

> Source/WebCore/platform/network/soup/WebKitSoupCertificatePinAuth.h:37
> +GType webkit_soup_certificate_pin_auth_get_type(void) G_GNUC_CONST;

grr

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:41
> +WEBKIT_DEFINE_TYPE(WebKitTlsInteraction, webkit_tls_interaction,
G_TYPE_TLS_INTERACTION);

What, now you've found it? :P

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:45
> +    WebKitTlsInteraction* self = (WebKitTlsInteraction*)interaction;

No C-style casts at all, unless you have some truly wild situation where no
combination of C++ casts can work. (I've only seen this once, with an EGL
typedef that could have been either an integer or a pointer.) Just use
WEBKIT_TLS_INTERACTION().

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:64
> +static void

Belongs on the next line

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:65
> +webkit_tls_interaction_request_certificate_async(GTlsInteraction*
interaction, GTlsConnection* connection,  GTlsCertificateRequestFlags flags,
GCancellable* cancellable, GAsyncReadyCallback callback, gpointer user_data)

Extra space before GTlsCertificateRequestFlags

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:85
> +static GTlsInteractionResult
> +webkit_tls_interaction_request_certificate_finish(GTlsInteraction*,
GAsyncResult* result, GError** error)

One line

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:91
> +static void
> +webkit_tls_interaction_class_init(WebKitTlsInteractionClass *klass)

One line.

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.cpp:98
> +    interactionClass->request_certificate_async =
webkit_tls_interaction_request_certificate_async;
> +    interactionClass->request_certificate_finish =
webkit_tls_interaction_request_certificate_finish;
> +    interactionClass->ask_password_async =
webkit_tls_interaction_ask_password_async;
> +    interactionClass->ask_password_finish =
webkit_tls_interaction_ask_password_finish;

Add your g_return checks for all four of these!

> Source/WebCore/platform/network/soup/WebKitTlsInteraction.h:39
> +GType webkit_tls_interaction_get_type(void) G_GNUC_CONST;

.

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:44
> +#include <WebCore/platform/network/soup/WebKitSoupCertificateAuth.h>

#include <WebCore/WebKitSoupCertificateAuth.h>

Make sure the right directories are added to the include path if it doesn't
work.

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

Since this is a programmer error, wouldn't g_critical() make more sense? Or an
ASSERT?

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:545
> +		       GRefPtr<GTlsCertificate> certificate =
credential.certificate();
> +		      
webkit_soup_certificate_auth_set_certificate(certificateAuth,
certificate.get());

Can't this be done on one line?

> Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:39
> +#include <WebCore/platform/network/soup/CredentialSoup.h>

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:140
> +    auto data = static_cast<const char*>(g_bytes_get_data(certificateData,
&size));

auto* (for readability)

> Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:141
> +    return webkitCredentialCreate(WebCore::Credential(CString(data, size)));

Now this is why CString&& is probably a better choice than const CString& for
the WebCore::Credential constructor. You wouldn't be able to use a temporary
here if the constructor used const CString&. Ditto for the other call down
below.

> Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:170
> +	       GError* errorPtr = innerError.release();
> +	       error = &errorPtr;

This looks like textbook use-after-free: you're returning a pointer to the
stack. Better use g_propagate_error()?

> Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:298
> +    const auto certificatePEM = credential->credential.certificatePEM;

auto*

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:43
> +    GCancellable* certificateLoaderCancellable;

Shouldn't it be cancelled in a destructor for the priv struct?

Also, use GRefPtr<GCancellable>.

The GtkWidgets don't get GRefPtrs because they're owned by the widget
hierarchy.

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:45
> +    GBytes* certificateData;

GRefPtr<GBytes>

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:54
> +    const bool isForCertificate =
webkit_authentication_request_get_scheme(priv->request.get()) ==
WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_REQUESTED;

const is silly here

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:125
> +    // Validate ahead of time if glib can actually read it

if -> whether

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:223
> +

No blank line here

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:247
> +
> +

One blank line suffices.

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationRequest.h:70
> + * @WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_PIN_REQUESTED: Client
certificate PIN required for use.

Since: 2.26

> Source/WebKit/UIProcess/API/gtk/WebKitCredential.h:62
> +WEBKIT_API WebKitCredential *
> +webkit_credential_new_for_pin	  (const gchar			*pin,
> +					   WebKitCredentialPersistence	
persistence);

The parameters are misaligned by one space relative to the other functions'
parameters.

> Source/WebKit/UIProcess/API/wpe/WebKitAuthenticationRequest.h:69
> + * @WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_PIN_REQUESTED: Client
certificate PIN required for use.

Ditto.

> Source/WebKit/UIProcess/API/wpe/WebKitCredential.h:62
> +WEBKIT_API WebKitCredential *
> +webkit_credential_new_for_pin	  (const gchar			*pin,
> +					   WebKitCredentialPersistence	
persistence);

Ditto.

> Tools/Scripts/webkitpy/style/checker.py:219
> +	 os.path.join('Source', 'WebCore', 'platform', 'network', 'soup',
'WebKitTlsInteraction.cpp'),

Hm, we don't normally allow .cpp files to be exempt from style checker without
strong reason.

The headers should definitely skip style checker.


More information about the webkit-reviews mailing list