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

Check for client cert with exec #1089

Merged
merged 6 commits into from Nov 29, 2022
Merged

Conversation

rcanderson23
Copy link
Contributor

Signed-off-by: Carson Anderson rcanderson23@gmail.com

Motivation

Correctly handle client certificates returned with kubeconfig configurations using exec. Current implementation only handles tokens

Solution

This adds Certificate to the Auth enum. The auth is checked when creating the respective TLS configs and uses the certificate if available.

Signed-off-by: Carson Anderson <rcanderson23@gmail.com>
Copy link
Member

@kazk kazk left a comment

Choose a reason for hiding this comment

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

I'm still not sure if this is the right approach, but left some comments.

kube-client/src/client/config_ext.rs Outdated Show resolved Hide resolved
kube-client/src/client/config_ext.rs Outdated Show resolved Hide resolved
kube-client/src/client/config_ext.rs Outdated Show resolved Hide resolved
kube-client/src/client/config_ext.rs Outdated Show resolved Hide resolved
@kazk kazk linked an issue Nov 22, 2022 that may be closed by this pull request
@clux clux added the changelog-add changelog added category for prs label Nov 22, 2022
@clux clux added this to the 0.77.0 milestone Nov 22, 2022
Signed-off-by: Carson Anderson <rcanderson23@gmail.com>
@rcanderson23
Copy link
Contributor Author

Thank you for the feedback. If you have other suggestions on you think this should be implemented I am all ears. 😃

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022

Codecov Report

Merging #1089 (079f460) into main (61f2f32) will decrease coverage by 0.06%.
The diff coverage is 41.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
- Coverage   72.43%   72.37%   -0.07%     
==========================================
  Files          65       65              
  Lines        4759     4774      +15     
==========================================
+ Hits         3447     3455       +8     
- Misses       1312     1319       +7     
Impacted Files Coverage Δ
kube-client/src/client/auth/mod.rs 56.38% <0.00%> (+0.33%) ⬆️
kube-client/src/client/config_ext.rs 50.00% <50.00%> (-1.22%) ⬇️

Signed-off-by: Carson Anderson <rcanderson23@gmail.com>
Signed-off-by: Carson Anderson <rcanderson23@gmail.com>
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.

I think everything in here has been addressed. A branch update + a just fmt would be appreciated, and I am happy to send it through.

Signed-off-by: Carson Anderson <rcanderson23@gmail.com>
@clux clux merged commit 593fa89 into kube-rs:main Nov 29, 2022
@clux
Copy link
Member

clux commented Nov 29, 2022

Thanks again for figuring this out!

@goenning
Copy link
Contributor

goenning commented Dec 3, 2022

I was about to start working on this, until I synced my fork and noticed this PR was recently merged. Thank you 🎉


@rcanderson23 @clux While testing it, I first got this error:

    1: failed to deserialize PEM-encoded chain of certificates: error:0908F066:PEM routines:get_header_and_data:bad end line:crypto/pem/pem_lib.c:856:

It worked on kubectl, but not on kube-rs. After some digging I noticed that PEM-encoded was missing a \n at the end, which seems to be what OpenSSL expects. Apparently Go is a bit more flexible with this and it's ok to not end with a new line. What you think of adding something into kube-rs to handle this?

EDIT: it is a small change so I decided to create a PR to discuss this separately

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client cert returned by exec not used in TLS connection
5 participants