Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125861
Precedence: bulk
MIME-Version: 1.0
Date: Fri, 25 Oct 2024 09:34:35 +0200
To: erictnorris@gmail.com
Cc: PHP internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [VOTE] Add persistent curl share handles
In-Reply-To: <CA+NMNO16_p1=vmjpKSfWgSVCVqgxC0v-VGgfX4H5RhfGNNpSvg@mail.gmail.com>
References: <CA+NMNO16_p1=vmjpKSfWgSVCVqgxC0v-VGgfX4H5RhfGNNpSvg@mail.gmail.com>
Message-ID: <6e533102b9a2e9c8f6a2183440b2601a@bastelstu.be>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

Am 2024-10-24 20:20, schrieb Eric Norris:
> I have opened the vote for "Add persistent curl share handles":
> https://wiki.php.net/rfc/curl_share_persistence

Apologies, I wanted to chime in before the vote started, but I was too 
busy. Nevertheless I want to share my reasons for voting "no" on this 
RFC:

Persistent handles / resources / objects violate PHP’s shared-nothing 
request model, which is one fundamental part of how PHP works from my 
mental model. That's why I generally believe that persisting data across 
requests is generally unsafe, especially when used with stateful 
connections like a database connection. HTTP is generally stateless and 
I generally trust curl to do the right thing, which makes this somewhat 
safer, nevertheless the RFC would still be exposing a configurable (and 
thus stateful) object to an unknown number of future requests.

I'm especially concerned, because the documentation for 
`curl_share_init()` uses `CURL_LOCK_DATA_COOKIE` as the example. I would 
also assume that sharing a cookie jar amongst several requests is the 
primary use case for leveraging a curl share handle, given that curl 
already performs in-request connection reuse when reusing a single 
CurlHandle or when performing requests with a CurlMultiHandle. 
Accidentally sharing a cookie jar for unrelated requests due to a badly 
chosen `$persistent_id` sounds like a vulnerability to is bound to 
happen to someone.

Your RFC then lists "DNS lookups" as one of the possible things that 
would be sped up. Caching DNS lookups is already a solved problem: 
Install a caching resolver on your server. These commonly come with 
features such as pre-fetching, ensuring that commonly-performed lookups 
will stay cached at all times. As for sharing a TLS or HTTP connection, 
this might or might not lead to vulnerabilities similarly to cookie 
sharing, when TLS client certificates are used for authentication.

Lastly there does not appear to be a way to close a persistent share 
handle either implicitly or explicitly, making it hard to reset the 
share handle to a well-defined state / to know what state the share 
handle is in. Also badly chosen `$persistent_id`s might result in a 
large number of handles accumulating, without any kind of garbage 
collection. For the existing persistent handles (e.g. for database 
connections), the ID is chosen by PHP itself, ensuring a somewhat 
predictable behavior.

All in all, this RFC sounds like a foot-gun just waiting to go off and 
the RFC text is little more than a stub which does not inspire 
confidence that all the possible consequences have been fully 
thought-through. After all the pros and cons would also need to be 
documented in PHP’s documentation and the RFC text would be a good 
source for the documentation team.

Best regards
Tim Düsterhus