Bug 230526 - [Curl] Show TLS connection information in the inspector
Summary: [Curl] Show TLS connection information in the inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Komori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-20 17:49 PDT by Takashi Komori
Modified: 2021-09-22 20:29 PDT (History)
11 users (show)

See Also:


Attachments
Added information in the network security tab. (406.82 KB, image/png)
2021-09-20 17:49 PDT, Takashi Komori
no flags Details
Patch (4.82 KB, patch)
2021-09-20 18:29 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (4.79 KB, patch)
2021-09-21 02:19 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch (4.95 KB, patch)
2021-09-22 13:31 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff
Patch for landing (4.95 KB, patch)
2021-09-22 20:00 PDT, Takashi Komori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Komori 2021-09-20 17:49:30 PDT
Created attachment 438761 [details]
Added information in the network security tab.

Add TLS version and cipher name in the network security tab.
Comment 1 Takashi Komori 2021-09-20 18:29:58 PDT
Created attachment 438765 [details]
Patch
Comment 2 Fujii Hironori 2021-09-20 19:01:46 PDT
Comment on attachment 438765 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlContext.cpp:875
> +            m_tlsConnectionInfo = makeUnique<TLSConnectionInfo>();

Why is m_tlsConnectionInfo needed? What's the problem if setting to networkLoadMetrics.tlsProtocol directly?
Comment 3 Fujii Hironori 2021-09-20 19:05:14 PDT
Comment on attachment 438765 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlContext.h:322
> +        WTF_MAKE_FAST_ALLOCATED;

I think it'd better to use struct and WTF_MAKE_STRUCT_FAST_ALLOCATED.
Comment 4 Fujii Hironori 2021-09-20 19:16:14 PDT
Comment on attachment 438765 [details]
Patch

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

> Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:329
> +String getTLSCipherName(const SSL* ssl)

You shouldn't use 'get' for these functions (Bug 227766). However, OpenSSLHelper.cpp has more 'get'. Sooner or later, they should be renamed.
Comment 5 Takashi Komori 2021-09-21 02:19:21 PDT
Created attachment 438797 [details]
Patch
Comment 6 Takashi Komori 2021-09-21 02:20:52 PDT
(In reply to Fujii Hironori from comment #2)
> Comment on attachment 438765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438765&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:875
> > +            m_tlsConnectionInfo = makeUnique<TLSConnectionInfo>();
> 
> Why is m_tlsConnectionInfo needed? What's the problem if setting to
> networkLoadMetrics.tlsProtocol directly?

A CurlRequest gets metrics in CurlRequest::didReceiveHeader and CurlRequest::didCompleteTransfer but in didCompleteTransfer, curl_easy_getinfo can't obtain SSL pointer.
This is because when the connection ends, libcurl clears ssl context in the curl handle internally.
Therefore, we should keep TLS information in m_tlsConnectionInfo when didReceiveHeader is called.
Comment 7 Takashi Komori 2021-09-21 02:21:55 PDT
(In reply to Fujii Hironori from comment #3)
> Comment on attachment 438765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438765&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.h:322
> > +        WTF_MAKE_FAST_ALLOCATED;
> 
> I think it'd better to use struct and WTF_MAKE_STRUCT_FAST_ALLOCATED.

Fixed.
Comment 8 Takashi Komori 2021-09-21 02:22:26 PDT
(In reply to Fujii Hironori from comment #4)
> Comment on attachment 438765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438765&action=review
> 
> > Source/WebCore/platform/network/curl/OpenSSLHelper.cpp:329
> > +String getTLSCipherName(const SSL* ssl)
> 
> You shouldn't use 'get' for these functions (Bug 227766). However,
> OpenSSLHelper.cpp has more 'get'. Sooner or later, they should be renamed.

Removed 'get' from function names.
Comment 9 EWS 2021-09-22 12:00:15 PDT
Tools/Scripts/svn-apply failed to apply attachment 438797 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 10 Takashi Komori 2021-09-22 13:31:03 PDT
Created attachment 438970 [details]
Patch
Comment 11 Takashi Komori 2021-09-22 13:34:52 PDT
Fix the conflict of the patch of bug 226982.
Comment 12 Takashi Komori 2021-09-22 20:00:53 PDT
Created attachment 439005 [details]
Patch for landing
Comment 13 EWS 2021-09-22 20:28:13 PDT
Committed r282904 (242032@main): <https://commits.webkit.org/242032@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439005 [details].
Comment 14 Radar WebKit Bug Importer 2021-09-22 20:29:22 PDT
<rdar://problem/83429738>