Allow client ALPN protocols to be specified at connection level #2429
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2429 +/- ##
=======================================
Coverage 95.94% 95.94%
=======================================
Files 94 94
Lines 22597 22657 +60
=======================================
+ Hits 21680 21738 +58
- Misses 917 919 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rustls/src/client/client_conn.rs
Outdated
| ) -> Result<Self, Error> { | ||
| let mut common_state = CommonState::new(Side::Client); | ||
| common_state.set_max_fragment_size(config.max_fragment_size)?; | ||
| common_state.protocol = proto; | ||
| common_state.enable_secret_extraction = config.enable_secret_extraction; | ||
| common_state.fips = config.fips(); | ||
| let mut data = ClientConnectionData::new(); | ||
| let mut data = ClientConnectionData::new(alpn_protocols); |
There was a problem hiding this comment.
I suggest this data is instead passed into hs::start_handshake, so it can be deallocated as soon as possible rather than kept for the full life of the connection.
There was a problem hiding this comment.
@CodeMan62 Did I miss a detail, or is this feedback still unaddressed? I see alpn_protocols passed into the ClientConnectionData::new() call on L864 of client_conn.rs, in addition to the call to hs::start_handshake() on L873.
|
Hey @ctz, check the code changes in the last commit I think all the reviews you have given is resolved and let me know if any changes still required |
@CodeMan62 It looks like there are some build failures in CI to sort out. |
|
@cpu I think all the CI should be sorted now. Can you check it, or let me know if any changes are required |
It looks like the clippy job is still failing. |
|
@cpu clippy fixed! |
|
If at all possible, please squash your commits into a single one. |
I have not done that before but let me try |
ef61f14 to
708752c
Compare
|
@djc it's done now you can merge it :) |
Don't forget this feedback from Djc earlier too:
|
|
@cpu, I did the work, first please check it.If all is well, then I will squash my commits into a single commit |
|
Clippy fixed 👍 |
|
Hope everything fixed this time |
|
Since there are still artifacts here of LLM-assisted coding and you have been less than forthcoming in responding to my question about this, I'm closing this PR and resubmitting as #2438. I will retain you as a co-author on the resulting commit. |
Thanks for review I will work more on my skills |
This PR implements issue #2260 by allowing ALPN protocols to be specified at the connection level instead of only in ClientConfig. This enables clients to reuse a single config for connections that differ only in their ALPN protocols.
The implementation adds new_with_alpn() methods to both client connection types while maintaining backward compatibility. The handshake code prioritizes connection-level protocols over config-level protocols when provided.
I have run tests locally all are going well no fails in issues