[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