Skip to content

Commit 0ca1c1d

Browse files
committed
fix(security): harden auth posture, credential flow, and error/output leakage
Closes a cluster of authentication, credential-handling, and error-output exposure gaps, then hardens the places where a fix reached one chokepoint while the live code path flowed through another. Authentication - Fail-closed config: reject enabled=True combined with a no-op strategy (none/empty) at construction; a bare ServerConfig() yields auth disabled (an honest 'unconfigured' posture) rather than enabled-with-no-strategy, which previously let every anonymous caller through with full permissions. - Bare-config fallbacks in server startup and the CLI now raise a ConfigurationError instead of silently booting unauthenticated. - The destructive-admin guard no longer passes under a no-op strategy. - Cognito: revoke_token performs real revocation (RevokeToken API for refresh tokens, denylist for access tokens); the denylist is resolved from the container so revocation takes effect on the live path; the token is always denylisted before any API call so a forged unsigned token_use cannot skip it. - IAM: implement SimulatePrincipalPolicy with deny-by-default on error, run the STS/IAM calls off the event loop, and paginate results fully. CORS - Reject credentials combined with wildcard origins, whitespace-padded or subdomain wildcards, and wildcard headers. Error / output - Not-found handlers return categorical messages instead of echoing entity ids; a details whitelist strips internal fields; unexpected errors are logged; the sibling response builder no longer echoes raw exception text. - REST routers return safe messages and log detail server-side. Credentials & tooling - AWS provider config wraps access_key_id/secret_access_key in SecretStr, the session factory forwards explicit credentials to boto3, and the config debug mask is recursive without over-masking non-secret keys. - Dev-tools installers drop shell=True, download-then-execute (incl. Windows), clean up temp files, and fix a broken pipe in the Docker install.
1 parent 6dcc536 commit 0ca1c1d

29 files changed

Lines changed: 3698 additions & 261 deletions

dev-tools/setup/install_dev_tools.py

Lines changed: 312 additions & 95 deletions
Large diffs are not rendered by default.

src/orb/api/dependencies.py

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -393,10 +393,21 @@ def check_destructive_admin_allowed(request: Request) -> None:
393393

394394
container = get_di_container()
395395

396-
# Guard 0: authentication must be enabled — fail closed if auth is off.
396+
# Guard 0: authentication must be enabled with a real strategy.
397+
#
398+
# Two conditions must both hold:
399+
# (a) auth.enabled is True — an anonymous caller must never reach a
400+
# destructive endpoint.
401+
# (b) auth.strategy is not the "none" pass-through — NoAuthStrategy
402+
# grants permissions=["*"] to every anonymous request, so even when
403+
# enabled=True is set in config the server is effectively unprotected.
404+
# Accepting "none" here would let any caller satisfy this guard while
405+
# logging a misleading "auth enabled" message.
406+
_NO_REAL_AUTH_STRATEGIES = frozenset({"none", "", None})
397407
try:
398408
server_config = get_server_config()
399-
if not server_config.auth.enabled:
409+
auth_cfg = server_config.auth
410+
if not auth_cfg.enabled:
400411
_logger.warning(
401412
"DESTRUCTIVE_ADMIN blocked: authentication is disabled; "
402413
"destructive operations require an authenticated identity"
@@ -408,6 +419,23 @@ def check_destructive_admin_allowed(request: Request) -> None:
408419
"message": "Destructive admin requires authentication enabled.",
409420
},
410421
)
422+
if getattr(auth_cfg, "strategy", None) in _NO_REAL_AUTH_STRATEGIES:
423+
_logger.warning(
424+
"DESTRUCTIVE_ADMIN blocked: auth.strategy=%r is a pass-through "
425+
"that grants every anonymous caller full permissions; "
426+
"a real authentication strategy is required",
427+
getattr(auth_cfg, "strategy", None),
428+
)
429+
raise HTTPException( # type: ignore[misc]
430+
status_code=status.HTTP_403_FORBIDDEN,
431+
detail={
432+
"code": "AUTH_STRATEGY_NONE",
433+
"message": (
434+
"Destructive admin requires a real authentication strategy. "
435+
"The configured strategy enforces no access control."
436+
),
437+
},
438+
)
411439
except HTTPException:
412440
raise
413441
except Exception:

src/orb/api/routers/admin.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -413,11 +413,15 @@ async def cleanup_database(
413413
),
414414
)
415415
except InvalidCleanupStatusError as exc:
416+
logger.warning("Cleanup rejected due to invalid status list: %s", exc)
416417
return JSONResponse(
417418
status_code=400,
418419
content={
419420
"success": False,
420-
"error": {"code": "INVALID_STATUS", "message": str(exc)},
421+
"error": {
422+
"code": "INVALID_STATUS",
423+
"message": "One or more supplied statuses are not valid terminal statuses.",
424+
},
421425
},
422426
)
423427

@@ -490,6 +494,9 @@ async def reload_config(request: Request, _user=Depends(require_role("admin")))
490494
status_code=500,
491495
content={
492496
"reloaded": False,
493-
"error": {"code": "RELOAD_FAILED", "message": str(exc)},
497+
"error": {
498+
"code": "RELOAD_FAILED",
499+
"message": "Configuration reload failed. Check server logs for details.",
500+
},
494501
},
495502
)

src/orb/api/routers/config.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,12 @@
1919
get_config_manager,
2020
require_role,
2121
)
22+
from orb.infrastructure.logging.logger import get_logger
2223

2324
router = APIRouter(prefix="/config", tags=["Configuration"])
2425

26+
logger = get_logger(__name__)
27+
2528
CONFIG_MANAGER = Depends(get_config_manager)
2629

2730
_NOT_FOUND_SENTINEL = object()
@@ -191,9 +194,13 @@ async def save_config(
191194
try:
192195
written_to = config_manager.save_config(resolved_save_path)
193196
except ValueError as exc:
197+
logger.warning("Config save rejected — no config path resolved: %s", exc)
194198
raise HTTPException(
195199
status_code=400,
196-
detail={"code": "NO_CONFIG_PATH", "message": str(exc)},
200+
detail={
201+
"code": "NO_CONFIG_PATH",
202+
"message": "No config file path could be resolved. Supply an explicit path or load a config file first.",
203+
},
197204
) from exc
198205
return JSONResponse(
199206
content=SaveResponse(persisted=True, path=written_to).model_dump(),

src/orb/api/routers/machines.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,12 @@
3535
)
3636
from orb.domain.base import UnitOfWorkFactory
3737
from orb.infrastructure.error.decorators import handle_rest_exceptions
38+
from orb.infrastructure.logging.logger import get_logger
3839

3940
router = APIRouter(prefix="/machines", tags=["Machines"])
4041

42+
logger = get_logger(__name__)
43+
4144
# Module-level dependency variables to avoid B008 warnings
4245
ACQUIRE_ORCHESTRATOR = Depends(get_acquire_machines_orchestrator)
4346
RETURN_ORCHESTRATOR = Depends(get_return_machines_orchestrator)
@@ -309,16 +312,24 @@ async def purge_machine(
309312
try:
310313
cleanup_result = service.delete_machine(machine_id)
311314
except KeyError as exc:
315+
logger.warning("Machine purge failed — not found: %s", exc)
312316
return JSONResponse(
313317
status_code=404,
314-
content={"success": False, "error": {"code": "NOT_FOUND", "message": str(exc)}},
318+
content={
319+
"success": False,
320+
"error": {"code": "NOT_FOUND", "message": "Machine not found."},
321+
},
315322
)
316323
except NonTerminalStatusError as exc:
324+
logger.warning("Machine purge rejected — non-terminal status: %s", exc)
317325
return JSONResponse(
318326
status_code=400,
319327
content={
320328
"success": False,
321-
"error": {"code": "NON_TERMINAL_STATUS", "message": str(exc)},
329+
"error": {
330+
"code": "NON_TERMINAL_STATUS",
331+
"message": "Machine cannot be purged because it is not in a terminal state.",
332+
},
322333
},
323334
)
324335

src/orb/api/routers/requests.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,16 +321,24 @@ async def purge_request(
321321
try:
322322
cleanup_result = service.delete_request(request_id, cascade_machines=True)
323323
except KeyError as exc:
324+
_logger.warning("Request purge failed — not found: %s", exc)
324325
return JSONResponse(
325326
status_code=404,
326-
content={"success": False, "error": {"code": "NOT_FOUND", "message": str(exc)}},
327+
content={
328+
"success": False,
329+
"error": {"code": "NOT_FOUND", "message": "Request not found."},
330+
},
327331
)
328332
except NonTerminalStatusError as exc:
333+
_logger.warning("Request purge rejected — non-terminal status: %s", exc)
329334
return JSONResponse(
330335
status_code=400,
331336
content={
332337
"success": False,
333-
"error": {"code": "NON_TERMINAL_STATUS", "message": str(exc)},
338+
"error": {
339+
"code": "NON_TERMINAL_STATUS",
340+
"message": "Request cannot be purged because it is not in a terminal state.",
341+
},
334342
},
335343
)
336344

src/orb/api/server.py

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -259,19 +259,22 @@ def create_fastapi_app(server_config: Any) -> Any:
259259

260260
logger = get_logger(__name__)
261261

262-
# Validate and default configuration
262+
# Validate configuration: refuse to boot with a None or structurally invalid
263+
# ServerConfig. Silently falling back to a default would produce an
264+
# unauthenticated server with no intentional auth posture, which is
265+
# fail-open in production. Callers must always supply a well-formed config.
263266
if server_config is None:
264-
logger.warning("No server configuration provided, using defaults")
265-
from orb.config.schemas.server_schema import ServerConfig
266-
267-
server_config = ServerConfig() # type: ignore[call-arg]
267+
raise ConfigurationError(
268+
"create_fastapi_app() requires a ServerConfig; received None. "
269+
"Ensure server configuration is loaded before calling this function."
270+
)
268271

269-
# Validate configuration object has required attributes
270272
if not hasattr(server_config, "docs_enabled"):
271-
logger.error("Invalid server configuration: missing docs_enabled attribute")
272-
from orb.config.schemas.server_schema import ServerConfig
273-
274-
server_config = ServerConfig() # type: ignore[call-arg]
273+
raise ConfigurationError(
274+
"create_fastapi_app() received an invalid ServerConfig object "
275+
"(missing required attribute 'docs_enabled'). "
276+
"Pass a properly constructed ServerConfig instance."
277+
)
275278

276279
from orb.api.documentation import configure_openapi
277280
from orb.api.middleware import (

src/orb/bootstrap/port_registrations.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,13 @@ def create_provider_selection_adapter(c):
128128

129129
container.register_singleton(SystemInfoPort, lambda _: PsutilSystemInfoAdapter())
130130

131+
# Register TokenDenylistPort with an in-process InMemoryTokenDenylist as the
132+
# default implementation. Production deployments that require a shared
133+
# denylist (e.g. Redis) should override this registration after bootstrap.
134+
from orb.infrastructure.auth.token_denylist import InMemoryTokenDenylist, TokenDenylistPort
135+
136+
container.register_singleton(TokenDenylistPort, lambda _: InMemoryTokenDenylist())
137+
131138
# Register response formatting service
132139
from orb.application.ports.scheduler_port import SchedulerPort
133140
from orb.interface.response_formatting_service import ResponseFormattingService

src/orb/config/schemas/server_schema.py

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,12 +128,15 @@ class ProviderAuthSubConfig(BaseModel):
128128
)
129129

130130

131+
_NO_AUTH_STRATEGIES: frozenset[str | None] = frozenset({"none", "", None})
132+
133+
131134
class AuthConfig(BaseModel):
132135
"""Authentication configuration."""
133136

134137
model_config = ConfigDict(extra="forbid")
135138

136-
enabled: bool = Field(False, description="Enable authentication")
139+
enabled: bool = Field(True, description="Enable authentication")
137140
strategy: str = Field(
138141
"none",
139142
description="Authentication strategy (none, bearer_token, bearer_token_enhanced, iam, cognito)",
@@ -152,6 +155,31 @@ class AuthConfig(BaseModel):
152155
None, description="Provider-specific auth configuration"
153156
)
154157

158+
@model_validator(mode="after")
159+
def _reject_enabled_with_no_strategy(self) -> "AuthConfig":
160+
"""Fail hard when auth.enabled=True but no real strategy is configured.
161+
162+
The "none" strategy (NoAuthStrategy) is a pass-through that grants every
163+
anonymous caller permissions=["*"]. Combining it with enabled=True is a
164+
silent fail-open: the server advertises "auth is on" while enforcing
165+
nothing. Any construction that would produce this combination is a
166+
misconfiguration and is rejected at build time so the error surfaces
167+
immediately rather than at request time.
168+
169+
To run with authentication disabled, use AuthConfig(enabled=False)
170+
explicitly. To enable auth, also set strategy to a real strategy name
171+
(e.g. "bearer_token", "iam", "cognito").
172+
"""
173+
if self.enabled and self.strategy in _NO_AUTH_STRATEGIES:
174+
raise ValueError(
175+
"auth.enabled=True requires a real authentication strategy. "
176+
f"Got strategy={self.strategy!r}, which is a pass-through that enforces nothing. "
177+
"Either set enabled=False (to disable auth explicitly) "
178+
"or set strategy to a real strategy name "
179+
"(e.g. 'bearer_token', 'bearer_token_enhanced', 'iam', 'cognito')."
180+
)
181+
return self
182+
155183

156184
class CORSConfig(BaseModel):
157185
"""CORS configuration.
@@ -161,6 +189,15 @@ class CORSConfig(BaseModel):
161189
Operators who bind the server to ``0.0.0.0`` (network exposure) MUST also
162190
update ``origins`` and ``trusted_hosts`` to the actual client origins they
163191
want to permit.
192+
193+
**Security note — credentials + wildcard origin:**
194+
The combination of ``credentials=True`` and ``origins=["*"]`` is rejected at
195+
validation time. Browsers refuse to honour ``Access-Control-Allow-Credentials:
196+
true`` when the server responds with ``Access-Control-Allow-Origin: *``
197+
(the spec requires an explicit origin in that case). Accepting this combo
198+
silently would produce a configuration that either breaks browsers or, in
199+
environments where the restriction is relaxed, grants credential access to
200+
any origin.
164201
"""
165202

166203
enabled: bool = Field(True, description="Enable CORS")
@@ -177,6 +214,49 @@ class CORSConfig(BaseModel):
177214
headers: list[str] = Field(["*"], description="Allowed headers")
178215
credentials: bool = Field(False, description="Allow credentials")
179216

217+
@model_validator(mode="after")
218+
def _reject_credentials_with_wildcard_origin(self) -> "CORSConfig":
219+
"""Reject insecure combinations of allow_credentials=True with wildcard origins/headers.
220+
221+
Browsers reject ``Access-Control-Allow-Credentials: true`` when the
222+
server sends ``Access-Control-Allow-Origin: *``. Beyond the bare ``*``
223+
case, this validator also catches:
224+
225+
- Whitespace-padded wildcards (e.g. ``" * "`` or ``" *"``)
226+
- Subdomain/path wildcards (any origin containing ``*``, e.g.
227+
``"https://*.example.com"``) — browsers reject credentials with any
228+
wildcard origin pattern, not just a bare ``*``
229+
- ``headers=["*"]`` with ``credentials=True`` — the spec forbids
230+
``Access-Control-Allow-Headers: *`` when credentials are in play
231+
232+
When ``credentials=False`` (the default) none of these restrictions apply.
233+
"""
234+
if not self.credentials:
235+
return self
236+
237+
# Check each origin: strip whitespace and reject any that contain '*'.
238+
for origin in self.origins:
239+
if "*" in origin.strip():
240+
raise ValueError(
241+
"cors.credentials=true is incompatible with cors.origins containing "
242+
f"a wildcard pattern (got {origin!r}). "
243+
"Browsers refuse to send credentials to wildcard origins, including "
244+
"subdomain wildcards like 'https://*.example.com'. "
245+
"Replace wildcard entries with the explicit origins you want to permit, "
246+
"or set cors.credentials=false if credentials are not needed."
247+
)
248+
249+
# headers=["*"] with credentials is also forbidden by the CORS spec.
250+
if "*" in self.headers:
251+
raise ValueError(
252+
"cors.credentials=true is incompatible with cors.headers containing '*'. "
253+
"The CORS spec forbids 'Access-Control-Allow-Headers: *' when credentials "
254+
"are in play. List the specific request headers you want to allow, "
255+
"or set cors.credentials=false if credentials are not needed."
256+
)
257+
258+
return self
259+
180260

181261
class ServerConfig(BaseModel):
182262
"""REST API server configuration."""
@@ -203,7 +283,17 @@ class ServerConfig(BaseModel):
203283
openapi_url: str = Field("/openapi.json", description="OpenAPI schema URL")
204284

205285
# Authentication and CORS
206-
auth: AuthConfig = Field(default_factory=AuthConfig, description="Authentication configuration") # type: ignore[arg-type]
286+
#
287+
# Default: auth disabled. The "none" strategy is a pass-through that grants
288+
# every anonymous caller permissions=["*"]; combining it with enabled=True
289+
# is silently fail-open. Operators who want auth MUST set both enabled=True
290+
# AND a real strategy name in their config file. A bare ServerConfig()
291+
# therefore boots with auth off (honest posture) rather than appearing to
292+
# have auth on while enforcing nothing.
293+
auth: AuthConfig = Field( # type: ignore[arg-type]
294+
default_factory=lambda: AuthConfig(enabled=False), # type: ignore[call-arg]
295+
description="Authentication configuration",
296+
)
207297
cors: CORSConfig = Field(default_factory=CORSConfig, description="CORS configuration") # type: ignore[arg-type]
208298

209299
# Security

0 commit comments

Comments
 (0)