diff --git a/.github/ISSUE_TEMPLATE/bug-issue-report.md b/.github/ISSUE_TEMPLATE/bug-issue-report.md index 0aa6cd77e..789c61974 100644 --- a/.github/ISSUE_TEMPLATE/bug-issue-report.md +++ b/.github/ISSUE_TEMPLATE/bug-issue-report.md @@ -31,9 +31,14 @@ Please specify whether you use NetBird Cloud or self-host NetBird's control plan `netbird version` -**NetBird status -d output:** +**NetBird status -dA output:** -If applicable, add the `netbird status -d' command output. +If applicable, add the `netbird status -dA' command output. + +**Do you face any client issues on desktop?** + +Please provide the file created by `netbird debug for 1m -AS`. +We advise reviewing the anonymized files for any remaining PII. **Screenshots** diff --git a/client/cmd/login.go b/client/cmd/login.go index 14c973d91..c7dd0fda1 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -39,6 +39,11 @@ var loginCmd = &cobra.Command{ ctx = context.WithValue(ctx, system.DeviceNameCtxKey, hostName) } + providedSetupKey, err := getSetupKey() + if err != nil { + return err + } + // workaround to run without service if logFile == "console" { err = handleRebrand(cmd) @@ -62,7 +67,7 @@ var loginCmd = &cobra.Command{ config, _ = internal.UpdateOldManagementURL(ctx, config, configPath) - err = foregroundLogin(ctx, cmd, config, setupKey) + err = foregroundLogin(ctx, cmd, config, providedSetupKey) if err != nil { return fmt.Errorf("foreground login failed: %v", err) } @@ -81,7 +86,7 @@ var loginCmd = &cobra.Command{ client := proto.NewDaemonServiceClient(conn) loginRequest := proto.LoginRequest{ - SetupKey: setupKey, + SetupKey: providedSetupKey, ManagementUrl: managementURL, IsLinuxDesktopClient: isLinuxRunningDesktop(), Hostname: hostName, diff --git a/client/cmd/root.go b/client/cmd/root.go index db02ff5ea..8dae6e273 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -56,6 +56,7 @@ var ( managementURL string adminURL string setupKey string + setupKeyPath string hostName string preSharedKey string natExternalIPs []string @@ -128,6 +129,8 @@ func init() { rootCmd.PersistentFlags().StringVarP(&logLevel, "log-level", "l", "info", "sets Netbird log level") rootCmd.PersistentFlags().StringVar(&logFile, "log-file", defaultLogFile, "sets Netbird log path. If console is specified the log will be output to stdout. If syslog is specified the log will be sent to syslog daemon.") rootCmd.PersistentFlags().StringVarP(&setupKey, "setup-key", "k", "", "Setup key obtained from the Management Service Dashboard (used to register peer)") + rootCmd.PersistentFlags().StringVar(&setupKeyPath, "setup-key-file", "", "The path to a setup key obtained from the Management Service Dashboard (used to register peer) This is ignored if the setup-key flag is provided.") + rootCmd.MarkFlagsMutuallyExclusive("setup-key", "setup-key-file") rootCmd.PersistentFlags().StringVar(&preSharedKey, preSharedKeyFlag, "", "Sets Wireguard PreSharedKey property. If set, then only peers that have the same key can communicate.") rootCmd.PersistentFlags().StringVarP(&hostName, "hostname", "n", "", "Sets a custom hostname for the device") rootCmd.PersistentFlags().BoolVarP(&anonymizeFlag, "anonymize", "A", false, "anonymize IP addresses and non-netbird.io domains in logs and status output") @@ -253,6 +256,21 @@ var CLIBackOffSettings = &backoff.ExponentialBackOff{ Clock: backoff.SystemClock, } +func getSetupKey() (string, error) { + if setupKeyPath != "" && setupKey == "" { + return getSetupKeyFromFile(setupKeyPath) + } + return setupKey, nil +} + +func getSetupKeyFromFile(setupKeyPath string) (string, error) { + data, err := os.ReadFile(setupKeyPath) + if err != nil { + return "", fmt.Errorf("failed to read setup key file: %v", err) + } + return strings.TrimSpace(string(data)), nil +} + func handleRebrand(cmd *cobra.Command) error { var err error if logFile == defaultLogFile { diff --git a/client/cmd/up.go b/client/cmd/up.go index f69e9eb27..2ed6e41d2 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -147,6 +147,11 @@ func runInForegroundMode(ctx context.Context, cmd *cobra.Command) error { ic.DNSRouteInterval = &dnsRouteInterval } + providedSetupKey, err := getSetupKey() + if err != nil { + return err + } + config, err := internal.UpdateOrCreateConfig(ic) if err != nil { return fmt.Errorf("get config file: %v", err) @@ -154,7 +159,7 @@ func runInForegroundMode(ctx context.Context, cmd *cobra.Command) error { config, _ = internal.UpdateOldManagementURL(ctx, config, configPath) - err = foregroundLogin(ctx, cmd, config, setupKey) + err = foregroundLogin(ctx, cmd, config, providedSetupKey) if err != nil { return fmt.Errorf("foreground login failed: %v", err) } @@ -199,8 +204,13 @@ func runInDaemonMode(ctx context.Context, cmd *cobra.Command) error { return nil } + providedSetupKey, err := getSetupKey() + if err != nil { + return err + } + loginRequest := proto.LoginRequest{ - SetupKey: setupKey, + SetupKey: providedSetupKey, ManagementUrl: managementURL, AdminURL: adminURL, NatExternalIPs: natExternalIPs, diff --git a/client/cmd/up_daemon_test.go b/client/cmd/up_daemon_test.go index 0295d2b21..daf8d0628 100644 --- a/client/cmd/up_daemon_test.go +++ b/client/cmd/up_daemon_test.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "os" "testing" "time" @@ -40,6 +41,36 @@ func TestUpDaemon(t *testing.T) { return } + // Test the setup-key-file flag. + tempFile, err := os.CreateTemp("", "setup-key") + if err != nil { + t.Errorf("could not create temp file, got error %v", err) + return + } + defer os.Remove(tempFile.Name()) + if _, err := tempFile.Write([]byte("A2C8E62B-38F5-4553-B31E-DD66C696CEBB")); err != nil { + t.Errorf("could not write to temp file, got error %v", err) + return + } + if err := tempFile.Close(); err != nil { + t.Errorf("unable to close file, got error %v", err) + } + rootCmd.SetArgs([]string{ + "login", + "--daemon-addr", "tcp://" + cliAddr, + "--setup-key-file", tempFile.Name(), + "--log-file", "", + }) + if err := rootCmd.Execute(); err != nil { + t.Errorf("expected no error while running up command, got %v", err) + return + } + time.Sleep(time.Second * 3) + if status, err := state.Status(); err != nil && status != internal.StatusIdle { + t.Errorf("wrong status after login: %s, %v", internal.StatusIdle, err) + return + } + rootCmd.SetArgs([]string{ "up", "--daemon-addr", "tcp://" + cliAddr, diff --git a/management/server/account.go b/management/server/account.go index ca53ebad0..972272746 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -922,7 +922,7 @@ func (a *Account) UserGroupsAddToPeers(userID string, groups ...string) { func (a *Account) UserGroupsRemoveFromPeers(userID string, groups ...string) { for _, gid := range groups { group, ok := a.Groups[gid] - if !ok { + if !ok || group.Name == "All" { continue } update := make([]string, 0, len(group.Peers)) diff --git a/management/server/setupkey.go b/management/server/setupkey.go index 8ef91755c..859f1b0b9 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -223,10 +223,8 @@ func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID s return nil, err } - for _, group := range autoGroups { - if _, ok := account.Groups[group]; !ok { - return nil, status.Errorf(status.NotFound, "group %s doesn't exist", group) - } + if err := validateSetupKeyAutoGroups(account, autoGroups); err != nil { + return nil, err } setupKey := GenerateSetupKey(keyName, keyType, keyDuration, autoGroups, usageLimit, ephemeral) @@ -279,6 +277,10 @@ func (am *DefaultAccountManager) SaveSetupKey(ctx context.Context, accountID str return nil, status.Errorf(status.NotFound, "setup key not found") } + if err := validateSetupKeyAutoGroups(account, keyToSave.AutoGroups); err != nil { + return nil, err + } + // only auto groups, revoked status, and name can be updated for now newKey := oldKey.Copy() newKey.Name = keyToSave.Name @@ -399,3 +401,16 @@ func (am *DefaultAccountManager) GetSetupKey(ctx context.Context, accountID, use return foundKey, nil } + +func validateSetupKeyAutoGroups(account *Account, autoGroups []string) error { + for _, group := range autoGroups { + g, ok := account.Groups[group] + if !ok { + return status.Errorf(status.NotFound, "group %s doesn't exist", group) + } + if g.Name == "All" { + return status.Errorf(status.InvalidArgument, "can't add All group to the setup key") + } + } + return nil +} diff --git a/management/server/setupkey_test.go b/management/server/setupkey_test.go index 034f4e2d6..aa5075b02 100644 --- a/management/server/setupkey_test.go +++ b/management/server/setupkey_test.go @@ -26,10 +26,17 @@ func TestDefaultAccountManager_SaveSetupKey(t *testing.T) { t.Fatal(err) } - err = manager.SaveGroup(context.Background(), account.Id, userID, &nbgroup.Group{ - ID: "group_1", - Name: "group_name_1", - Peers: []string{}, + err = manager.SaveGroups(context.Background(), account.Id, userID, []*nbgroup.Group{ + { + ID: "group_1", + Name: "group_name_1", + Peers: []string{}, + }, + { + ID: "group_2", + Name: "group_name_2", + Peers: []string{}, + }, }) if err != nil { t.Fatal(err) @@ -70,6 +77,19 @@ func TestDefaultAccountManager_SaveSetupKey(t *testing.T) { assert.NotEmpty(t, ev.Meta["key"]) assert.Equal(t, userID, ev.InitiatorID) assert.Equal(t, key.Id, ev.TargetID) + + groupAll, err := account.GetGroupAll() + assert.NoError(t, err) + + // saving setup key with All group assigned to auto groups should return error + autoGroups = append(autoGroups, groupAll.ID) + _, err = manager.SaveSetupKey(context.Background(), account.Id, &SetupKey{ + Id: key.Id, + Name: newKeyName, + Revoked: revoked, + AutoGroups: autoGroups, + }, userID) + assert.Error(t, err, "should not save setup key with All group assigned in auto groups") } func TestDefaultAccountManager_CreateSetupKey(t *testing.T) { @@ -102,6 +122,9 @@ func TestDefaultAccountManager_CreateSetupKey(t *testing.T) { t.Fatal(err) } + groupAll, err := account.GetGroupAll() + assert.NoError(t, err) + type testCase struct { name string @@ -134,8 +157,14 @@ func TestDefaultAccountManager_CreateSetupKey(t *testing.T) { expectedGroups: []string{"FAKE"}, expectedFailure: true, } + testCase3 := testCase{ + name: "Create Setup Key should fail because of All group", + expectedKeyName: "my-test-key", + expectedGroups: []string{groupAll.ID}, + expectedFailure: true, + } - for _, tCase := range []testCase{testCase1, testCase2} { + for _, tCase := range []testCase{testCase1, testCase2, testCase3} { t.Run(tCase.name, func(t *testing.T) { key, err := manager.CreateSetupKey(context.Background(), account.Id, tCase.expectedKeyName, SetupKeyReusable, expiresIn, tCase.expectedGroups, SetupKeyUnlimitedUsage, userID, false) diff --git a/management/server/user.go b/management/server/user.go index 8fd0a1627..727bc5c6b 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -944,10 +944,14 @@ func validateUserUpdate(account *Account, initiatorUser, oldUser, update *User) } for _, newGroupID := range update.AutoGroups { - if _, ok := account.Groups[newGroupID]; !ok { + group, ok := account.Groups[newGroupID] + if !ok { return status.Errorf(status.InvalidArgument, "provided group ID %s in the user %s update doesn't exist", newGroupID, update.Id) } + if group.Name == "All" { + return status.Errorf(status.InvalidArgument, "can't add All group to the user") + } } return nil