Use SecretString in AuthInfo to avoid credential leaking

[ChinYing-Li]

Motivation

This PR aims to solve #751.

Solution

the secrecy crate is used, and AuthInfo.password is now of type Option<SecretString>. Are there other structs/fields that would require secrets?
Since SecretString has no PartialEq implementation, there's a custom impl PartialEq for AuthInfo, which is a hack that compares serialized structs. Please let me know if such a hack is not acceptable.
Any suggestion is appreciated!

[ChinYing-Li]

Seems like the CI is having connection issues; would be great if someone can kick the CI again.

[kazk]

Thanks for doing this!

I think token field should be Option<SecretString> as well.

There's also client_key_data, but we might want to clean up around identity_pem first in a separate PR.

[ChinYing-Li]

@kazk Seems like no one else is working on refactoring identity_pem, perhaps I can give it a try in a separate PR?

[kazk]

@ChinYing-Li Thanks, but I've been looking into it already (originally started looking into it because I have an idea for #542 and working with concatenated identity_pem is not ideal), and planning to work on it after #768.

As far as I can tell, the concatenated identity_pem shouldn't be necessary. At least, it shouldn't be necessary to be stored in Config because the client certificates and the private key are in config.auth_info:

https://github.com/kube-rs/kube-rs/blob/52f69b9a89aa9e23605eb7f9c3ca673260e21853/kube-client/src/config/file_loader.rs#L99-L105

But we'll need to look into why we're doing the following:

https://github.com/kube-rs/kube-rs/blob/52f69b9a89aa9e23605eb7f9c3ca673260e21853/kube-client/src/config/mod.rs#L269-L278

to set accept_invalid_certs only when loading identity_pem fails.

If there's no problem with doing

accept_invalid_certs = loader.cluster.insecure_skip_tls_verify.unwrap_or(false);

then, I think we can just get rid of identity_pem and rewrite TLS methods to accept the cert chain and the private key separately.

[kazk]

Opened #771 that removes identity_pem with minimal changes

[kazk]

@ChinYing-Li #771 is merged, so you can do something like the following after rebasing:

diff --git i/kube-client/src/config/file_config.rs w/kube-client/src/config/file_config.rs
index a824699..4e16f90 100644
--- i/kube-client/src/config/file_config.rs
+++ w/kube-client/src/config/file_config.rs
@@ -181,7 +181,11 @@ pub struct AuthInfo {
     /// PEM-encoded data from a client key file for TLS. Overrides `client_key`
     #[serde(rename = "client-key-data")]
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub client_key_data: Option<String>,
+    #[serde(
+        serialize_with = "serialize_secretstring",
+        deserialize_with = "deserialize_secretstring"
+    )]
+    pub client_key_data: Option<SecretString>,
 
     /// The username to act-as.
     #[serde(rename = "as")]
@@ -430,8 +434,11 @@ impl Cluster {
             return Ok(None);
         }
 
-        let ca = load_from_base64_or_file(&self.certificate_authority_data, &self.certificate_authority)
-            .map_err(KubeconfigError::LoadCertificateAuthority)?;
+        let ca = load_from_base64_or_file(
+            &self.certificate_authority_data.as_deref(),
+            &self.certificate_authority,
+        )
+        .map_err(KubeconfigError::LoadCertificateAuthority)?;
         Ok(Some(ca))
     }
 }
@@ -448,24 +455,26 @@ impl AuthInfo {
     pub(crate) fn load_client_certificate(&self) -> Result<Vec<u8>, KubeconfigError> {
         // TODO Shouldn't error when `self.client_certificate_data.is_none() && self.client_certificate.is_none()`
 
-        load_from_base64_or_file(&self.client_certificate_data, &self.client_certificate)
+        load_from_base64_or_file(&self.client_certificate_data.as_deref(), &self.client_certificate)
             .map_err(KubeconfigError::LoadClientCertificate)
     }
 
     pub(crate) fn load_client_key(&self) -> Result<Vec<u8>, KubeconfigError> {
         // TODO Shouldn't error when `self.client_key_data.is_none() && self.client_key.is_none()`
 
-        load_from_base64_or_file(&self.client_key_data, &self.client_key)
-            .map_err(KubeconfigError::LoadClientKey)
+        load_from_base64_or_file(
+            &self.client_key_data.as_ref().map(|s| s.expose_secret().as_str()),
+            &self.client_key,
+        )
+        .map_err(KubeconfigError::LoadClientKey)
     }
 }
 
 fn load_from_base64_or_file<P: AsRef<Path>>(
-    value: &Option<String>,
+    value: &Option<&str>,
     file: &Option<P>,
 ) -> Result<Vec<u8>, LoadDataError> {
     let data = value
-        .as_deref()
         .map(load_from_base64)
         .or_else(|| file.as_ref().map(load_from_file))
         .unwrap_or(Err(LoadDataError::NoBase64DataOrFile))?;

[ChinYing-Li]

I have updated the PR, so now client_key_data is also `Option. PR ready for review!

[kazk]

Looks good to me. Just a few minor nits.

[kazk]

Going to merge this to unblock #768. Thanks again.