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

feat(awc): allow to set a specific sni host on the request #3522

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented Dec 9, 2024

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

This PR allow client to set a specific sni host for a request, this can be useful if you want to connect to a specific hostname, but do SSL validation on another one

In order to do that some thing have been changed :

  • Uri is no longer cloned during connection, instead a specific object is created with only wanted information, it may be better as we don't need all uri information in the connection (only hostname, port and if it should use tls most of the time), so this should also reduce memory consumption on certain case.
  • A new object is used for tcp / tls connection : HostnameWithSni (can be changed to another name), basically it's the host with the optional sni host and a state (to know which host to use given the connector: we use hostname during tcp, for dns resolution if needed, we use sni_host if available during tls connection for server name)
  • The pool now use the connect object for its key of pooled connection, i believe the old way was not adequate (did not do test, since it was only relying on the authority of the uri, if you pass a specific local address by example, after a first request it would not have use the existing pool, and i believe it should not in this case ?)

There may be better implementation on some point, as i have done what it's the most pragmatic way as a first step, will be happy to fix those if you have some inputs.

let authority = if let Some(authority) = head.uri.authority() {
authority
} else {
return ConnectRequestFuture::Error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there may be a better way, but a lot of code need to be updated and was not sure about the correct / standard thing to do here, this is a pragmatic way of doing it, but if you have an idea on how to avoid this will be glad to hear.

awc/src/frozen.rs Outdated Show resolved Hide resolved
@joelwurtz joelwurtz force-pushed the feat/awc-sni-host branch 2 times, most recently from ffef23c to 1647a03 Compare December 9, 2024 10:59
@joelwurtz joelwurtz force-pushed the feat/awc-sni-host branch 6 times, most recently from 50f296f to d151822 Compare December 9, 2024 11:36
@robjtede robjtede added B-semver-major breaking change requiring a major version bump A-awc project: awc labels Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-awc project: awc B-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants