Bug 216038 - [SOUP] webkit_web_view_get_https_status() broken with service workers
Summary: [SOUP] webkit_web_view_get_https_status() broken with service workers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-01 08:46 PDT by Michael Catanzaro
Modified: 2020-10-13 11:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.86 KB, patch)
2020-10-11 19:09 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2020-10-11 19:18 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2020-10-13 08:11 PDT, Michael Catanzaro
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-09-01 08:46:15 PDT
Bug #206403 is probably not fixed for libsoup ports because CertificateInfo::isolated_copy is not implemented.

I don't know how to test service workers. If anybody knows of a test page to see if it works or not, that would be cool.
Comment 1 Alex Christensen 2020-09-01 08:49:04 PDT
https://twitter.com
Comment 2 Michael Catanzaro 2020-09-01 09:18:57 PDT
OK thanks. We just got a report that was broken in https://gitlab.gnome.org/GNOME/epiphany/-/issues/1323, which is how I wound up here, although I had incorrectly assumed it was using offline web application cache rather than service workers.

I can reproduce this problem on twitter.com in WebKitGTK 2.28.4, but not in 2.29.91, which indicates it is probably fixed. HOWEVER... CertificateInfo::isolated_copy is still not implemented, so that's weird. Magic?
Comment 3 Michael Catanzaro 2020-09-01 09:31:52 PDT
(In reply to Michael Catanzaro from comment #2)
> I can reproduce this problem on twitter.com in WebKitGTK 2.28.4, but not in
> 2.29.91, which indicates it is probably fixed. HOWEVER...
> CertificateInfo::isolated_copy is still not implemented, so that's weird.
> Magic?

So without having bisected it, I would guess that you (Alex) fixed it in r264724, but I don't understand how it's possible with that function unimplemented. WebKit is actually somehow loading the correct TLS certificate -- I can inspect it in Epiphany and view its properties -- whereas previously it reported that the website had no security (webkit_web_view_get_tls_info() returned NULL), indicating that the main resource was loaded via service workers. So there *should* be no way for WebKit to get the CertificateInfo if it's not properly encoded. But the encoding depends on CertificateInfo::isolated_copy, which just returns an empty struct, so ??????

I did think about how to implement CertificateInfo::isolated_copy. It's currently impossible to do without adding new GLib API, because the GTlsCertificate:private-key property is not readable. Internally to GTlsCertificate, it will be a PKCS#11 token, which is just a string that is simple to copy for use across threads. But no way to get it without new API. I've been considering adding it anyway for an unrelated project.
Comment 4 Michael Catanzaro 2020-09-01 09:33:22 PDT
(In reply to Michael Catanzaro from comment #3)
> But no way to get it without new API. I've been considering adding it anyway for an unrelated project.

(It would be: g_tls_certificate_copy().)
Comment 5 Michael Catanzaro 2020-10-08 08:14:09 PDT
Definitely still broken, I can reproduce on https://twitter.com/othermaciej. We will either need to add new GLib API to allow implementing CertificateInfo::isolated_copy, or else reconsider other possible solutions (e.g. we might be able to use a partial isolated copy that doesn't copy everything).
Comment 6 Michael Catanzaro 2020-10-08 08:16:33 PDT
(In reply to Michael Catanzaro from comment #5)
> Definitely still broken, I can reproduce on https://twitter.com/othermaciej.

I'm using 2.30.1 currently. Not sure why I wasn't able to reproduce in 2.29.91 last month.
Comment 7 Michael Catanzaro 2020-10-11 18:50:43 PDT
(In reply to Michael Catanzaro from comment #6)
> I'm using 2.30.1 currently. Not sure why I wasn't able to reproduce in
> 2.29.91 last month.

I probably didn't refresh the page.
Comment 8 Michael Catanzaro 2020-10-11 19:09:56 PDT
Created attachment 411087 [details]
Patch
Comment 10 Michael Catanzaro 2020-10-11 19:18:27 PDT
Created attachment 411088 [details]
Patch
Comment 11 Carlos Garcia Campos 2020-10-13 01:07:45 PDT
Comment on attachment 411088 [details]
Patch

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

> Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:73
> +    RELEASE_ASSERT(!error);

Since you are using the GError just to assert it should never fail, why not just pass nullptr and assert cert is not nullptr?
Comment 12 Michael Catanzaro 2020-10-13 07:37:13 PDT
(In reply to Carlos Garcia Campos from comment #11)
> Since you are using the GError just to assert it should never fail, why not
> just pass nullptr and assert cert is not nullptr?

Good idea.
Comment 13 Michael Catanzaro 2020-10-13 08:11:05 PDT
Created attachment 411220 [details]
Patch
Comment 14 EWS 2020-10-13 08:39:04 PDT
Committed r268394: <https://trac.webkit.org/changeset/268394>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411220 [details].
Comment 15 Alex Christensen 2020-10-13 11:23:41 PDT
Comment on attachment 411220 [details]
Patch

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

> Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:81
> +    // certificate. So be careful?

CertificateInfo::isolatedCopy should only be used for server certificates.  We need better abstractions.

> Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:83
> +    // We should add g_tls_certificate_copy() to GLib so that we can copy the private portion too.

I wonder how you would handle the case where the private key is on a USB device that does not allow access to the private key.
Comment 16 Michael Catanzaro 2020-10-13 11:29:46 PDT
(In reply to Alex Christensen from comment #15)
> > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:83
> > +    // We should add g_tls_certificate_copy() to GLib so that we can copy the private portion too.
> 
> I wonder how you would handle the case where the private key is on a USB
> device that does not allow access to the private key.

That's no problem for GLib, because GTlsCertificate knows its PKCS#11 URI. (Actually, that is already going to be public API in GLib 2.68, since https://gitlab.gnome.org/GNOME/glib/-/issues/1809.) But that doesn't help WebKit, since if it's not a PKCS#11 certificate, then we really do need to copy the private key to copy the entire certificate, but the private-key property cannot ever be made readable (because then it would be impossible for PKCS#11-backed certificates to implement the API).