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

Change default TLS stack to rustls-tls #1261

Merged
merged 10 commits into from
Sep 8, 2023
Merged

Change default TLS stack to rustls-tls #1261

merged 10 commits into from
Sep 8, 2023

Conversation

clux
Copy link
Member

@clux clux commented Jul 20, 2023

Motivation

Want to encourage the use of the more secure and robust TLS stack by default.
Based on feedback (see e.g. #1192) and personal experience have not seen or heard about any current issues.
The main blocker bug was resolved some months ago
This also allows us to not have a huge windows hack in our windows CI.

Solution

  • switch our explicit default to feature to kube/rustls-tls
  • switch our precedence when both features are enabled to rustls-tls
  • make windows run tests in the same way as other platforms

As a clean-up while in the area, have also prevented erroneous Client construction on a TLS requiring cluster, when TLS stacks are disabled. That caused an ugly freezen in #1275

clux added 2 commits July 20, 2023 23:10
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #1261 (90040bd) into main (259ed96) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1261      +/-   ##
==========================================
- Coverage   72.44%   72.30%   -0.15%     
==========================================
  Files          75       75              
  Lines        6337     6347      +10     
==========================================
- Hits         4591     4589       -2     
- Misses       1746     1758      +12     
Files Changed Coverage Δ
kube-client/src/client/auth/oauth.rs 0.00% <ø> (ø)
kube-client/src/client/auth/oidc.rs 57.24% <ø> (ø)
kube-client/src/client/builder.rs 71.42% <100.00%> (+0.51%) ⬆️

... and 4 files with indirect coverage changes

📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kube-rs).

@clux clux marked this pull request as ready for review July 20, 2023 22:39
Copy link
Contributor

@aviramha aviramha left a comment

Choose a reason for hiding this comment

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

Exciting!!
Need to update the README.md

By default openssl is used for TLS, but rustls is supported. To switch, turn off default-features, and enable the rustls-tls feature:

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: Eirik A <sszynrae@gmail.com>
…esent

fixes #1275

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue Aug 8, 2023 that may be closed by this pull request
@clux clux added this to the 0.86.0 milestone Aug 8, 2023
@clux clux requested review from Dav1dde, kazk and nightkr August 8, 2023 13:25
@clux
Copy link
Member Author

clux commented Sep 7, 2023

Getting ready to push this in as it is getting close to release time.

Some quick testing on no-tls feature against tls clusters now gives:

WARN kube_client::client: Unsuccessful data error parse: Client sent an HTTP request to an HTTPS server

maybe that should be promoted to a harder error also, ill look later

EDIT: ok this doesn't happen in all cases. on my k3d where auth layer is set we properly get a hard error:

Error: TLS required but no TLS stack selected

this is testable with turning off rustls + refresh feature in examples/Cargo.toml (but keeping client)

but am seeing it on a rancher cluster where the auth is just user.token: some-kubeconfig-uid. in this case we can detect it with cluster.server url being https scheme. EDIT2: fixed.

@clux clux merged commit 3d68ec1 into main Sep 8, 2023
17 checks passed
@clux clux deleted the new-default-features branch September 8, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
3 participants