[GTK][WPE] Expose support for client certificate auth
Created attachment 376468 [details] Patch
This adds API to handle client certificate authentication, both using certificate files and PKCS #11 URIs. This isn't ready for commit as it is waiting on upstream work in GLib and GLib-Networking and there are some remaining issues to be tracked down. It should however be ready for basic review.
Comment on attachment 376468 [details] 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.
Created attachment 378266 [details] Patch
(In reply to Michael Catanzaro from comment #3) > Comment on attachment 376468 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376468&action=review > > > 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. Other files in the same directory work so I'm not sure what I'm missing, any ideas? > > 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. Other GObject cpp files do the same, I believe for vfunc names having underscores.
Comment on attachment 378266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378266&action=review I'm sure Carlos Garcia will want to do the final review for this, but I'll take another look again soon. > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:44 > +#include <WebCore/platform/network/soup/WebKitSoupCertificateAuth.h> Add it to WebCore_PRIVATE_FRAMEWORK_HEADERS in Source/WebCore/platform/Soup.cmake. > Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:39 > +#include <WebCore/platform/network/soup/CredentialSoup.h> Ditto.
It's missing #if GLIB_CHECK_VERSION here: In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-3c72abbe-46.cpp:2:0: ../../Source/WebCore/platform/network/soup/CredentialSoup.cpp: In member function ‘WTF::GRefPtr<_GTlsCertificate> WebCore::Credential::certificate() const’: ../../Source/WebCore/platform/network/soup/CredentialSoup.cpp:91:33: error: ‘g_tls_certificate_new_from_pkcs11_uris’ was not declared in this scope certificate = adoptGRef(g_tls_certificate_new_from_pkcs11_uris(certificateURI->data(), privateKeyURI->data(), &error.outPtr())); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../Source/WebCore/platform/network/soup/CredentialSoup.cpp:91:33: note: suggested alternative: ‘g_tls_certificate_new_from_files’ certificate = adoptGRef(g_tls_certificate_new_from_pkcs11_uris(certificateURI->data(), privateKeyURI->data(), &error.outPtr())); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ g_tls_certificate_new_from_files
Created attachment 378296 [details] Patch
Comment on attachment 378296 [details] Patch You may be interested in https://trac.webkit.org/changeset/246605/webkit
Comment on attachment 378296 [details] 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.
(In reply to Michael Catanzaro from comment #10) > Comment on attachment 378296 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378296&action=review > > > 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. I just wanted to avoid touching other ports. > 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 What issue would that cause? > > 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! No this isn't exposed. I went back to ASSERTs. > > 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. Less clear but OK. > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:541 > > + g_critical("Certificate set for non-certificate auth"); > > Should it be an ASSERT? No this is something exposed by our API, they can return the wrong credential type.
(In reply to Patrick Griffis from comment #11) > What issue would that cause? The variadic parameters would be all messed up. g_object_new() will read four bytes for the parameter it expects to be gboolean/G_TYPE_BOOLEAN, but if bool is actually used, that's three bytes shorter, so it will read three bytes into the next parameter. So all subsequent parameters will be three bytes off. So good thing you did it right. :P
Comment on attachment 378296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378296&action=review > Source/WebCore/platform/network/soup/CredentialSoup.cpp:37 > +Credential::Credential(CString&& certificatePEM) Also this one should be explicit
Comment on attachment 378296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378296&action=review > Source/WebCore/platform/network/soup/CredentialSoup.h:65 > + Optional<CString> certificatePEM; > + Optional<CString> certificateURI; > + Optional<CString> privateKeyURI; Data members should use the m_ prefix. Also, this will be better using String instead of Optional<CString>. You can just check e.g. m_certificatePEM.isEmpty() instead of testing whether the optional is engaged. (Less-important: optional is really inefficient for struct packing since it's one bool to indicate whether the optional is engaged and then the templated type.)
*** Bug 164509 has been marked as a duplicate of this bug. ***
*** Bug 180957 has been marked as a duplicate of this bug. ***
Comment on attachment 378296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378296&action=review I'd really recommend adding a unit test similar to https://trac.webkit.org/changeset/246605/webkit >>> Source/WebCore/platform/network/ProtectionSpaceBase.h:58 >>> +#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. > > I just wanted to avoid touching other ports. This new type of authentication is interesting. Could you describe the flow and why this is necessary? Do you have a plan for how to handle client certificate authentication using USB or NFC hardware that does signing but does not let the private key leave its hardware?
(In reply to Alex Christensen from comment #17) > This new type of authentication is interesting. Could you describe the flow > and why this is necessary? > Do you have a plan for how to handle client certificate authentication using > USB or NFC hardware that does signing but does not let the private key leave > its hardware? That's supported by this patch, that's what the ProtectionSpaceAuthenticationSchemeClientCertificatePINRequested is used for (PIN to unlock smartcard). Regarding a test, yes I think that's the holdup here. Tests are often a significant extra effort: (In reply to Michael Catanzaro from comment #10) > 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. I don't think it's worth adding a mock PKCS#11 module to WebKit to test the smartcard support -- we should be content with our glib-networking tests for that feature -- but there should at least be a test using a vanilla filesystem-based certificate.
Let's not forget about this. I guess this is stalled on: * a test. I still think testing a single filesystem-based certificate would suffice. We can just assume the PKCS#11 interactions are tested at glib-networking level, because they are. * the Epiphany implementation. We should continue to ensure the API user is ready before adding new API. And wouldn't want to forget the final step anyway.
(In reply to Michael Catanzaro from comment #19) > * the Epiphany implementation. We should continue to ensure the API user is > ready before adding new API. And wouldn't want to forget the final step > anyway. (I agreed with Patrick that this step isn't needed if WebKitGTK has a default implementation that looks good.)
Created attachment 419864 [details] Rebased patch
Rebased the patch to current HEAD. No functional changes done, other than ifdef'ing some new API usage behind `GLIB_CHECK_VERSION(2, 68, 0)`.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 419864 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=419864&action=review Testing this locally and everything still functions as expected after the rebase. I'd like somebody else to do the final review though. > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:275 > + * Since: 2.26 These need to be updated to 2.32
gonna need to apply the other review comments as well
(In reply to Patrick Griffis from comment #24) > Comment on attachment 419864 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419864&action=review > > Testing this locally and everything still functions as expected after the > rebase. I'd like somebody else to do the final review though. > > > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:275 > > + * Since: 2.26 > > These need to be updated to 2.32 Maybe it's a bit late for 2.32? we should be frozen already...
I would shoot for 2.34 indeed. (In reply to Daniel Kolesa from comment #25) > gonna need to apply the other review comments as well It looks like the first round of review comments are already addressed, but those from comment #10 down are not. Most important missing part is the API test.
Discussed in private this will target the next cycle with some of this work being moved into libsoup most likely. As-is the patch does function for this release though.
Comment on attachment 419864 [details] 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?
I think we have several options here: a) Move the client side certs support to libsoup, and try to make it fit in SoupAuth API. b) Keep the implementation in WebKit, but without using SoupAuth APIs. We can provide our own GTlsInteraction to libsoup (problem here is that a global one might not work), but handle it internally without using the authenticate interface. c) Move the client side certs support to libsoup but exposing specific API for that instead of re-using SoupAuth. It would be a wrapping to GTlsInteraction, but doing the right association between SoupMessage and connection. WE might need to use a different GTlsInteraction per connection, for example. For all the cases there are a few features to implement: - Proxy support. I guess the proxy can be protected by client TLS certificate? - Retrying failures and error handling. - Caching. By default we should cache the certificates and password for the certificates temporarily for the session (a new connection for the same host should reuse the previous certificate). But we should also implement persistent caching. Other browsers don't even show a dialog to enter the certificates, they are expected to be "installed". I'm ok with asking for certs as long as we provide a way to cache that (so "installing" them).
(In reply to Carlos Garcia Campos from comment #30) > - Proxy support. I guess the proxy can be protected by client TLS > certificate? In theory, any TLS server could require a client certificate. In practice, I doubt anybody would do this. I wouldn't worry about it. > - Caching. By default we should cache the certificates and password for the > certificates temporarily for the session (a new connection for the same host > should reuse the previous certificate). A new connection for the same host *and port* should reuse the previous certificate. At least if the previous certificate seems to have been accepted, but guessing that requires a heuristic, like: "read data from server after certificate was provided," which could theoretically fail if the data is the server presenting some error page to ask you to provide a different certificate....
Created attachment 431539 [details] WIP patch This is a new patch using the new libsoup API for handling client side certificates. It exposes just two new methods to create a credential for a pin or GTlsCertificate object. It's WIP because it depends on future GLib. The plan is to make libsoup3 depend on new GLib release, since nit will be required for other things too, and then bump the libsoup3 version in WebKit once we hae a new nlibsoup release requiring the new GLib.
Comment on attachment 431539 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=431539&action=review Looks really good overall. One question of mine is the previous patch contained a generic file chooser fallback and now it is limited to the minibrowser. I suppose that is fine but knowing it worked in all browsers without them handling it was nice IMO. A concern of mine is that we don't expose `g_tls_password_get_flags()` in the `WebKitAuthenticationRequest` which is useful for PIN requests as there is a chance a specific PIN will be required. See https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2126 > Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:154 > + "pkcs11-uri", pkcs11Uri.data(), glib-networking asserts that only one of either "certificate" or "pkcs11-uri" is set, not both. Same applies to private keys.
Comment on attachment 431539 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=431539&action=review Out of curiosity, do we have a test website for this? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:739 > +#if USE(SOUP2) > + ASSERT_NOT_REACHED(); > +#else > + soup_message_tls_client_certificate_password_request_complete(m_soupMessage.get()); > +#endif > + break; > + case ProtectionSpaceAuthenticationSchemeClientCertificateRequested: > +#if USE(SOUP2) > + ASSERT_NOT_REACHED(); > +#else > + soup_message_set_tls_client_certificate(m_soupMessage.get(), nullptr); > +#endif Are these the only APIs that would need to be backported to libsoup2 for this to work with libsoup2? Anything else that would definitely be needed? (I'm asking for RHEL 9. libsoup3 has missed that boat, and I'm not afraid to backport patches to our downstream libsoup2 if it gets us client authentication sooner.) > Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:96 > + g_object_get(certificate.get(), "private-key", &privateKey.outPtr(), "pkcs11-uri", &pkcs11Uri.outPtr(), "private-key-pkcs11-uri", &privateKeyPKCS11Uri.outPtr(), nullptr); Oh... so *this* is why you needed https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2087. I suppose I didn't realize what I was helping with. :D > Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationRequest.h:71 > + * @WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_PIN_REQUESTED: Client certificate PIN required for use. Since: 2.26 Since: 2.34 > Source/WebKit/UIProcess/API/wpe/WebKitAuthenticationRequest.h:70 > + * @WEBKIT_AUTHENTICATION_SCHEME_CLIENT_CERTIFICATE_PIN_REQUESTED: Client certificate PIN required for use. Since: 2.26 2.34
Comment on attachment 431539 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=431539&action=review >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:739 >> +#endif > > Are these the only APIs that would need to be backported to libsoup2 for this to work with libsoup2? Anything else that would definitely be needed? (I'm asking for RHEL 9. libsoup3 has missed that boat, and I'm not afraid to backport patches to our downstream libsoup2 if it gets us client authentication sooner.) Yes, I don't know if they will be easy to backport, though. >> Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:96 >> + g_object_get(certificate.get(), "private-key", &privateKey.outPtr(), "pkcs11-uri", &pkcs11Uri.outPtr(), "private-key-pkcs11-uri", &privateKeyPKCS11Uri.outPtr(), nullptr); > > Oh... so *this* is why you needed https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2087. I suppose I didn't realize what I was helping with. :D Yes! >> Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:154 >> + "pkcs11-uri", pkcs11Uri.data(), > > glib-networking asserts that only one of either "certificate" or "pkcs11-uri" is set, not both. Same applies to private keys. Here we are serializing/deserializing a existing GTlsCertifciate, so I assumed that's always the case, otherwise it will end up hitting the glib-networking asserts.
(In reply to Michael Catanzaro from comment #34) > Comment on attachment 431539 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431539&action=review > > Out of curiosity, do we have a test website for this? > I tested it with badssl.com, but I have to convert the certificate to use unencrypted private key. We also have unit tests in WebKit and libsoup.
(In reply to Patrick Griffis from comment #33) > Comment on attachment 431539 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=431539&action=review > > Looks really good overall. > > One question of mine is the previous patch contained a generic file chooser > fallback and now it is limited to the minibrowser. I suppose that is fine > but knowing it worked in all browsers without them handling it was nice IMO. The plan for now is to not provide default implementation for client side certificates, and let apps handle it, because a filechooser isn't probable the best way. Other browsers require the user to first "import" certificates. The MiniBrowser implementation is just for testing. > A concern of mine is that we don't expose `g_tls_password_get_flags()` in > the `WebKitAuthenticationRequest` which is useful for PIN requests as there > is a chance a specific PIN will be required. See > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2126 > I thought about it, but forgot to ask you in the end.
(In reply to Carlos Garcia Campos from comment #36) > I tested it with badssl.com, but I have to convert the certificate to use > unencrypted private key. We also have unit tests in WebKit and libsoup. I was about to say: "Let's figure out how to do encrypted PKCS #8 via GTlsCertificate. That is the only important standard that we are missing currently." The problem here is that GTlsCertificate has no API for providing the encryption passphrase. But I see badssl.com is using PKCS #12: https://badssl.com/download/. I didn't know that existed. So that's two standards that we are missing. The problem is the same though: we need some API to provide the passphrase. These both seem like low-hanging fruit now that you have done all the hard work in this bug. Both OpenSSL and GnuTLS can do PKCS #12, so it should be no problem once we agree on API. (In reply to Carlos Garcia Campos from comment #37) > The plan for now is to not provide default implementation for client side > certificates, and let apps handle it, because a filechooser isn't probable > the best way. Other browsers require the user to first "import" > certificates. The MiniBrowser implementation is just for testing. Hm, a file chooser is a nice fallback in case the application doesn't have its own implementation, though.
Created https://gitlab.gnome.org/GNOME/glib/-/issues/2432 "GTlsCertificate should support PKCS #12 and encrypted PKCS #8 formats"
Created attachment 432472 [details] WIP patch Addressed review comments and added webkit_authentication_request_get_certificate_pin_flags(). I prefer to leave any default UI for follow up patches. There's one more thing missing, persistent storage. Session persistence works, it's saved like for any other auth requests and I've checked that the cert is correctly set even for new connections. I don't know if persistence storage is possible or even desirable in WebKit, but we should either document it's not supported (and maybe add a g_warning is persistent is passed to the API) or implementing it.
Having a simple default UI would be good, but I agree that can be a future patch. I would leave it to the application to implement persistent storage. Persistent storage implies a more complex UI that should be managed by the application.
Comment on attachment 432472 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=432472&action=review > Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:156 > +#if GLIB_CHECK_VERSION(2, 69, 0) > + "private-key", privateKey.get(), > + "pkcs11-uri", pkcs11Uri.data(), > + "private-key-pkcs11-uri", privateKeyPKCS11Uri.data(), > +#endif I've just realized these should only be set for the last cert in the chain (which is the first one), and not in every cert in the chain. I'll update the patch
Created attachment 432476 [details] WIP patch
Created attachment 432479 [details] WIP patch
Comment on attachment 432479 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=432479&action=review > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:75 > + GUniqueOutPtr<char> certificateData; > + g_object_get(nextCertificate, "certificate-pem", &certificateData.outPtr(), nullptr); > + GUniquePtr<char> certificateDataCopy(certificateData.release()); > + certificatesDataList.append(WTFMove(certificateDataCopy)); You don't need certificateDataCopy anymore because certificateData is already an isolated copy of the data. It's a string, not a refcounted object. That's why I suggested switching to certificate-pem during our discussion earlier today. > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:96 > + "pkcs11-uri", certificatesDataList.isEmpty() ? pkcs11Uri.get() : nullptr, Hm, to copy as much as possible, we would also want to copy the pkcs11-uri for every certificate in the chain, not just the first one. But that would complicate this function quite a bit, and it probably doesn't matter much... I think. Patrick, is this OK?
(In reply to Michael Catanzaro from comment #45) > Comment on attachment 432479 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432479&action=review > > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:96 > > + "pkcs11-uri", certificatesDataList.isEmpty() ? pkcs11Uri.get() : nullptr, > > Hm, to copy as much as possible, we would also want to copy the pkcs11-uri > for every certificate in the chain, not just the first one. But that would > complicate this function quite a bit, and it probably doesn't matter much... > I think. Patrick, is this OK? It turns out the `"pkcs11-uri"` property isn't needed to serialize. The backend immediately loads the certificate URI into its normal `"certificate"` property. Only the private key uri needs to be kept.
(In reply to Michael Catanzaro from comment #45) > Comment on attachment 432479 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432479&action=review > > > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:75 > > + GUniqueOutPtr<char> certificateData; > > + g_object_get(nextCertificate, "certificate-pem", &certificateData.outPtr(), nullptr); > > + GUniquePtr<char> certificateDataCopy(certificateData.release()); > > + certificatesDataList.append(WTFMove(certificateDataCopy)); > > You don't need certificateDataCopy anymore because certificateData is > already an isolated copy of the data. It's a string, not a refcounted > object. That's why I suggested switching to certificate-pem during our > discussion earlier today. Yes, there isn't any copy there, I probably used a confusing name for the variable. The problem is that GUniqueOutPtr is not copyable, so what I'm doing here is transferring the string from a GUniqueOutPtr to a GUniquePtr to add it to the vector, without copying. > > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:96 > > + "pkcs11-uri", certificatesDataList.isEmpty() ? pkcs11Uri.get() : nullptr, > > Hm, to copy as much as possible, we would also want to copy the pkcs11-uri > for every certificate in the chain, not just the first one. But that would > complicate this function quite a bit, and it probably doesn't matter much... > I think. Patrick, is this OK?
Created attachment 432573 [details] WIP patch
Created attachment 433306 [details] Patch
Comment on attachment 433306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433306&action=review I have a serious question about the PINs, but the rest looks good. I like the load events tests. > Source/WebCore/platform/network/soup/AuthenticationChallenge.h:50 > + AuthenticationChallenge(SoupMessage*, GTlsClientConnection*); The GTlsClientConnection* parameter looks like a mistake, since it is unused. That should be removed. Right? (Then this constructor would need to become explicit.) > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:111 > + * Note that %WEBKIT_CREDENTIAL_PERSISTENCE_PERMANENT is not support for certificate pin credentials. supported > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:122 > + g_warning("Permanent persistence is not supported for certificate pin credentials, session persistence will be used instead."); "Permanent persistence is not supported for certificate pin credentials. Session persistence will be used instead." > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:135 > + * Note that %WEBKIT_CREDENTIAL_PERSISTENCE_PERMANENT is not support for certificate credentials. supported > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:146 > + g_warning("Permanent persistence is not supported for certificate credentials, session persistence will be used instead."); Ditto > Source/WebKit/UIProcess/API/glib/WebKitCredential.cpp:247 > + * Returns: (transfer none): a #GtlsCertifciate, or %BULL Wow, three typos on this line. :D #GTlsCertificate (note: two typos there) and %NULL > Tools/ChangeLog:9 > + tests for the client certificate request. Unfortunately we can't easily test pin certificates. Hm, how have you tested the PIN codepaths? Thing is, we only support PKCS #1 and unencrypted PKCS #8 certificates currently, which is all plaintext, so there is nothing that can be protected by the PIN. Right? What exactly is the PIN for? I think it's supposed to be used for encrypted PKCS #8 and PKCS #12 certificates, which are currently not supported and therefore not testable. Am I wrong about something here? > Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:593 > + // Sometimes glib-networking fails to report the error as certificate required and we end up > + // with connection reset by peer because the server closes the connection. > + if (!g_error_matches(test->m_error.get(), G_IO_ERROR, G_IO_ERROR_CONNECTION_CLOSED)) > + g_assert_error(test->m_error.get(), G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED); I was hoping this would be fixed by https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/146. Not so?
Comment on attachment 433306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433306&action=review >> Source/WebCore/platform/network/soup/AuthenticationChallenge.h:50 >> + AuthenticationChallenge(SoupMessage*, GTlsClientConnection*); > > The GTlsClientConnection* parameter looks like a mistake, since it is unused. That should be removed. Right? > > (Then this constructor would need to become explicit.) Not really, it's indeed unused, it was a trick to add a constructor for the connection case, since that's not obvious for a construtor that just takes a SoupMessage, but it's specific to client side certificate auth. I can remove the parameter and just add a comment though. >> Tools/ChangeLog:9 >> + tests for the client certificate request. Unfortunately we can't easily test pin certificates. > > Hm, how have you tested the PIN codepaths? Thing is, we only support PKCS #1 and unencrypted PKCS #8 certificates currently, which is all plaintext, so there is nothing that can be protected by the PIN. Right? What exactly is the PIN for? I think it's supposed to be used for encrypted PKCS #8 and PKCS #12 certificates, which are currently not supported and therefore not testable. Am I wrong about something here? Patrick tested it. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:593 >> + g_assert_error(test->m_error.get(), G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED); > > I was hoping this would be fixed by https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/146. Not so? Probably, but we don't depend on a specific version og glib-networking.
(In reply to Carlos Garcia Campos from comment #51) > Not really, it's indeed unused, it was a trick to add a constructor for the > connection case, since that's not obvious for a construtor that just takes a > SoupMessage, but it's specific to client side certificate auth. I can remove > the parameter and just add a comment though. Oh, OK. That's not a bad trick actually, but maybe too clever to do it without a comment. Anyway, whatever you prefer is fine. > >> Tools/ChangeLog:9 > >> + tests for the client certificate request. Unfortunately we can't easily test pin certificates. > > > > Hm, how have you tested the PIN codepaths? Thing is, we only support PKCS #1 and unencrypted PKCS #8 certificates currently, which is all plaintext, so there is nothing that can be protected by the PIN. Right? What exactly is the PIN for? I think it's supposed to be used for encrypted PKCS #8 and PKCS #12 certificates, which are currently not supported and therefore not testable. Am I wrong about something here? > > Patrick tested it. Patrick, can you explain it please? You'll get r=me once I understand how this works. :)
(In reply to Michael Catanzaro from comment #50) > > Tools/ChangeLog:9 > > + tests for the client certificate request. Unfortunately we can't easily test pin certificates. > > Hm, how have you tested the PIN codepaths? Thing is, we only support PKCS #1 > and unencrypted PKCS #8 certificates currently, which is all plaintext, so > there is nothing that can be protected by the PIN. Right? What exactly is > the PIN for? I think it's supposed to be used for encrypted PKCS #8 and PKCS > #12 certificates, which are currently not supported and therefore not > testable. Am I wrong about something here? > PKCS #11 uses PINs of course. I manually set the GTlsCertificate to a PKCS #11 URI to test.
(In reply to Carlos Garcia Campos from comment #51) > > I was hoping this would be fixed by https://gitlab.gnome.org/GNOME/glib-networking/-/merge_requests/146. Not so? > > Probably, but we don't depend on a specific version og glib-networking. Note that libsoup3 implicitly depends on glib-networking 2.70
(In reply to Patrick Griffis from comment #53) > PKCS #11 uses PINs of course. I manually set the GTlsCertificate to a PKCS > #11 URI to test. Ah OK, so the PIN unlocks the smartcard and then the smartcard provides the certificate... or something like that. OK. (In reply to Carlos Garcia Campos from comment #51) > Probably, but we don't depend on a specific version og glib-networking. Um, yes, good point. Worth checking to see if this leniency can be removed before landing. If it's still needed, it's fine as-is, no worries.
(In reply to Michael Catanzaro from comment #55) > Um, yes, good point. I meant: "Patrick has a good point. We can probably assume it's new glib-networking."
Created attachment 433395 [details] Patch for landing
(In reply to Michael Catanzaro from comment #56) > (In reply to Michael Catanzaro from comment #55) > > Um, yes, good point. > > I meant: "Patrick has a good point. We can probably assume it's new > glib-networking." ERROR:../../Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:619:void testClientSideCertificate(ClientSideCertificateTest*, gconstpointer): assertion failed (test->m_error.get() == (g-tls-error-quark, 5)): Error receiving data: Connection reset by peer (g-io-error-quark, 44) It doesn't seem to be fixed.
Created attachment 433398 [details] Patch for landing
Committed r279872 (239625@main): <https://commits.webkit.org/239625@main>