Fix creation of containing directories for files in tar packages

[pchila]

What does this PR do?

This change fixes the creation of destination directories for file contained in a tar package.
If a file needs to be extracted to a destination directory that does not yet exist we create the necessary directories on the fly with permission 0750.
If we encounter the same directories after the file, we fix the permissions according to what is specified in the tar archive

Why is it important?

When running without superuser privileges we need to create a directory structure that has permissions that do not block access to the current user

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] I have made corresponding change to the default configuration files
• [x] I have added tests that prove my fix is effective or that my feature works
• [ ] I have added an entry in ./changelog/fragments using the changelog tool
• [ ] I have added an integration test or an E2E test

Author's Checklist

• [ ]

How to test this PR locally

Related issues

• Closes [Unprivileged] Upgrade broken due to permission issues #4093

Use cases

Screenshots

Logs

Questions to ask yourself

• How are we going to support this in production?
• How are we going to measure its adoption?
• How are we going to debug this?
• What are the metrics I should take care of?
• ...

[elasticmachine]

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[mergify]

This pull request does not have a backport label. Could you fix it @pchila? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

[blakerouse]

Looks good. Thanks for the change to remove the need for path adjustments in the unit tests.

[cmacknz]

This should have a changelog entry shouldn't it? It's a bug fix. It's not one anyone is hitting today, that doesn't mean we shouldn't document the change.

[cmacknz]

1. Thank you for this, this is great.
2. This test still passes if you take out the os.Stat call and associated logic

diff --git a/internal/pkg/agent/application/upgrade/step_unpack.go b/internal/pkg/agent/application/upgrade/step_unpack.go
index 4f5b0bd744..82551616dc 100644
--- a/internal/pkg/agent/application/upgrade/step_unpack.go
+++ b/internal/pkg/agent/application/upgrade/step_unpack.go
@@ -10,7 +10,6 @@ import (
        "compress/gzip"
        "fmt"
        "io"
-       "io/fs"
        "os"
        "path/filepath"
        "runtime"
@@ -213,20 +212,8 @@ func untar(log *logger.Logger, version string, archivePath, dataDir string) (str
                        }
                case mode.IsDir():
                        log.Debugw("Unpacking directory", "archive", "tar", "file.path", abs)
-                       // remove any world permissions from the directory
-                       _, err = os.Stat(abs)
-                       if errors.Is(err, fs.ErrNotExist) {
-                               if err := os.MkdirAll(abs, mode.Perm()&0770); err != nil {
-                                       return "", errors.New(err, "TarInstaller: creating directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
-                               }
-                       } else if err != nil {
-                               return "", errors.New(err, "TarInstaller: stat() directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
-                       } else {
-                               // set the appropriate permissions
-                               err = os.Chmod(abs, mode.Perm()&0o770)
-                               if err != nil {
-                                       return "", errors.New(err, fmt.Sprintf("TarInstaller: setting permissions %O for directory %q", mode.Perm()&0o770, abs), errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
-                               }
+                       if err := os.MkdirAll(abs, mode.Perm()&0770); err != nil {
+                               return "", errors.New(err, "TarInstaller: creating directory for file "+abs, errors.TypeFilesystem, errors.M(errors.MetaKeyPath, abs))
                        }
                default:
                        return "", errors.New(fmt.Sprintf("tar file entry %s contained unsupported file type %v", fileName, mode), errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fileName))

go test ./internal/pkg/agent/application/upgrade/... -run TestUpgrader_unpack -count 1 -v
?       github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download/localremote     [no test files]
=== RUN   TestUpgrader_unpack
=== RUN   TestUpgrader_unpack/targz_with_file_before_containing_folder
--- PASS: TestUpgrader_unpack (0.00s)
    --- PASS: TestUpgrader_unpack/targz_with_file_before_containing_folder (0.00s)
PASS
ok      github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade 0.591s

[pchila]

The test passes because of the default 0750 permission when we create the directory on the fly (so we are able to write the file anyways). The code you removed is needed to restore permissions once we come across the directory in the tar archive, but that is not in the assertions. I will modify the test

[pchila]

This should have a changelog entry shouldn't it? It's a bug fix. It's not one anyone is hitting today, that doesn't mean we shouldn't document the change.

@cmacknz not sure about the changelog as the change that introduced the issue is related to the unprivileged elastic-agent feature. Before such change, directories created on the fly would always have 0755 permissions and that would not be corrected if we encountered the directory with different permission later in the tar.gz archive. So a customer cannot have encountered the exact same bug in the released versions.
I added a changelog regardless as we improve on the previous behavior by fixing the permissions as specified in the tar.gz (minus the world ones, which I think was the original intention of the unprivileged change)

[elastic-sonarqube]

Quality Gate passed

The SonarQube Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube