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.

Sign up for GitHub

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

Jump to bottom

SSL, invoke shutdown implementation when closing a connection #11858

Closed
wants to merge 1 commit into from
Closed

SSL, invoke shutdown implementation when closing a connection #11858

wants to merge 1 commit into from

Conversation

icing
Copy link
Contributor

@icing icing commented Sep 15, 2023 •
edited

  • fix http2 and h2-proxy filter to call the next filters on close
  • make openssl's shutdown handle a one-time effort with added tracing for what it achieved

@github-actions github-actions bot added the TLS label Sep 15, 2023
@icing icing mentioned this pull request Sep 15, 2023
Closed
@icing icing requested a review from bagder September 18, 2023 08:58
@icing
Copy link
Contributor Author

icing commented Sep 18, 2023

Should be merged, imo.

@jay
Copy link
Member

jay commented Sep 18, 2023

While it may be necessary to wait for a shutdown in some cases, I don't like this fix because it's blocking (for 1+ seconds depending on time granularity) and not specific. Please see my comments here.

@icing
Copy link
Contributor Author

icing commented Sep 18, 2023

While it may be necessary to wait for a shutdown in some cases, I don't like this fix because it's blocking (for 1+ seconds depending on time granularity) and not specific. Please see my comments here.

I agree that possibly blocking for a second on each connection individually is not good. I opened a discussion at #11878.

@icing
Copy link
Contributor Author

icing commented Sep 19, 2023

Amended PR with a simpler version, no delays.

@mkauf mkauf self-requested a review September 21, 2023 09:45
mkauf
mkauf reviewed Sep 21, 2023
View reviewed changes
lib/vtls/openssl.c Outdated Show resolved Hide resolved
jay
jay reviewed Sep 22, 2023
View reviewed changes
lib/vtls/openssl.c Outdated Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
lib/vtls/openssl.c Outdated Show resolved Hide resolved
@icing @jay
openssl: improve ssl shutdown handling
c419abb 
- If SSL shutdown is not finished then make an additional call to
  SSL_read to gather additional tracing.

- Fix http2 and h2-proxy filters to forward do_close() calls to the next
  filter.

For example h2 and SSL shutdown before and after this change:

Before:

Curl_conn_close -> cf_hc_close -> Curl_conn_cf_discard_chain ->
ssl_cf_destroy

After:

Curl_conn_close -> cf_hc_close -> cf_h2_close -> cf_setup_close ->
ssl_cf_close

Closes #xxxx
@jay
Copy link
Member

jay commented Sep 24, 2023 •
edited

I've squashed all the commits to prepare for merge however the change does not work as I expected. The internal handle used by the connection cache to close connections (ie the closure_handle) has verbose mode false and CURL_LOG_LVL_NONE so none of those trace messages are output when CURL_DEBUG=ssl

@icing
Copy link
Contributor Author

icing commented Sep 25, 2023

I've squashed all the commits to prepare for merge however the change does not work as I expected. The internal handle used by the connection cache to close connections (ie the closure_handle) has verbose mode false and CURL_LOG_LVL_NONE so none of those trace messages are output when CURL_DEBUG=ssl

Yes, that is a known current limitation of libcurl/curl as documented in #11878. I think we should enable curl to also trace actions on connection cache cleanup.

This is outside of this PR really, it applies to all close() handling, with or without SSL.

@bagder
Copy link
Member

bagder commented Sep 25, 2023

The internal handle used by the connection cache to close connections (ie the closure_handle) has verbose mode false and CURL_LOG_LVL_NONE so none of those trace messages are output when CURL_DEBUG=ssl

This is an old and long-standing issue that we need to fix, but it is separate and needs a larger take.

@jay jay closed this in 34cdcb9 Sep 26, 2023
@jay
Copy link
Member

jay commented Sep 26, 2023

Thanks. Let's try this out and see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jay jay jay left review comments

@mkauf mkauf mkauf left review comments

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@icing @jay @bagder @mkauf