Re: [VOTE] Add persistent curl share handles

Hi Am 2024-10-25 16:29, schrieb Eric Norris:

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.

I don't believe that sharing a cookie jar is the primary use case; it certainly wouldn't be for us. Our primary use case is DNS and connection sharing, for the aforementioned fanout.

FWIW, the curl documentation also lists cookie sharing as the first thing when describing the "share interface": https://curl.se/libcurl/c/libcurl-share.html.

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.

I'll admit that I don't have a good response to this, since while I agree this is possible, I don't think it outweighs the benefit that persistence gives to the applications that need it (again, we won't be using the cookie jar). I see you've noted concerns about documentation, would this be something you'd at least feel more comfortable with if curl persistence documentation included a strong warning?

Yes, I would be *more* comfortable if the documentation would ensure to appropriately the possible issues with persisting the share handle across multiple requests, possibly giving suggestions as to how a good persistent ID could be constructed (e.g. by using a hash of the current security context or something like that). I would also be more comfortable, if persisting a share handle with cookie jar sharing enabled would not be possible / would be ignored, because that only leaves the TLS session sharing (see below) as a possible safety issue.

Interestingly, one of the reasons that I began this work was that we found a measurable performance impact of talking to the caching resolver on localhost, let alone the connection overhead. Essentially

That honestly surprises me.

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.

I believe curl itself handles this - it won't re-use a connection if the settings aren't the same.

Okay, the curl documentation does not say anything about this in https://curl.se/libcurl/c/CURLSHOPT_SHARE.html#CURLLOCKDATASSLSESSION.

re-raise it there. We do already have a curl_share_close method, so the RFC would not need to add a new method, and since the method is called curl_share_close, it's not really changing the meaning of the method either.

That makes sense to me.

Also badly chosen $persistent_ids 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.

I agree that this may be a risk, but it's not clear to me that this is a risk that people would realistically run into, considering you'd want to have a small number of persistent IDs in order to benefit from re-using them. Dynamically picking your persistent IDs would lead to less re-use.

I believe in misuse-resistant API design. Folks should be steered towards doing the right thing by default. As an example: What if a library internally choses to use persistent share handles for whatever reason? Or an add-on you install in your CMS software? PHP for better or worse is not just used for in-house software with an experienced development team.

that developers are reaching for. Frankly, I see very little reason to use it without persistence, considering that curl_multi_init does the same thing, and is simpler to use.

Yes. And that probably explains why you are not seeing much use of it until now. But if/when it gets “marketed” as the next-big-thing in performance by tech bloggers that don't understand the possible caveats, it will probably start to get used by folks that don't fully understand it.

Regarding the RFC text, I apologize for not filling it out more, but it seemed straightforward to me: curl supports the share interface as a primary feature, and there are no glaring warnings in the documentation (https://curl.se/libcurl/c/libcurl-share.html) indicating that you might hold the share interface incorrectly. As I

The difference to me is that when using libcurl directly, you would generally have a more explicit data flow and don't pull a curl share handle in an unknown state out of thin air. See the library example above.

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.

My plan was to help add the documentation (and tests) after this passed; it was unclear to me that documentation may need to be sorted out up-front in the RFC process. That said, I believe I can summarize

The actual documentation is generally written during the RC phase, but if the RFC author is no longer available when that happens, the documentation team needs something to work with.

the remaining cons from your email: - A developer could reuse a cookie jar with cookies from user A in a subsequent request from user B to origin Z if they opted in to persistence, used CURL_LOCK_DATA_COOKIE, and used the same persistent handle for both users.

Yes.

- A developer could balloon memory usage by using dynamic persistent IDs, especially so if curl_share_close doesn't close them.

Yes.

If I were to add notes about these both to the documentation, and curl_share_close closed persistent share handles, would you change your vote for this or a future RFC?

I'm not sure about whether I consider documentation alone to be sufficient, but when share handles with cookie sharing enabled would not be persisted (and removed from persistence when enabling the option after the fact), then I would be fine with trusting libcurl to do the right thing. Ideally this could be confirmed with some reference to the libcurl source code or getting upstream confirmation that this is safe to do. Best regards Tim Düsterhus