From 62e7b19f496adb45e481ac0cb45db3798b1760a2 Mon Sep 17 00:00:00 2001 From: meowjesty <43983236+meowjesty@users.noreply.github.com> Date: Mon, 21 Oct 2024 14:38:08 -0300 Subject: [PATCH] Better error for invalid certificate. (#2852) * Better error for invalid certificate. * docs * move error to cli * better error message and fn * add tests for InvalidCertificate. // Fix contains to use Debug. * docs * fix docs * Generate certificate in the test. * increase test timeout for outgoing_tcp_bound_socket * fix docs * changelog --- Cargo.lock | 4 + changelog.d/2824.changed.md | 1 + mirrord/cli/Cargo.toml | 5 + mirrord/cli/src/connection.rs | 15 +-- mirrord/cli/src/error.rs | 177 +++++++++++++++++++++++++++++--- mirrord/cli/src/main.rs | 23 ++--- mirrord/cli/src/operator.rs | 2 +- mirrord/cli/src/vpn.rs | 2 +- mirrord/kube/src/error.rs | 2 +- mirrord/layer/tests/outgoing.rs | 2 +- 10 files changed, 199 insertions(+), 34 deletions(-) create mode 100644 changelog.d/2824.changed.md diff --git a/Cargo.lock b/Cargo.lock index ccf1e61a384..11ef3af5ed3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3980,7 +3980,11 @@ dependencies = [ "drain", "exec", "futures", + "http-body 1.0.1", + "http-body-util", "humantime", + "hyper 1.5.0", + "hyper-util", "k8s-openapi", "kube", "local-ip-address", diff --git a/changelog.d/2824.changed.md b/changelog.d/2824.changed.md new file mode 100644 index 00000000000..470c0c0faf8 --- /dev/null +++ b/changelog.d/2824.changed.md @@ -0,0 +1 @@ +Improve error checking for InvalidCertificate errors in mirrord-cli. \ No newline at end of file diff --git a/mirrord/cli/Cargo.toml b/mirrord/cli/Cargo.toml index 47b0298b8d1..c72bfc6a6a5 100644 --- a/mirrord/cli/Cargo.toml +++ b/mirrord/cli/Cargo.toml @@ -80,3 +80,8 @@ mirrord-layer = { artifact = "cdylib", path = "../layer" } [dev-dependencies] rstest = "0.23" +hyper.workspace = true +hyper-util = { workspace = true, "features" = ["server"] } +http-body.workspace = true +http-body-util.workspace = true +kube.workspace = true diff --git a/mirrord/cli/src/connection.rs b/mirrord/cli/src/connection.rs index b310d346219..bbd29efa8b8 100644 --- a/mirrord/cli/src/connection.rs +++ b/mirrord/cli/src/connection.rs @@ -162,11 +162,14 @@ where let k8s_api = KubernetesAPI::create(config) .await - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateAgentFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateAgentFailed))?; - if let Err(error) = k8s_api.detect_openshift(progress).await { - tracing::debug!(?error, "Failed to detect OpenShift"); - }; + k8s_api + .detect_openshift(progress) + .await + .map_err(|fail| CliError::friendlier_error_or_else(fail, CliError::CreateAgentFailed)) + .inspect_err(|fail| tracing::debug!(?fail, "Failed to detect OpenShift!")) + .ok(); let agent_connect_info = tokio::time::timeout( Duration::from_secs(config.agent.startup_timeout), @@ -174,14 +177,14 @@ where ) .await .unwrap_or(Err(KubeApiError::AgentReadyTimeout)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateAgentFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateAgentFailed))?; let (sender, receiver) = wrap_raw_connection( k8s_api .create_connection(agent_connect_info.clone()) .await .map_err(|error| { - CliError::auth_exec_error_or(error, CliError::AgentConnectionFailed) + CliError::friendlier_error_or_else(error, CliError::AgentConnectionFailed) })?, ); diff --git a/mirrord/cli/src/error.rs b/mirrord/cli/src/error.rs index 176b3afceb2..99c2996a36b 100644 --- a/mirrord/cli/src/error.rs +++ b/mirrord/cli/src/error.rs @@ -165,8 +165,8 @@ pub(crate) enum OperatorSetupError { #[derive(Debug, Error, Diagnostic)] pub(crate) enum CliError { - /// Do not construct this variant directly, use [`CliError::auth_exec_error_or`] to allow for - /// more granular error detection. + /// Do not construct this variant directly, use [`CliError::friendlier_error_or_else`] to allow + /// for more granular error detection. #[error("Failed to create Kubernetes API client: {0}")] #[diagnostic(help("Please check that Kubernetes is configured correctly and test your connection with `kubectl get pods`.{GENERAL_HELP}"))] CreateKubeApiFailed(KubeApiError), @@ -175,8 +175,8 @@ pub(crate) enum CliError { #[diagnostic(help("Please check that Kubernetes is configured correctly and test your connection with `kubectl get pods`.{GENERAL_HELP}"))] ListTargetsFailed(KubeApiError), - /// Do not construct this variant directly, use [`CliError::auth_exec_error_or`] to allow for - /// more granular error detection. + /// Do not construct this variant directly, use [`CliError::friendlier_error_or_else`] to allow + /// for more granular error detection. #[error("Failed to create mirrord-agent: {0}")] #[diagnostic(help( r"1. Please check the status of the agent pod, using `kubectl get pods` in the relevant namespace. @@ -185,8 +185,8 @@ pub(crate) enum CliError { ))] CreateAgentFailed(KubeApiError), - /// Do not construct this variant directly, use [`CliError::auth_exec_error_or`] to allow for - /// more granular error detection. + /// Do not construct this variant directly, use [`CliError::friendlier_error_or_else`] to allow + /// for more granular error detection. #[error("Failed to connect to the created mirrord-agent: {0}")] #[diagnostic(help( "Please check the following: @@ -195,6 +195,15 @@ pub(crate) enum CliError { ))] AgentConnectionFailed(KubeApiError), + /// Friendlier version of the invalid certificate error that comes from a + /// [`kube::Error::Service`]. + #[error("Kube API operation failed due to missing or invalid certificate: {0}")] + #[diagnostic(help( + "Consider enabling `accept_invalid_certificates` in your \ + `mirrord.json`, or running `mirrord exec` with the `-c` flag." + ))] + InvalidCertificate(KubeApiError), + #[error("Failed to communicate with the agent: {0}")] #[diagnostic(help("Please check agent status and logs.{GENERAL_HELP}"))] InitialAgentCommFailed(String), @@ -397,10 +406,12 @@ pub(crate) enum CliError { } impl CliError { - /// If the given [`KubeApiError`] originates from failed authentication command exec, produces - /// [`CliError::KubeAuthExecFailed`]. Otherwise, uses the given `fallback` function to - /// produce the result. - pub fn auth_exec_error_or Self>( + /// Here we give more meaning to some errors, instead of just letting them pass as + /// whatever [`KubeApiError`] we're getting. + /// + /// If `error` is not something we're interested in (no need for a special diagnostic message), + /// then we turn it into a `fallback` [`CliError`]. + pub fn friendlier_error_or_else Self>( error: KubeApiError, fallback: F, ) -> Self { @@ -408,7 +419,14 @@ impl CliError { match error { KubeApiError::KubeError(Error::Auth(AuthError::AuthExec(error))) => { - Self::KubeAuthExecFailed(error) + Self::KubeAuthExecFailed(error.to_owned()) + } + // UGH(alex): Type-erased errors are messy, and this one is especially bad. + // See `kube_service_error_dependency_is_in_sync` for a "what's going on here". + KubeApiError::KubeError(Error::Service(ref fail)) + if format!("{fail:?}").contains("InvalidCertificate") => + { + Self::InvalidCertificate(error) } error => fallback(error), } @@ -428,7 +446,7 @@ impl From for CliError { operator_version, }, OperatorApiError::CreateKubeClient(e) => { - Self::auth_exec_error_or(e, Self::CreateKubeApiFailed) + Self::friendlier_error_or_else(e, Self::CreateKubeApiFailed) } OperatorApiError::ConnectRequestBuildError(e) => Self::ConnectRequestBuildError(e), OperatorApiError::KubeError { @@ -470,3 +488,138 @@ impl From for CliError { #[derive(Debug, Error)] #[error("unsupported runtime version")] pub struct UnsupportedRuntimeVariant; + +#[cfg(test)] +mod tests { + use std::{ + net::{Ipv4Addr, SocketAddr}, + sync::Arc, + }; + + use http_body_util::Full; + use hyper::{ + body::{Bytes, Incoming}, + service::service_fn, + Request, Response, + }; + use hyper_util::{ + rt::{TokioExecutor, TokioIo}, + server::conn::auto::Builder, + }; + use k8s_openapi::api::core::v1::Pod; + use kube::{api::ListParams, Api}; + use rustls::{ + crypto::aws_lc_rs::default_provider, + pki_types::{pem::PemObject, CertificateDer, PrivateKeyDer}, + ServerConfig, + }; + use tokio::{net::TcpListener, sync::Notify}; + use tokio_rustls::TlsAcceptor; + + /// With this test we're trying to `assert` that our [`kube`] crate is (somewhat) + /// version-synced with [`rustls`]. To give a friendlier error message on kube requests + /// when there's a certificate problem, we must dig down into the [`kube::Error`]. + /// + /// Certificate errors come under the [`kube::Error::Service`] variant, which holds + /// a very annoying type-erased boxed error. Trying to `downcast` this is messy, so + /// instead we check the debug message of the error. + /// + /// We want this error (or something similar) to happen: + /// + /// `Service(hyper_util::client::legacy::Error(Connect, Custom { kind: Other, error: Custom { + /// kind: InvalidData, error: InvalidCertificate(Expired) } }))`. + /// + /// The part that we care about is the `InvalidCertificate`, the rest is not relevant. + /// + /// This test may fail if [`rustls`] changes the `error: InvalidCertificate` message, + /// or any of the upper errors change. + /// + /// Relying on the `Debug` string version of the error, as the `Display` version is + /// just a generic `ServiceError: client error (Connect)`. + #[tokio::test] + async fn kube_service_error_dependency_is_in_sync() { + use kube::{Client, Config}; + + let provider = default_provider(); + provider.install_default().unwrap(); + + let notify = Arc::new(Notify::new()); + let wait_notify = notify.clone(); + + tokio::spawn(async move { + run_hyper_tls_server(notify).await; + }); + + // Wait until the server is listening before we start connecting. + wait_notify.notified().await; + + let kube_config = Config::new("https://127.0.0.1:9669".parse().unwrap()); + let client = Client::try_from(kube_config).unwrap(); + + // Manage pods + let pods: Api = Api::default_namespaced(client); + + let lp = ListParams::default().fields(&format!("legend={}", "hank")); + + let list = pods.list(&lp).await; + assert!( + list.as_ref() + .is_err_and(|fail| { format!("{fail:?}").contains("InvalidCertificate") }), + "We were expecting an error with `InvalidCertificate`, but got {list:?}!" + ); + } + + /// Creates an [`hyper`] server with a broken certificate and a _catch-all_ route on + /// `localhost:9669`. + /// + /// The certificate is generated here with [`rcgen::generate_simple_self_signed`]. + async fn run_hyper_tls_server(notify: Arc) { + use rcgen::generate_simple_self_signed; + + let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 9669); + + let incoming = TcpListener::bind(&addr).await.unwrap(); + + // Generate a certificate that should not work with kube. + let cert_key = generate_simple_self_signed(vec!["mieszko.i".to_string()]).unwrap(); + let cert_pem = cert_key.cert.pem().into_bytes(); + let cert = CertificateDer::from_pem_slice(&cert_pem).unwrap(); + + let key_pem = cert_key.key_pair.serialize_pem().into_bytes(); + let key = PrivateKeyDer::from_pem_slice(&key_pem).unwrap(); + + // Build TLS configuration. + let mut server_config = ServerConfig::builder() + .with_no_client_auth() + .with_single_cert(vec![cert], key) + .unwrap(); + + server_config.alpn_protocols = + vec![b"h2".to_vec(), b"http/1.1".to_vec(), b"http/1.0".to_vec()]; + let tls_acceptor = TlsAcceptor::from(Arc::new(server_config)); + + /// Catch-all handler, we don't care about the response, any request is supposed + /// to fail due to `InvalidCertificate`. + async fn handle_any_route( + _: Request, + ) -> Result>, hyper::Error> { + Ok(Response::new(Full::default())) + } + + let service = service_fn(handle_any_route); + + // We're ready for the client side to start. + notify.notify_waiters(); + let (tcp_stream, _remote_addr) = incoming.accept().await.unwrap(); + + let tls_acceptor = tls_acceptor.clone(); + tokio::spawn(async move { + let tls_stream = tls_acceptor.accept(tcp_stream).await.unwrap(); + + Builder::new(TokioExecutor::new()) + .serve_connection(TokioIo::new(tls_stream), service) + .await + .unwrap(); + }); + } +} diff --git a/mirrord/cli/src/main.rs b/mirrord/cli/src/main.rs index de645b1f824..fcae81024af 100644 --- a/mirrord/cli/src/main.rs +++ b/mirrord/cli/src/main.rs @@ -388,7 +388,7 @@ async fn list_targets(layer_config: &LayerConfig, args: &ListTargetArgs) -> Resu ) .await .and_then(|config| Client::try_from(config).map_err(From::from)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateKubeApiFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateKubeApiFailed))?; let namespace = args .namespace @@ -422,15 +422,13 @@ async fn list_targets(layer_config: &LayerConfig, args: &ListTargetArgs) -> Resu if ALL_TARGETS_SUPPORTED_OPERATOR_VERSION .matches(&api.operator().spec.operator_version) => { - seeker - .all() - .await - .map_err(|error| CliError::auth_exec_error_or(error, CliError::ListTargetsFailed)) + seeker.all().await.map_err(|error| { + CliError::friendlier_error_or_else(error, CliError::ListTargetsFailed) + }) } - _ => seeker - .all_open_source() - .await - .map_err(|error| CliError::auth_exec_error_or(error, CliError::ListTargetsFailed)), + _ => seeker.all_open_source().await.map_err(|error| { + CliError::friendlier_error_or_else(error, CliError::ListTargetsFailed) + }), } } @@ -593,9 +591,10 @@ async fn port_forward(args: &PortForwardArgs, watch: drain::Watch) -> Result<()> .map_err(|agent_con_error| match agent_con_error { AgentConnectionError::Io(error) => CliError::PortForwardingSetupError(error.into()), AgentConnectionError::Operator(operator_api_error) => operator_api_error.into(), - AgentConnectionError::Kube(kube_api_error) => { - CliError::auth_exec_error_or(kube_api_error, CliError::PortForwardingSetupError) - } + AgentConnectionError::Kube(kube_api_error) => CliError::friendlier_error_or_else( + kube_api_error, + CliError::PortForwardingSetupError, + ), AgentConnectionError::Tls(connection_tls_error) => connection_tls_error.into(), AgentConnectionError::NoConnectionMethod => CliError::PortForwardingNoConnectionMethod, })?; diff --git a/mirrord/cli/src/operator.rs b/mirrord/cli/src/operator.rs index bdc1d8e3ff3..840355fa418 100644 --- a/mirrord/cli/src/operator.rs +++ b/mirrord/cli/src/operator.rs @@ -142,7 +142,7 @@ async fn get_status_api(config: Option<&Path>) -> Result ) .await .and_then(|config| Client::try_from(config).map_err(From::from)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateKubeApiFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateKubeApiFailed))?; Ok(Api::all(client)) } diff --git a/mirrord/cli/src/vpn.rs b/mirrord/cli/src/vpn.rs index f2768ee9efa..a6734e0824c 100644 --- a/mirrord/cli/src/vpn.rs +++ b/mirrord/cli/src/vpn.rs @@ -36,7 +36,7 @@ pub async fn vpn_command(args: VpnArgs) -> Result<()> { ) .await .and_then(|config| kube::Client::try_from(config).map_err(From::from)) - .map_err(|error| CliError::auth_exec_error_or(error, CliError::CreateKubeApiFailed))?; + .map_err(|error| CliError::friendlier_error_or_else(error, CliError::CreateKubeApiFailed))?; let mut sub_progress = progress.subtask("fetching vpn info"); diff --git a/mirrord/kube/src/error.rs b/mirrord/kube/src/error.rs index ce898f0501f..df816ab8565 100644 --- a/mirrord/kube/src/error.rs +++ b/mirrord/kube/src/error.rs @@ -7,7 +7,7 @@ pub type Result = std::result::Result; #[derive(Debug, Error)] pub enum KubeApiError { - #[error("Kube failed: {0}")] + #[error(transparent)] KubeError(#[from] kube::Error), #[error("Connection to agent failed: {0}")] diff --git a/mirrord/layer/tests/outgoing.rs b/mirrord/layer/tests/outgoing.rs index 355d4266f2f..6366552576d 100644 --- a/mirrord/layer/tests/outgoing.rs +++ b/mirrord/layer/tests/outgoing.rs @@ -187,7 +187,7 @@ async fn outgoing_tcp_from_the_local_app_broken( /// application. #[rstest] #[tokio::test] -#[timeout(Duration::from_secs(10))] +#[timeout(Duration::from_secs(15))] async fn outgoing_tcp_bound_socket(dylib_path: &Path) { let (mut test_process, mut intproxy) = Application::RustIssue2438 .start_process_with_layer(dylib_path, vec![], None)