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

fail Client build if Identity + default backend + !cfg(native-tls) #1852

Merged
merged 2 commits into from Aug 21, 2023

Conversation

eric-seppanen
Copy link
Contributor

If ClientBuilder::build encounters a TlsBackend::Default, and cfg(native-tls) is not enabled, it doesn't know how to load the client certificate. Rather than silently ignore the attempt to use a client Identity, return an error.

This fix feels a little unsatisfying, for a couple of reasons:

  1. The problem might come back. It's not obvious which cfg gates should be applied at the various places Identity gets touched, so the code seems fragile in this area.
    Perhaps there should be a way to indicate that the Identity was successfully consumed? Then if we reach the end of build() and we still have an unused identity, we know there's a problem and should return an error.
  2. There's no test coverage. I'm looking into how to do this, but I'm not there yet. So far all I can offer is a flimsy "works for me."

I think this fixes #903, though it contains a few different reports over the years and it's hard to tell if they're all the same thing.

If ClientBuilder::build encounters a TlsBackend::Default, and
cfg(native-tls) is not enabled, it doesn't know how to load the client
certificate. Rather than silently ignore the attempt to use a client
identity, return an error.
@cpu
Copy link
Contributor

cpu commented Aug 16, 2023

@seanmonstar How do you feel about this branch's proposed solution to #903 ?

I think this fixes #903, though it contains a few different reports over the years and it's hard to tell if they're all the same thing.

We ran into this problem debugging a user bug report over in the Rustls repo (see my comment on 903 for more details). I was able to confirm the patch in this branch produces a helpful error for the situation we debugged.

Can I do anything to help move this fix (or an alternative) along?

@seanmonstar
Copy link
Owner

I'm always grateful for better errors, so thank you! I see your comment in the issue, and agree it'd be best if the different formats could be made to work with whatever back-end is selected. I'll include this for now, though.

@seanmonstar seanmonstar merged commit 87ff5d9 into seanmonstar:master Aug 21, 2023
31 checks passed
@cpu
Copy link
Contributor

cpu commented Aug 21, 2023

Thanks!

kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Aug 22, 2023
Bumps reqwest from 0.11.18 to 0.11.19.

Release notes
Sourced from reqwest's releases.

v0.11.19
What's Changed

Add ClientBuilder::http1_ignore_invalid_headers_in_responses() option.
Add ClientBuilder::http1_allow_spaces_after_header_name_in_responses() option.
Add support for ALL_PROXY environment variable.
Add support for use_preconfigured_tls when combined with HTTP/3.
Fix deflate decompression from using the zlib decoder.
Fix Response::{text, text_with_charset}() to strip BOM characters.
Fix a panic when HTTP/3 is used if UDP isn't able to connect.
Fix some dependencies for HTTP/3.
Increase MSRV to 1.63.

New Contributors

@​nyurik made their first contribution in seanmonstar/reqwest#1849
@​smndtrl made their first contribution in seanmonstar/reqwest#1856
@​attila-lin made their first contribution in seanmonstar/reqwest#1869
@​ollyswanson made their first contribution in seanmonstar/reqwest#1898
@​VivekPanyam made their first contribution in seanmonstar/reqwest#1903
@​bouzuya made their first contribution in seanmonstar/reqwest#1922
@​cipherbrain made their first contribution in seanmonstar/reqwest#1927
@​T-Sujeeban made their first contribution in seanmonstar/reqwest#1926
@​eric-seppanen made their first contribution in seanmonstar/reqwest#1852




Changelog
Sourced from reqwest's changelog.

v0.11.19

Add ClientBuilder::http1_ignore_invalid_headers_in_responses() option.
Add ClientBuilder::http1_allow_spaces_after_header_name_in_responses() option.
Add support for ALL_PROXY environment variable.
Add support for use_preconfigured_tls when combined with HTTP/3.
Fix deflate decompression from using the zlib decoder.
Fix Response::{text, text_with_charset}() to strip BOM characters.
Fix a panic when HTTP/3 is used if UDP isn't able to connect.
Fix some dependencies for HTTP/3.
Increase MSRV to 1.63.




Commits

8b49fc9 v0.11.19
87ff5d9 improve error message if incompabitle Identity with selected backend (#1852)
42f57b4 Fix panic in building h3 client when udp is forbidden (#1945)
4aa8516 msrv: bump to 1.63 (#1947)
1f6c2cf Add ClientBuilder::http1_allow_spaces_after_header_name_in_responses() (#1932)
6f07b9f Add ClientBuilder::http1_ignore_invalid_headers_in_responses() (#1926)
8396233 Fix deflate decompression (#1927)
b0c07a2 Bump rustls to v0.21.6 (#1928)
99bbae6 Bump webpki-roots to v0.25 (#1922)
61b1b2b Bump wasm-streams dependency to 0.3 (#1903)
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
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.

Client certificates seem to be ignored when using rustls
3 participants