security: harden gateway auth defaults and restrict auto-pair#123
security: harden gateway auth defaults and restrict auto-pair#123kakuteki wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
…VIDIA#117) - dangerouslyDisableDeviceAuth now defaults to false; only enabled when NEMOCLAW_DISABLE_DEVICE_AUTH=1 is explicitly set (removes the "temporary" bypass from commit 5fb629f that was never reverted) - allowInsecureAuth is now derived from the CHAT_UI_URL scheme: true for http (local dev), false for https (production/remote) - auto-pair watcher now only approves known client types (openclaw-control-ui, webchat) instead of unconditionally approving every pending device request
kakuteki
force-pushed
the
fix/insecure-auth-configuration
branch
from
0516af4 to
6f603c7
Compare
📝 WalkthroughWalkthroughThe script now makes insecure authentication settings configurable via environment variables instead of hardcoded values, and introduces client/mode whitelisting for auto-pairing device approval to restrict which clients can self-register. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 145-151: The skipped entries are left in pending and reprocessed;
update the handling in the device validation branch (the checks using device,
clientId/clientMode, ALLOWED_CLIENTS and ALLOWED_MODES) to explicitly mark or
reject the request so it won't stay pending: call the existing rejection routine
(e.g. reject_request(requestId) or send a failure response) when device is not a
dict or when client_id/client_mode are not allowed, or alternatively record the
requestId into a handled_requests set and ensure the watcher filters
handled_requests out before the if pending: path; reference the variables
device, clientId/clientMode, client_id/client_mode, ALLOWED_CLIENTS,
ALLOWED_MODES, requestId and pending to locate where to insert the
rejection/marking logic.
- Around line 46-50: The current logic sets allow_insecure = parsed.scheme !=
'https', which enables insecure auth for malformed or unsupported schemes;
change this to only allow insecure when the parsed.scheme is explicitly 'http'
(e.g., allow_insecure = parsed.scheme == 'http'), and treat any other value
(empty or unknown scheme) as secure/closed (allow_insecure = False) or abort
early if you prefer stricter behavior; update the code that builds
gateway['controlUi'] (the allowInsecure and dangerouslyDisableDeviceAuth fields)
so only an explicit 'http' origin enables allowInsecure, referencing
disable_device_auth, parsed.scheme, gateway['controlUi'], allowInsecure, and
dangerouslyDisableDeviceAuth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cc4f01e-c4ce-4b1b-b808-15ed22d53c8d
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
| disable_device_auth = os.environ.get('NEMOCLAW_DISABLE_DEVICE_AUTH', '') == '1' | ||
| allow_insecure = parsed.scheme != 'https' | ||
| gateway['controlUi'] = { | ||
| 'allowInsecureAuth': True, | ||
| 'dangerouslyDisableDeviceAuth': True, | ||
| 'allowInsecureAuth': allow_insecure, | ||
| 'dangerouslyDisableDeviceAuth': disable_device_auth, |
There was a problem hiding this comment.
Fail closed for invalid CHAT_UI_URL values.
allow_insecure = parsed.scheme != 'https' enables insecure auth for any non-HTTPS input, including malformed values like example.com or unsupported schemes. That weakens the new default unless the caller gets the URL exactly right. Only an explicit http://... origin should enable this path; everything else should disable insecure auth or abort early.
🔐 Suggested fix
chat_ui_url = os.environ.get('CHAT_UI_URL', 'http://127.0.0.1:18789')
parsed = urlparse(chat_ui_url)
-chat_origin = f"{parsed.scheme}://{parsed.netloc}" if parsed.scheme and parsed.netloc else 'http://127.0.0.1:18789'
local_origin = f'http://127.0.0.1:{os.environ.get("PUBLIC_PORT", "18789")}'
+if parsed.scheme in {'http', 'https'} and parsed.netloc:
+ chat_origin = f"{parsed.scheme}://{parsed.netloc}"
+ allow_insecure = parsed.scheme == 'http'
+else:
+ chat_origin = local_origin
+ allow_insecure = False
origins = [local_origin]
if chat_origin not in origins:
origins.append(chat_origin)
gateway = cfg.setdefault('gateway', {})
gateway['mode'] = 'local'
disable_device_auth = os.environ.get('NEMOCLAW_DISABLE_DEVICE_AUTH', '') == '1'
-allow_insecure = parsed.scheme != 'https'
gateway['controlUi'] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nemoclaw-start.sh` around lines 46 - 50, The current logic sets
allow_insecure = parsed.scheme != 'https', which enables insecure auth for
malformed or unsupported schemes; change this to only allow insecure when the
parsed.scheme is explicitly 'http' (e.g., allow_insecure = parsed.scheme ==
'http'), and treat any other value (empty or unknown scheme) as secure/closed
(allow_insecure = False) or abort early if you prefer stricter behavior; update
the code that builds gateway['controlUi'] (the allowInsecure and
dangerouslyDisableDeviceAuth fields) so only an explicit 'http' origin enables
allowInsecure, referencing disable_device_auth, parsed.scheme,
gateway['controlUi'], allowInsecure, and dangerouslyDisableDeviceAuth.
| if not isinstance(device, dict): | ||
| continue | ||
| client_id = device.get('clientId', '') | ||
| client_mode = device.get('clientMode', '') | ||
| if client_id not in ALLOWED_CLIENTS and client_mode not in ALLOWED_MODES: | ||
| print(f'[auto-pair] rejected unknown client={client_id} mode={client_mode}') | ||
| continue |
There was a problem hiding this comment.
Skipped requests never leave pending.
These continue paths only ignore or log bad entries. Because those requests remain pending, the watcher reprocesses them every second and never reaches the convergence path while they exist. Please reject them explicitly, or at least record handled requestIds and filter them out before the if pending: branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nemoclaw-start.sh` around lines 145 - 151, The skipped entries are
left in pending and reprocessed; update the handling in the device validation
branch (the checks using device, clientId/clientMode, ALLOWED_CLIENTS and
ALLOWED_MODES) to explicitly mark or reject the request so it won't stay
pending: call the existing rejection routine (e.g. reject_request(requestId) or
send a failure response) when device is not a dict or when client_id/client_mode
are not allowed, or alternatively record the requestId into a handled_requests
set and ensure the watcher filters handled_requests out before the if pending:
path; reference the variables device, clientId/clientMode,
client_id/client_mode, ALLOWED_CLIENTS, ALLOWED_MODES, requestId and pending to
locate where to insert the rejection/marking logic.
|
Hi maintainers, could you please approve the CI workflow run for this PR? I've rebased onto the latest main. Thank you! |
|
Thanks for addressing the security concern and providing a fix for the gateway auth defaults, this is a significant improvement to the security posture of NemoClaw. |
wscurran
added
the
priority: high
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The security intent here is correct — device auth should default to enabled, and insecure auth should derive from the URL scheme. But the implementation is superseded by other in-flight work.
Stale against main
Based on the old inline Python config writer in nemoclaw-start.sh. Main has since locked openclaw.json to root-owned mode 444 — the inline config write will fail at runtime.
Auto-pair filtering superseded by #690
The clientId/clientMode allowlist is nearly identical to what #690 does. #690 also adds timeout reduction (600s→180s), one-shot exit after first approval, and token cleanup on start/exit. Note: we flagged in our #690 review that clientId is client-supplied and spoofable (the gateway stores connectParams.client.id verbatim — no server-side validation). The same issue applies to this PR's ALLOWED_CLIENTS check.
Contradicts #114
PR #114 unconditionally sets dangerouslyDisableDeviceAuth: true for remote access. This PR defaults it to false. These take opposite positions — they need to be reconciled before either merges.
Conflicts with #721
The gateway isolation PR restructures the entire entrypoint with gosu privilege separation, config integrity hashing, and PATH hardening. Both the inline config block and the auto-pair watcher are significantly changed in #721.
Recommendation
The auth hardening (points 1 and 2) is worth preserving but needs to be re-implemented against the current entrypoint structure. The auto-pair changes (point 3) should be dropped in favor of #690 which is more comprehensive. Consider rebasing and scoping this PR to just the auth defaults after #690 and #721 land.
4 participants
Summary
Fixes #117 — Insecure authentication configuration in gateway setup.
The gateway's
controlUiconfig unconditionally setdangerouslyDisableDeviceAuth: TrueandallowInsecureAuth: True. This was added as a temporary workaround (commit5fb629f, message: "Disable deviceauth temporarily") but was never reverted. While the currentmainbranch is protected by sandbox network isolation andgateway.mode = 'local', these settings become dangerous if combined with a LAN-bind change (e.g. PR #114) or a cloudflared tunnel in remote deployments — resulting in an unauthenticated, publicly reachable dashboard.Changes
1.
dangerouslyDisableDeviceAuth— default tofalseDevice-pairing auth is now enabled by default. For development or headless environments that genuinely need it disabled, set
NEMOCLAW_DISABLE_DEVICE_AUTH=1explicitly.2.
allowInsecureAuth— derive from URL schemeInstead of unconditionally allowing insecure auth, it is now enabled only when
CHAT_UI_URLuseshttp://(local development). Whenhttps://is configured (production/remote), insecure auth is automatically disabled.3.
start_auto_pair— restrict to known client typesThe auto-pair watcher previously approved every pending device request unconditionally. It now only approves devices with recognized
clientId(openclaw-control-ui) orclientMode(webchat). Unknown clients are logged and rejected.Test plan
NEMOCLAW_DISABLE_DEVICE_AUTH=1: device auth is disabled (backward-compatible escape hatch)CHAT_UI_URL=https://...:allowInsecureAuthisfalseCHAT_UI_URL=http://127.0.0.1:18789(default):allowInsecureAuthistrueopenclaw-control-ui/webchatdevices: auto-pair approves as beforeSummary by CodeRabbit
New Features
Improvements