From e6503fc5ad6ddb7575254d990834ef09f2e4462c Mon Sep 17 00:00:00 2001 From: Bartosz Oleaczek Date: Wed, 3 Jul 2024 11:07:52 +0200 Subject: [PATCH 1/4] Restart norduser/tray when user switches from GUI interface to text only --- cmd/daemon/main.go | 12 +- norduser/environ.go | 39 +++ norduser/environ_test.go | 55 ++++ norduser/group.go | 95 ++++++ norduser/group_monitor.go | 310 ------------------- norduser/group_monitor_test.go | 226 -------------- norduser/group_test.go | 91 ++++++ norduser/process_monitor.go | 246 ++++++++++++++++ norduser/process_monitor_snap.go | 78 +++++ norduser/process_monitor_test.go | 393 +++++++++++++++++++++++++ norduser/utmp.go | 82 +++--- test/mock/norduser/service/combined.go | 73 ++++- 12 files changed, 1112 insertions(+), 588 deletions(-) create mode 100644 norduser/environ.go create mode 100644 norduser/environ_test.go create mode 100644 norduser/group.go delete mode 100644 norduser/group_monitor.go delete mode 100644 norduser/group_monitor_test.go create mode 100644 norduser/group_test.go create mode 100644 norduser/process_monitor.go create mode 100644 norduser/process_monitor_snap.go create mode 100644 norduser/process_monitor_test.go diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index 27f64ff0..01f3aa65 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -488,10 +488,16 @@ func main() { grpc.Creds(internal.NewUnixSocketCredentials(internal.NewDaemonAuthenticator())), } - norduserMonitor := norduser.NewNordVPNGroupMonitor(norduserService) + norduserMonitor := norduser.NewNorduserProcessMonitor(norduserService) go func() { - if err := norduserMonitor.Start(); err != nil { - log.Println("Error when starting norduser monitor: ", err.Error()) + if snapconf.IsUnderSnap() { + if err := norduserMonitor.StartSnap(); err != nil { + log.Println("Error when starting norduser monitor for snap: ", err.Error()) + } + } else { + if err := norduserMonitor.Start(); err != nil { + log.Println("Error when starting norduser monitor: ", err.Error()) + } } }() diff --git a/norduser/environ.go b/norduser/environ.go new file mode 100644 index 00000000..5f0445af --- /dev/null +++ b/norduser/environ.go @@ -0,0 +1,39 @@ +package norduser + +import ( + "fmt" + "os" + "strings" +) + +func findVariable(name string, environment string) string { + variables := strings.Split(environment, "\000") + for _, variable := range variables { + split := strings.Split(variable, "=") + if len(split) < 1 { + continue + } + + if split[0] != name { + continue + } + + if len(split) != 2 { + return "" + } + + return split[1] + } + + return "" +} + +func findEnvVariableForPID(pid uint32, name string) (string, error) { + path := fmt.Sprintf("/proc/%d/environ", pid) + environment, err := os.ReadFile(path) + if err != nil { + return "", fmt.Errorf("reading file: %w", err) + } + + return findVariable(name, string(environment)), nil +} diff --git a/norduser/environ_test.go b/norduser/environ_test.go new file mode 100644 index 00000000..0f8d4c45 --- /dev/null +++ b/norduser/environ_test.go @@ -0,0 +1,55 @@ +package norduser + +import ( + "fmt" + "testing" + + "github.com/NordSecurity/nordvpn-linux/test/category" + "github.com/stretchr/testify/assert" +) + +func Test_findVariable(t *testing.T) { + category.Set(t, category.Unit) + + const variable1 string = "VAR_1" + const value1 string = "val_1" + + const variable2 string = "VAR_2" + const value2 string = "val_2" + + const emptyVariable string = "VAR_3" + + environment := fmt.Sprintf("%s=%s\000%s=%s\000%s=", + variable1, value1, + variable2, value2, + emptyVariable) + + tests := []struct { + name string + variableName string + expectedValue string + }{ + { + name: "normal variable", + variableName: variable1, + expectedValue: value1, + }, + { + name: "empty variable", + variableName: emptyVariable, + expectedValue: "", + }, + { + name: "variable doesnt exist", + variableName: "NO_VARIABLE", + expectedValue: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + variableValue := findVariable(test.variableName, environment) + assert.Equal(t, test.expectedValue, variableValue) + }) + } +} diff --git a/norduser/group.go b/norduser/group.go new file mode 100644 index 00000000..345052a3 --- /dev/null +++ b/norduser/group.go @@ -0,0 +1,95 @@ +package norduser + +import ( + "fmt" + "os/user" + "regexp" + "strconv" + "strings" + + "github.com/NordSecurity/nordvpn-linux/internal" +) + +func findGroupEntry(groups string, groupName string) string { + r, _ := regexp.Compile(fmt.Sprintf("^%s:", groupName)) + + for _, groupEntry := range strings.Split(groups, "\n") { + if r.MatchString(groupEntry) { + return groupEntry + } + } + + return "" +} + +func getGroupEntry(groupName string) (string, error) { + file, err := internal.FileRead(groupFilePath) + if err != nil { + return "", fmt.Errorf("failed to read group file: %w", err) + } + + groupEntry := findGroupEntry(string(file), groupName) + if groupEntry == "" { + return "", fmt.Errorf("group entry not found: %w", err) + } + + return groupEntry, nil +} + +func getGroupMembers(groupEntry string) []string { + groupEntry = strings.TrimSuffix(groupEntry, "\n") + sepIndex := strings.LastIndex(groupEntry, ":") + groupMembers := groupEntry[sepIndex+1:] + + if groupMembers == "" { + return []string{} + } + + return strings.Split(groupMembers, ",") +} + +func getNordVPNGroupMembers() ([]string, error) { + const nordvpnGroupName = "nordvpn" + + groupEntry, err := getGroupEntry(nordvpnGroupName) + if err != nil { + return nil, fmt.Errorf("getting group entry: %w", err) + } + + return getGroupMembers(groupEntry), nil +} + +type userIDs struct { + uid uint32 + gid uint32 + home string +} + +type userIDGetter interface { + getUserID(username string) (userIDs, error) +} + +type osGetter struct{} + +func (osGetter) getUserID(username string) (userIDs, error) { + user, err := user.Lookup(username) + if err != nil { + return userIDs{}, fmt.Errorf("looking up user: %w", err) + } + + uid, err := strconv.Atoi(user.Uid) + if err != nil { + return userIDs{}, fmt.Errorf("converting uid string to int: %w", err) + } + + gid, err := strconv.Atoi(user.Gid) + if err != nil { + return userIDs{}, fmt.Errorf("converting uid string to int: %w", err) + } + + return userIDs{ + uid: uint32(uid), + gid: uint32(gid), + home: user.HomeDir, + }, nil +} diff --git a/norduser/group_monitor.go b/norduser/group_monitor.go deleted file mode 100644 index a4417add..00000000 --- a/norduser/group_monitor.go +++ /dev/null @@ -1,310 +0,0 @@ -package norduser - -import ( - "fmt" - "log" - "os/user" - "regexp" - "slices" - "strconv" - "strings" - - "github.com/fsnotify/fsnotify" - - "github.com/NordSecurity/nordvpn-linux/internal" - "github.com/NordSecurity/nordvpn-linux/norduser/service" - "github.com/NordSecurity/nordvpn-linux/snapconf" -) - -const ( - groupFilePath = "/etc/group" - utmpFilePath = "/var/run/utmp" -) - -type userState int - -const ( - notActive = iota - userActive - norduserRunning -) - -type userSet map[string]userState - -func findGroupEntry(groups string, groupName string) string { - r, _ := regexp.Compile(fmt.Sprintf("^%s:", groupName)) - - for _, groupEntry := range strings.Split(groups, "\n") { - if r.MatchString(groupEntry) { - return groupEntry - } - } - - return "" -} - -func getGroupEntry(groupName string) (string, error) { - file, err := internal.FileRead(groupFilePath) - if err != nil { - return "", fmt.Errorf("failed to read group file: %w", err) - } - - groupEntry := findGroupEntry(string(file), groupName) - if groupEntry == "" { - return "", fmt.Errorf("group entry not found: %w", err) - } - - return groupEntry, nil -} - -func getGroupMembers(groupEntry string) userSet { - groupEntry = strings.TrimSuffix(groupEntry, "\n") - sepIndex := strings.LastIndex(groupEntry, ":") - groupMembers := groupEntry[sepIndex+1:] - members := make(userSet) - - if groupMembers == "" { - return members - } - - for _, member := range strings.Split(groupMembers, ",") { - members[member] = notActive - } - - return members -} - -func getNordVPNGroupMembers() (userSet, error) { - const nordvpnGroupName = "nordvpn" - - groupEntry, err := getGroupEntry(nordvpnGroupName) - if err != nil { - return nil, fmt.Errorf("getting group entry: %w", err) - } - - groupMembers := getGroupMembers(groupEntry) - - return groupMembers, nil -} - -type userIDs struct { - uid uint32 - gid uint32 - home string -} - -type userIDGetter interface { - getUserID(username string) (userIDs, error) -} - -type osGetter struct{} - -func (osGetter) getUserID(username string) (userIDs, error) { - user, err := user.Lookup(username) - if err != nil { - return userIDs{}, fmt.Errorf("looking up user: %w", err) - } - - uid, err := strconv.Atoi(user.Uid) - if err != nil { - return userIDs{}, fmt.Errorf("converting uid string to int: %w", err) - } - - gid, err := strconv.Atoi(user.Gid) - if err != nil { - return userIDs{}, fmt.Errorf("converting uid string to int: %w", err) - } - - return userIDs{ - uid: uint32(uid), - gid: uint32(gid), - home: user.HomeDir, - }, nil -} - -func updateGroupMembersState(groupMembers userSet, activeUsers []string) userSet { - for member := range groupMembers { - if idx := slices.Index(activeUsers, member); idx != -1 { - groupMembers[member] = userActive - } - } - - return groupMembers -} - -// NordVPNGroupMonitor monitors the nordvpn system group and starts/stops norduserd for users added/removed from the -// group. -type NordVPNGroupMonitor struct { - norduserd service.NorduserService - isSnap bool - userIDGetter -} - -func NewNordVPNGroupMonitor(service service.NorduserService) NordVPNGroupMonitor { - return NordVPNGroupMonitor{ - norduserd: service, - isSnap: snapconf.IsUnderSnap(), - userIDGetter: osGetter{}, - } -} - -func (n *NordVPNGroupMonitor) handleGroupUpdate(currentGroupMembers userSet, newGroupMembers userSet) userSet { - for member, state := range currentGroupMembers { - // member is still in group and session is active, do nothing - if newState, ok := newGroupMembers[member]; ok && newState == userActive { - if state == norduserRunning { - newGroupMembers[member] = norduserRunning - } - continue - } - - if state != norduserRunning { - continue - } - - userIDs, err := n.getUserID(member) - if err != nil { - // store the old state, since norduserd is still running for this member - newGroupMembers[member] = state - log.Println(internal.ErrorPrefix, "failed to look up UID/GID of deleted group member: ", err) - continue - } - - if err := n.norduserd.Stop(userIDs.uid, false); err != nil { - newGroupMembers[member] = state - log.Println(internal.ErrorPrefix, "disabling norduserd for user:", err.Error()) - } - } - - if n.isSnap { - // in snap environment norduser is enabled on the cli side - return newGroupMembers - } - - // enable norduserd for new members - for member, status := range newGroupMembers { - // we only want to start norduser when user is active but norduser is not active - if status != userActive { - continue - } - - userID, err := n.getUserID(member) - if err != nil { - log.Println("failed to lookup UID/GID for new group member:", err) - continue - } - - if err := n.norduserd.Enable(userID.uid, userID.gid, userID.home); err != nil { - log.Println(internal.ErrorPrefix, "enabling norduserd for member:", err) - continue - } - - newGroupMembers[member] = norduserRunning - } - - return newGroupMembers -} - -func (n *NordVPNGroupMonitor) handleGropuFileUpdate(currentGroupMembers userSet) (userSet, error) { - newGroupMembers, err := getNordVPNGroupMembers() - if err != nil { - return nil, fmt.Errorf("failed to get new group members: %w", err) - } - - activeUsers, err := getActiveUsers() - if err != nil { - return nil, fmt.Errorf("failed to get active users after group file update: %w", err) - } - newGroupMembers = updateGroupMembersState(newGroupMembers, activeUsers) - - return n.handleGroupUpdate(currentGroupMembers, newGroupMembers), nil -} - -func (n *NordVPNGroupMonitor) startForEveryGroupMember(groupMembers userSet) { - for member, status := range groupMembers { - if status == notActive { - continue - } - - user, err := n.getUserID(member) - if err != nil { - log.Println(internal.ErrorPrefix, "failed to get UID/GID for group member:", err) - continue - } - - if err := n.norduserd.Enable(user.uid, user.gid, user.home); err != nil { - log.Println(internal.ErrorPrefix, "failed to start norduser for group member:", err) - continue - } - - groupMembers[member] = norduserRunning - } -} - -// Start blocks the thread and starts monitoring for changes in the nordvpn group. -func (n *NordVPNGroupMonitor) Start() error { - const etcPath = "/etc" - - watcher, err := fsnotify.NewWatcher() - if err != nil { - return fmt.Errorf("creating new watcher: %w", err) - } - - if err := watcher.Add(etcPath); err != nil { - return fmt.Errorf("adding group file to watcher: %w", err) - } - - if err := watcher.Add(utmpFilePath); err != nil { - return fmt.Errorf("adding utmp file to watcher: %w", err) - } - - currentGrupMembers, err := getNordVPNGroupMembers() - if err != nil { - return fmt.Errorf("getting initial group members: %w", err) - } - - activeUsers, err := getActiveUsers() - if err != nil { - return fmt.Errorf("getting initial active users: %w", err) - } - - currentGrupMembers = updateGroupMembersState(currentGrupMembers, activeUsers) - if !n.isSnap { // in snap environment norduser is enabled on the cli side - n.startForEveryGroupMember(currentGrupMembers) - } - - defer watcher.Close() - for { - select { - case event, ok := <-watcher.Events: - if !ok { - return fmt.Errorf("groupfile monitor channel closed") - } - - if event.Name == groupFilePath { - // Because utilities used to modify the group do so atomically, we also need to monitor for creation of - // the file instead of modifications. - if (event.Has(fsnotify.Create) || event.Has(fsnotify.Write)) && event.Name == groupFilePath { - if newGroupMembers, err := n.handleGropuFileUpdate(currentGrupMembers); err != nil { - log.Println(internal.ErrorPrefix, "failed to handle change of groupfile: ", err) - } else { - currentGrupMembers = newGroupMembers - } - } - } else if event.Name == utmpFilePath { - activeUsers, err := getActiveUsers() - if err == nil { - newGroupMembers := updateGroupMembersState(currentGrupMembers, activeUsers) - currentGrupMembers = n.handleGroupUpdate(currentGrupMembers, newGroupMembers) - } else { - log.Println(internal.ErrorPrefix, "failed to get active users after utmp file update: ", err) - } - } - case err, ok := <-watcher.Errors: - if !ok { - return fmt.Errorf("groupfile monitor error channel closed") - } - log.Println(internal.ErrorPrefix, "group monitor error:", err) - } - } -} diff --git a/norduser/group_monitor_test.go b/norduser/group_monitor_test.go deleted file mode 100644 index bf8de3dc..00000000 --- a/norduser/group_monitor_test.go +++ /dev/null @@ -1,226 +0,0 @@ -package norduser - -import ( - "fmt" - "strings" - "testing" - - "github.com/NordSecurity/nordvpn-linux/test/category" - nordusermock "github.com/NordSecurity/nordvpn-linux/test/mock/norduser/service" - "github.com/stretchr/testify/assert" -) - -type userIDGetterMock struct { - UsernameToIDs map[string]userIDs - GetErr error -} - -func newUserIDGetterMock(usernameToUserIDs map[string]userIDs) *userIDGetterMock { - return &userIDGetterMock{ - UsernameToIDs: usernameToUserIDs, - } -} - -func (u *userIDGetterMock) getUserID(username string) (userIDs, error) { - if u.GetErr != nil { - return userIDs{}, u.GetErr - } - - userID, ok := u.UsernameToIDs[username] - if !ok { - return userIDs{}, fmt.Errorf("user ids not found") - } - - return userID, nil -} - -func Test_findGroupEntry(t *testing.T) { - category.Set(t, category.Unit) - - rootGroup := "root:x:0:" - groups := []string{ - rootGroup, - "daemon:x:1:user", - "bin:x:2:", - "sys:x:3:", - "adm:x:4:syslog,user", - } - - groupFile := strings.Join(groups, "\n") - - tests := []struct { - name string - groupName string - expectedResult string - }{ - { - name: "group exists", - groupName: "root", - expectedResult: rootGroup, - }, - { - name: "group does not exist", - groupName: "test", - expectedResult: "", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - result := findGroupEntry(groupFile, test.groupName) - assert.Equal(t, test.expectedResult, result) - }) - } -} - -func Test_getGroupMembers(t *testing.T) { - category.Set(t, category.Unit) - - tests := []struct { - name string - groupEntry string - expectedResult userSet - }{ - { - name: "single user group", - groupEntry: "nordvpn:x:996:user", - expectedResult: userSet{ - "user": notActive, - }, - }, - { - name: "multi user group", - groupEntry: "nordvpn:x:996:user1,user2,user3", - expectedResult: userSet{ - "user1": notActive, - "user2": notActive, - "user3": notActive, - }, - }, - { - name: "empty group", - groupEntry: "nordvpn:x:996:", - expectedResult: userSet{}, - }, - { - name: "group name starts with nordvpn", - groupEntry: "nordvpn_ddd:x:996:", - expectedResult: userSet{}, - }, - { - name: "group name starts with nordvpn", - groupEntry: "nordvpn:", - expectedResult: userSet{}, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - result := getGroupMembers(test.groupEntry) - assert.Equal(t, test.expectedResult, result) - }) - } -} - -func Test_handleGroupUpdate(t *testing.T) { - category.Set(t, category.Unit) - - // normally userIDs struct should also contain gid and home directory, but for purpose of this test uid is enough - user1ID := userIDs{uid: 1000} - user1Name := "user1" - - user2ID := userIDs{uid: 100001} - user2Name := "user2" - - usernameToUserIDs := map[string]userIDs{ - user1Name: user1ID, - user2Name: user2ID, - } - - tests := []struct { - name string - oldGroup userSet - newGroup userSet - getUsersErr error - expectedEnabled []uint32 - expectedDisabled []uint32 - expectedUserStates userSet - }{ - { - name: "enable all", - oldGroup: userSet{}, - newGroup: userSet{user1Name: userActive}, - getUsersErr: nil, - expectedEnabled: []uint32{user1ID.uid}, - expectedDisabled: []uint32{}, - expectedUserStates: userSet{user1Name: norduserRunning}, - }, - { - name: "disable all", - oldGroup: userSet{user1Name: norduserRunning}, - newGroup: userSet{}, - getUsersErr: nil, - expectedEnabled: []uint32{}, - expectedDisabled: []uint32{user1ID.uid}, - expectedUserStates: userSet{}, - }, - { - name: "enalbe and disable", - oldGroup: userSet{user1Name: norduserRunning}, - newGroup: userSet{user2Name: userActive}, - getUsersErr: nil, - expectedEnabled: []uint32{user2ID.uid}, - expectedDisabled: []uint32{user1ID.uid}, - expectedUserStates: userSet{user2Name: norduserRunning}, - }, - { - name: "no users", - oldGroup: userSet{}, - newGroup: userSet{}, - getUsersErr: nil, - expectedEnabled: []uint32{}, - expectedDisabled: []uint32{}, - expectedUserStates: userSet{}, - }, - { - name: "no state change", - oldGroup: userSet{user1Name: norduserRunning}, - newGroup: userSet{user1Name: userActive}, - getUsersErr: nil, - expectedEnabled: []uint32{}, - expectedDisabled: []uint32{}, - expectedUserStates: userSet{user1Name: norduserRunning}, - }, - // In this test, as function fails to obtain user IDs/take any actions, we expect the resulting state to be - // unchanged combination of oldGroup and newGroup, i.e norduser is still running for user1 and user2 is active - // but norduser is not running for them. - { - name: "error when getting users", - oldGroup: userSet{user1Name: norduserRunning}, - newGroup: userSet{user2Name: userActive}, - getUsersErr: fmt.Errorf("failed to get user"), - expectedEnabled: []uint32{}, - expectedDisabled: []uint32{}, - expectedUserStates: userSet{user1Name: norduserRunning, user2Name: userActive}, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - mockService := nordusermock.NewMockNorduserCombinedService() - - mockUserIDGetter := newUserIDGetterMock(usernameToUserIDs) - mockUserIDGetter.GetErr = test.getUsersErr - - groupMonitor := NordVPNGroupMonitor{ - norduserd: &mockService, - userIDGetter: mockUserIDGetter, - } - - userStates := groupMonitor.handleGroupUpdate(test.oldGroup, test.newGroup) - assert.Equal(t, test.expectedEnabled, mockService.Enabled, - "norduserd was not enabled for the expected users") - assert.Equal(t, test.expectedUserStates, userStates, "State was not properly updated for some users.") - }) - } -} diff --git a/norduser/group_test.go b/norduser/group_test.go new file mode 100644 index 00000000..8bb8d1ad --- /dev/null +++ b/norduser/group_test.go @@ -0,0 +1,91 @@ +package norduser + +import ( + "strings" + "testing" + + "github.com/NordSecurity/nordvpn-linux/test/category" + "github.com/stretchr/testify/assert" +) + +func Test_findGroupEntry(t *testing.T) { + category.Set(t, category.Unit) + + const rootGroup string = "root:x:0:" + groups := []string{ + rootGroup, + "daemon:x:1:user", + "bin:x:2:", + "sys:x:3:", + "adm:x:4:syslog,user", + } + + groupFile := strings.Join(groups, "\n") + + tests := []struct { + name string + groupName string + expectedResult string + }{ + { + name: "group exists", + groupName: "root", + expectedResult: rootGroup, + }, + { + name: "group does not exist", + groupName: "test", + expectedResult: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := findGroupEntry(groupFile, test.groupName) + assert.Equal(t, test.expectedResult, result) + }) + } +} + +func Test_getGroupMembers(t *testing.T) { + category.Set(t, category.Unit) + + tests := []struct { + name string + groupEntry string + expectedResult []string + }{ + { + name: "single user group", + groupEntry: "nordvpn:x:996:user", + expectedResult: []string{"user"}, + }, + { + name: "multi user group", + groupEntry: "nordvpn:x:996:user1,user2,user3", + expectedResult: []string{"user1", "user2", "user3"}, + }, + { + name: "empty group", + groupEntry: "nordvpn:x:996:", + expectedResult: []string{}, + }, + { + name: "group name starts with nordvpn", + groupEntry: "nordvpn_ddd:x:996:", + expectedResult: []string{}, + }, + { + name: "group name starts with nordvpn", + groupEntry: "nordvpn:", + expectedResult: []string{}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := getGroupMembers(test.groupEntry) + assert.Equal(t, test.expectedResult, result) + }) + } +} diff --git a/norduser/process_monitor.go b/norduser/process_monitor.go new file mode 100644 index 00000000..60a9dca8 --- /dev/null +++ b/norduser/process_monitor.go @@ -0,0 +1,246 @@ +package norduser + +import ( + "fmt" + "log" + "slices" + + "github.com/fsnotify/fsnotify" + + "github.com/NordSecurity/nordvpn-linux/internal" + "github.com/NordSecurity/nordvpn-linux/norduser/service" + "github.com/NordSecurity/nordvpn-linux/snapconf" +) + +const ( + etcPath = "/etc" + groupFilePath = etcPath + "/group" + utmpFilePath = "/var/run/utmp" +) + +type norduserState int + +const ( + notActive norduserState = iota + loginGUI + loginText + runningGUI + runningText +) + +// changeState will execute actions appropriate for newState for the given username and then update the state to +// appropriate new state based on result of those actions +// Desired state transitions: +// - notActive => loginGUI - start application, update state to runningGUI +// - notActive => loginText - start application, update state to runningText +// - runningGUI => notActive - stop application, update state to notActive +// - runningText => notActive - stop application, update state to notActive +// - runningGUI => loginText - restart application, update state to runningText +// - runningText => loginGUI - update state to runningGUI +// +// Other sate transitions should result in a noop. +// +// More on runningGUI to loginText transition: +// Due to library limitations, when user doesn't have any GUI sessions, tray will be disabled. In order to enable it on +// subsequent GUI logins, we need to restart the application. +// +// Such actions are not necessary in case of transitioning from runningText to loginGUI, since in this case tray was +// not started. +func (s *norduserState) changeState(newState norduserState, + username string, + userIDGetter userIDGetter, + norduserSrevice service.NorduserService) { + if *s == notActive && + (newState == loginGUI || newState == loginText) { // user logged in, start norduserd + userIDs, err := userIDGetter.getUserID(username) + if err != nil { + log.Println(internal.ErrorPrefix, "getting user IDs when enabling norduser: ", err) + return + } + + if err := norduserSrevice.Enable(userIDs.uid, userIDs.gid, userIDs.home); err != nil { + log.Println(internal.ErrorPrefix, "enabling norduserd for member:", err) + return + } + + if newState == loginGUI { + *s = runningGUI + } else { + *s = runningText + } + } else if (*s == runningText || *s == runningGUI) && + newState == notActive { // user logged out when norduser was running, stop norduserd + userIDs, err := userIDGetter.getUserID(username) + if err != nil { + log.Println(internal.ErrorPrefix, "getting user IDs when disabling norduser: ", err) + return + } + + if err := norduserSrevice.Stop(userIDs.uid, false); err != nil { + log.Println(internal.ErrorPrefix, "disabling norduserd for user:", err.Error()) + return + } + + *s = notActive + } else if *s == runningGUI && newState == loginText { // user logged out of the GUI process, we need + // to restart norduserd in order to re-enable tray when user logs back in to GUI + userIDs, err := userIDGetter.getUserID(username) + if err != nil { + log.Println(internal.ErrorPrefix, "getting user IDs when restarting norduser: ", err) + return + } + + if err := norduserSrevice.Restart(userIDs.uid); err != nil { + log.Println(internal.ErrorPrefix, "failed to restart norduserd: ", err) + return + } + + *s = runningText + } else if *s == runningText && newState == loginGUI { // when user is initially logged in via text + // interface, we only need to update the state so that subsequent switch from GUI to text can be handled + // correctly + *s = runningGUI + } +} + +type userSet map[string]norduserState + +// NorduserProcessMonitor monitors the nordvpn system group and starts/stops norduserd for users added/removed from the +// group. +type NorduserProcessMonitor struct { + norduserd service.NorduserService + isSnap bool + userIDGetter +} + +func NewNorduserProcessMonitor(service service.NorduserService) NorduserProcessMonitor { + return NorduserProcessMonitor{ + norduserd: service, + isSnap: snapconf.IsUnderSnap(), + userIDGetter: osGetter{}, + } +} + +func (n *NorduserProcessMonitor) handleGroupFileUpdate(currentGroupMembers userSet) (userSet, error) { + newGroupMembers, err := getNordVPNGroupMembers() + if err != nil { + return currentGroupMembers, fmt.Errorf("getting nordvpn group members: %w", err) + } + + activeUsers, err := getActiveUsers() + if err != nil { + return currentGroupMembers, fmt.Errorf("getting active users after group file update: %w", err) + } + + // initialize new group members + for _, newGroupMemberUsername := range newGroupMembers { + _, ok := currentGroupMembers[newGroupMemberUsername] + if ok { + continue + } + + state := notActive + userStatus, ok := activeUsers[newGroupMemberUsername] + if ok { + state.changeState(userStatus, newGroupMemberUsername, n.userIDGetter, n.norduserd) + } + currentGroupMembers[newGroupMemberUsername] = state + } + + // update state for removed group members + for memberUsername, memberState := range currentGroupMembers { + if contains := slices.Contains(newGroupMembers, memberUsername); !contains { + memberState.changeState(notActive, memberUsername, n.userIDGetter, n.norduserd) + delete(currentGroupMembers, memberUsername) + } + } + + return currentGroupMembers, nil +} + +func (n *NorduserProcessMonitor) handleUTMPFileUpdate(currentGroupMembers userSet) (userSet, error) { + activeUsers, err := getActiveUsers() + if err != nil { + return currentGroupMembers, fmt.Errorf("getting active users after utmp file update: %w", err) + } + + for username, state := range currentGroupMembers { + userState, ok := activeUsers[username] + if ok { + state.changeState(userState, username, n.userIDGetter, n.norduserd) + } else { + state.changeState(notActive, username, n.userIDGetter, n.norduserd) + } + + currentGroupMembers[username] = state + } + + return currentGroupMembers, nil +} + +func getWatcher(pathsToMonitor ...string) (watcher *fsnotify.Watcher, err error) { + watcher, err = fsnotify.NewWatcher() + if err != nil { + return nil, fmt.Errorf("creating new watcher: %w", err) + } + + defer func() { + if err != nil { + watcher.Close() + } + }() + + for _, file := range pathsToMonitor { + if err := watcher.Add(file); err != nil { + return nil, fmt.Errorf("adding group file to watcher: %w", err) + } + } + + return watcher, nil +} + +// Start blocks the thread and starts monitoring for changes in the nordvpn group. +func (n *NorduserProcessMonitor) Start() error { + watcher, err := getWatcher(etcPath, utmpFilePath) + if err != nil { + return fmt.Errorf("creating file watcher: %w", err) + } + defer watcher.Close() + + currentGrupMembers, err := n.handleGroupFileUpdate(make(userSet)) + if err != nil { + return fmt.Errorf("starting norduserd for the initial group members: %w", err) + } + + for { + select { + case event, ok := <-watcher.Events: + if !ok { + return fmt.Errorf("groupfile monitor channel closed") + } + + if event.Name == groupFilePath { + // Because utilities used to modify the group do so atomically, we also need to monitor for creation of + // the file instead of modifications. + if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) { + if newGroupMembers, err := n.handleGroupFileUpdate(currentGrupMembers); err != nil { + log.Println(internal.ErrorPrefix, "failed to handle change of groupfile: ", err) + } else { + currentGrupMembers = newGroupMembers + } + } + } else if event.Name == utmpFilePath { + if newGroupMembers, err := n.handleUTMPFileUpdate(currentGrupMembers); err != nil { + log.Println(internal.ErrorPrefix, "failed to handle change of utmp file: ", err) + } else { + currentGrupMembers = newGroupMembers + } + } + case err, ok := <-watcher.Errors: + if !ok { + return fmt.Errorf("groupfile monitor error channel closed") + } + log.Println(internal.ErrorPrefix, "group monitor error: ", err) + } + } +} diff --git a/norduser/process_monitor_snap.go b/norduser/process_monitor_snap.go new file mode 100644 index 00000000..7558fba9 --- /dev/null +++ b/norduser/process_monitor_snap.go @@ -0,0 +1,78 @@ +package norduser + +import ( + "fmt" + "log" + "slices" + + "github.com/NordSecurity/nordvpn-linux/internal" + "github.com/fsnotify/fsnotify" +) + +func (n *NorduserProcessMonitor) stopForDeletedGroupMembers(currentGroupMembers []string, + newGroupMembers []string) []string { + groupMembersUpdate := []string{} + for _, username := range currentGroupMembers { + if slices.Contains(newGroupMembers, username) { + groupMembersUpdate = append(groupMembersUpdate, username) + continue + } + + userIDs, err := n.userIDGetter.getUserID(username) + if err != nil { + log.Println(internal.ErrorPrefix, "getting users ID: ", err) + groupMembersUpdate = append(groupMembersUpdate, username) + continue + } + + if err := n.norduserd.Stop(userIDs.uid, false); err != nil { + groupMembersUpdate = append(groupMembersUpdate, username) + log.Println(internal.ErrorPrefix, "stopping norduser: ", err) + } + } + + return groupMembersUpdate +} + +// StartSnap starts a simplified norduser process monitor routine. norduser processes will be stopped for users removed +// form the nordvpn group, no other actions will be taken. Because of snap, starting/restarting the process has to be +// handled in the process itself. +func (n *NorduserProcessMonitor) StartSnap() error { + watcher, err := getWatcher(etcPath, utmpFilePath) + if err != nil { + return fmt.Errorf("creating file watcher: %w", err) + } + defer watcher.Close() + + groupMembers, err := getNordVPNGroupMembers() + if err != nil { + return fmt.Errorf("getting initial nordvpn group members: %w", err) + } + + for { + select { + case event, ok := <-watcher.Events: + if !ok { + return fmt.Errorf("groupfile monitor channel closed") + } + + if event.Name == groupFilePath { + // Because utilities used to modify the group do so atomically, we also need to monitor for creation of + // the file instead of modifications. + if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) { + newGroupMembers, err := getNordVPNGroupMembers() + if err != nil { + log.Println(internal.ErrorPrefix, "getting new group members: ", err) + } else { + groupMembers = n.stopForDeletedGroupMembers(groupMembers, newGroupMembers) + } + } + } + case err, ok := <-watcher.Errors: + if !ok { + return fmt.Errorf("groupfile monitor error channel closed") + } + log.Println(internal.ErrorPrefix, "group monitor error: ", err) + } + } +} diff --git a/norduser/process_monitor_test.go b/norduser/process_monitor_test.go new file mode 100644 index 00000000..215671f3 --- /dev/null +++ b/norduser/process_monitor_test.go @@ -0,0 +1,393 @@ +package norduser + +import ( + "fmt" + "slices" + "testing" + + "github.com/NordSecurity/nordvpn-linux/test/category" + testnorduser "github.com/NordSecurity/nordvpn-linux/test/mock/norduser/service" + "github.com/stretchr/testify/assert" +) + +type userIDGetterMock struct { + UsernameToIDs map[string]userIDs + GetErr error +} + +func newUserIDGetterMock(usernameToUserIDs map[string]userIDs) *userIDGetterMock { + return &userIDGetterMock{ + UsernameToIDs: usernameToUserIDs, + } +} + +func (u *userIDGetterMock) getUserID(username string) (userIDs, error) { + if u.GetErr != nil { + return userIDs{}, u.GetErr + } + + userID, ok := u.UsernameToIDs[username] + if !ok { + return userIDs{}, fmt.Errorf("user ids not found") + } + + return userID, nil +} + +func Test_changeState(t *testing.T) { + category.Set(t, category.Unit) + + // Desired state transitions: + // - notActive => loginGUI - start application, update state to runningGUI + // - notActive => loginText - start application, update state to runningText + // - runningGUI => notActive - stop application, update state to notActive + // - runningText => notActive - stop application, update state to notActive + // - runningGUI => loginText - restart application, update state to runningText + // - runningText => loginGUI - update state to runningGUI + // Other transitions should result in an noop + + getUserIDErr := fmt.Errorf("get user id error") + enableErr := fmt.Errorf("enalbe error") + stopErr := fmt.Errorf("stop error") + restartErr := fmt.Errorf("restart error") + + const username string = "user" + const uid uint32 = 100001 + + userIDsMap := map[string]userIDs{username: { + uid: uid, + }} + + tests := []struct { + name string + initialState norduserState + newState norduserState + expectedState norduserState + expectedAction testnorduser.Action + noAction bool + enableErr error + stopErr error + restartErr error + getUserIDErr error + }{ + // notActive to loginGUI + { + name: "notActive to loginGUI", + initialState: notActive, + newState: loginGUI, + expectedState: runningGUI, + expectedAction: testnorduser.Enable, + }, + { + name: "notActive to loginGUI get user ID error", + initialState: notActive, + newState: loginGUI, + expectedState: notActive, + noAction: true, + getUserIDErr: getUserIDErr, + }, + { + name: "notActive to loginGUI enalbe error", + initialState: notActive, + newState: loginGUI, + expectedState: notActive, + noAction: true, + enableErr: enableErr, + }, + // notActive to loginText + { + name: "notActive to loginText", + initialState: notActive, + newState: loginText, + expectedState: runningText, + expectedAction: testnorduser.Enable, + }, + { + name: "notActive to loginText get user ID error", + initialState: notActive, + newState: loginText, + expectedState: notActive, + noAction: true, + getUserIDErr: getUserIDErr, + }, + { + name: "notActive to loginGUI enalbe error", + initialState: notActive, + newState: loginText, + expectedState: notActive, + noAction: true, + enableErr: enableErr, + }, + // runningGUI to notActive + { + name: "runningGUI to notActive", + initialState: runningGUI, + newState: notActive, + expectedState: notActive, + expectedAction: testnorduser.Stop, + }, + { + name: "runningGUI to notActive get user ID error", + initialState: runningGUI, + newState: notActive, + expectedState: runningGUI, + noAction: true, + getUserIDErr: getUserIDErr, + }, + { + name: "runningGUI to notActive get stop error", + initialState: runningGUI, + newState: notActive, + expectedState: runningGUI, + noAction: true, + stopErr: stopErr, + }, + // runningText to notActive + { + name: "runningText to notActive", + initialState: runningText, + newState: notActive, + expectedState: notActive, + expectedAction: testnorduser.Stop, + }, + { + name: "runningGUI to notActive get user ID error", + initialState: runningText, + newState: notActive, + expectedState: runningText, + noAction: true, + getUserIDErr: getUserIDErr, + }, + { + name: "runningGUI to notActive get stop error", + initialState: runningText, + newState: notActive, + expectedState: runningText, + noAction: true, + stopErr: stopErr, + }, + // runningGUI to loginText + { + name: "runningGUI to loginText", + initialState: runningGUI, + newState: loginText, + expectedState: runningText, + expectedAction: testnorduser.Restart, + }, + { + name: "runningGUI to loginText get user ID error", + initialState: runningGUI, + newState: loginText, + expectedState: runningGUI, + noAction: true, + getUserIDErr: getUserIDErr, + }, + { + name: "runningGUI to loginText restart error", + initialState: runningGUI, + newState: loginText, + expectedState: runningGUI, + noAction: true, + restartErr: restartErr, + }, + // runningText to loginGUI + { + name: "runningText to loginGUI", + initialState: runningText, + newState: loginGUI, + expectedState: runningGUI, + noAction: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + userIDGetterMock := newUserIDGetterMock(userIDsMap) + userIDGetterMock.GetErr = test.getUserIDErr + + norduserProcessManagerMock := testnorduser.NewMockNorduserCombinedService() + norduserProcessManagerMock.EnableErr = test.enableErr + norduserProcessManagerMock.StopErr = test.stopErr + norduserProcessManagerMock.RestartErr = test.restartErr + + test.initialState.changeState(test.newState, username, userIDGetterMock, &norduserProcessManagerMock) + + assert.Equal(t, test.expectedState, test.initialState, + "State was not properly updated after handling the state transition.") + + if test.noAction { + assert.True(t, norduserProcessManagerMock.CheckNoAction(), + "Unexpected action was taken when changing state when no action was expected.") + return + } + + actions := norduserProcessManagerMock.ActionToUIDs[test.expectedAction] + assert.Len(t, actions, 1, "More than one action taken") + + // check that no other type of action was executed + assert.True(t, norduserProcessManagerMock.CheckNoAction(test.expectedAction), + "Unexpected action was taken when changing state.") + }) + } +} + +func Test_changeState_noop(t *testing.T) { + type transition struct { + initial norduserState + new norduserState + } + nonNoopTransitions := []transition{ + {initial: notActive, new: loginGUI}, + {initial: notActive, new: loginText}, + {initial: runningGUI, new: notActive}, + {initial: runningText, new: notActive}, + {initial: runningGUI, new: loginText}, + {initial: runningText, new: loginGUI}, + } + + states := []norduserState{ + notActive, + loginGUI, + loginText, + runningGUI, + runningText, + } + + const username string = "user" + const uid uint32 = 100001 + + userIDsMap := map[string]userIDs{username: { + uid: uid, + }} + + for _, initialState := range states { + for _, newState := range states { + isNonNoop := slices.ContainsFunc(nonNoopTransitions, func(t transition) bool { + if t.initial == initialState && t.new == newState { + return true + } + return false + }) + + if isNonNoop { + continue + } + + userIDGetterMock := newUserIDGetterMock(userIDsMap) + norduserProcessManagerMock := testnorduser.NewMockNorduserCombinedService() + + // copy initial state so we can verify that state did not change after the transition + expectedState := initialState + + initialState.changeState(newState, username, userIDGetterMock, &norduserProcessManagerMock) + + assert.Equal(t, expectedState, initialState, + "Unexpected state change after noop state transition.") + assert.True(t, norduserProcessManagerMock.CheckNoAction(), + "Unexpected actions executed after noop state transition.") + } + } +} + +func Test_stopForDeletedGroupMembers(t *testing.T) { + category.Set(t, category.Unit) + + const username1 string = "user1" + const uid1 uint32 = 1001 + + const username2 string = "user2" + const uid2 uint32 = 100001 + + const username3 string = "user3" + const uid3 uint32 = 53200 + + userIDsMap := map[string]userIDs{ + username1: {uid: uid1}, + username2: {uid: uid2}, + username3: {uid: uid3}, + } + + tests := []struct { + name string + initialGroupMembers []string + newGroupMembers []string + expectedStoppedUIDs []uint32 + getUIDErr error + stopErr error + isNoop bool + }{ + { + name: "all group members removed", + initialGroupMembers: []string{username1, username2, username3}, + newGroupMembers: []string{}, + expectedStoppedUIDs: []uint32{uid1, uid2, uid3}, + }, + { + name: "stop for single user", + initialGroupMembers: []string{username1, username2, username3}, + newGroupMembers: []string{username2, username3}, + expectedStoppedUIDs: []uint32{uid1}, + }, + { + name: "initial startup", + initialGroupMembers: []string{}, + newGroupMembers: []string{username1, username2, username3}, + isNoop: true, + }, + { + name: "getUID error", + initialGroupMembers: []string{username1, username2, username3}, + newGroupMembers: []string{username2}, + getUIDErr: fmt.Errorf("getUID error"), + isNoop: true, + }, + { + name: "Stop error", + initialGroupMembers: []string{username1, username2, username3}, + newGroupMembers: []string{username2}, + stopErr: fmt.Errorf("Stop error"), + isNoop: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + userIDGetterMock := newUserIDGetterMock(userIDsMap) + userIDGetterMock.GetErr = test.getUIDErr + + norduserProcessManagerMock := testnorduser.NewMockNorduserCombinedService() + norduserProcessManagerMock.StopErr = test.stopErr + + norduserProcessMonitor := NorduserProcessMonitor{ + norduserd: &norduserProcessManagerMock, + userIDGetter: userIDGetterMock, + } + + groupMembersAfterUpdate := + norduserProcessMonitor.stopForDeletedGroupMembers(test.initialGroupMembers, test.newGroupMembers) + if test.isNoop { + assert.True(t, norduserProcessManagerMock.CheckNoAction(), + `Unexpected actions taken when stopping norduser + for deleted group members(no action should be taken).`) + assert.Equal(t, groupMembersAfterUpdate, test.initialGroupMembers, + "Group update has failed, but new group state has replaced the old group state.") + return + } + + stoppedUIDs := norduserProcessManagerMock.ActionToUIDs[testnorduser.Stop] + assert.Len(t, stoppedUIDs, len(test.expectedStoppedUIDs), + `Invalid number of stop operations performed when stopping norduser for users removed from group, + should be equal to number of removed users.`) + + for _, expectedStoppedUID := range test.expectedStoppedUIDs { + assert.Contains(t, stoppedUIDs, expectedStoppedUID, + "norduser was not stopped for UID %d.", expectedStoppedUID) + } + + assert.True(t, norduserProcessManagerMock.CheckNoAction(testnorduser.Stop), + `Unexpected actions(other than stopping norduser) taken + when stopping norduser for removed group members.`) + assert.Equal(t, test.newGroupMembers, groupMembersAfterUpdate, "Group members were not properly updated.") + }) + } +} diff --git a/norduser/utmp.go b/norduser/utmp.go index d7cf2cb8..1537ae9a 100644 --- a/norduser/utmp.go +++ b/norduser/utmp.go @@ -9,21 +9,12 @@ package norduser const int ERROR_REALLOC = -1; const int ERROR_MALLOC_USERNAME = -2; -void free_users_table(char*** users, int size) { - for (int index = 0; index < size; index++) { - if ((*users)[index] != NULL) { - free((*users)[index]); - (*users)[index] = NULL; - } - } +typedef struct user { + pid_t login_pid; + char username[__UT_NAMESIZE + 1]; +} user; - if (*users != NULL) { - free(*users); - *users = NULL; - } -} - -int get_utmp_user_processes(char*** users) { +int get_utmp_user_processes(user** users) { int index = 0; int size = 0; setutxent(); @@ -37,10 +28,10 @@ int get_utmp_user_processes(char*** users) { } if (index == size) { - char** tmp; - tmp = realloc(*users, (size + 1) * sizeof(char*)); + user* tmp; + tmp = realloc(*users, (size + 1) * sizeof(user)); if (tmp == NULL) { - free_users_table(users, index); + free(*users); endutxent(); return ERROR_REALLOC; } @@ -49,15 +40,8 @@ int get_utmp_user_processes(char*** users) { size++; } - (*users)[index] = malloc(__UT_NAMESIZE + 1); - if ((*users)[index] == NULL) { - free_users_table(users, size); - endutxent(); - return ERROR_MALLOC_USERNAME; - } - (*users)[index][__UT_NAMESIZE]='\0'; - - strncpy((*users)[index], u->ut_user, __UT_NAMESIZE); + (*users)[index].login_pid = u->ut_pid; + strncpy((*users)[index].username, u->ut_user, __UT_NAMESIZE + 1); index++; } @@ -75,25 +59,51 @@ import ( "github.com/NordSecurity/nordvpn-linux/internal" ) -func getActiveUsers() ([]string, error) { - var usersCArray **C.char +type userData map[string]norduserState + +// getActiveUsers returns a map of [username]userType where type can be text or gui. If any of the given users login +// processes will be detected to have a gui(done based on the environment), user type will be gui. Otherwise user type +// will be text. +func getActiveUsers() (userData, error) { + var usersCArray *C.user size := C.get_utmp_user_processes(&usersCArray) + defer C.free(unsafe.Pointer(usersCArray)) if size == C.ERROR_REALLOC { - return []string{}, fmt.Errorf("failed to reallocate space for the users table") + return userData{}, fmt.Errorf("failed to reallocate space for the users table") } else if size == C.ERROR_MALLOC_USERNAME { - return []string{}, fmt.Errorf("failed to allocate space for new user in the users table") + return userData{}, fmt.Errorf("failed to allocate space for new user in the users table") } log.Printf("%s %d active user processes found", internal.DebugPrefix, size) - users := []string{} - for i := 0; i < int(size); i++ { - usernameCStr := (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(usersCArray)) + uintptr(i)*unsafe.Sizeof(*usersCArray))) - username := C.GoString(*usernameCStr) - users = append(users, username) + users := make(userData) + for index := 0; index < int(size); index++ { + userC := (*C.user)(unsafe.Pointer(uintptr(unsafe.Pointer(usersCArray)) + + uintptr(index)*unsafe.Sizeof(*usersCArray))) + usernameC := (*C.char)(unsafe.Pointer(&userC.username)) + username := C.GoString(usernameC) + + // userType was determined to be gui for any of the users login processes, so we skip this user. + if userType, ok := users[username]; ok && userType == loginGUI { + continue + } + + loginPID := uint32(userC.login_pid) + + desktopSession, err := findEnvVariableForPID(loginPID, "XDG_CURRENT_DESKTOP") + if err != nil { + log.Printf("%s looking up XDG_CURRENT_DESKTOP for %s: %s", internal.ErrorPrefix, username, err) + continue + } + + userType := loginText + if desktopSession != "" { + userType = loginGUI + } + + users[username] = userType } - C.free_users_table(&usersCArray, size) return users, nil } diff --git a/test/mock/norduser/service/combined.go b/test/mock/norduser/service/combined.go index 90eeedec..51f4d8be 100644 --- a/test/mock/norduser/service/combined.go +++ b/test/mock/norduser/service/combined.go @@ -1,42 +1,89 @@ package service +import "slices" + +type Action int + +const ( + Enable Action = iota + Stop + Restart +) + +// ActionToUIDs maps particular action to UIDs for which this action was executed +type ActionToUIDs map[Action][]uint32 + type MockNorduserCombinedService struct { - Enabled []uint32 - EnableErr error + ActionToUIDs ActionToUIDs - Disabled []uint32 - DeisableErr error + EnableErr error + StopErr error + RestartErr error } func NewMockNorduserCombinedService() MockNorduserCombinedService { + actionToUIDs := make(ActionToUIDs) + actionToUIDs[Enable] = []uint32{} + actionToUIDs[Stop] = []uint32{} + actionToUIDs[Restart] = []uint32{} + return MockNorduserCombinedService{ - Enabled: []uint32{}, - Disabled: []uint32{}, + ActionToUIDs: actionToUIDs, } } +// CheckNoAction is a helper method for test callers, it returns true if no action was taken by the mock in its +// lifetime. Actions provided in the optional filters parameter will be ingored when checking. +func (m *MockNorduserCombinedService) CheckNoAction(filters ...Action) bool { + for action, actionUIDs := range m.ActionToUIDs { + if slices.Contains(filters, action) { + continue + } + + if len(actionUIDs) > 0 { + return false + } + } + + return true +} + +func (m *MockNorduserCombinedService) addUIDToActon(uid uint32, action Action) { + uids := m.ActionToUIDs[action] + uids = append(uids, uid) + m.ActionToUIDs[action] = uids +} + func (m *MockNorduserCombinedService) Enable(uid uint32, _ uint32, _ string) error { if m.EnableErr != nil { return m.EnableErr } - m.Enabled = append(m.Enabled, uid) + m.addUIDToActon(uid, Enable) return nil } -func (m *MockNorduserCombinedService) Disable(uid uint32) error { - if m.DeisableErr != nil { - return m.DeisableErr +func (m *MockNorduserCombinedService) Disable(uid uint32) error { return nil } + +func (m *MockNorduserCombinedService) Stop(uid uint32, wait bool) error { + if m.StopErr != nil { + return m.StopErr } - m.Disabled = append(m.Disabled, uid) + m.addUIDToActon(uid, Stop) return nil } -func (m *MockNorduserCombinedService) Stop(uint32, bool) error { return nil } +func (m *MockNorduserCombinedService) Restart(uid uint32) error { + if m.RestartErr != nil { + return m.RestartErr + } + + m.addUIDToActon(uid, Restart) -func (m *MockNorduserCombinedService) Restart(uint32) error { return nil } + return nil +} func (m *MockNorduserCombinedService) StopAll() {} From c53f9ea229705a42ff0d42bf00ab09eea6e44803 Mon Sep 17 00:00:00 2001 From: Bartosz Oleaczek Date: Mon, 8 Jul 2024 07:45:29 +0200 Subject: [PATCH 2/4] review fixes --- cmd/daemon/main.go | 6 +++--- daemon/rpc.go | 4 ++-- norduser/group.go | 4 +--- norduser/grpc_interceptor.go | 4 ++-- norduser/process_monitor.go | 8 ++++---- norduser/service/service.go | 2 +- norduser/utmp.go | 6 +++--- 7 files changed, 16 insertions(+), 18 deletions(-) diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index 01f3aa65..978af3e6 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -402,7 +402,7 @@ func main() { log.Fatalln(err) } - var norduserService norduserservice.NorduserService + var norduserService norduserservice.Service if snapconf.IsUnderSnap() { norduserService = norduserservice.NewNorduserSnapService() } else { @@ -492,11 +492,11 @@ func main() { go func() { if snapconf.IsUnderSnap() { if err := norduserMonitor.StartSnap(); err != nil { - log.Println("Error when starting norduser monitor for snap: ", err.Error()) + log.Println(internal.ErrorPrefix, "Error when starting norduser monitor for snap:", err.Error()) } } else { if err := norduserMonitor.Start(); err != nil { - log.Println("Error when starting norduser monitor: ", err.Error()) + log.Println(internal.ErrorPrefix, "Error when starting norduser monitor:", err.Error()) } } }() diff --git a/daemon/rpc.go b/daemon/rpc.go index a19da02b..bf642598 100644 --- a/daemon/rpc.go +++ b/daemon/rpc.go @@ -50,7 +50,7 @@ type RPC struct { nameservers dns.Getter ncClient nc.NotificationClient analytics events.Analytics - norduser service.NorduserService + norduser service.Service meshRegistry mesh.Registry systemShutdown atomic.Bool pb.UnimplementedDaemonServer @@ -77,7 +77,7 @@ func NewRPC( nameservers dns.Getter, ncClient nc.NotificationClient, analytics events.Analytics, - norduser service.NorduserService, + norduser service.Service, meshRegistry mesh.Registry, ) *RPC { scheduler, _ := gocron.NewScheduler(gocron.WithLocation(time.UTC)) diff --git a/norduser/group.go b/norduser/group.go index 345052a3..beba8f37 100644 --- a/norduser/group.go +++ b/norduser/group.go @@ -49,9 +49,7 @@ func getGroupMembers(groupEntry string) []string { } func getNordVPNGroupMembers() ([]string, error) { - const nordvpnGroupName = "nordvpn" - - groupEntry, err := getGroupEntry(nordvpnGroupName) + groupEntry, err := getGroupEntry(internal.NordvpnGroup) if err != nil { return nil, fmt.Errorf("getting group entry: %w", err) } diff --git a/norduser/grpc_interceptor.go b/norduser/grpc_interceptor.go index 5d514ff4..a289428d 100644 --- a/norduser/grpc_interceptor.go +++ b/norduser/grpc_interceptor.go @@ -15,10 +15,10 @@ import ( // StartNorduserdMiddleware provides a way to start/stop norduserd when handling nordvpnd gRPCs. type StartNorduserdMiddleware struct { - norduserd service.NorduserService + norduserd service.Service } -func NewStartNorduserMiddleware(norduserd_service service.NorduserService) StartNorduserdMiddleware { +func NewStartNorduserMiddleware(norduserd_service service.Service) StartNorduserdMiddleware { return StartNorduserdMiddleware{ norduserd: norduserd_service, } diff --git a/norduser/process_monitor.go b/norduser/process_monitor.go index 60a9dca8..a6b58b86 100644 --- a/norduser/process_monitor.go +++ b/norduser/process_monitor.go @@ -38,7 +38,7 @@ const ( // - runningGUI => loginText - restart application, update state to runningText // - runningText => loginGUI - update state to runningGUI // -// Other sate transitions should result in a noop. +// Other state transitions should result in a noop. // // More on runningGUI to loginText transition: // Due to library limitations, when user doesn't have any GUI sessions, tray will be disabled. In order to enable it on @@ -49,7 +49,7 @@ const ( func (s *norduserState) changeState(newState norduserState, username string, userIDGetter userIDGetter, - norduserSrevice service.NorduserService) { + norduserSrevice service.Service) { if *s == notActive && (newState == loginGUI || newState == loginText) { // user logged in, start norduserd userIDs, err := userIDGetter.getUserID(username) @@ -108,12 +108,12 @@ type userSet map[string]norduserState // NorduserProcessMonitor monitors the nordvpn system group and starts/stops norduserd for users added/removed from the // group. type NorduserProcessMonitor struct { - norduserd service.NorduserService + norduserd service.Service isSnap bool userIDGetter } -func NewNorduserProcessMonitor(service service.NorduserService) NorduserProcessMonitor { +func NewNorduserProcessMonitor(service service.Service) NorduserProcessMonitor { return NorduserProcessMonitor{ norduserd: service, isSnap: snapconf.IsUnderSnap(), diff --git a/norduser/service/service.go b/norduser/service/service.go index c05daa6e..245999b3 100644 --- a/norduser/service/service.go +++ b/norduser/service/service.go @@ -1,6 +1,6 @@ package service -type NorduserService interface { +type Service interface { Enable(uid uint32, gid uint32, home string) error Stop(uid uint32, wait bool) error StopAll() diff --git a/norduser/utmp.go b/norduser/utmp.go index 1537ae9a..b4ef4c25 100644 --- a/norduser/utmp.go +++ b/norduser/utmp.go @@ -41,7 +41,8 @@ int get_utmp_user_processes(user** users) { } (*users)[index].login_pid = u->ut_pid; - strncpy((*users)[index].username, u->ut_user, __UT_NAMESIZE + 1); + strncpy((*users)[index].username, u->ut_user, __UT_NAMESIZE); + (*users)[index].username[__UT_NAMESIZE] = '\0'; index++; } @@ -67,13 +68,12 @@ type userData map[string]norduserState func getActiveUsers() (userData, error) { var usersCArray *C.user size := C.get_utmp_user_processes(&usersCArray) - defer C.free(unsafe.Pointer(usersCArray)) - if size == C.ERROR_REALLOC { return userData{}, fmt.Errorf("failed to reallocate space for the users table") } else if size == C.ERROR_MALLOC_USERNAME { return userData{}, fmt.Errorf("failed to allocate space for new user in the users table") } + defer C.free(unsafe.Pointer(usersCArray)) log.Printf("%s %d active user processes found", internal.DebugPrefix, size) From 7275869711a58b16c4898f8d5e511bbc193692ed Mon Sep 17 00:00:00 2001 From: Bartosz Oleaczek Date: Mon, 8 Jul 2024 07:49:02 +0200 Subject: [PATCH 3/4] remove spaces from logs when printing errors --- norduser/process_monitor.go | 14 +++++++------- norduser/process_monitor_snap.go | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/norduser/process_monitor.go b/norduser/process_monitor.go index a6b58b86..16abe73f 100644 --- a/norduser/process_monitor.go +++ b/norduser/process_monitor.go @@ -54,7 +54,7 @@ func (s *norduserState) changeState(newState norduserState, (newState == loginGUI || newState == loginText) { // user logged in, start norduserd userIDs, err := userIDGetter.getUserID(username) if err != nil { - log.Println(internal.ErrorPrefix, "getting user IDs when enabling norduser: ", err) + log.Println(internal.ErrorPrefix, "getting user IDs when enabling norduser:", err) return } @@ -72,7 +72,7 @@ func (s *norduserState) changeState(newState norduserState, newState == notActive { // user logged out when norduser was running, stop norduserd userIDs, err := userIDGetter.getUserID(username) if err != nil { - log.Println(internal.ErrorPrefix, "getting user IDs when disabling norduser: ", err) + log.Println(internal.ErrorPrefix, "getting user IDs when disabling norduser:", err) return } @@ -86,12 +86,12 @@ func (s *norduserState) changeState(newState norduserState, // to restart norduserd in order to re-enable tray when user logs back in to GUI userIDs, err := userIDGetter.getUserID(username) if err != nil { - log.Println(internal.ErrorPrefix, "getting user IDs when restarting norduser: ", err) + log.Println(internal.ErrorPrefix, "getting user IDs when restarting norduser:", err) return } if err := norduserSrevice.Restart(userIDs.uid); err != nil { - log.Println(internal.ErrorPrefix, "failed to restart norduserd: ", err) + log.Println(internal.ErrorPrefix, "failed to restart norduserd:", err) return } @@ -224,14 +224,14 @@ func (n *NorduserProcessMonitor) Start() error { // the file instead of modifications. if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) { if newGroupMembers, err := n.handleGroupFileUpdate(currentGrupMembers); err != nil { - log.Println(internal.ErrorPrefix, "failed to handle change of groupfile: ", err) + log.Println(internal.ErrorPrefix, "failed to handle change of groupfile:", err) } else { currentGrupMembers = newGroupMembers } } } else if event.Name == utmpFilePath { if newGroupMembers, err := n.handleUTMPFileUpdate(currentGrupMembers); err != nil { - log.Println(internal.ErrorPrefix, "failed to handle change of utmp file: ", err) + log.Println(internal.ErrorPrefix, "failed to handle change of utmp file:", err) } else { currentGrupMembers = newGroupMembers } @@ -240,7 +240,7 @@ func (n *NorduserProcessMonitor) Start() error { if !ok { return fmt.Errorf("groupfile monitor error channel closed") } - log.Println(internal.ErrorPrefix, "group monitor error: ", err) + log.Println(internal.ErrorPrefix, "group monitor error:", err) } } } diff --git a/norduser/process_monitor_snap.go b/norduser/process_monitor_snap.go index 7558fba9..7ca659c6 100644 --- a/norduser/process_monitor_snap.go +++ b/norduser/process_monitor_snap.go @@ -20,14 +20,14 @@ func (n *NorduserProcessMonitor) stopForDeletedGroupMembers(currentGroupMembers userIDs, err := n.userIDGetter.getUserID(username) if err != nil { - log.Println(internal.ErrorPrefix, "getting users ID: ", err) + log.Println(internal.ErrorPrefix, "getting users ID:", err) groupMembersUpdate = append(groupMembersUpdate, username) continue } if err := n.norduserd.Stop(userIDs.uid, false); err != nil { groupMembersUpdate = append(groupMembersUpdate, username) - log.Println(internal.ErrorPrefix, "stopping norduser: ", err) + log.Println(internal.ErrorPrefix, "stopping norduser:", err) } } @@ -62,7 +62,7 @@ func (n *NorduserProcessMonitor) StartSnap() error { if event.Has(fsnotify.Create) || event.Has(fsnotify.Write) { newGroupMembers, err := getNordVPNGroupMembers() if err != nil { - log.Println(internal.ErrorPrefix, "getting new group members: ", err) + log.Println(internal.ErrorPrefix, "getting new group members:", err) } else { groupMembers = n.stopForDeletedGroupMembers(groupMembers, newGroupMembers) } @@ -72,7 +72,7 @@ func (n *NorduserProcessMonitor) StartSnap() error { if !ok { return fmt.Errorf("groupfile monitor error channel closed") } - log.Println(internal.ErrorPrefix, "group monitor error: ", err) + log.Println(internal.ErrorPrefix, "group monitor error:", err) } } } From 1daaef3e0414c0cde22d62a51eb6a14cb1491b9d Mon Sep 17 00:00:00 2001 From: Bartosz Oleaczek Date: Tue, 9 Jul 2024 07:06:52 +0200 Subject: [PATCH 4/4] fix tests --- norduser/process_monitor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/norduser/process_monitor.go b/norduser/process_monitor.go index 16abe73f..76328e39 100644 --- a/norduser/process_monitor.go +++ b/norduser/process_monitor.go @@ -185,7 +185,7 @@ func getWatcher(pathsToMonitor ...string) (watcher *fsnotify.Watcher, err error) } defer func() { - if err != nil { + if err != nil && watcher != nil { watcher.Close() } }()