From 6790c34b081e4598227cca0ea302049b5fd0ff93 Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 16:24:00 +0200 Subject: [PATCH] [client] Let an unprivileged caller log out a profile with no identity The empty-key check sat behind requirePrivilegeForDeregistration, so an unprivileged logout of an identity-less profile was refused with PermissionDenied instead of completing as the no-op it is. And it was refused for most profiles, not a corner case: the gate arms whenever the SSH server is enabled, and sshServerEnabled reads an absent ServerSSHAllowed as enabled, so every legacy profile qualifies. The check now runs first. What the gate protects against is handing this machine's registered key to another management server; with no key there is nothing to hand over and nothing to protect. Reported by CodeRabbit and cubic-dev-ai on PR #7398, both on the same defect. --- client/server/server.go | 21 ++++++++++++--------- client/server/update_settings_gate_test.go | 9 +++++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/client/server/server.go b/client/server/server.go index a9f3a3c6d..61e234fb9 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -1578,20 +1578,23 @@ func (s *Server) sendLogoutRequestWithConfig(ctx context.Context, config *profil // Privilege gate: deregistering frees this machine's key to be registered // against another management server, which is only restricted while the SSH // server makes that a privilege handover. - if err := requirePrivilegeForDeregistration(ctx, config); err != nil { - return err - } - - // A profile with no identity was never registered — a logout clears the - // keys in place, so logging the same profile out twice lands here — and - // there is nothing to deregister. Reads no longer mint a key on the way - // past, so this is where that case surfaces instead of dialing management - // with a key it has never seen. + // Ahead of the privilege gate on purpose. A profile with no identity was + // never registered — a logout clears the keys in place, so logging the same + // profile out twice lands here — so there is nothing to deregister and + // nothing for the gate to protect: what it guards against is handing this + // machine's registered key to another management server. Behind the gate, + // an unprivileged caller would be refused instead, and for a profile whose + // ServerSSHAllowed is unset that is every caller, since an absent value + // counts as SSH enabled. if config.PrivateKey == "" { log.Infof("profile carries no identity, nothing to deregister") return nil } + if err := requirePrivilegeForDeregistration(ctx, config); err != nil { + return err + } + key, err := wgtypes.ParseKey(config.PrivateKey) if err != nil { return fmt.Errorf("parse private key: %w", err) diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go index 812ef3554..f167b593b 100644 --- a/client/server/update_settings_gate_test.go +++ b/client/server/update_settings_gate_test.go @@ -377,4 +377,13 @@ func TestLogout_ProfileWithoutAnIdentityIsANoOp(t *testing.T) { require.NoError(t, err) require.NoError(t, s.sendLogoutRequestWithConfig(privilegedTestCtx(), stored), "logging out an identity-less profile must not fail") + + // And for an unprivileged caller too: the deregistration privilege gate + // guards the handover of a registered key, so with no key there is nothing + // to guard. An unset SSH setting is what arms that gate — sshServerEnabled + // reads an absent value as enabled — so this stands in for every legacy + // profile, where behind the gate the caller would be refused. + stored.ServerSSHAllowed = nil + require.NoError(t, s.sendLogoutRequestWithConfig(userCtx(), stored), + "an unprivileged caller could not log out a profile with nothing to deregister") }