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

Support tls-server-name for rustls #1054

Closed
wants to merge 7 commits into from

Conversation

MikailBag
Copy link
Contributor

Motivation

Note that this PR currently is in RFC state. It depends on not-yet-released hyper-rustls version, and can be splitted into several PRs. Also we may want to make new workaround opt-in instead of opt-out and so on.

Partially solves #991 ("partially" because the same thing should be repeated for openssl).

Should improve #1003, as with new workaround kube-rs+rustls will work for every control plane (both in-cluster and outside) as long as apiserver certificate contains kubernetes.default.svc SAN. This is less strict assumption that incluster_dns does (i.e. we no longer assume that kubernetes.default.svc actually resolves to the apiserver IP), and I expect it to hold for the most control plane architectures. And as long as apiserver has at least one DNS SAN entry, it will be possible to connect to such a cluster with manual tls-server-name modification.

Solution

This PR does following partially independent things:

  1. Add support for tls-server-name in config loading.
  2. Pass tls-server-name to rustls if enabled.
  3. Set tls-server-name to kubernetes.default-svc if rustls is in use.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #1054 (3ea6e1c) into main (d1cc635) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1054      +/-   ##
==========================================
- Coverage   72.47%   72.46%   -0.01%     
==========================================
  Files          65       65              
  Lines        4847     4849       +2     
==========================================
+ Hits         3513     3514       +1     
- Misses       1334     1335       +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.94% <50.00%> (+0.11%) ⬆️

@clux clux added the changelog-add changelog added category for prs label Oct 27, 2022
@clux clux added this to the 0.77.0 milestone Oct 27, 2022
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

initial comments, but i like the general approach!

have you tested this with teleport (from the original bug report) or something else where this results in an improvement?

kube-client/Cargo.toml Outdated Show resolved Hide resolved
kube-client/src/client/config_ext.rs Show resolved Hide resolved
kube-client/src/config/file_config.rs Outdated Show resolved Hide resolved
kube-client/src/config/file_loader.rs Outdated Show resolved Hide resolved
kube-client/src/config/file_loader.rs Outdated Show resolved Hide resolved
kube-client/src/client/config_ext.rs Outdated Show resolved Hide resolved
@nightkr nightkr added client http issues with the client rustls rustls-tls related labels Oct 31, 2022
Comment on lines +158 to +159
/// If set, apiserver certificate will be validated to contain this string
/// (instead of hostname or IP address from the url).
Copy link
Member

Choose a reason for hiding this comment

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

So the Config option exposed here is now technically only used to signal whether to opt-in to extra rustls behaviour. If i understand correctly it might be worth either:

  • pass the tls_server_name into whatever is needed on the openssl side
  • raise an issue to support tls_server_name on openssl (and reference it here)

Do you think that makes sense? I can imagine this property being useful on both tls stacks (outside of the rustls hack), but afaikt it also isn't really needed except to grant more security/strictness in the certificate validation stage.

Copy link
Member

Choose a reason for hiding this comment

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

i'll raise an issue about supporting tls_server_name after merge so we remember.

@clux
Copy link
Member

clux commented Nov 20, 2022

I think this is very good now 👍

Have left one doc/issue follow-up suggestion for the property (given it's only actually half-supported), and we should at least document that somewhere. Other than that, if it's updated, and DCO'd etc I'm very happy with this!

@clux
Copy link
Member

clux commented Nov 29, 2022

Hey @MikailBag , you ok with rebasing this with --signoff to fix DCO? PR ultimately looks good and would like to merge this :-)

kube-client/Cargo.toml Outdated Show resolved Hide resolved
kube-client/Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Eirik A <sszynrae@gmail.com>
@clux
Copy link
Member

clux commented Dec 12, 2022

@MikailBag sorry to keep pinging you, but this should just be missing DCO check, are you able to sync and rebase according to the check so we can get this into the release?

@clux
Copy link
Member

clux commented Dec 13, 2022

Going to redo this pr to get some movement on a 0.77 release.

@clux
Copy link
Member

clux commented Dec 13, 2022

Replaced by #1104

@clux clux closed this Dec 13, 2022
@clux clux removed this from the 0.77.0 milestone Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs client http issues with the client rustls rustls-tls related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants