Enforce authorized keys permission

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## [Unreleased](https://github.com/digitalocean/droplet-agent/tree/HEAD)
 
+## [1.2.10](https://github.com/digitalocean/droplet-agent/tree/1.2.10) (2025-02-12)
+### Updated
+- Enforce file permission when updating authorized_keys file
+
 ## [1.2.9](https://github.com/digitalocean/droplet-agent/tree/1.2.9) (2024-12-11)
 ### Updated
 - config: include version in user agent

Makefile

@@ -129,7 +129,7 @@ build: $(binary)
 $(binary): $(gofiles)
 	$(print)
 	$(mkdir)
-	$(go) build -ldflags $(ldflags) -trimpath -o "$@" ./cmd/agent/
+	$(go) build -buildvcs=false -ldflags $(ldflags) -trimpath -o "$@" ./cmd/agent/
 
 shellcheck: $(cache)/shellcheck
 $(cache)/shellcheck: $(shellscripts)
@@ -231,6 +231,9 @@ $(tar_package): $(base_linux_package)
 ## mockgen: generates the mocks for the droplet agent service
 mockgen:
 	@echo "Generating mocks"
+	mockgen -package=mock_os -destination=internal/sysutil/internal/mocks/os_mocks.go os FileInfo
+	mockgen -source=internal/sysutil/common.go -package=sysutil -destination=internal/sysutil/common_mocks.go
+	mockgen -source=internal/sysutil/os_operations_helper.go -package=sysutil -destination=internal/sysutil/os_operations_helper_mocks.go
 	mockgen -source=internal/sysaccess/common.go -package=mocks -destination=internal/sysaccess/internal/mocks/mocks.go
 	mockgen -source=internal/sysaccess/ssh_helper.go -package=sysaccess -destination=internal/sysaccess/ssh_helper_mocks.go
 	mockgen -source=internal/sysaccess/authorized_keys_file_updater.go -package=sysaccess -destination=internal/sysaccess/authorized_keys_file_updater_mocks.go

internal/config/version.go

@@ -3,4 +3,4 @@
 package config
 
 // Version is the current package version.
-const Version = "v1.2.9"
+const Version = "v1.2.10"

internal/sysaccess/authorized_keys_file_updater.go

@@ -3,8 +3,8 @@
 package sysaccess
 
 import (
+	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -42,9 +42,9 @@ func (u *updaterImpl) updateAuthorizedKeysFile(osUsername string, managedKeys []
 		return err
 	}
 	fileExist := true
-	localKeysRaw, err := u.sshMgr.sysMgr.ReadFile(authorizedKeysFile)
+	localKeysRaw, err := u.sshMgr.sysMgr.ReadFileOfUser(authorizedKeysFile, osUser)
 	if err != nil {
-		if !os.IsNotExist(err) {
+		if !errors.Is(err, sysutil.ErrFileNotFound) {
 			return fmt.Errorf("%w:%v", ErrReadAuthorizedKeysFileFailed, err)
 		}
 		fileExist = false
@@ -59,16 +59,16 @@ func (u *updaterImpl) updateAuthorizedKeysFile(osUsername string, managedKeys []
 
 func (u *updaterImpl) do(authorizedKeysFile string, user *sysutil.User, lines []string, srcFileExist bool) (retErr error) {
 	log.Debug("updating [%s]", authorizedKeysFile)
-	tmpFilePath := authorizedKeysFile + ".dotty"
-	tmpFile, err := u.sshMgr.sysMgr.CreateFileForWrite(tmpFilePath, user, 0600)
+	dir := filepath.Dir(authorizedKeysFile)
+	tmpFile, err := u.sshMgr.sysMgr.CreateTempFile(dir, "authorized_keys-*.dotty", user)
 	if err != nil {
 		return fmt.Errorf("%w: failed to create tmp file: %v", ErrWriteAuthorizedKeysFileFailed, err)
 	}
 	defer func() {
 		log.Debug("[%s] updated", authorizedKeysFile)
 		_ = tmpFile.Close()
 		if retErr != nil {
-			_ = u.sshMgr.sysMgr.RemoveFile(tmpFilePath)
+			_ = u.sshMgr.sysMgr.RemoveFile(tmpFile.Name())
 		}
 	}()
 
@@ -77,14 +77,14 @@ func (u *updaterImpl) do(authorizedKeysFile string, user *sysutil.User, lines []
 	}
 
 	if srcFileExist {
-		log.Debug("copying file attribute from [%s] to [%s]", authorizedKeysFile, tmpFilePath)
-		err := u.sshMgr.sysMgr.CopyFileAttribute(authorizedKeysFile, tmpFilePath)
+		log.Debug("copying file attribute from [%s] to [%s]", authorizedKeysFile, tmpFile.Name())
+		err := u.sshMgr.sysMgr.CopyFileAttribute(authorizedKeysFile, tmpFile.Name())
 		if err != nil {
 			return fmt.Errorf("%w:failed to apply file attribute :%v", ErrWriteAuthorizedKeysFileFailed, err)
 		}
 	}
 
-	if err := u.sshMgr.sysMgr.RenameFile(tmpFilePath, authorizedKeysFile); err != nil {
+	if err := u.sshMgr.sysMgr.RenameFile(tmpFile.Name(), authorizedKeysFile); err != nil {
 		return fmt.Errorf("%w:failed to rename:%v", ErrWriteAuthorizedKeysFileFailed, err)
 	}
 	return nil

internal/sysaccess/authorized_keys_file_updater_test.go

@@ -26,12 +26,19 @@ type recorder struct {
 	bytes.Buffer
 	closeCalled int
 	expectedRes string
+	filename    string
 }
 
 func (r *recorder) Close() error {
 	r.closeCalled++
 	return nil
 }
+func (r *recorder) Name() string {
+	return r.filename
+}
+func (r *recorder) Stat() (os.FileInfo, error) {
+	return nil, nil
+}
 
 func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 	log.Mute()
@@ -98,7 +105,7 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return(nil, readFileErr)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return(nil, readFileErr)
 			},
 			[]*SSHKey{
 				validKey1,
@@ -112,9 +119,9 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return(nil, os.ErrNotExist)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return(nil, sysutil.ErrFileNotFound)
 				sshHelper.EXPECT().prepareAuthorizedKeys(gomock.Any(), gomock.Any()).Return([]string{})
-				sysMgr.EXPECT().CreateFileForWrite(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, createFileErr)
+				sysMgr.EXPECT().CreateTempFile(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, createFileErr)
 			},
 			[]*SSHKey{
 				validKey1,
@@ -128,9 +135,9 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return(nil, os.ErrNotExist)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return(nil, sysutil.ErrFileNotFound)
 				sshHelper.EXPECT().prepareAuthorizedKeys([]string{}, []*SSHKey{validKey1}).Return([]string{"line1", "line2"})
-				sysMgr.EXPECT().CreateFileForWrite(authorizedKeyFile+".dotty", validUser1, os.FileMode(0600)).Return(nil, createFileErr)
+				sysMgr.EXPECT().CreateTempFile(authorizedKeyFileDir, "authorized_keys-*.dotty", validUser1).Return(nil, createFileErr)
 			},
 			[]*SSHKey{
 				validKey1,
@@ -142,13 +149,13 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 			"should properly write files to tmp file and remove it if error happens",
 			func(sysMgr *mocks.MocksysManager, sshHelper *MocksshHelper, recorder *recorder) {
 				tmpFile := authorizedKeyFile + ".dotty"
-
+				recorder.filename = tmpFile
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return([]byte{}, nil)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return([]byte{}, nil)
 				sshHelper.EXPECT().prepareAuthorizedKeys([]string{""}, []*SSHKey{validKey1}).Return([]string{"line1", "line2"})
-				sysMgr.EXPECT().CreateFileForWrite(tmpFile, validUser1, os.FileMode(0600)).Return(recorder, nil)
+				sysMgr.EXPECT().CreateTempFile(authorizedKeyFileDir, "authorized_keys-*.dotty", validUser1).Return(recorder, nil)
 				sysMgr.EXPECT().CopyFileAttribute(authorizedKeyFile, tmpFile).Return(nil)
 				sysMgr.EXPECT().RenameFile(gomock.Any(), gomock.Any()).Return(errors.New("rename-error"))
 				sysMgr.EXPECT().RemoveFile(tmpFile).Return(nil)
@@ -166,13 +173,13 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 			"should properly write files to tmp file and rename it to original file",
 			func(sysMgr *mocks.MocksysManager, sshHelper *MocksshHelper, recorder *recorder) {
 				tmpFile := authorizedKeyFile + ".dotty"
-
+				recorder.filename = tmpFile
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return([]byte{}, nil)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return([]byte{}, nil)
 				sshHelper.EXPECT().prepareAuthorizedKeys([]string{""}, []*SSHKey{validKey1}).Return([]string{"line1", "line2"})
-				sysMgr.EXPECT().CreateFileForWrite(tmpFile, validUser1, os.FileMode(0600)).Return(recorder, nil)
+				sysMgr.EXPECT().CreateTempFile(authorizedKeyFileDir, "authorized_keys-*.dotty", validUser1).Return(recorder, nil)
 				sysMgr.EXPECT().CopyFileAttribute(authorizedKeyFile, tmpFile).Return(nil)
 				sysMgr.EXPECT().RenameFile(tmpFile, authorizedKeyFile).Return(nil)
 			},
@@ -189,16 +196,17 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 			"should read existing keys and attempt to merge",
 			func(sysMgr *mocks.MocksysManager, sshHelper *MocksshHelper, recorder *recorder) {
 				tmpFile := authorizedKeyFile + ".dotty"
+				recorder.filename = tmpFile
 				localKeysRaw := []byte("local1\nlocal2\nlocal3\n\n\n")
 				localKeys := []string{
 					"local1", "local2", "local3",
 				}
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return(localKeysRaw, nil)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return(localKeysRaw, nil)
 				sshHelper.EXPECT().prepareAuthorizedKeys(localKeys, []*SSHKey{validKey1}).Return([]string{"local1", "local2", "local3", "line1", "line2"})
-				sysMgr.EXPECT().CreateFileForWrite(tmpFile, validUser1, os.FileMode(0600)).Return(recorder, nil)
+				sysMgr.EXPECT().CreateTempFile(authorizedKeyFileDir, "authorized_keys-*.dotty", validUser1).Return(recorder, nil)
 				sysMgr.EXPECT().CopyFileAttribute(authorizedKeyFile, tmpFile).Return(nil)
 				sysMgr.EXPECT().RenameFile(tmpFile, authorizedKeyFile).Return(nil)
 			},
@@ -215,13 +223,14 @@ func Test_updaterImpl_updateAuthorizedKeysFile(t *testing.T) {
 			"should skip copying file attribute if original file not exists",
 			func(sysMgr *mocks.MocksysManager, sshHelper *MocksshHelper, recorder *recorder) {
 				tmpFile := authorizedKeyFile + ".dotty"
+				recorder.filename = tmpFile
 
 				sysMgr.EXPECT().GetUserByName(osUsername).Return(validUser1, nil)
 				sshHelper.EXPECT().authorizedKeysFile(validUser1).Return(authorizedKeyFile)
 				sysMgr.EXPECT().MkDirIfNonExist(authorizedKeyFileDir, validUser1, os.FileMode(0700)).Return(nil)
-				sysMgr.EXPECT().ReadFile(authorizedKeyFile).Return(nil, os.ErrNotExist)
+				sysMgr.EXPECT().ReadFileOfUser(authorizedKeyFile, validUser1).Return(nil, sysutil.ErrFileNotFound)
 				sshHelper.EXPECT().prepareAuthorizedKeys([]string{}, []*SSHKey{validKey1}).Return([]string{"line1", "line2"})
-				sysMgr.EXPECT().CreateFileForWrite(tmpFile, validUser1, os.FileMode(0600)).Return(recorder, nil)
+				sysMgr.EXPECT().CreateTempFile(authorizedKeyFileDir, "authorized_keys-*.dotty", validUser1).Return(recorder, nil)
 				sysMgr.EXPECT().RenameFile(tmpFile, authorizedKeyFile).Return(nil)
 			},
 			[]*SSHKey{
@@ -322,11 +331,12 @@ func Test_updaterImpl_updateAuthorizedKeysFile_threadSafe(t *testing.T) {
 
 			originalFile := ""
 			for j := 0; j != concurrentUpdatePerUser; j++ {
+				recorders[i][j].filename = tmpFilePath
 				originalFile += "key\n"
 				localKeys := strings.Split(strings.TrimRight(originalFile, "\n"), "\n")
-				sysMgrMock.EXPECT().ReadFile(keysFile).Return([]byte(originalFile), nil)
+				sysMgrMock.EXPECT().ReadFileOfUser(keysFile, user).Return([]byte(originalFile), nil)
 				sshHelperMock.EXPECT().prepareAuthorizedKeys(localKeys, fakeKeys).Return(append(localKeys, "key"))
-				sysMgrMock.EXPECT().CreateFileForWrite(tmpFilePath, user, os.FileMode(0600)).Return(recorders[i][j], nil)
+				sysMgrMock.EXPECT().CreateTempFile(filepath.Dir(keysFile), "authorized_keys-*.dotty", user).Return(recorders[i][j], nil)
 			}
 
 		}

internal/sysaccess/common.go

@@ -4,7 +4,6 @@ package sysaccess
 
 import (
 	"errors"
-	"io"
 	"os"
 	"time"
 
@@ -55,9 +54,10 @@ type sshKeyInfo struct {
 type sysManager interface {
 	GetUserByName(username string) (*sysutil.User, error)
 	MkDirIfNonExist(dir string, user *sysutil.User, perm os.FileMode) error
-	CreateFileForWrite(file string, user *sysutil.User, perm os.FileMode) (io.WriteCloser, error)
+	CreateTempFile(dir, pattern string, user *sysutil.User) (sysutil.File, error)
 	CopyFileAttribute(from, to string) error
 	ReadFile(filename string) ([]byte, error)
+	ReadFileOfUser(filename string, user *sysutil.User) ([]byte, error)
 	RenameFile(oldpath, newpath string) error
 	RemoveFile(name string) error
 	FileExists(name string) (bool, error)

internal/sysutil/common.go

@@ -18,8 +18,18 @@ var (
 	ErrMakeDirFailed = fmt.Errorf("failed to make directory")
 	// ErrCreateFileFailed indicates the error of failing to create a file
 	ErrCreateFileFailed = fmt.Errorf("failed to create file")
+	// ErrFileNotFound indicates a file not exist error
+	ErrFileNotFound = fmt.Errorf("file not found")
+	// ErrOpenFileFailed indicates the error of failing to open a file
+	ErrOpenFileFailed = fmt.Errorf("failed to open file")
 	// ErrRunCmdFailed is returned when a command is failed to run
 	ErrRunCmdFailed = fmt.Errorf("failed to run command")
+	// ErrInvalidFileType is returned when the file type is unexpected
+	ErrInvalidFileType = fmt.Errorf("invalid file type")
+	// ErrUnexpected indicates an unexpected error
+	ErrUnexpected = fmt.Errorf("unexpected error")
+	// ErrPermissionDenied indicates the given permission is not sufficient to perform the designated operation
+	ErrPermissionDenied = fmt.Errorf("insufficient permission")
 )
 
 // User struct contains information of a user
@@ -38,8 +48,18 @@ type CmdResult struct {
 	StdErr   string
 }
 
+// File contains common operations on *os.File
+type File interface {
+	Name() string
+	Close() error
+	Stat() (os.FileInfo, error)
+
+	io.ReadWriteCloser
+}
+
 type osOperator interface {
 	getpwnam(username string) (*User, error)
 	mkdir(dir string, user *User, perm os.FileMode) error
-	createFileForWrite(file string, user *User, perm os.FileMode) (io.WriteCloser, error)
+	createTempFile(dir, pattern string, user *User) (File, error)
+	openFile(name string, flag int, perm os.FileMode) (File, error)
 }