mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-12 17:59:06 +02:00
[client] Treat a profile with no identity as already deregistered
Two findings on the same consequence of pure reads: a profile can legitimately carry no keys, because logging out clears them in place. - sendLogoutRequestWithConfig went straight to wgtypes.ParseKey and failed with "incorrect key size: 0" on the second logout of the same profile. There is nothing to deregister for a peer that was never registered, so it returns cleanly. Before pure reads this case was hidden: the read minted a key and the daemon dialed management with one it had never seen. - The mobile logout read the config with the generating reader right after checking the file exists. The two are not atomic, so a profile removed in between was resolved from the defaults and recreated by the write that follows. It uses the existing-file reader now. Reported by cubic-dev-ai and CodeRabbit on PR #7398.
This commit is contained in:
@@ -174,7 +174,10 @@ func (pm *ProfileManager) LogoutProfile(id string) error {
|
||||
return fmt.Errorf("profile %q does not exist", id)
|
||||
}
|
||||
|
||||
config, err := profilemanager.ReadOrGenerateConfig(configPath)
|
||||
// The existing-file reader, not the generating one: the check above is not
|
||||
// atomic with this read, so a profile removed in between would otherwise be
|
||||
// resolved from the defaults here and recreated by the write below.
|
||||
config, err := profilemanager.GetExistingConfig(configPath)
|
||||
if err != nil {
|
||||
return fmt.Errorf("read profile config: %w", err)
|
||||
}
|
||||
|
||||
@@ -1577,6 +1577,16 @@ func (s *Server) sendLogoutRequestWithConfig(ctx context.Context, config *profil
|
||||
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.
|
||||
if config.PrivateKey == "" {
|
||||
log.Infof("profile carries no identity, nothing to deregister")
|
||||
return nil
|
||||
}
|
||||
|
||||
key, err := wgtypes.ParseKey(config.PrivateKey)
|
||||
if err != nil {
|
||||
return fmt.Errorf("parse private key: %w", err)
|
||||
|
||||
@@ -360,3 +360,21 @@ func TestLogin_ChangeThatAppearsMidRequestIsRefused(t *testing.T) {
|
||||
require.Equal(t, "https://mgmt.elsewhere.example:443", stored.ManagementURL.String(),
|
||||
"the refused login wrote the management URL it was asked for")
|
||||
}
|
||||
|
||||
// Logging out a profile that was already logged out must not fail: the logout
|
||||
// clears the keys in place, so the second attempt finds a profile with no
|
||||
// identity, which was never registered and has nothing to deregister.
|
||||
func TestLogout_ProfileWithoutAnIdentityIsANoOp(t *testing.T) {
|
||||
s, _, _, _, cfgPath := setupServerWithProfile(t)
|
||||
|
||||
loggedOut, err := profilemanager.GetExistingConfig(cfgPath)
|
||||
require.NoError(t, err)
|
||||
loggedOut.PrivateKey = ""
|
||||
loggedOut.SSHKey = ""
|
||||
require.NoError(t, profilemanager.WriteOutConfig(cfgPath, loggedOut))
|
||||
|
||||
stored, err := profilemanager.GetExistingConfig(cfgPath)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, s.sendLogoutRequestWithConfig(privilegedTestCtx(), stored),
|
||||
"logging out an identity-less profile must not fail")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user