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

feat: multiple client CA. #1090

Merged
merged 2 commits into from
Feb 17, 2025
Merged

feat: multiple client CA. #1090

merged 2 commits into from
Feb 17, 2025

Conversation

jvanz
Copy link
Member

@jvanz jvanz commented Feb 13, 2025

Description

Updates the policy server to allow loading multiple CA to validate the certificate used by client in a mTLS scenario.

Fix #1078

Test

make test

@jvanz jvanz self-assigned this Feb 13, 2025
@jvanz jvanz requested a review from a team as a code owner February 13, 2025 13:57
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.

Project coverage is 59.34%. Comparing base (2094eaa) to head (866cff2).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/config.rs 12.50% 7 Missing ⚠️
src/lib.rs 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1090      +/-   ##
==========================================
- Coverage   63.06%   59.34%   -3.73%     
==========================================
  Files          17       17              
  Lines        1186     1198      +12     
==========================================
- Hits          748      711      -37     
- Misses        438      487      +49     
Flag Coverage Δ
integration-tests 50.58% <70.58%> (-3.66%) ⬇️
unit-tests 36.05% <2.94%> (-0.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

let config = build_tls_server_config(&tls_config).await?;

let rust_config = RustlsConfig::from_config(Arc::new(config));
// Build initial TLS configuration
Copy link
Member Author

Choose a reason for hiding this comment

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

I know that the issue asked to refactor this code to improve readability. But during my tests, I though that the code ended to be worst not better. If you have some idea how to improve this code. Please, let me know. I would be happy to apply your suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

I thin it is fine for the scope of this PR, I noticed a few things we could improve though - I started working on these in #1091 which will also help clean up this function.

@jvanz jvanz force-pushed the issue1078 branch 2 times, most recently from dcd6e40 to 5a0f4df Compare February 13, 2025 16:42
@jvanz
Copy link
Member Author

jvanz commented Feb 13, 2025

@kubewarden/kubewarden-developers I see the integration tests failing. But they are not failing locally. Anyway, I'm working on this.

Copy link
Contributor

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM

Updates the policy server to allow loading multiple CA to validate the
certificate used by client in a mTLS scenario.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
@jvanz
Copy link
Member Author

jvanz commented Feb 16, 2025

@kubewarden/kubewarden-developers I think we can merge this and iterate over the mTLS story or @fabriziosestito can base on this PR for his future changes refactoring the code. I was reviewing the CI jobs and I'm not sure if the failure in the cert rotation test is caused by this PR. Because the tests fails when sending request to the readiness probe endpoint. I have difficulties to simulate the error to have 100% sure if this PR is related or not (maybe a concurrency issue or just a hiccup in the CI environment?). The only consistent failure test seems to be OTEL one, which we already know that is flaky.

As I'm writing on a Sunday, I'll let you decide if you want to continue the refactoring on this branch or merge it and open another one. Maybe, during the process we spot where the issue happens. Let me know what you prefer, I'll continue to work on this later.

Furthermore, during my explorations, I've change a little bit the code to split some functions.

@fabriziosestito
Copy link
Contributor

@jvanz I think we can merge this one. I've pushed the refactor to: #1094, which is a branch of this branch and needs to be rebased once this PR is merged.

The otel test is failing consistently, we might want to take care of that in a separate PR.

OTEL test is flaky and is necessary to revisit. But for now, let's
disable it to avoid CI failures.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
@jvanz jvanz merged commit c1f0b62 into kubewarden:main Feb 17, 2025
11 of 12 checks passed
@jvanz jvanz deleted the issue1078 branch February 17, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mTLS: handle multiple CAs
4 participants