KMS: enforce TLS requirement#1069
Conversation
mxsrc
left a comment
mxsrc
left a comment
There was a problem hiding this comment.
Thanks for the changes, I have a few comments that merit discussion.
| @api.post('/', name='clusters:storage-pools:create', status_code=201, responses={201: {"content": None}}) | ||
| def add(request: Request, cluster: Cluster, parameters: StoragePoolParams) -> Response: | ||
| @api.post('/', name='clusters:storage-pools:create', status_code=201) | ||
| def add(cluster: Cluster, parameters: StoragePoolParams): | ||
| for pool in db.get_pools(cluster.get_id()): | ||
| if pool.pool_name == parameters.name: | ||
| raise HTTPException(409, f'Pool {parameters.name} already exists') | ||
|
|
||
| id_or_false = pool_controller.add_pool( | ||
| pool_id = pool_controller.add_pool( | ||
| parameters.name, parameters.pool_max, parameters.volume_max_size, parameters.max_rw_iops, parameters.max_rw_mbytes, | ||
| parameters.max_r_mbytes, parameters.max_w_mbytes, cluster.get_id(), | ||
| parameters.cr_name, parameters.cr_namespace, parameters.cr_plural, | ||
| dhchap=parameters.dhchap, | ||
| ) | ||
|
|
||
| if not id_or_false: | ||
| raise ValueError('Failed to create pool') | ||
| if not pool_id: | ||
| raise HTTPException(500, 'Failed to create pool') | ||
|
|
||
| pool = db.get_pool_by_id(id_or_false) | ||
| return pool.to_dict() |
There was a problem hiding this comment.
Not sure about the modifications here, why are they necessary? In particular:
- The name
id_or_falseis meant to provide some indication of what this value indicates, given the inadequate error reporting of the top-level function - The other modifications do make some sense, it seems like there was a modification to this endpoint that's inconsistent with the original API design (simply returning an untyped dict instead of the original empty response, I imagine this was a hotfix to avoid a duplicate fetch. A proper way of handling this would be a parameter indicating the desired response).
There was a problem hiding this comment.
My implementation is a bit incomplete here. I wanted to have exception based handling rather than depending on True or False. But then later realised that this will invite a lot of new diff.
So I'll revert variable change. And then add a query parameter to so that user can decide if they need response or not.
There was a problem hiding this comment.
Great, thanks! I don't think we have these parameters in this codebase yet so I think we can just copy the approach from vela. I think this is also an opportunity to properly type the endpoint and return the appropriate DTO instead of a random dict.
| raise ValueError( | ||
| "HashiCorp Vault requires TLS certificates that are not present: " | ||
| + ", ".join(map(str, missing)) | ||
| ) |
There was a problem hiding this comment.
This was meant to be part of the original review, apparently it got lost in transit:
This makes sense to introduce semantically, but belongs in the settings module imho. I see that simplyblock_core.settings.Settings.validate_tls_files fails to extend the check to mTLS:
@@ -58,7 +58,7 @@ class Settings(BaseSettings):
if not self.tls_serve and self.tls_connect == "disabled":
return self
- if self.tls_serve and (
+ if (self.tls_serve or (self.tls_connect == "authenticated")) and (
missing := [
name
for name in ["tls_certificate", "tls_key"]
@@ -66,7 +66,7 @@ class Settings(BaseSettings):
]
):
raise ValueError(
- "SB_TLS_SERVE=true requires TLS files to exist: " + ", ".join(missing)
+ "SB_TLS_SERVE=true/SB_TLS_CONNECT=authenticated require TLS files to exist: " + ", ".join(missing)
)
if (
Once this is fixed, we could simply check Settings().tls_connect == "authenticated", which is much more readable. I can open a PR to introduce the change if you like, given that it's quite small it might make more sense to include in this PR.
There was a problem hiding this comment.
Cool, thanks. would it be great if you can propose changes with a new PR 👍
2 participants
TLS is hard requirement for KMS feature. So adding a validation as a part of Cluster create.