Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTPS OTA fails when server sends Close-notify message (IDFGH-13879) #14724

Closed
3 tasks done
pptz opened this issue Oct 14, 2024 · 4 comments
Closed
3 tasks done

HTTPS OTA fails when server sends Close-notify message (IDFGH-13879) #14724

pptz opened this issue Oct 14, 2024 · 4 comments
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally

Comments

@pptz
Copy link

pptz commented Oct 14, 2024

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

[Disclaimer: I know little about HTTP and TLS, I may be missing something]

When testing https OTA with standard examples (simple/advanced_https_ota), I found (for multiple versions of IDF incl. 5.3.1) the following issue:

If I use $ openssl s_server -WWW as the https server (as mentioned in the OTA examples README), then once the server sends out the entire file it sends a TLS message with the PSH flag set and an encrypted close-notify alert. My understanding of the expected behavior from the client in this case is that it should respond with close-notify and proceed to close the connection: https://datatracker.ietf.org/doc/html/rfc6101#section-5.4.1
However, what actually happens is that the client keeps sending TCP keepalives indefinitely and the OTA never completes (adding a custom header with esp_http_client_set_header(http_client, "Connection", "Close"); doesn't help). The client keeps calling esp_https_ota_perform() with a result of ESP_ERR_HTTPS_OTA_IN_PROGRESS, even though it's unable to read any additional data. I can tell from the byte count that the entire file has been received by that point. When mbedtls debugging is enabled one can see that the close-notify message is received and handled (see attached screenshot).

But if I use a simple python3 server (see attached) the behaviour is different: when the server has sent the entire file, instead of sending a "close-notify" alert in the TLS message it sets the FIN flag (along with PSH as before). This is handled successfully by the client, which now proceeds to send a TCP message with FIN and close the connection. The OTA successfully completes.

It appears that the esp http/s stack doesn't properly handle the first case, when a close-notify alert is sent.

Wireshark capture of successful and unsuccessful runs (one file, separate by timestamp) attached along with python https server. (client is 192.168.43.117, server is 192.168.43.203)

debug.zip

@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 14, 2024
@github-actions github-actions bot changed the title HTTPS OTA fails when server sends Close-notify message HTTPS OTA fails when server sends Close-notify message (IDFGH-13879) Oct 14, 2024
@nileshkale123
Copy link
Collaborator

Hello @pptz ,

Thank you for bringing up this issue. We've submitted a merge request to address it. In the meantime, you can apply the patch below to resolve the problem on your end. Your feedback is welcome!

14724_patch.txt
In this MR, we improved the handling of the close-notify alert during chunked transfers in HTTPS OTA. Previously, there was an unnecessary check for errno when receiving the close-notify, leading to issues with completing the OTA update. The system now correctly processes close-notify by bypassing the errno check, allowing the OTA to complete as expected when the server closes the connection gracefully. This change ensures proper detection of the end of the transfer and prevents redundant retries.

@pptz
Copy link
Author

pptz commented Oct 15, 2024

I can confirm that the example OTA completes with the patch. Ideally, the TLS session would be closed gracefully via calling mbedtls_ssl_close_notify(), which doesn't happen here, but I don't have the desire to investigate how to achieve this :).

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels Oct 17, 2024
espressif-bot pushed a commit that referenced this issue Nov 26, 2024
If the TLS server (e.g., openssl) closes connection with encrypted close-notify alert
then `errno` is not explicitly set on the socket by LwIP stack.
For this scenario, we must rely only on `ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN`
return value as the connection close case and do the graceful connection closure.

Closes #14724
espressif-bot pushed a commit that referenced this issue Nov 26, 2024
If the TLS server (e.g., openssl) closes connection with encrypted close-notify alert
then `errno` is not explicitly set on the socket by LwIP stack.
For this scenario, we must rely only on `ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN`
return value as the connection close case and do the graceful connection closure.

Closes #14724
espressif-bot pushed a commit that referenced this issue Nov 26, 2024
If the TLS server (e.g., openssl) closes connection with encrypted close-notify alert
then `errno` is not explicitly set on the socket by LwIP stack.
For this scenario, we must rely only on `ERR_TCP_TRANSPORT_CONNECTION_CLOSED_BY_FIN`
return value as the connection close case and do the graceful connection closure.

Closes #14724
@pchevali
Copy link

I have the issue on the branch 5.3 will the patch be backported to it ? ( I see it only on branches 5.0,5.1, 5.2)

@nileshkale123
Copy link
Collaborator

Hello @pchevali ,

The backport is already raised. It will get merged soon. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

4 participants