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

Remove crate private identity_pem field from Config #771

Merged
merged 1 commit into from Dec 24, 2021

Conversation

kazk
Copy link
Member

@kazk kazk commented Dec 23, 2021

The concatenated identity_pem field 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.

I'm not sure why we're doing the following:

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

where accept_invalid_certs only respects the config when failing to load identity_pem and when insecure_skip_tls_verify is set true.

The following should be fine:

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

Also, the error from identity_pem() is ignored. I don't think load_client_certificate() and load_client_key() called in identity_pem() are defined correctly.

https://github.com/kube-rs/kube-rs/blob/52f69b9a89aa9e23605eb7f9c3ca673260e21853/kube-client/src/config/file_config.rs#L404-L414

These errors when the fields are not present in config, but that's valid and should return None in that case.

Ideally, we should validate the configured fields when creating a Config and error when it's configured with an invalid Base64 or path.

This PR removes identity_pem field with minimal changes.

Signed-off-by: kazk <kazk.dev@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #771 (88d190e) into master (52f69b9) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
+ Coverage   71.56%   71.64%   +0.08%     
==========================================
  Files          54       54              
  Lines        3569     3562       -7     
==========================================
- Hits         2554     2552       -2     
+ Misses       1015     1010       -5     
Impacted Files Coverage Δ
kube-client/src/config/file_loader.rs 82.60% <ø> (-2.01%) ⬇️
kube-client/src/client/config_ext.rs 70.00% <100.00%> (ø)
kube-client/src/config/file_config.rs 76.11% <100.00%> (+1.11%) ⬆️
kube-client/src/config/mod.rs 63.93% <100.00%> (+3.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52f69b9...88d190e. Read the comment docs.

proxy_url: loader.proxy_url()?,
auth_info: loader.user,
})
}

/// Client certificate and private key in PEM.
pub(crate) fn identity_pem(&self) -> Option<Vec<u8>> {
Copy link
Member Author

@kazk kazk Dec 23, 2021

Choose a reason for hiding this comment

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

This is only necessary at the moment because of the way the TLS functions work. The identity PEM is split into a certificate chain and a private key anyway, so I'm planning to change TLS functions to accept them separately.

Having a private key separate makes it easier to implement some workarounds like converting a private key format without having to parse and extract it first. For example, #542 can be fixed by converting to PKCS#8 and that can be done in pure Rust (topk8).

@clux
Copy link
Member

clux commented Dec 24, 2021

I'm not sure why we're doing the following:

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

where accept_invalid_certs only respects the config when failing to load identity_pem and when insecure_skip_tls_verify is set true.

The following should be fine:

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

Yep. I don't know why it is that way, it might have been me trying to be defensive 2 years back - even though it's ultimately more aggressive.

@kazk kazk merged commit b43600d into kube-rs:master Dec 24, 2021
@kazk kazk deleted the identity-pem branch December 24, 2021 19:53
@kazk kazk mentioned this pull request Dec 24, 2021
5 tasks
@kazk kazk added the changelog-change changelog change category for prs label Dec 24, 2021
@clux clux added this to the 0.66.0 milestone Jan 15, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants