[HttpClient] Close gracefull when the server closes the connection abruptly

[discordier]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
License	MIT

curl will return -1.0 for CURLINFO_CONTENT_LENGTH_DOWNLOAD since 7.19.4 if not known (means not specified by the server response).

When handling data for empty responses, this will cause us to compare 0.0 (CURLINFO_SIZE_DOWNLOAD) with -1.0 (CURLINFO_CONTENT_LENGTH_DOWNLOAD) and thus error out with SSL error 0 then (which means normal close).

We therefore now explicitly allow to download 0 bytes, when no size has been indicated.

I'm unsure how to add tests here and also unsure what's the lowest version to be affected.

I don't think this affects BC, as the usecase to expect to get an error for an empty response seems very unlikely.

I tried to come up with a reproducer but failed as I can only reproduce it via HTTPS in my application.
Stacktrace:

In Stream.php line 266:
                                                                                                                                    
  [RuntimeException]                                                                                                                
  Unable to read stream contents: OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 0 for "https://[redacted]/api".  

Exception trace:
  at /project/vendor/nyholm/psr7/src/Stream.php:266
 Nyholm\Psr7\Stream::Nyholm\Psr7\{closure}() at n/a:n/a
 trigger_error() at /project/vendor/symfony/http-client/Response/StreamWrapper.php:129
 Symfony\Component\HttpClient\Response\StreamWrapper->stream_read() at n/a:n/a
 stream_get_contents() at /project/vendor/nyholm/psr7/src/Stream.php:270
 Nyholm\Psr7\Stream->getContents() at /project/vendor/nyholm/psr7/src/StreamTrait.php:23
 Nyholm\Psr7\Stream->__toString() at /project/src/ApiClient823/Generated/Endpoint/VersionVersion.php:57
[...]

The same request via curl -v:

curl -v https://[redacted]/api
*   Trying [redacted]...
* Connected to [redacted] ([redacted]) port 8006 (#0)
* ALPN: offers h2,http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: /etc/ssl/certs
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_256_GCM_SHA384
* ALPN: server did not agree on a protocol. Uses default.
* Server certificate:
*  subject: CN=[redacted]
*  start date: Aug 22 00:39:02 2024 GMT
*  expire date: Nov 20 00:39:01 2024 GMT
*  subjectAltName: host "[redacted]" matched cert's "[redacted]"
*  issuer: C=US; O=Let's Encrypt; CN=R10
*  SSL certificate verify ok.
* using HTTP/1.x
> GET /api HTTP/1.1
> Host: [redacted]
> User-Agent: curl/7.88.1
> Accept: */*
> 
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* old SSL session ID is stale, removing

< HTTP/1.1 401 No ticket
< Cache-Control: max-age=0
< Connection: close
< Date: Mon, 14 Oct 2024 13:52:18 GMT
< Pragma: no-cache
< Server: [redacted]
< Expires: Mon, 14 Oct 2024 13:52:18 GMT
< 
* Closing connection 0
* TLSv1.3 (OUT), TLS alert, close notify (256):

[discordier]

I rebased against 5.4, as this seems to be the first affected version.

[discordier]

Unit tests pass on my machine - unsure how to proceed here. Found the culprit, fixed.

[nicolas-grekas]

This HTTP transaction is not valid per HTTP/1.1

Connection: close is how HTTP/1.0 behaved, but since 1.1, either Content-Length should be sent, or Transfer-Encoding should (to chunked typically).

Are you in control of the server sending these responses?

The change you propose here will accept broken responses that should have sent some bytes but didn't before closing.

This might make the client less robust.

[nicolas-grekas]

HTTP/1.1 actually allows the server to close the connection to tell about the end of the response:
https://www.rfc-editor.org/rfc/rfc2616#section-4.4 (item 5.)
We now need to be sure the code you proposed is specific enough about the server connection being closed.
What's surprising is that curl reports this as an SSL error, while it could return CURLE_OK.

[nicolas-grekas]

(and yes, a test case would be great, to confirm the behavior of all implementations, not only curl)

[discordier]

Are you in control of the server sending these responses?

Somewhat, it's a standard proxmox server API endpoint (curl https://pve:8006/api).

HTTP/1.1 actually allows the server to close the connection to tell about the end of the response: https://www.rfc-editor.org/rfc/rfc2616#section-4.4 (item 5.) We now need to be sure the code you proposed is specific enough about the server connection being closed. What's surprising is that curl reports this as an SSL error, while it could return CURLE_OK.

I am puzzled as well - furthermore, it works fine on command line (see transcript in OP).

Maybe the "TLSv1.3 (OUT), TLS alert, close notify (256):" hints an exit code of 256 despite the shell exit code is 0?
I tried again on CLI via curl -v https://[redacted]/api; echo $? and got 0.

I wonder how we can ensure that it is specific enough... I mean it's pretty easy for me to validate that it's working like this but how to determine the side effects are minimized?
We could add in a check for curl version there but I lack an installation that is older than 7.19.4 (released March 3 2009) to check against - tbh. I strongly doubt that it will compile against current PHP - so I assume the behaviour should be pretty consistent across curl versions here.

Regarding a test, I tried to come up with one but failed except when testing against a real server.
I wonder if it is possible with the used php -s implementation the tests use to replicate the behaviour. Most likely we need SSL + direct socket close, both seem to be not possible using the test implementation if I'm not mistaken.

Any advisory on how to proceed here is greatly appreciated.

[nicolas-grekas]

See curl/curl#4624

It looks like curl ignores this error unless it's built with the debug flag.
But we need to make this specific for the SSL_ERROR_SYSCALL error.
Not sure if this means parsing the error message or if we could do better.
Can you have a look?

[discordier]

I'm not sure I understand what you mean by making this specific for the SSL_ERROR_SYSCALL.
Shall we swap the check of 'C' === $waitFor[0] with curl_error($ch) === 'OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 0' then yey, this works - did so.
I'd still keep the restriction on 0.0 === curl_getinfo($ch, \CURLINFO_SIZE_DOWNLOAD) and -1.0 === curl_getinfo($ch, \CURLINFO_CONTENT_LENGTH_DOWNLOAD) here (as seen in the PR).

Regarding an unit test, I have written a socket server which gets used in src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php. However, I failed to find a way to "hard-close" the socket as PHP socket handling always seems to gracefully close the socket - therefore it is not a reproducer yet. Maybe you have an idea on how I can force the immediate shutdown that confuses curl in the real server.

By trying to implement a test, I found that the \Symfony\Component\HttpClient\Tests\NativeHttpClientTest omits the options ['verify_peer' => false, 'verify_host' => false] and therefore always fails when connecting to https.

Sidenote: carsonbot tagged this as 7.2 wheras I currently target 5.4 - do I have to rebase against 7.2 now or will you change the milestone?

[stof]

carsonbot probably failed to identify the milestone (and so used the default one) because of your answer being 5.4 I guess, please advise, which is not a valid branch name.

[nicolas-grekas]

curl_error($ch) === 'OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 0'

yes

I'd still keep the restriction on 0.0 === curl_getinfo($ch, \CURLINFO_SIZE_DOWNLOAD)

nope, that'd be wrong

-1.0 === curl_getinfo($ch, \CURLINFO_CONTENT_LENGTH_DOWNLOAD) here (as seen in the PR).

yes

(but as the PR is now, it contains nothing but tentative tests)

Let forget about tests if this cannot be tested without complex setup 🤷

[discordier]

curl_error($ch) === 'OpenSSL SSL_read: SSL_ERROR_SYSCALL, errno 0'

yes

Changed.

I'd still keep the restriction on 0.0 === curl_getinfo($ch, \CURLINFO_SIZE_DOWNLOAD)

nope, that'd be wrong

Removed

-1.0 === curl_getinfo($ch, \CURLINFO_CONTENT_LENGTH_DOWNLOAD) here (as seen in the PR).

yes

Kept

(but as the PR is now, it contains nothing but tentative tests)

Let forget about tests if this cannot be tested without complex setup 🤷

Yes, I tried to put a reproducer here but as I said, I did not succeed for this problem (it is pretty hard to create an "improper" ssl shutdown in PHP).

I now dropped the test stuff and fixed the initial problem - I kept the fix in the NativeHttpClientTest though but am happy to make a separate PR out of this if desired.

[nicolas-grekas]

Re-reading https://www.rfc-editor.org/rfc/rfc2616#section-4.4, we might want to also check that no content-length / transfer-encoding header have been sent

Maybe we should be even stricter and do this only when the server sends Connection: close, WDYT?

[discordier]

Sorry, had not time to work on this the past days.

I added the check at the end so the calls to in_array and array_map are minimized.

Tested it against my server.

Is this now ok?

[nicolas-grekas]

Thank you @discordier.

[PhilETaylor]

Sorry to be the bearer of bad news :) #59039