Skip to content

KMS: enforce TLS requirement #1069

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.

Sign up for GitHub

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

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

KMS: enforce TLS requirement #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions simplyblock_core/cluster_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

from docker.errors import DockerException
from simplyblock_core import utils, scripts, constants, mgmt_node_ops, storage_node_ops
from simplyblock_core.settings import Settings
from simplyblock_core.controllers import backup_controller, cluster_events, device_controller, qos_controller, tasks_controller, tcp_ports_events
from simplyblock_core.fw_api_client import FirewallClient
from simplyblock_core.db_controller import DBController
Expand Down Expand Up @@ -563,6 +564,18 @@ def add_cluster(blk_size, page_size_in_blocks, cap_warn, cap_crit, prov_cap_warn
cluster.nvmf_base_port = nvmf_base_port
cluster.rpc_base_port = rpc_base_port
cluster.snode_api_port = snode_api_port
if hashicorp_vault_settings:
settings = Settings()
missing = [
path
for path in [settings.tls_certificate_authority, settings.tls_certificate, settings.tls_key]
if not path.is_file()
]
if missing:
raise ValueError(
"HashiCorp Vault requires TLS certificates that are not present: "
+ ", ".join(map(str, missing))
)
Copy link
Copy Markdown
Contributor

@mxsrc mxsrc Jun 3, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@boddumanohar boddumanohar Jun 3, 2026

Choose a reason for hiding this comment

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

Cool, thanks. would it be great if you can propose changes with a new PR 👍

Copy link
Copy Markdown
Contributor

@mxsrc mxsrc Jun 3, 2026

Choose a reason for hiding this comment

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

On it.

cluster.hashicorp_vault_settings = hashicorp_vault_settings
if backup_config:
cluster.backup_config = backup_config
Expand Down
10 changes: 5 additions & 5 deletions simplyblock_core/controllers/pool_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,13 @@ def add_pool(name, pool_max, lvol_max, max_rw_iops, max_rw_mbytes, max_r_mbytes,
pool.dhchap_ctrlr_key = utils.generate_dhchap_key(length=32)


with create_kms_connection(cluster) as kms:
try:
try:
with create_kms_connection(cluster) as kms:
kms.create_key_encryption_key(pool.get_id())
logger.info("Created pool key")
except KMSException:
logger.exception("Failed to create pool key")
return False
except KMSException:
logger.exception("Failed to create pool key")
return False

pool.status = "active"
pool.write_to_db(db_controller.kv_store)
Expand Down
15 changes: 7 additions & 8 deletions simplyblock_web/api/v2/pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import Annotated, List, Optional
from uuid import UUID

from fastapi import APIRouter, Depends, HTTPException, Request, Response
from fastapi import APIRouter, Depends, HTTPException, Response
from pydantic import BaseModel

from simplyblock_core.db_controller import DBController
Expand Down Expand Up @@ -42,24 +42,23 @@ class StoragePoolParams(BaseModel):
cr_plural: str = ""


@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()
Comment on lines -45 to -62
Copy link
Copy Markdown
Contributor

@mxsrc mxsrc Jun 3, 2026

Choose a reason for hiding this comment

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

Not sure about the modifications here, why are they necessary? In particular:

  • The name id_or_false is 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).

Copy link
Copy Markdown
Member Author

@boddumanohar boddumanohar Jun 3, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@mxsrc mxsrc Jun 3, 2026

Choose a reason for hiding this comment

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

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.

return db.get_pool_by_id(pool_id).to_dict()


instance_api = APIRouter(prefix='/{pool_id}')
Expand Down
Loading