Harden linux/windows vault implementation for concurrent access. #3240

aleksmaus · 2023-08-15T02:15:47Z

What does this PR do?

Harden linux/windows vault implementation for concurrent access.

Problem details

During repetitive E2E tests runs (took over 5 hours to reproduce) the file based vault implementation (that is used for linux and Windows) got into the state where the key value was corrupted, the resulted in the error:

Enrollment failed after failed to store agent config: could not save enrollment information: failed to ensure key: 
could not get agent key: could not decrypt key: cipher: message authentication failed

It's difficult to reproduce in the normal agent usage scenario, but looking at the implementation there is no mitigation against multiple processes accessing vault. The lower level API allows to reproduce the situation artificially see TestVaultConcurrent test https://github.com/elastic/elastic-agent/compare/main...aleksmaus:feature/harden_fsvault?expand=1#diff-8a5388ab5e868a37ee07711c7978dc51561a302be8fa560946038e71375fe70bR183

Solution

Harden linux/windows vault implementation for concurrent access.

Use "atomic" write/rename for the data files https://github.com/elastic/elastic-agent/compare/main...aleksmaus:feature/harden_fsvault?expand=1#diff-5176984040d0726ff96ca6e27e372355b57829a8f657c706fd6960a6e4f96a45R17
Use file locking https://github.com/elastic/elastic-agent/compare/main...aleksmaus:feature/harden_fsvault?expand=1#diff-6c9bf532380b28932cd23bc8d308c07263fa77a517d41fc0e2fd390d5e361a9eR71. The .lock file is created inside of the vault directory now.
For the file locking API needed to pass context.Context down to the storage, that lead to the storage API change and the code that uses the storage is modified in order to pass the context down.
Removed migrate_secret.go that doesn't seem to be used anywhere anymore.

Why is it important?

Hardens the vault data agains corruption when it's being accessed concurrently.

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
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

mergify · 2023-08-15T02:16:23Z

This pull request does not have a backport label. Could you fix it @aleksmaus? 🙏
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.

elasticmachine · 2023-08-15T02:28:09Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-08-24T15:44:54.124+0000
Duration: 27 min 59 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6217
Skipped	47
Total	6264

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-08-15T03:25:25Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.765% (`80/81`)	👍 0.015
Files	67.368% (`192/285`)	👎 -0.464
Classes	66.105% (`353/534`)	👎 -0.187
Methods	52.874% (`1104/2088`)	👍
Lines	38.572% (`12596/32656`)	👎 -0.141
Conditionals	100.0% (`0/0`)	💚

elasticmachine · 2023-08-15T08:54:04Z

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

internal/pkg/agent/vault/vault_linux.go

cmacknz · 2023-08-15T15:59:45Z

internal/pkg/agent/migration/migrate_secret.go

-	darwin = "darwin"
-)
-
-// MigrateAgentSecret migrates agent secret if the secret doesn't exists agent upgrade from 8.3.0 - 8.3.2 to 8.x and above on Linux and Windows platforms.


Why don't we need this anymore? Upgrading from 8.3.x -> 8.9.x is still possible.

I have no idea, but it's not called from anywhere.

Looks like we lost this in the big 8.6 merge

elastic-agent/internal/pkg/agent/cmd/run.go

Lines 132 to 140 in 0067844

// This is specific for the agent upgrade from 8.3.0 - 8.3.2 to 8.x and above on Linux and Windows platforms.

// Addresses the issue: https://github.com/elastic/elastic-agent/issues/682

// The vault directory was located in the hash versioned "Home" directory of the agent.

// This moves the vault directory two levels up into the "Config" directory next to fleet.enc file

// in order to be able to "upgrade" the agent from deb/rpm that is not invoking the upgrade handle and

// doesn't perform the migration of the state or vault.

// If the agent secret doesn't exist, then search for the newest agent secret in the agent data directories

// and migrate it into the new vault location.

err = migration.MigrateAgentSecret(logger)

Here was the original bug: #682

Would need to confirm if that situation is still possible with the change in directory structure in 8.6 and above.

Whooops. Been doing ok without it for few releases. 🤷

I've been trying to decide what to do about this one. I am fine with deleting it.

The original fix went into 8.3.3 which also permanently moved the vault directory location outside of the versioned data path:

Here's the commit in 8.3.3 74a597c

Here it is in 8.3.3 as the last commit on July 19th: https://github.com/elastic/elastic-agent/commits/v8.3.3

At that point it should be fixed forever as the migration isn't needed anymore. Anyone on 8.3.0 would have upgraded to 8.3.3 and got this fix (or gone directly to 8.4.x or 8.5.x and gotten it).

If we ever seen a version of this problem it can be solved by upgrading to one of 8.3.3, 8.4.x or 8.5.x before going to a version higher than that so the fix hasn't been lost.

mergify · 2023-08-15T18:04:21Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature/harden_fsvault upstream/feature/harden_fsvault
git merge upstream/main
git push upstream feature/harden_fsvault

internal/pkg/agent/vault/vault_linux.go

AndersonQ · 2023-08-17T12:20:56Z

internal/pkg/agent/vault/vault_linux.go

+	if err != nil {
+		return fmt.Errorf("failed to acquire shared lock: %v, err: %w", v.lock.Path(), err)
+	}
+	return err


[Blocker]
Please, err is always nil so return nil.

Suggested change

return err

return nil

internal/pkg/agent/vault/vault_linux.go

AndersonQ · 2023-08-17T12:31:14Z

internal/pkg/agent/vault/vault_linux.go

+// writeFile "atomic" file write, utilizes temp file and replace
+// os.CreateTemp creates the file with 0600 mask, which is what we need
+func writeFile(fp string, data []byte) (err error) {
+	dir, fn := filepath.Split(fp)
+	if dir == "" {
+		dir = "."
+	}
+
+	f, err := os.CreateTemp(dir, fn)
+	if err != nil {
+		return fmt.Errorf("failed creating temp file: %w", err)
+	}
+	defer func() {


[Suggestion]
1 - move the comment about the os.CreateTem to where it's used.
2 - I think the sentence could be improved

Suggested change

// writeFile "atomic" file write, utilizes temp file and replace

// os.CreateTemp creates the file with 0600 mask, which is what we need

func writeFile(fp string, data []byte) (err error) {

dir, fn := filepath.Split(fp)

if dir == "" {

dir = "."

}

f, err := os.CreateTemp(dir, fn)

if err != nil {

return fmt.Errorf("failed creating temp file: %w", err)

}

defer func() {

// writeFile performs an "atomic" file write operation, using a temporary file

// and then replacing the original.

func writeFile(fp string, data []byte) (err error) {

dir, fn := filepath.Split(fp)

if dir == "" {

dir = "."

}

// os.CreateTemp creates the file with a 0600 permission mask,

// which is the required permission setting.

f, err := os.CreateTemp(dir, fn)

if err != nil {

return fmt.Errorf("failed creating temp file: %w", err)

}

defer func() {

internal/pkg/agent/vault/vault_linux.go

internal/pkg/agent/vault/vault_options_nondarwin.go

internal/pkg/agent/vault/vault_test.go

AndersonQ · 2023-08-17T12:45:54Z

internal/pkg/agent/vault/vault_test.go

+	}
+}
+
+func doCrud(t *testing.T, ctx context.Context, vaultPath, key string) error {


[Suggestion]
I don't see t being used, so you could remove it.

Suggested change

func doCrud(t *testing.T, ctx context.Context, vaultPath, key string) error {

func doCrud(ctx context.Context, vaultPath, key string) error {

AndersonQ · 2023-08-17T12:47:20Z

internal/pkg/agent/vault/vault_windows.go

@@ -59,42 +65,90 @@ func New(path string, opts ...OptionFunc) (v *Vault, err error) {
 		}
 	}

-	entropy, err := getOrCreateSeed(path, options.readonly)
+	r := &Vault{


[NIT]
Why are you using r instead of v?

want to return v as nil if err happens after this line

internal/pkg/agent/vault/vault_windows.go

…ew comments

aleksmaus · 2023-08-22T13:33:27Z

buildkite test this

pierrehilbert · 2023-08-22T13:41:45Z

Buildkite is failing on main because we are unable to deploy 8.11.0 on Elastic Cloud for now. @AndersonQ is working on a workaround to unlock us in the meantime.

AndersonQ

just one small blocker, I marked it as [Blocker]

AndersonQ · 2023-08-23T14:18:17Z

internal/pkg/agent/vault/vault_notdarwin.go

+		return nil, err
+	}
+	defer func() {
+		err = r.unlock(err)


[Blocker]
It'll override the error of getOrCreateSeed. Better to errors.Join the errors.

it will not, it's a helper func using errors.Join

AndersonQ · 2023-08-23T14:19:00Z

internal/pkg/agent/vault/vault_notdarwin.go

+		return nil, err
+	}
+	defer func() {
+		err = v.unlock(err)


[Blocker]
Same here, it'll override any other error

AndersonQ · 2023-08-23T14:25:28Z

internal/pkg/agent/vault/vault_notdarwin.go

+// unlock unlocks the file lock and preserves the original error if there was a error
+func (v *Vault) unlock(err error) error {
+	return errors.Join(err, v.lock.Unlock())
+}


[Blocker]
I don't understand why an unlock function needs to receive an error. The caller is responsible to handle any previous error and then decide what to do if unlock fails as well.

Besides it harms the single responsibility principle, unlock is also responsible to aggregate an error for no obvious reason.

do not agree with making it a blocker, it's a helper function to make the deferred call code above more compact and consistent, since it's done in multiple places

AndersonQ · 2023-08-23T14:37:46Z

internal/pkg/agent/vault/vault_notdarwin.go

+	if _, err = os.Stat(v.filepathFromKey(key)); err == nil {
+		ok = true
+	} else if errors.Is(err, fs.ErrNotExist) {
+		err = nil
+	}
+	return ok, err


[Suggestion]
Avoid if else for a clearer, more explicit code

Suggested change

if _, err = os.Stat(v.filepathFromKey(key)); err == nil {

ok = true

} else if errors.Is(err, fs.ErrNotExist) {

err = nil

}

return ok, err

if _, err = os.Stat(v.filepathFromKey(key)); err != nil {

if errors.Is(err, fs.ErrNotExist) {

return false, nil

}

return false, err

}

return true, nil

AndersonQ · 2023-08-23T14:38:12Z

internal/pkg/agent/vault/vault_notdarwin.go

+		return err
+	}
+	defer func() {
+		err = v.unlock(err)


[Blocker]
same as the others

explained above why this is done the way it's done

AndersonQ · 2023-08-23T14:39:23Z

internal/pkg/agent/vault/vault_notdarwin.go

+// fileNameFromKey returns the filename as a hash of the vault seed combined with the key
+// this ties the key with the vault seed eliminating the change of attempting
+// to decrypt the key for the wrong vault seed value.


Suggested change

// fileNameFromKey returns the filename as a hash of the vault seed combined with the key

// this ties the key with the vault seed eliminating the change of attempting

// to decrypt the key for the wrong vault seed value.

// fileNameFromKey returns the filename as a hash of the vault seed combined with the key.

// This ties the key with the vault seed eliminating the chance of attempting

// to decrypt the key for the wrong vault seed value.

AndersonQ · 2023-08-23T14:43:08Z

internal/pkg/agent/vault/vault_windows.go

+func writeFile(fp string, data []byte) (err error) {
+	return os.WriteFile(fp, data, 0600)
+}


[Suggestion]

Suggested change

func writeFile(fp string, data []byte) (err error) {

return os.WriteFile(fp, data, 0600)

}

func writeFile(fp string, data []byte) error {

return os.WriteFile(fp, data, 0600)

}

aleksmaus · 2023-08-23T15:11:51Z

just one small blocker, I marked it as [Blocker]

addressed the comments. do not agree with the blocker, added explanation why, it's a private helper function to avoid repeating the same code in 5 places. could you review again?

cmacknz · 2023-08-24T14:43:51Z

This is going to go into 8.10 first and then get forward ported to main. This will get us around the issue with the 8.11 snapshot.

cmacknz · 2023-08-24T14:44:07Z

/test

cmacknz · 2023-08-24T15:44:37Z

/test

* Harden linux/windows vault implementation for concurrent access * Spit off some platform-specific code to make linter happy * Add missing field for windows implementation * Fix return params for Get on windows * Replace deprecated ioutil with os package API for windows implementation * Reorganise platform specific code * Refactored vault code, extracted shared code, addressed the code review comments * Split off platform specific options * Remove wrong build directive from the common options * do not try to generate HTML test report if test XML output isn't present --------- Co-authored-by: Anderson Queiroz <[email protected]> (cherry picked from commit c4c314a)

AndersonQ · 2023-08-24T17:42:18Z

Merged with failing integration tests because this PR fixes another integration tests issue affecting all tests installing the agent. Therefore we need to have it merged to reduce the number of failing tests and focus on one issue at a time.

…) (#3288) * Harden linux/windows vault implementation for concurrent access * Spit off some platform-specific code to make linter happy * Add missing field for windows implementation * Fix return params for Get on windows * Replace deprecated ioutil with os package API for windows implementation * Reorganise platform specific code * Refactored vault code, extracted shared code, addressed the code review comments * Split off platform specific options * Remove wrong build directive from the common options * do not try to generate HTML test report if test XML output isn't present --------- Co-authored-by: Anderson Queiroz <[email protected]> (cherry picked from commit c4c314a) Co-authored-by: Aleksandr Maus <[email protected]>

…ss. (elastic#3240) (elastic#3288)" This reverts commit 996043d.

aleksmaus added 2 commits August 14, 2023 21:37

Harden linux/windows vault implementation for concurrent access

2843c19

Fix comment typo

a4c9fc8

aleksmaus added the enhancement New feature or request label Aug 15, 2023

aleksmaus requested review from AndersonQ, cmacknz and blakerouse August 15, 2023 02:15

aleksmaus requested a review from a team as a code owner August 15, 2023 02:15

aleksmaus changed the title ~~Feature/harden fsvault~~ Harden linux/windows vault implementation for concurrent access. Aug 15, 2023

aleksmaus added the skip-changelog label Aug 15, 2023

mergify bot assigned aleksmaus Aug 15, 2023

mergify bot added the backport-skip label Aug 15, 2023

Some corrections and linter cleanup

0db1cc8

aleksmaus added 4 commits August 14, 2023 22:33

Looks like CI linter doesn't like this particular linter comment

dd37e53

Spit off some pltform-specific code to make linter happy

47aff40

Add missing field for windows implementation

be46f04

Fix return params for Get on windows

7fca502

aleksmaus force-pushed the feature/harden_fsvault branch from 7ac89ee to 7fca502 Compare August 15, 2023 03:01

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Aug 15, 2023

aleksmaus added 2 commits August 15, 2023 09:38

Replace depraced ioutil with os package API for windows implementation

77c2a19

Reorganize platform specific code

abce205

cmacknz reviewed Aug 15, 2023

View reviewed changes

aleksmaus added 2 commits August 15, 2023 15:27

Merge branch 'main' into feature/harden_fsvault

d4d07a7

Resolve issues after main rebase

3c031b1

cmacknz approved these changes Aug 16, 2023

View reviewed changes

cmacknz added the backport-v8.10.0 Automated backport with mergify label Aug 16, 2023

mergify bot removed the backport-skip label Aug 16, 2023

AndersonQ requested changes Aug 17, 2023

View reviewed changes

AndersonQ reviewed Aug 17, 2023

View reviewed changes

Refactored vault code, extracted shared code, addressed the code revi…

ce5f5e6

…ew comments

aleksmaus requested a review from AndersonQ August 17, 2023 18:22

aleksmaus added 2 commits August 17, 2023 14:42

Split off platform specific options

e522044

Remove wrong build directove from the common options

0d2965f

AndersonQ requested changes Aug 23, 2023

View reviewed changes

Address more code review comments

47bd75f

aleksmaus requested a review from AndersonQ August 23, 2023 15:11

Rename the helper .unlock function to unlockAndJoinErrors

5b64a18

AndersonQ approved these changes Aug 23, 2023

View reviewed changes

do not try to generate HTML test report if test XML output isn't present

f67f034

cmacknz changed the base branch from main to 8.10 August 24, 2023 14:42

cmacknz added forwardport-main and removed backport-v8.10.0 Automated backport with mergify labels Aug 24, 2023

AndersonQ merged commit c4c314a into elastic:8.10 Aug 24, 2023
14 checks passed

mergify bot mentioned this pull request Aug 24, 2023

[main](backport #3240) Harden linux/windows vault implementation for concurrent access. #3288

Merged

This was referenced Aug 29, 2023

[Flaky Test] Integration/e2e tests installing the agent randomly fail #3154

Closed

Flaky Test TestFleetManagedUpgrade #3262

Closed

cmacknz added a commit to cmacknz/elastic-agent that referenced this pull request Aug 30, 2023

Revert "harden linux/windows vault implementation for concurrent acce…

34bd046

…ss. (elastic#3240) (elastic#3288)" This reverts commit 996043d.

	// This is specific for the agent upgrade from 8.3.0 - 8.3.2 to 8.x and above on Linux and Windows platforms.
	// Addresses the issue: https://github.com/elastic/elastic-agent/issues/682
	// The vault directory was located in the hash versioned "Home" directory of the agent.
	// This moves the vault directory two levels up into the "Config" directory next to fleet.enc file
	// in order to be able to "upgrade" the agent from deb/rpm that is not invoking the upgrade handle and
	// doesn't perform the migration of the state or vault.
	// If the agent secret doesn't exist, then search for the newest agent secret in the agent data directories
	// and migrate it into the new vault location.
	err = migration.MigrateAgentSecret(logger)

	func doCrud(t *testing.T, ctx context.Context, vaultPath, key string) error {
	func doCrud(ctx context.Context, vaultPath, key string) error {

Harden linux/windows vault implementation for concurrent access. #3240

Harden linux/windows vault implementation for concurrent access. #3240

Conversation

aleksmaus commented Aug 15, 2023

What does this PR do?

Problem details

Solution

Why is it important?

Checklist

mergify bot commented Aug 15, 2023

elasticmachine commented Aug 15, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Aug 15, 2023 • edited Loading

🌐 Coverage report

elasticmachine commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksmaus commented Aug 22, 2023

pierrehilbert commented Aug 22, 2023

AndersonQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksmaus commented Aug 23, 2023

cmacknz commented Aug 24, 2023

cmacknz commented Aug 24, 2023

cmacknz commented Aug 24, 2023

AndersonQ commented Aug 24, 2023

elasticmachine commented Aug 15, 2023 •

edited

Loading

elasticmachine commented Aug 15, 2023 •

edited

Loading