Add configurable mTLS client auth via server_tls_client_auth #1182
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
muralibasani
force-pushed
the
mtls-config
branch
from
09cbb99 to
86be71b
Compare
src/karapace/core/config.py
Outdated
| if config.server_tls_cafile: | ||
| ssl_context.load_verify_locations(config.server_tls_cafile) | ||
|
|
||
| if client_auth_mode == "required": |
There was a problem hiding this comment.
[nit]
you could use match/case
src/karapace/core/config.py
Outdated
| server_tls_certfile: str | None = None | ||
| server_tls_keyfile: str | None = None | ||
| server_tls_cafile: str | None = None | ||
| server_tls_client_auth: Literal["none", "optional", "required"] = "none" |
There was a problem hiding this comment.
Can we extract "none", "optional", "required" into an enum, we can then use that for reference.
src/karapace/__main__.py
Outdated
| config = karapace_container.config() | ||
| app = create_karapace_application(config=config, lifespan=karapace_schema_registry_lifespan) | ||
| client_auth_mode = config.server_tls_client_auth.lower() | ||
| if client_auth_mode == "required": |
There was a problem hiding this comment.
[nit]
maybe you can extract this logic into a function and then use that in create_server_ssl_context() and in this file.
| from tests.integration.utils.cluster import start_schema_registry_cluster | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
[nit]
you do not need the pytest asyncio marker.
tests/integration/test_mtls.py
Outdated
| server_cert: str, | ||
| server_key: str, | ||
| ) -> None: | ||
| # Skip cleanly if test credentials are not available in the environment. |
tests/integration/test_mtls.py
Outdated
| if resp.status >= 400: | ||
| failed = True | ||
| except ( | ||
| aiohttp.ClientConnectorCertificateError, |
There was a problem hiding this comment.
Are we expecting such an exhaustive list of errors, just from this single API call?
There was a problem hiding this comment.
Actually not, replaced with only client and ssl error.
muralibasani
force-pushed
the
mtls-config
branch
from
7f0e6af to
48cb45b
Compare
|
|
||
| client_auth = ( | ||
| config.server_tls_client_auth.value | ||
| if isinstance(config.server_tls_client_auth, enum.Enum) |
There was a problem hiding this comment.
its good to keep it imo, as it normalizes the value just in case if it is not enum rather its a raw string. doesn't hurt
There was a problem hiding this comment.
But it also looks redundant, I can remove it
About this change - What it does
Add configurable mTLS client auth via server_tls_client_auth and pass ssl_cert_reqs to uvicorn, enforcing client certs when set to required.
#1175
Why this way