-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
let config = build_tls_server_config(&tls_config).await?; | ||
|
||
let rust_config = RustlsConfig::from_config(Arc::new(config)); | ||
// Build initial TLS configuration |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
dcd6e40
to
5a0f4df
Compare
@kubewarden/kubewarden-developers I see the integration tests failing. But they are not failing locally. Anyway, I'm working on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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>
@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. |
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>
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