Skip to content

feat: add SSH settings for device and namespace#6148

Open
henrybarreto wants to merge 1 commit intomasterfrom
feat/allow-block-password-logins
Open

feat: add SSH settings for device and namespace#6148
henrybarreto wants to merge 1 commit 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.

@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 on lines 78 to +81
func (p *publicKeyAuth) Evaluate(session *Session) error {
if session.Namespace.Settings.DisablePublicKey {
return ErrPublicKeyDisabled
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: DisablePublicKey check can be bypassed via SSH auth method fallback

This Evaluate() check and the corresponding DisablePassword check in passwordAuth.Evaluate() are only called from Auth() in session.go when the session state is StateEvaluated (line 650). When the first auth method (e.g., public key) passes Evaluate() and register(), the state advances to StateRegistered (line 658) before connect() runs. If connect() then fails (unauthorized key), the state remains StateRegistered. When the SSH client falls back to a second auth method (e.g., password), Auth() enters the StateRegistered case directly, completely skipping auth.Evaluate().

Attack scenario: Admin sets disable_password: true. An attacker with no authorized key: (1) attempts public key auth — Evaluate() passes (public key not disabled), register() succeeds → state becomes StateRegistered, connect() fails; (2) falls back to password auth — enters StateRegistered, passwordAuth.Evaluate() is never called, DisablePassword check is bypassed, password is forwarded to the agent. Same bypass works in reverse for disable_public_key.

Fix: In session.go Auth() method, move the auth.Evaluate(sess) call before the switch state block so it runs on every authentication attempt regardless of cached session state.

Fix this →

@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
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 b2afb9b to 32cfb65 Compare April 11, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

2 participants