From 18cef8280ad7dc0eebf38a57aad7e6a3988a8825 Mon Sep 17 00:00:00 2001 From: David Merris <annika@adhdgirl.dev> Date: Fri, 9 Aug 2024 11:32:09 -0400 Subject: [PATCH 1/5] [client] Allow setup keys to be provided in a file (#2337) Adds a flag and a bit of logic to allow a setup key to be passed in using a file. The flag should be exclusive with the standard --setup-key flag. --- client/cmd/login.go | 7 +++++++ client/cmd/root.go | 8 ++++++++ client/cmd/up.go | 7 +++++++ client/cmd/up_daemon_test.go | 31 +++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+) diff --git a/client/cmd/login.go b/client/cmd/login.go index 14c973d91..512fbb081 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -39,6 +39,13 @@ var loginCmd = &cobra.Command{ ctx = context.WithValue(ctx, system.DeviceNameCtxKey, hostName) } + if setupKeyPath != "" && setupKey == "" { + setupKey, err = getSetupKeyFromFile(setupKeyPath) + if err != nil { + return err + } + } + // workaround to run without service if logFile == "console" { err = handleRebrand(cmd) diff --git a/client/cmd/root.go b/client/cmd/root.go index db02ff5ea..b6d6694ee 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,11 @@ var CLIBackOffSettings = &backoff.ExponentialBackOff{ Clock: backoff.SystemClock, } +func getSetupKeyFromFile(setupKeyPath string) (string, error) { + data, err := os.ReadFile(setupKeyPath) + return string(data), err +} + 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..0eaf7bc0d 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -73,6 +73,13 @@ func upFunc(cmd *cobra.Command, args []string) error { ctx = context.WithValue(ctx, system.DeviceNameCtxKey, hostName) } + if setupKeyPath != "" && setupKey == "" { + setupKey, err = getSetupKeyFromFile(setupKeyPath) + if err != nil { + return err + } + } + if foregroundMode { return runInForegroundMode(ctx, cmd) } 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, From 12f9d12a11ba4dab74dc6af5a2bab8c647986759 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Fri, 9 Aug 2024 19:17:28 +0200 Subject: [PATCH 2/5] [misc] Update bug-issue-report.md to include netbird debug cmd (#2413) --- .github/ISSUE_TEMPLATE/bug-issue-report.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/ISSUE_TEMPLATE/bug-issue-report.md b/.github/ISSUE_TEMPLATE/bug-issue-report.md index 0aa6cd77e..8971972a2 100644 --- a/.github/ISSUE_TEMPLATE/bug-issue-report.md +++ b/.github/ISSUE_TEMPLATE/bug-issue-report.md @@ -35,6 +35,11 @@ Please specify whether you use NetBird Cloud or self-host NetBird's control plan If applicable, add the `netbird status -d' 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** If applicable, add screenshots to help explain your problem. From af1b42e538e74fe01b3f4398f95b4521d8ac3298 Mon Sep 17 00:00:00 2001 From: Maycon Santos <mlsmaycon@gmail.com> Date: Fri, 9 Aug 2024 20:38:58 +0200 Subject: [PATCH 3/5] [client] Parse data from setup key (#2411) refactor functions and variable assignment --- client/cmd/login.go | 12 +++++------- client/cmd/root.go | 12 +++++++++++- client/cmd/up.go | 21 ++++++++++++--------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/client/cmd/login.go b/client/cmd/login.go index 512fbb081..c7dd0fda1 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -39,11 +39,9 @@ var loginCmd = &cobra.Command{ ctx = context.WithValue(ctx, system.DeviceNameCtxKey, hostName) } - if setupKeyPath != "" && setupKey == "" { - setupKey, err = getSetupKeyFromFile(setupKeyPath) - if err != nil { - return err - } + providedSetupKey, err := getSetupKey() + if err != nil { + return err } // workaround to run without service @@ -69,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) } @@ -88,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 b6d6694ee..8dae6e273 100644 --- a/client/cmd/root.go +++ b/client/cmd/root.go @@ -256,9 +256,19 @@ 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) - return string(data), err + 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 { diff --git a/client/cmd/up.go b/client/cmd/up.go index 0eaf7bc0d..2ed6e41d2 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -73,13 +73,6 @@ func upFunc(cmd *cobra.Command, args []string) error { ctx = context.WithValue(ctx, system.DeviceNameCtxKey, hostName) } - if setupKeyPath != "" && setupKey == "" { - setupKey, err = getSetupKeyFromFile(setupKeyPath) - if err != nil { - return err - } - } - if foregroundMode { return runInForegroundMode(ctx, cmd) } @@ -154,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) @@ -161,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) } @@ -206,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, From 15eb752a7d574b33506cce9d90f4ff0b3700b827 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Sun, 11 Aug 2024 15:01:04 +0200 Subject: [PATCH 4/5] [misc] Update bug-issue-report.md to include anon flag (#2412) --- .github/ISSUE_TEMPLATE/bug-issue-report.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug-issue-report.md b/.github/ISSUE_TEMPLATE/bug-issue-report.md index 8971972a2..789c61974 100644 --- a/.github/ISSUE_TEMPLATE/bug-issue-report.md +++ b/.github/ISSUE_TEMPLATE/bug-issue-report.md @@ -31,9 +31,9 @@ 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?** From 539480a713c621917fd1de348ada8cc0ffe41e8f Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga <bethuelmbaga12@gmail.com> Date: Mon, 12 Aug 2024 13:48:05 +0300 Subject: [PATCH 5/5] [management] Prevent removal of All group from peers during user groups propagation (#2410) * Prevent removal of "All" group from peers * Prevent adding "All" group to users and setup keys * Refactor setup key group validation --- management/server/account.go | 2 +- management/server/setupkey.go | 23 +++++++++++++++--- management/server/setupkey_test.go | 39 ++++++++++++++++++++++++++---- management/server/user.go | 6 ++++- 4 files changed, 59 insertions(+), 11 deletions(-) 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