Skip to content

Allow the server_name_resolver to be overriden #2708

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

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

Conversation

MarkusTieger
Copy link

@MarkusTieger MarkusTieger commented May 31, 2025

This adds the

with_server_name_resolver(mut self, server_name_resolver: Arc<dyn hyper_rustls::ResolveServerName + Send + Sync>)

to ClientBuilder (if rustls is used).

It allows to override, which name is used for tls instead of using the one from the url.

This pr relys on rustls/hyper-rustls#301 being merged.

Also this hasn't been fully tested yet, I will remove this disclaimer after it has been tested.

@MarkusTieger MarkusTieger marked this pull request as draft June 1, 2025 12:14
@seanmonstar
Copy link
Owner

Thanks for contributing!

I notice this makes the dependency on hyper-rustls public. We try to avoid that, so we can increase versions internally without affecting the public API.

But a more general SNI mechanism could be explored.

@MarkusTieger
Copy link
Author

Thanks for contributing!

I notice this makes the dependency on hyper-rustls public. We try to avoid that, so we can increase versions internally without affecting the public API.

But a more general SNI mechanism could be explored.

Would you prefer it, to have an own trait, which then gets converted to hyper-rustls?

@MarkusTieger
Copy link
Author

Thanks for contributing!

I notice this makes the dependency on hyper-rustls public. We try to avoid that, so we can increase versions internally without affecting the public API.

But a more general SNI mechanism could be explored.

On the second time reading, do you mean to not export the "ResolveServerName" trait and instead let the user add the crate himself?

@MarkusTieger
Copy link
Author

@seanmonstar Could you comment on rustls/hyper-rustls#301 ?

Tldr: This pr first needs a change from hyper-rustls (which I implemented in the pr linked above). The question now is, would this pr get merged in the first place? (if not, what needs to be changed, or is this something that will never be a feature in reqwest?)

@seanmonstar
Copy link
Owner

I don't have a strong opinion on how any such API would look in hyper-rustls.

But what I mean is designing a possible generic way to define server name resolution might be fine to add, but not something that publicly exposed the dependency. That might mean using the PR you mentioned internally, I cannot say. But it will require a bit of research and design iteration to come up with an API we can try to keep stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants