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

Add Config::tls_server_name and validate when using rustls #1104

Merged
merged 8 commits into from
Dec 15, 2022
Merged

Conversation

clux
Copy link
Member

@clux clux commented Dec 13, 2022

Functionally equivalent to #1054 which it replaces due to lack of DCO. Main differences:

  • no defaulting of tls_server_name inside the ConfigExt (left to Config to set)
  • feature selection now respects tls stack precedence

Exposes Config::tls_server_name option

Gets it primarily from Kubeconfig when it exists and it's picked up by the ConfigExt method (only rustls does this).

In theory this can also be done for openssl, but with hyper-openssl is significantly more involved than rustls.
(AFAICT; You can configure the SslConnector, which lets you set the domain using into_ssl, but don't seem to have the same equivalent for the SslConnectorBuilder where we only have an some possible callbacks). This was ultimately not needed for to validate it with rustls where the problem is actually pressing.

Changes Config::incluster behaviour for rustls

Stops using the deprecated _dns variant and instead uses _env like openssl. This works once we start setting the server name (and we default to the kubernetes.default.svc url). Upstream PR #1054 has the explaination for why this is less strict than the current behaviour and should be better. Users have already reported the original PR fixing certain teleport cases on discord. Tested this with a couple of clusters myself using rustls and it seems to work.

Feature-based opt-in

The opt-in mechanism is done when only the rustls-tls feature is set. If/when rustls it gains the ability to validate IPs then we can remove the hack and have Config::incluster be the same across stacks. If both openssl and rustls are enabled then the hack is not added due to how we currently pick openssl as the tls stack in the Client.

@clux clux added the changelog-change changelog change category for prs label Dec 13, 2022
@clux clux added this to the 0.77.0 milestone Dec 13, 2022
@clux clux added rustls rustls-tls related config Kube config related labels Dec 13, 2022
@clux clux requested a review from kazk December 13, 2022 15:33
@codecov-commenter
Copy link

Codecov Report

Merging #1104 (8079682) into main (d28a715) will decrease coverage by 0.00%.
The diff coverage is 25.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1104      +/-   ##
==========================================
- Coverage   72.50%   72.50%   -0.01%     
==========================================
  Files          65       65              
  Lines        4845     4848       +3     
==========================================
+ Hits         3513     3515       +2     
- Misses       1332     1333       +1     
Impacted Files Coverage Δ
kube-client/src/client/builder.rs 70.90% <ø> (ø)
kube-client/src/client/config_ext.rs 48.07% <ø> (ø)
kube-client/src/config/file_config.rs 75.75% <ø> (ø)
kube-client/src/config/mod.rs 44.44% <25.00%> (-0.39%) ⬇️
kube-runtime/src/wait.rs 77.35% <0.00%> (+1.88%) ⬆️

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 config Kube config related rustls rustls-tls related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants