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

Expose option for setting TLS handshake timeout #2752

Merged
merged 6 commits into from Jun 27, 2022

Conversation

lulf
Copy link
Contributor

@lulf lulf commented May 5, 2022

PR Type

Feature

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

Apologies for just raising this PR without any prior discussion. At present the TLS handshake timeout is hardcoded at 3 seconds internally in actix-tls. Having the ability to up this timeout is important for servers handling embedded clients that may take a while to do a handshake.

I'm attempting to expose the TLS handshake timeout as available on the Acceptor type in the actix-tls crate. The way I've done this is to add another option on the HttpServer if built with openssl or rustls. I'm very unfamiliar with the code, and I'm not particularily happy with the modification to the openssl/rustls-specific bits in actix-http (which is a bit incomplete missing the h1/h2 specific bits), which somewhat breaks the API, so I thought I'd just raise a draft PR as is to get the discussion going and get a better understanding on how you'd like this to be exposed.

@robjtede
Copy link
Member

robjtede commented May 5, 2022

This breaking API change would certainly not be ok to merge. It would need a new method adding.

@lulf
Copy link
Contributor Author

lulf commented May 6, 2022

Thanks for the feedback @robjtede. I've made the following changes:

  • Create the type TlsAcceptorConfig which holds the desired acceptor configuration, default being to use whatever actix-tls chooses.
  • Create a rustls_with_config/openssl_with_config which takes the TlsAcceptorConfig parameter

Longer term, it feels like this could use some refactoring.

@lulf lulf marked this pull request as ready for review May 12, 2022 09:20
@lulf lulf force-pushed the expose-handshake-timeout branch from ad07d1d to fcba8a3 Compare June 3, 2022 12:42
@lulf
Copy link
Contributor Author

lulf commented Jun 3, 2022

Would anyone be able to review this again?

ctron added a commit to drogue-iot/drogue-cloud that referenced this pull request Jun 3, 2022
ctron added a commit to drogue-iot/drogue-cloud that referenced this pull request Jun 3, 2022
@robjtede robjtede added A-web project: actix-web B-semver-minor labels Jun 3, 2022
@robjtede robjtede requested a review from a team June 3, 2022 14:28
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't reviewed deeply but adding timeout config makes sense to me (if no one reviews for a few weeks, I'm going to do a more detailed review).

@robjtede robjtede added this to the actix-web v4.2 milestone Jun 11, 2022
@robjtede robjtede added the A-http project: actix-http label Jun 27, 2022
Copy link
Member

@robjtede robjtede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reduced the API surface a bit and added relevant documentation but this approach is good to be added.

@robjtede robjtede enabled auto-merge (squash) June 27, 2022 02:14
@robjtede robjtede merged commit 0dba631 into actix:master Jun 27, 2022
@lulf lulf deleted the expose-handshake-timeout branch June 27, 2022 07:04
@lulf
Copy link
Contributor Author

lulf commented Jun 27, 2022

Thanks!

@lulf lulf restored the expose-handshake-timeout branch June 27, 2022 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http project: actix-http A-web project: actix-web B-semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants