Skip to content

feat: add SSH settings for device and namespace#6148

Open
henrybarreto wants to merge 7 commits intomasterfrom
feat/allow-block-password-logins
Open

feat: add SSH settings for device and namespace#6148
henrybarreto wants to merge 7 commits intomasterfrom
feat/allow-block-password-logins

Conversation

@henrybarreto
Copy link
Copy Markdown
Collaborator

@henrybarreto henrybarreto commented Apr 8, 2026

Add allow-based SSH settings for both namespace and device configuration, update the backend migrations and auth flow, and wire the React console to edit the new settings consistently.

Closes #6136


Namespace's settings:

Screenshot 2026-04-11 at 09-04-26 ShellHub

Device's settings:
Screenshot 2026-04-11 at 09-05-02 ShellHub

@henrybarreto henrybarreto self-assigned this Apr 8, 2026
@henrybarreto henrybarreto requested review from a team as code owners April 8, 2026 23:03
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 8, 2026

Claude finished @henrybarreto's task in 7m 11s —— View job


Code Review Complete

Reviewed 28 files. 6 inline issues posted (2 critical, 2 high, 2 medium).

Critical: Cloud compilation breakage (non-inlinable)

DeviceIsOnline and DeviceOnlineThreshold were removed from api/store/pg/device.go, but cloud/pkg/store/pg/session.go still references both symbols via the shpg import alias (lines 220, 258, 262). This will cause a compilation failure in the cloud repo. These removals appear unrelated to the disable password/public key feature — either restore them or submit a companion cloud PR first.

Additional notes

  • This PR includes several unrelated changes mixed into the same commit: removal of DeviceIsOnline/DeviceOnlineThreshold, removal of fromLt filter, session online check removals, CLI refactoring, and UUID mock changes. Consider splitting these into separate commits or PRs.
  • Missing migration test: migration_121_test.go does not exist, while virtually every other migration (1–120) has one. Consider adding it following the existing pattern.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread pkg/models/namespace.go Outdated
Comment thread api/services/namespace.go Outdated
Comment thread ui/src/components/Terminal/TerminalLoginForm.vue Outdated
Comment thread openapi/spec/components/schemas/namespaceSettings.yaml Outdated
Comment thread ssh/session/auther_test.go
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch 5 times, most recently from 2f24e04 to a34b829 Compare April 10, 2026 10:53
@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 10, 2026

Claude finished @gustavosbarreto's task in 7m 6s —— View job


Code Review Complete

Reviewed 32 files. 2 inline issues posted (1 critical, 1 high).

Critical: Auth method restriction bypass via SSH fallback

The DisablePassword/DisablePublicKey checks in Evaluate() are only called when the session state is StateEvaluated (session.go:650). When an SSH client's first auth method passes Evaluate() and register() but fails at connect(), the state is already saved as StateRegistered. On fallback to a second auth method, Auth() enters the StateRegistered case directly, skipping Evaluate() entirely. This allows bypassing disable_password by first attempting (and failing) public key auth, then falling back to password — the DisablePassword check never runs. See inline comment for details and fix.

Additional notes

  • Cloud admin NamespaceUpdate silently drops new settings: cloud/internal/admin/services/namespace.go:68-71 copies settings fields individually (SessionRecord, ConnectionAnnouncement) but does not copy the new DisablePassword/DisablePublicKey fields. When an admin updates a namespace through the cloud admin service, these settings will be silently reset to false. A companion cloud change is needed.

  • Previous review findings still open: The critical bson:"omitempty" bug on boolean fields (pkg/models/namespace.go:54-55) and the disable_public_key not being handled in the terminal login form (ui/src/components/Terminal/TerminalLoginForm.vue) from the prior review remain unaddressed.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread api/services/namespace_test.go Outdated
Comment thread api/services/namespace_test.go Outdated
Comment thread ssh/session/auther.go
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch from a34b829 to 486eeca Compare April 11, 2026 12:05
@henrybarreto henrybarreto changed the title feat: add disable password and disapble public key on namespace config feat: add SSH settings for device and namespace Apr 11, 2026
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch 3 times, most recently from f422ac4 to b2afb9b Compare April 11, 2026 12:55
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch 4 times, most recently from 305adb5 to d51218a Compare April 17, 2026 23:03
@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 21, 2026

Claude finished @gustavosbarreto's task in 1m 33s —— View job


Code Review Complete

Reviewed 69 files. 4 inline issues posted (2 high, 1 high, 1 medium).

Additional notes

  • Previous review findings still open: The critical bson:"omitempty" bug on boolean fields (pkg/models/namespace.go:54-62, pkg/models/device.go:104-112), the auth method restriction bypass via SSH fallback (ssh/session/session.go Auth() method), the upsert overwriting created_at and PK id columns (api/store/pg/device.go:165, api/store/pg/namespace.go:168), and the shallow copy sharing the Settings pointer in namespace tests (api/services/namespace_test.go) from prior reviews remain unaddressed.

  • Cloud admin NamespaceUpdate still drops new settings: cloud/internal/admin/services/namespace.go:69-70 copies only SessionRecord and ConnectionAnnouncement but not the 9 new Allow* fields. Admin namespace updates will silently reset these settings to false. A companion cloud change is still needed.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui-react/apps/console/src/pages/DeviceDetails.tsx
Comment thread openapi/spec/components/schemas/namespaceSettings.yaml Outdated
Comment thread ui/src/components/Setting/SettingAllowPassword.vue Outdated
Comment thread api/store/pg/device.go Outdated
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch 2 times, most recently from 9a6f26c to f2ba7e8 Compare April 21, 2026 22:47
@gustavosbarreto
Copy link
Copy Markdown
Member

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

Claude finished @gustavosbarreto's task in 39s —— View job


Code Review Complete

Reviewed 69 files. 1 inline issue posted (1 high).

Additional notes

  • Previous review findings still open: The critical bson:"omitempty" bug on boolean fields (pkg/models/namespace.go:54-62, pkg/models/device.go:104-112), the auth method restriction bypass via SSH fallback (ssh/session/session.go Auth() method), the upsert overwriting created_at and PK id columns (api/store/pg/device.go:184, api/store/pg/namespace.go:187), the shallow copy sharing the Settings pointer in namespace tests (api/services/namespace_test.go:1223), the allow_password and allow_public_key added to required in namespaceSettings.yaml:51-55 (breaking API change), and the dead code SettingAllowPassword.vue/SettingAllowPublicKey.vue from prior reviews remain unaddressed.

  • Cloud admin NamespaceUpdate still drops new settings: cloud/internal/admin/services/namespace.go:69-70 copies only SessionRecord and ConnectionAnnouncement but not the 9 new Allow* fields. Admin namespace updates will silently reset these settings to false. A companion cloud change is still needed.


If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui/src/utils/permission.ts Outdated
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch 2 times, most recently from 6491df4 to 1ad09f2 Compare April 26, 2026 16:27
@otavio
Copy link
Copy Markdown
Member

otavio commented May 6, 2026

Is it possible for you to do the rebase of this so we can trigger a not review?

@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch 2 times, most recently from 82451da to c5d0ed0 Compare May 6, 2026 22:18
Add allow-based SSH settings for both namespace and device
configuration, update the backend migrations and auth flow, and wire the
React console to edit the new settings consistently.

Closes #6136
@henrybarreto henrybarreto force-pushed the feat/allow-block-password-logins branch from c5d0ed0 to 8805817 Compare May 6, 2026 22:55
@otavio
Copy link
Copy Markdown
Member

otavio commented May 7, 2026

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

Claude finished @otavio's task in 3m 13s —— View job


Code Review Complete

Reviewed 74 files. 0 new inline issues posted — all findings match prior review threads.

Progress since prior reviews

Several issues from earlier reviews have been addressed:

  • FOREIGN KEY ... ON DELETE CASCADE added to both device_settings and namespace_settings tables
  • ✅ Stale permission entries (namespace:updateDisablePassword / namespace:updateDisablePublicKey) removed
  • TerminalLoginForm.vue now reads allow_password instead of the removed disable_password field
  • ✅ Fail-open TCP forwarding fixed — LocalPortForwardingCallback returns false when session/device is nil
  • ✅ Multi-step inserts/upserts wrapped in transactions (DeviceCreate, DeviceUpdate, NamespaceCreate, NamespaceUpdate)

Previous review findings still open

Critical:

  • Auth method restriction bypass via SSH fallback (ssh/session/session.go:661): When the first auth method passes Evaluate() and register() but fails at connect(), the state stays StateRegistered. On fallback to a second auth method, Auth() enters the StateRegistered case directly, skipping auth.Evaluate() entirely. This allows bypassing AllowPassword=false by first attempting public key auth, then falling back to password.

High:

  • bson:"omitempty" on security-critical boolean fields (pkg/models/namespace.go:55-63, pkg/models/device.go:106-114): All 9 new Allow* fields use bson:"...,omitempty". For booleans, omitempty causes false values to be omitted during BSON serialization. Note the pre-existing DeviceAutoAccept field correctly omits omitempty.

  • Upsert overwrites created_at to zero-time (api/store/pg/device.go:184, api/store/pg/namespace.go:187): The bare On("conflict (...) do update") without an explicit SET clause causes bun to auto-generate SET created_at = EXCLUDED.created_at using the Go zero time.Time. Compare with the scoped pattern in api/store/pg/tag.go:26.

  • Shallow copy shares Settings pointer in tests (api/services/namespace_test.go:1259-1260): expectedNamespace := *namespace creates a shallow copy where both variables point to the same *NamespaceSettings. Mutating one mutates both, so the mock always matches regardless of whether the service code actually applied the change. Same issue at lines 1307-1308 for AllowPublicKey.

  • allow_password and allow_public_key added to required in OpenAPI (openapi/spec/components/schemas/namespaceSettings.yaml:59-60): This is a breaking API change for existing clients. Only 2 of 9 new fields are marked required, creating inconsistency. The backend handles missing fields gracefully, so these don't need to be required.

  • Dead code: ui/src/components/Setting/SettingAllowPassword.vue and SettingAllowPublicKey.vue are not imported anywhere — the Vue UI uses the generic SettingNamespaceToggle.vue instead.

  • Device SSH toggles have no permission checks (ui-react/apps/console/src/pages/DeviceDetails.tsx:766-1006): All 9 SettingToggle components are rendered without a disabled prop. Unlike the namespace Settings.tsx page which gates each toggle with useHasPermission, any logged-in user can toggle device SSH settings. Also missing disabled={updateSSH.isPending} to prevent race conditions.

  • Cloud admin NamespaceUpdate still drops new settings (cloud/internal/admin/services/namespace.go:68-72): Copies only SessionRecord, ConnectionAnnouncement, MaxDevices, and DeviceAutoAccept but not the 9 new Allow* fields. Admin cannot manage SSH settings through the cloud admin API. A companion cloud change is needed.

Medium:

  • TerminalLoginForm.vue does not filter public key auth when allow_public_key=false — only password disable is handled
  • TestEditNamespace only covers 2 of 9 new settings — the test struct and request mapping at lines 1141-1394 only wire AllowPassword and AllowPublicKey, leaving 7 new service branches untested
  • Missing migration_121_test.go — virtually every other migration (1–120) has a test

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to disable password login and allow only public-key authentication

3 participants