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
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions kube-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ rust-version = "1.60.0"
edition = "2021"

[features]
default = ["client", "openssl-tls"]
clux marked this conversation as resolved.
Show resolved Hide resolved
default = ["client", "rustls-tls"]
clux marked this conversation as resolved.
Show resolved Hide resolved
rustls-tls = ["rustls", "rustls-pemfile", "hyper-rustls"]
openssl-tls = ["openssl", "hyper-openssl"]
ws = ["client", "tokio-tungstenite", "rand", "kube-core/ws"]
Expand Down Expand Up @@ -57,7 +57,7 @@ kube-core = { path = "../kube-core", version = "=0.75.0" }
jsonpath_lib = { version = "0.3.0", optional = true }
tokio-util = { version = "0.7.0", optional = true, features = ["io", "codec"] }
hyper = { version = "0.14.13", optional = true, features = ["client", "http1", "stream", "tcp"] }
hyper-rustls = { version = "0.23.0", optional = true }
hyper-rustls = { git = "https://github.com/rustls/hyper-rustls", version = "0.23.0", optional = true }
MikailBag marked this conversation as resolved.
Show resolved Hide resolved
tokio-tungstenite = { version = "0.17.1", optional = true }
tower = { version = "0.4.6", optional = true, features = ["buffer", "filter", "util"] }
tower-http = { version = "0.3.2", optional = true, features = ["auth", "map-response-body", "trace"] }
Expand Down
11 changes: 9 additions & 2 deletions kube-client/src/client/config_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,17 @@ impl ConfigExt for Config {

#[cfg(feature = "rustls-tls")]
fn rustls_https_connector(&self) -> Result<hyper_rustls::HttpsConnector<hyper::client::HttpConnector>> {
let rustls_config = std::sync::Arc::new(self.rustls_client_config()?);
let rustls_config = self.rustls_client_config()?;
clux marked this conversation as resolved.
Show resolved Hide resolved
let mut http = hyper::client::HttpConnector::new();
http.enforce_http(false);
Ok(hyper_rustls::HttpsConnector::from((http, rustls_config)))
let mut b = hyper_rustls::HttpsConnectorBuilder::new()
.with_tls_config(rustls_config)
.https_or_http();
if let Some(tsn) = self.tls_server_name.as_ref() {
b = b.with_server_name(tsn.clone());
}

Ok(b.enable_http1().wrap_connector(http))
clux marked this conversation as resolved.
Show resolved Hide resolved
}

#[cfg(feature = "openssl-tls")]
Expand Down
5 changes: 5 additions & 0 deletions kube-client/src/config/file_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ pub struct Cluster {
#[serde(rename = "proxy-url")]
#[serde(skip_serializing_if = "Option::is_none")]
pub proxy_url: Option<String>,
/// If set, apiserver certificate will be validated to contain this string
/// (instead of hostname or IP address from the url).
clux marked this conversation as resolved.
Show resolved Hide resolved
#[serde(rename = "tls-server-name")]
#[serde(skip_serializing_if = "Option::is_none")]
pub tls_server_name: Option<String>,
/// Additional information for extenders so that reads and writes don't clobber unknown fields
#[serde(skip_serializing_if = "Option::is_none")]
pub extensions: Option<Vec<NamedExtension>>,
Expand Down
12 changes: 10 additions & 2 deletions kube-client/src/config/file_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ pub struct KubeConfigOptions {
pub cluster: Option<String>,
/// The user to load
pub user: Option<String>,
/// Disable rustls-specific override for `tls-server-name`
pub disable_rustls_override: bool
clux marked this conversation as resolved.
Show resolved Hide resolved
}

/// ConfigLoader loads current context, cluster, and authentication information
Expand Down Expand Up @@ -74,12 +76,13 @@ impl ConfigLoader {
.ok_or_else(|| KubeconfigError::LoadContext(context_name.clone()))?;

let cluster_name = cluster.unwrap_or(&current_context.cluster);
let cluster = config
let mut cluster = config
.clusters
.iter()
.find(|named_cluster| &named_cluster.name == cluster_name)
.map(|named_cluster| &named_cluster.cluster)
.ok_or_else(|| KubeconfigError::LoadClusterOfContext(cluster_name.clone()))?;
.ok_or_else(|| KubeconfigError::LoadClusterOfContext(cluster_name.clone()))?
.clone();

let user_name = user.unwrap_or(&current_context.user);
let user = config
Expand All @@ -89,6 +92,11 @@ impl ConfigLoader {
.map(|named_user| &named_user.auth_info)
.ok_or_else(|| KubeconfigError::FindUser(user_name.clone()))?;

// TODO: docs
if cfg!(feature = "rustls-tls") && true && cluster.tls_server_name.is_none() {
cluster.tls_server_name = Some("kubernetes.default.svc".to_string());
clux marked this conversation as resolved.
Show resolved Hide resolved
}

Ok(ConfigLoader {
current_context: current_context.clone(),
cluster: cluster.clone(),
Expand Down
30 changes: 24 additions & 6 deletions kube-client/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ pub struct Config {
// TODO Actually support proxy or create an example with custom client
/// Optional proxy URL.
pub proxy_url: Option<http::Uri>,
/// If set, apiserver certificate will be validated to contain this string
/// (instead of hostname or IP address from the url).
Comment on lines +162 to +163
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.

pub tls_server_name: Option<String>,
}

impl Config {
Expand All @@ -176,6 +179,7 @@ impl Config {
accept_invalid_certs: false,
auth_info: AuthInfo::default(),
proxy_url: None,
tls_server_name: None,
}
}

Expand All @@ -188,6 +192,13 @@ impl Config {
///
/// [`Config::apply_debug_overrides`] is used to augment the loaded
/// configuration based on the environment.
/// # Rustls-specific behavior
/// Rustls does not support validating IP addresses (see
/// <https://github.com/kube-rs/kube/issues/1003>).
/// To work around this, when rustls is configured, this function automatically appends
/// `tls-server-name = "kubernetes.default.svc"` to the resulting configuration.
/// To opt out of this behavior, use lower-level methods [`Config::from_kubeconfig`] with customized options,
/// [`Config::incluster_dns`] and [`Config::incluster_env`].
pub async fn infer() -> Result<Self, InferConfigError> {
let mut config = match Self::from_kubeconfig(&KubeConfigOptions::default()).await {
Err(kubeconfig_err) => {
Expand Down Expand Up @@ -215,14 +226,19 @@ impl Config {
}

/// Load an in-cluster Kubernetes client configuration using
/// [`Config::incluster_dns`].
///
/// The `rustls-tls` feature is currently incompatible with
/// [`Config::incluster_env`]. See
/// <https://github.com/kube-rs/kube/issues/1003>.
/// [`Config::incluster_env`].
/// # Rustls-specific behavior
/// Rustls does not support validating IP addresses (see
/// <https://github.com/kube-rs/kube/issues/1003>).
/// To work around this, when rustls is configured, this function automatically appends
/// `tls-server-name = "kubernetes.default.svc"` to the resulting configuration.
/// To opt out of this behavior, use [`Config::incluster_env`] or
/// [`Config::incluster_dns`] instead.
#[cfg(feature = "rustls-tls")]
pub fn incluster() -> Result<Self, InClusterError> {
Self::incluster_dns()
let mut cfg = Self::incluster_env()?;
cfg.tls_server_name = Some("kubernetes.default.svc".to_string());
Ok(cfg)
}

/// Load an in-cluster config using the `KUBERNETES_SERVICE_HOST` and
Expand Down Expand Up @@ -271,6 +287,7 @@ impl Config {
..Default::default()
},
proxy_url: None,
tls_server_name: None,
})
}

Expand Down Expand Up @@ -327,6 +344,7 @@ impl Config {
accept_invalid_certs,
proxy_url: loader.proxy_url()?,
auth_info: loader.user,
tls_server_name: loader.cluster.tls_server_name
})
}

Expand Down