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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: multiple client CA. #1090

wants to merge 1 commit into from

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 3.84615% with 25 lines in your changes missing coverage. Please review.

Project coverage is 36.49%. Comparing base (f8b201b) to head (5a0f4df).

Files with missing lines Patch % Lines
src/lib.rs 0.00% 18 Missing ⚠️
src/config.rs 12.50% 7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (f8b201b) and HEAD (5a0f4df). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (f8b201b) HEAD (5a0f4df)
integration-tests 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1090       +/-   ##
===========================================
- Coverage   63.69%   36.49%   -27.20%     
===========================================
  Files          17       16        -1     
  Lines        1179     1148       -31     
===========================================
- Hits          751      419      -332     
- Misses        428      729      +301     
Flag Coverage Δ
integration-tests ?
unit-tests 36.49% <3.84%> (-0.16%) ⬇️

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.

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 <[email protected]>
@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

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