diff --git a/changelog/fragments/1720713910-Fix-issue-with-Windows-install-when-user-has-no-home-directory.yaml b/changelog/fragments/1720713910-Fix-issue-with-Windows-install-when-user-has-no-home-directory.yaml new file mode 100644 index 00000000000..9f0e6badfac --- /dev/null +++ b/changelog/fragments/1720713910-Fix-issue-with-Windows-install-when-user-has-no-home-directory.yaml @@ -0,0 +1,32 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: bug-fix + +# Change summary; a 80ish characters long description of the change. +summary: Fix issue with Windows install when user has no home directory + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +#description: + +# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. +component: + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +pr: https://github.com/elastic/elastic-agent/pull/5118 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +issue: https://github.com/elastic/elastic-agent/issues/5019 diff --git a/internal/pkg/agent/cmd/enroll_unix.go b/internal/pkg/agent/cmd/enroll_unix.go index 228b0bc624c..26d86e228d0 100644 --- a/internal/pkg/agent/cmd/enroll_unix.go +++ b/internal/pkg/agent/cmd/enroll_unix.go @@ -31,7 +31,10 @@ func getFileOwnerFromCmd(cmd *cobra.Command) (utils.FileOwner, error) { if err != nil { return utils.FileOwner{}, err } - ownership := utils.CurrentFileOwner() + ownership, err := utils.CurrentFileOwner() + if err != nil { + return utils.FileOwner{}, err + } if uid != -1 { ownership.UID = uid } diff --git a/internal/pkg/agent/cmd/run.go b/internal/pkg/agent/cmd/run.go index fe9aa61f37c..e9f2cc4e406 100644 --- a/internal/pkg/agent/cmd/run.go +++ b/internal/pkg/agent/cmd/run.go @@ -724,7 +724,11 @@ func ensureInstallMarkerPresent() error { // Otherwise, we're being upgraded from a version of an installed Agent // that didn't use an installation marker file (that is, before v8.8.0). // So create the file now. - if err := install.CreateInstallMarker(paths.Top(), utils.CurrentFileOwner()); err != nil { + ownership, err := utils.CurrentFileOwner() + if err != nil { + return fmt.Errorf("failed to get current file owner: %w", err) + } + if err := install.CreateInstallMarker(paths.Top(), ownership); err != nil { return fmt.Errorf("unable to create installation marker file during upgrade: %w", err) } diff --git a/internal/pkg/agent/install/install_test.go b/internal/pkg/agent/install/install_test.go index c3a36dfb885..d12cc00ef16 100644 --- a/internal/pkg/agent/install/install_test.go +++ b/internal/pkg/agent/install/install_test.go @@ -177,7 +177,9 @@ func TestCopyFiles(t *testing.T) { func TestSetupInstallPath(t *testing.T) { tmpdir := t.TempDir() - err := setupInstallPath(tmpdir, utils.CurrentFileOwner()) + ownership, err := utils.CurrentFileOwner() + require.NoError(t, err) + err = setupInstallPath(tmpdir, ownership) require.NoError(t, err) require.FileExists(t, filepath.Join(tmpdir, paths.MarkerFileName)) } diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index 703aef69e4e..c3deb43ca33 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -136,7 +136,10 @@ func EnsureStoppedService(topPath string, pt *progressbar.ProgressBar) (service. func checkForUnprivilegedVault(ctx context.Context, opts ...vault.OptionFunc) (bool, error) { // check if we have a file vault to detect if we have to use it for reading config opts = append(opts, vault.WithReadonly(true)) - vaultOpts := vault.ApplyOptions(opts...) + vaultOpts, err := vault.ApplyOptions(opts...) + if err != nil { + return false, err + } fileVault, fileVaultErr := vault.NewFileVault(ctx, vaultOpts) if fileVaultErr == nil { ok, keyErr := fileVault.Exists(ctx, secret.AgentSecretKey) diff --git a/internal/pkg/agent/install/uninstall_test.go b/internal/pkg/agent/install/uninstall_test.go index 50b41fc4898..9f83c2b658e 100644 --- a/internal/pkg/agent/install/uninstall_test.go +++ b/internal/pkg/agent/install/uninstall_test.go @@ -106,7 +106,9 @@ func Test_checkForUnprivilegedVault(t *testing.T) { } func initFileVault(t *testing.T, ctx context.Context, testVaultPath string, keys map[string][]byte) { - newFileVault, err := vault.NewFileVault(ctx, vault.ApplyOptions(vault.WithVaultPath(testVaultPath))) + opts, err := vault.ApplyOptions(vault.WithVaultPath(testVaultPath)) + require.NoError(t, err) + newFileVault, err := vault.NewFileVault(ctx, opts) require.NoError(t, err, "setting up test file vault store") defer func(newFileVault *vault.FileVault) { err := newFileVault.Close() diff --git a/internal/pkg/agent/perms/opts.go b/internal/pkg/agent/perms/opts.go index 1735b084dcf..67c0a696210 100644 --- a/internal/pkg/agent/perms/opts.go +++ b/internal/pkg/agent/perms/opts.go @@ -5,6 +5,7 @@ package perms import ( + "fmt" "os" "github.com/elastic/elastic-agent/pkg/utils" @@ -35,13 +36,17 @@ func WithOwnership(ownership utils.FileOwner) OptFunc { } } -func newOpts(optFuncs ...OptFunc) *opts { +func newOpts(optFuncs ...OptFunc) (*opts, error) { + ownership, err := utils.CurrentFileOwner() + if err != nil { + return nil, fmt.Errorf("failed to get current file owner: %w", err) + } o := &opts{ mask: defaultMask, - ownership: utils.CurrentFileOwner(), + ownership: ownership, } for _, f := range optFuncs { f(o) } - return o + return o, nil } diff --git a/internal/pkg/agent/perms/unix.go b/internal/pkg/agent/perms/unix.go index 098c5b3462d..81b02eb7bbc 100644 --- a/internal/pkg/agent/perms/unix.go +++ b/internal/pkg/agent/perms/unix.go @@ -16,7 +16,10 @@ import ( // FixPermissions fixes the permissions so only root:root is the owner and no world read-able permissions func FixPermissions(topPath string, opts ...OptFunc) error { - o := newOpts(opts...) + o, err := newOpts(opts...) + if err != nil { + return err + } return filepath.Walk(topPath, func(name string, info fs.FileInfo, err error) error { if errors.Is(err, fs.ErrNotExist) { return nil diff --git a/internal/pkg/agent/perms/windows.go b/internal/pkg/agent/perms/windows.go index 6a2fb208f5e..07124e590f1 100644 --- a/internal/pkg/agent/perms/windows.go +++ b/internal/pkg/agent/perms/windows.go @@ -22,7 +22,10 @@ import ( // FixPermissions fixes the permissions so only SYSTEM and Administrators have access to the files in the install path func FixPermissions(topPath string, opts ...OptFunc) error { - o := newOpts(opts...) + o, err := newOpts(opts...) + if err != nil { + return err + } // SYSTEM and Administrators always get permissions // https://support.microsoft.com/en-us/help/243330/well-known-security-identifiers-in-windows-operating-systems diff --git a/internal/pkg/agent/vault/vault.go b/internal/pkg/agent/vault/vault.go index 22ae9ccc769..83e9ccfdce3 100644 --- a/internal/pkg/agent/vault/vault.go +++ b/internal/pkg/agent/vault/vault.go @@ -27,7 +27,10 @@ type Vault interface { } func New(ctx context.Context, opts ...OptionFunc) (Vault, error) { - options := ApplyOptions(opts...) + options, err := ApplyOptions(opts...) + if err != nil { + return nil, err + } if runtime.GOOS == "darwin" && !options.unprivileged { return NewDarwinKeyChainVault(ctx, options) diff --git a/internal/pkg/agent/vault/vault_file_test.go b/internal/pkg/agent/vault/vault_file_test.go index 526c58961ea..80f73db76ef 100644 --- a/internal/pkg/agent/vault/vault_file_test.go +++ b/internal/pkg/agent/vault/vault_file_test.go @@ -33,7 +33,10 @@ func TestFileVaultRekey(t *testing.T) { defer cn() vaultPath := getTestFileVaultPath(t) - options := ApplyOptions(WithVaultPath(vaultPath)) + options, err := ApplyOptions(WithVaultPath(vaultPath)) + if err != nil { + t.Fatal(err) + } v, err := NewFileVault(ctx, options) if err != nil { t.Fatal(err) @@ -83,7 +86,10 @@ func TestFileVault(t *testing.T) { ctx, cn := context.WithCancel(context.Background()) defer cn() - options := ApplyOptions(WithVaultPath(vaultPath)) + options, err := ApplyOptions(WithVaultPath(vaultPath)) + if err != nil { + t.Fatal(err) + } v, err := NewFileVault(ctx, options) if err != nil { t.Fatal(err) @@ -209,7 +215,10 @@ func TestFileVaultConcurrent(t *testing.T) { } func doCrud(t *testing.T, ctx context.Context, vaultPath, key string) error { - options := ApplyOptions(WithVaultPath(vaultPath)) + options, err := ApplyOptions(WithVaultPath(vaultPath)) + if err != nil { + return err + } v, err := NewFileVault(ctx, options) if err != nil { return fmt.Errorf("could not create new vault: %w", err) diff --git a/internal/pkg/agent/vault/vault_options.go b/internal/pkg/agent/vault/vault_options.go index 81b2a976903..157616e081b 100644 --- a/internal/pkg/agent/vault/vault_options.go +++ b/internal/pkg/agent/vault/vault_options.go @@ -5,6 +5,7 @@ package vault import ( + "fmt" "time" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" @@ -70,7 +71,11 @@ func WithUnprivileged(unprivileged bool) OptionFunc { } // ApplyOptions applies options for Windows, Linux and Mac, not all the options may be used -func ApplyOptions(opts ...OptionFunc) Options { +func ApplyOptions(opts ...OptionFunc) (Options, error) { + ownership, err := utils.CurrentFileOwner() + if err != nil { + return Options{}, fmt.Errorf("failed to get current file owner: %w", err) + } o := Options{ CommonVaultOptions: CommonVaultOptions{ readonly: false, @@ -79,7 +84,7 @@ func ApplyOptions(opts ...OptionFunc) Options { FileVaultOptions: FileVaultOptions{ vaultPath: paths.AgentVaultPath(), lockRetryDelay: defaultFlockRetryDelay, - ownership: utils.CurrentFileOwner(), + ownership: ownership, }, KeychainVaultOptions: KeychainVaultOptions{ entryName: paths.AgentKeychainName(), @@ -89,5 +94,5 @@ func ApplyOptions(opts ...OptionFunc) Options { for _, opt := range opts { opt(&o) } - return o + return o, nil } diff --git a/pkg/utils/perm_unix.go b/pkg/utils/perm_unix.go index 4adcfc178aa..97d428d18ff 100644 --- a/pkg/utils/perm_unix.go +++ b/pkg/utils/perm_unix.go @@ -18,11 +18,11 @@ type FileOwner struct { } // CurrentFileOwner returns the executing UID and GID of the current process. -func CurrentFileOwner() FileOwner { +func CurrentFileOwner() (FileOwner, error) { return FileOwner{ UID: os.Getuid(), GID: os.Getgid(), - } + }, nil } // HasStrictExecPerms ensures that the path is executable by the owner, cannot be written by anyone other than the diff --git a/pkg/utils/perm_windows.go b/pkg/utils/perm_windows.go index 16d66c09a25..bcdf03df62f 100644 --- a/pkg/utils/perm_windows.go +++ b/pkg/utils/perm_windows.go @@ -7,7 +7,8 @@ package utils import ( - "os/user" + "fmt" + "syscall" ) const ( @@ -26,20 +27,38 @@ type FileOwner struct { } // CurrentFileOwner returns the executing UID and GID of the current process. -// -// Note: Very unlikely for this to panic if this function is unable to get the current -// user. Not being able to get the current user, is a critical problem and nothing -// can continue so a panic is appropriate. -func CurrentFileOwner() FileOwner { - u, err := user.Current() +func CurrentFileOwner() (FileOwner, error) { + // os/user.Current() is not used here, because it tries to access the users home + // directory. It is possible during installation that the users home directory + // is not created yet. See issue https://github.com/elastic/elastic-agent/issues/5019 + // for more information. + t, err := syscall.OpenCurrentProcessToken() if err != nil { - // should not fail; if it does then there is a big problem - panic(err) + return FileOwner{}, fmt.Errorf("failed to open current process token: %w", err) } - return FileOwner{ - UID: u.Uid, - GID: u.Gid, + defer func() { + _ = t.Close() + }() + u, err := t.GetTokenUser() + if err != nil { + return FileOwner{}, fmt.Errorf("failed to get token user: %w", err) } + pg, err := t.GetTokenPrimaryGroup() + if err != nil { + return FileOwner{}, fmt.Errorf("failed to get token primary group: %w", err) + } + uid, err := u.User.Sid.String() + if err != nil { + return FileOwner{}, fmt.Errorf("failed to convert token user sid to string: %w", err) + } + gid, err := pg.PrimaryGroup.String() + if err != nil { + return FileOwner{}, fmt.Errorf("failed to convert token primary group sid to string: %w", err) + } + return FileOwner{ + UID: uid, + GID: gid, + }, nil } // HasStrictExecPerms ensures that the path is executable by the owner and that the owner of the file