Merge pull request #302 from finos/fix/security-p1-cluster

fix(security): close auth, credential, and error-handling exposure gaps

Author: fgogolli

dev-tools/setup/install_dev_tools.py

@@ -396,10 +396,21 @@ def check_destructive_admin_allowed(request: Request) -> None:
 
     container = get_di_container()
 
-    # Guard 0: authentication must be enabled — fail closed if auth is off.
+    # Guard 0: authentication must be enabled with a real strategy.
+    #
+    # Two conditions must both hold:
+    #   (a) auth.enabled is True — an anonymous caller must never reach a
+    #       destructive endpoint.
+    #   (b) auth.strategy is not the "none" pass-through — NoAuthStrategy
+    #       grants permissions=["*"] to every anonymous request, so even when
+    #       enabled=True is set in config the server is effectively unprotected.
+    #       Accepting "none" here would let any caller satisfy this guard while
+    #       logging a misleading "auth enabled" message.
+    _NO_REAL_AUTH_STRATEGIES = frozenset({"none", "", None})
     try:
         server_config = get_server_config()
-        if not server_config.auth.enabled:
+        auth_cfg = server_config.auth
+        if not auth_cfg.enabled:
             _logger.warning(
                 "DESTRUCTIVE_ADMIN blocked: authentication is disabled; "
                 "destructive operations require an authenticated identity"
@@ -411,6 +422,23 @@ def check_destructive_admin_allowed(request: Request) -> None:
                     "message": "Destructive admin requires authentication enabled.",
                 },
             )
+        if getattr(auth_cfg, "strategy", None) in _NO_REAL_AUTH_STRATEGIES:
+            _logger.warning(
+                "DESTRUCTIVE_ADMIN blocked: auth.strategy=%r is a pass-through "
+                "that grants every anonymous caller full permissions; "
+                "a real authentication strategy is required",
+                getattr(auth_cfg, "strategy", None),
+            )
+            raise HTTPException(  # type: ignore[misc]
+                status_code=status.HTTP_403_FORBIDDEN,
+                detail={
+                    "code": "AUTH_STRATEGY_NONE",
+                    "message": (
+                        "Destructive admin requires a real authentication strategy. "
+                        "The configured strategy enforces no access control."
+                    ),
+                },
+            )
     except HTTPException:
         raise
     except Exception:

src/orb/api/dependencies.py

@@ -413,11 +413,15 @@ async def cleanup_database(
             ),
         )
     except InvalidCleanupStatusError as exc:
+        logger.warning("Cleanup rejected due to invalid status list: %s", exc)
         return JSONResponse(
             status_code=400,
             content={
                 "success": False,
-                "error": {"code": "INVALID_STATUS", "message": str(exc)},
+                "error": {
+                    "code": "INVALID_STATUS",
+                    "message": "One or more supplied statuses are not valid terminal statuses.",
+                },
             },
         )
 
@@ -490,6 +494,9 @@ async def reload_config(request: Request, _user=Depends(require_role("admin")))
             status_code=500,
             content={
                 "reloaded": False,
-                "error": {"code": "RELOAD_FAILED", "message": str(exc)},
+                "error": {
+                    "code": "RELOAD_FAILED",
+                    "message": "Configuration reload failed. Check server logs for details.",
+                },
             },
         )

src/orb/api/routers/admin.py

@@ -19,9 +19,12 @@
     get_config_manager,
     require_role,
 )
+from orb.infrastructure.logging.logger import get_logger
 
 router = APIRouter(prefix="/config", tags=["Configuration"])
 
+logger = get_logger(__name__)
+
 CONFIG_MANAGER = Depends(get_config_manager)
 
 _NOT_FOUND_SENTINEL = object()
@@ -191,9 +194,13 @@ async def save_config(
     try:
         written_to = config_manager.save_config(resolved_save_path)
     except ValueError as exc:
+        logger.warning("Config save rejected — no config path resolved: %s", exc)
         raise HTTPException(
             status_code=400,
-            detail={"code": "NO_CONFIG_PATH", "message": str(exc)},
+            detail={
+                "code": "NO_CONFIG_PATH",
+                "message": "No config file path could be resolved. Supply an explicit path or load a config file first.",
+            },
         ) from exc
     return JSONResponse(
         content=SaveResponse(persisted=True, path=written_to).model_dump(),

src/orb/api/routers/config.py

@@ -35,9 +35,12 @@
 )
 from orb.domain.base import UnitOfWorkFactory
 from orb.infrastructure.error.decorators import handle_rest_exceptions
+from orb.infrastructure.logging.logger import get_logger
 
 router = APIRouter(prefix="/machines", tags=["Machines"])
 
+logger = get_logger(__name__)
+
 # Module-level dependency variables to avoid B008 warnings
 ACQUIRE_ORCHESTRATOR = Depends(get_acquire_machines_orchestrator)
 RETURN_ORCHESTRATOR = Depends(get_return_machines_orchestrator)
@@ -309,16 +312,24 @@ async def purge_machine(
     try:
         cleanup_result = service.delete_machine(machine_id)
     except KeyError as exc:
+        logger.warning("Machine purge failed — not found: %s", exc)
         return JSONResponse(
             status_code=404,
-            content={"success": False, "error": {"code": "NOT_FOUND", "message": str(exc)}},
+            content={
+                "success": False,
+                "error": {"code": "NOT_FOUND", "message": "Machine not found."},
+            },
         )
     except NonTerminalStatusError as exc:
+        logger.warning("Machine purge rejected — non-terminal status: %s", exc)
         return JSONResponse(
             status_code=400,
             content={
                 "success": False,
-                "error": {"code": "NON_TERMINAL_STATUS", "message": str(exc)},
+                "error": {
+                    "code": "NON_TERMINAL_STATUS",
+                    "message": "Machine cannot be purged because it is not in a terminal state.",
+                },
             },
         )
 

src/orb/api/routers/machines.py

@@ -321,16 +321,24 @@ async def purge_request(
     try:
         cleanup_result = service.delete_request(request_id, cascade_machines=True)
     except KeyError as exc:
+        _logger.warning("Request purge failed — not found: %s", exc)
         return JSONResponse(
             status_code=404,
-            content={"success": False, "error": {"code": "NOT_FOUND", "message": str(exc)}},
+            content={
+                "success": False,
+                "error": {"code": "NOT_FOUND", "message": "Request not found."},
+            },
         )
     except NonTerminalStatusError as exc:
+        _logger.warning("Request purge rejected — non-terminal status: %s", exc)
         return JSONResponse(
             status_code=400,
             content={
                 "success": False,
-                "error": {"code": "NON_TERMINAL_STATUS", "message": str(exc)},
+                "error": {
+                    "code": "NON_TERMINAL_STATUS",
+                    "message": "Request cannot be purged because it is not in a terminal state.",
+                },
             },
         )
 

src/orb/api/routers/requests.py

@@ -259,19 +259,22 @@ def create_fastapi_app(server_config: Any) -> Any:
 
     logger = get_logger(__name__)
 
-    # Validate and default configuration
+    # Validate configuration: refuse to boot with a None or structurally invalid
+    # ServerConfig.  Silently falling back to a default would produce an
+    # unauthenticated server with no intentional auth posture, which is
+    # fail-open in production.  Callers must always supply a well-formed config.
     if server_config is None:
-        logger.warning("No server configuration provided, using defaults")
-        from orb.config.schemas.server_schema import ServerConfig
-
-        server_config = ServerConfig()  # type: ignore[call-arg]
+        raise ConfigurationError(
+            "create_fastapi_app() requires a ServerConfig; received None. "
+            "Ensure server configuration is loaded before calling this function."
+        )
 
-    # Validate configuration object has required attributes
     if not hasattr(server_config, "docs_enabled"):
-        logger.error("Invalid server configuration: missing docs_enabled attribute")
-        from orb.config.schemas.server_schema import ServerConfig
-
-        server_config = ServerConfig()  # type: ignore[call-arg]
+        raise ConfigurationError(
+            "create_fastapi_app() received an invalid ServerConfig object "
+            "(missing required attribute 'docs_enabled'). "
+            "Pass a properly constructed ServerConfig instance."
+        )
 
     from orb.api.documentation import configure_openapi
     from orb.api.middleware import (

src/orb/api/server.py

@@ -128,6 +128,13 @@ def create_provider_selection_adapter(c):
 
     container.register_singleton(SystemInfoPort, lambda _: PsutilSystemInfoAdapter())
 
+    # Register TokenDenylistPort with an in-process InMemoryTokenDenylist as the
+    # default implementation.  Production deployments that require a shared
+    # denylist (e.g. Redis) should override this registration after bootstrap.
+    from orb.infrastructure.auth.token_denylist import InMemoryTokenDenylist, TokenDenylistPort
+
+    container.register_singleton(TokenDenylistPort, lambda _: InMemoryTokenDenylist())
+
     # Register response formatting service
     from orb.application.ports.scheduler_port import SchedulerPort
     from orb.interface.response_formatting_service import ResponseFormattingService

src/orb/bootstrap/port_registrations.py

@@ -128,12 +128,15 @@ class ProviderAuthSubConfig(BaseModel):
     )
 
 
+_NO_AUTH_STRATEGIES: frozenset[str | None] = frozenset({"none", "", None})
+
+
 class AuthConfig(BaseModel):
     """Authentication configuration."""
 
     model_config = ConfigDict(extra="forbid")
 
-    enabled: bool = Field(False, description="Enable authentication")
+    enabled: bool = Field(True, description="Enable authentication")
     strategy: str = Field(
         "none",
         description="Authentication strategy (none, bearer_token, bearer_token_enhanced, iam, cognito)",
@@ -152,6 +155,31 @@ class AuthConfig(BaseModel):
         None, description="Provider-specific auth configuration"
     )
 
+    @model_validator(mode="after")
+    def _reject_enabled_with_no_strategy(self) -> "AuthConfig":
+        """Fail hard when auth.enabled=True but no real strategy is configured.
+
+        The "none" strategy (NoAuthStrategy) is a pass-through that grants every
+        anonymous caller permissions=["*"].  Combining it with enabled=True is a
+        silent fail-open: the server advertises "auth is on" while enforcing
+        nothing.  Any construction that would produce this combination is a
+        misconfiguration and is rejected at build time so the error surfaces
+        immediately rather than at request time.
+
+        To run with authentication disabled, use AuthConfig(enabled=False)
+        explicitly.  To enable auth, also set strategy to a real strategy name
+        (e.g. "bearer_token", "iam", "cognito").
+        """
+        if self.enabled and self.strategy in _NO_AUTH_STRATEGIES:
+            raise ValueError(
+                "auth.enabled=True requires a real authentication strategy. "
+                f"Got strategy={self.strategy!r}, which is a pass-through that enforces nothing. "
+                "Either set enabled=False (to disable auth explicitly) "
+                "or set strategy to a real strategy name "
+                "(e.g. 'bearer_token', 'bearer_token_enhanced', 'iam', 'cognito')."
+            )
+        return self
+
 
 class CORSConfig(BaseModel):
     """CORS configuration.
@@ -161,6 +189,15 @@ class CORSConfig(BaseModel):
     Operators who bind the server to ``0.0.0.0`` (network exposure) MUST also
     update ``origins`` and ``trusted_hosts`` to the actual client origins they
     want to permit.
+
+    **Security note — credentials + wildcard origin:**
+    The combination of ``credentials=True`` and ``origins=["*"]`` is rejected at
+    validation time.  Browsers refuse to honour ``Access-Control-Allow-Credentials:
+    true`` when the server responds with ``Access-Control-Allow-Origin: *``
+    (the spec requires an explicit origin in that case).  Accepting this combo
+    silently would produce a configuration that either breaks browsers or, in
+    environments where the restriction is relaxed, grants credential access to
+    any origin.
     """
 
     enabled: bool = Field(True, description="Enable CORS")
@@ -177,6 +214,49 @@ class CORSConfig(BaseModel):
     headers: list[str] = Field(["*"], description="Allowed headers")
     credentials: bool = Field(False, description="Allow credentials")
 
+    @model_validator(mode="after")
+    def _reject_credentials_with_wildcard_origin(self) -> "CORSConfig":
+        """Reject insecure combinations of allow_credentials=True with wildcard origins/headers.
+
+        Browsers reject ``Access-Control-Allow-Credentials: true`` when the
+        server sends ``Access-Control-Allow-Origin: *``.  Beyond the bare ``*``
+        case, this validator also catches:
+
+        - Whitespace-padded wildcards (e.g. ``" * "`` or ``"  *"``)
+        - Subdomain/path wildcards (any origin containing ``*``, e.g.
+          ``"https://*.example.com"``) — browsers reject credentials with any
+          wildcard origin pattern, not just a bare ``*``
+        - ``headers=["*"]`` with ``credentials=True`` — the spec forbids
+          ``Access-Control-Allow-Headers: *`` when credentials are in play
+
+        When ``credentials=False`` (the default) none of these restrictions apply.
+        """
+        if not self.credentials:
+            return self
+
+        # Check each origin: strip whitespace and reject any that contain '*'.
+        for origin in self.origins:
+            if "*" in origin.strip():
+                raise ValueError(
+                    "cors.credentials=true is incompatible with cors.origins containing "
+                    f"a wildcard pattern (got {origin!r}). "
+                    "Browsers refuse to send credentials to wildcard origins, including "
+                    "subdomain wildcards like 'https://*.example.com'. "
+                    "Replace wildcard entries with the explicit origins you want to permit, "
+                    "or set cors.credentials=false if credentials are not needed."
+                )
+
+        # headers=["*"] with credentials is also forbidden by the CORS spec.
+        if "*" in self.headers:
+            raise ValueError(
+                "cors.credentials=true is incompatible with cors.headers containing '*'. "
+                "The CORS spec forbids 'Access-Control-Allow-Headers: *' when credentials "
+                "are in play.  List the specific request headers you want to allow, "
+                "or set cors.credentials=false if credentials are not needed."
+            )
+
+        return self
+
 
 class ServerConfig(BaseModel):
     """REST API server configuration."""
@@ -203,7 +283,17 @@ class ServerConfig(BaseModel):
     openapi_url: str = Field("/openapi.json", description="OpenAPI schema URL")
 
     # Authentication and CORS
-    auth: AuthConfig = Field(default_factory=AuthConfig, description="Authentication configuration")  # type: ignore[arg-type]
+    #
+    # Default: auth disabled.  The "none" strategy is a pass-through that grants
+    # every anonymous caller permissions=["*"]; combining it with enabled=True
+    # is silently fail-open.  Operators who want auth MUST set both enabled=True
+    # AND a real strategy name in their config file.  A bare ServerConfig()
+    # therefore boots with auth off (honest posture) rather than appearing to
+    # have auth on while enforcing nothing.
+    auth: AuthConfig = Field(  # type: ignore[arg-type]
+        default_factory=lambda: AuthConfig(enabled=False),  # type: ignore[call-arg]
+        description="Authentication configuration",
+    )
     cors: CORSConfig = Field(default_factory=CORSConfig, description="CORS configuration")  # type: ignore[arg-type]
 
     # Security