keystore: add --from-dir to import secrets from a directory by aelnahas · Pull Request #22305 · elastic/apm-server

aelnahas · 2026-10-28T18:28:16Z

Motivation/summary

This commit will allow the user to pass in a directory to the apm-server keystore add command to store files as values with their filenames as keys by passing a --from-dir flag. The command will simply scan the directory and not walk it recursively. Additionally, it will skip hidden files.

e.g

apm-server keystore add --from-dir /path/to/dir

Checklist

Update CHANGELOG.asciidoc
Documentation has been updated

For functional changes, consider:

Is it observable through the addition of either logging or metrics?
Is its use being published in telemetry to enable product improvement?
Have system tests been added to avoid regression?

How to test these changes

Added System test and unit tests for coverage but for manual you can refer to the steps below

# 1. compile the changes in some. Optionally place conduct the test in a temp directory
cd /path/to/apm-server
mkdir -p /tmp/test && go build -o /tmp/ks-test/apm-server ./x-pack/apm-server

# 2. make sure to add an apm-server.yaml file in the same folder


# 3. Create some secrets
mkdir -p /tmp/test/secrets
echo 'token-123'   > /tmp/test/secrets/secret_token
echo 'some-other-secret' > /tmp/test/secrets/some.other.secret

# 4. Optional reduce typing with alias
apm() { /tmp/test/apm-server --path.home /tmp/test/home "$@"; }```

# 5. test with --from-dir
apm keystore add --from-dir /tmp/test/secrets
apm keystore list  # should show the keys secret_token, some.other.secret

# 6. test with --stdin
apm keystore add stdkey --stdin
apm keystore list  # should show 3 keys now

I found it tricky to try and pull the secret content to verify it if is written correctly. So one workaround is to setup the secrets so that they are used for request and then attempt to use a correct and incorrect secret token to authenticate the request.

Steps:

setup your apm-server.yml as following:

apm-server:
  host: "127.0.0.1:8200"
  auth:
    secret_token: "${SECRET_TOKEN}"
output.elasticsearch:
  hosts: ["http://localhost:9200"]
  username: admin
  password: changeme

Add the SECRET_TOKEN file something like
`echo "some-token" >> /tmp/test/secrets/SECRET_TOKEN
add the token via --from-dir apm keystore add --from-dir /tmp/test/secrets
4 start the apm server
send a request to intake with the correct token

# request with invalid token should respond back with 401
curl -H 'Authorization: Bearer wrong'  -H 'Content-Type: application/x-ndjson' --data-binary @/path/to/event.ndjson http://127.0.0.1:8200/intake/v2/events

# request with valid token should respond back with 202
curl -H 'Authorization: Bearersome-token'  -H 'Content-Type: application/x-ndjson' --data-binary @/path/to/event.ndjson http://127.0.0.1:8200/intake/v2/events

Related issues

Fixes: #21863

This commit will allow the user to pass in a directory to the `apm-server keystore add` command to store files as values with their filenames as keys. The command will simply scan the directory and not walk it recursively. Additionally, it will skip hidden files.

github-actions · 2026-10-28T18:28:25Z

🤖 GitHub comments

Just comment with:

run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

mergify · 2026-10-28T18:29:21Z

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

backport-8.19 is the label to automatically backport to the 8.19 branch.
backport-9./d is the label to automatically backport to the 9./d branch. /d is the digit.
backport-active-all is the label that automatically backports to all active branches.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

Copilot

🟡 Changes recommended

The keystore system tests run against the FIPS binary, which omits the keystore command, causing both tests to fail.

3 open issues

What changed in this PR

Adds directory-based secret imports to apm-server keystore add, supporting bulk setup in shell-less containers such as those used by ECK.

Changes:

Adds --from-dir to import filenames as keys and file contents as values, skipping hidden entries and subdirectories.
Refactors input handling and supports noninteractive keystore creation for directory and stdin imports.
Adds unit and system tests for imports, validation, and overwrite behavior.

File	Description
`systemtest/keystore_test.go`	Adds end-to-end directory and stdin import tests.
`internal/beatcmd/keystore_nofips.go`	Implements directory imports and shared secret-input handling.
`internal/beatcmd/keystore_nofips_test.go`	Tests directory filtering, validation, persistence, and conflicts.

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

Removing whitespace normalization breaks existing positional-key removal: a key added as ' token ' is stored as 'token' but can no longer be removed using the original argument.

1 open issue

Preserve whitespace normalization when removing keys · New

3 resolved since last review

_{Review effort: Balanced}

Copilot

🟢 Approval recommended

No active review issues remain for the directory-based keystore import change.

1 open issue

Preserve whitespace normalization when removing keys

_{Review effort: Balanced}

carsonip

i think we can drop 9.4 backport because there is no scheduled release. And for 9.6 please observe FF.

carsonip · 2026-11-06T15:08:19Z

+	c.keys = make([]string, len(args))
+	for i, arg := range args {
+		c.keys[i] = strings.TrimSpace(arg)
+	}


nit: there's no need to go through this if loading from directory

That's true, I can guard this with the flag check.

carsonip · 2026-11-06T15:19:27Z

-		if err != nil {
-			return fmt.Errorf("could not read value from the input, error: %w", err)
+
+		if err = writableKeystore.Store(key, keyValue); err != nil {


Suggested change

if err = writableKeystore.Store(key, keyValue); err != nil {

if err := writableKeystore.Store(key, keyValue); err != nil {

carsonip · 2026-11-06T15:19:37Z

-		return fmt.Errorf("could not add the key in the keystore, error: %w", err)
-	}
+
 	if err = writableKeystore.Save(); err != nil {


Suggested change

if err = writableKeystore.Save(); err != nil {

if err := writableKeystore.Save(); err != nil {

carsonip · 2026-11-06T15:25:28Z

suggestion: will it be helpful for --from-dir to print the number of keys added / updated?

carsonip · 2026-11-06T15:30:44Z


 	if !store.IsPersisted() {
-		if !force {
+		if !force && interactive {


q: why change the condition? --from-dir and --stdin can be in an interactive environment as well

More of an opinion than strong reason. When I was working on this, I took a look at some other cli commands such as kubectl unzip mv cp and the like. some ignore the interactive option while others would have additional options such as All/None or -n do not overwrite.

It is a bit split. I ended up following kubectl approach with kubectl create secret --from-file since that felt the closest to what we are doing and I assumed this is utility is mostly used in automated script.

I can changes this bit if it is undesired.

carsonip · 2026-11-06T15:32:18Z

+	for _, key := range keys {
+		if _, err := store.Retrieve(key); err == nil {
+			if !force {
+				if !interactive {


ditto. I'm not sure deriving interactive from from-dir vs stdin vs input is correct.

carsonip · 2026-11-06T15:37:03Z

+		if len(c.keys) == 0 {
+			return fmt.Errorf("directory %q is empty", c.fromDir)


i think this is likely correct, but i wonder if there is any case if an empty directory is not an error case and should be no-op and exit cleanly.

- empty directory is demoted to a warn message - added updated/added count - allow both --from-dir and --stdin to be used interactively - add tests

Copilot

🟡 Changes recommended

Creation tests omit required keystore confirmation, an overwrite test expects the wrong error, and empty-directory imports bypass mutually exclusive option validation.

4 open issues

1 resolved since last review

Preserve whitespace normalization when removing keys

_{Review effort: Balanced}

+			files:    map[string]string{"a": "1", "b": "new"},
+			wantErr:  "the settings b already exist",


+				seedKeystore(t, ksPath, tt.seed)
+			}
+
+			c := &addKeystoreCmd{force: tt.force}


+	require.NoError(tb, os.WriteFile(filepath.Join(homedir, "apm-server.yml"), nil, 0644))
+	return homedir


+	if c.fromDir != "" {
+		keys, err := getKeysFromDir(c.fromDir)


infra-vault-gh-plugin-prod · 2026-11-09T22:26:13Z

💚 Build Succeeded

Buildkite Build
Commit: 4c11c90

History

💚 Build #13340 succeeded c822271
💚 Build #13337 succeeded a476bf9
💚 Build #13335 succeeded 4af8952

aelnahas added 2 commits October 28, 2026 12:11

add new system tests for keystore add command

4af8952

Copilot AI balanced review requested due to automatic review settings October 28, 2026 18:28

aelnahas requested a review from a team as a code owner October 28, 2026 18:28

Copilot started reviewing on behalf of aelnahas October 28, 2026 18:28

View session

Copilot AI reviewed Oct 28, 2026

View reviewed changes

Comment thread systemtest/keystore_nofips_test.go Resolved

Comment thread internal/beatcmd/keystore_nofips.go Resolved

Comment thread internal/beatcmd/keystore_nofips.go Resolved

aelnahas added 2 commits October 28, 2026 17:13

do not trim keys when they --from-dir is being used

8937a01

add fips flag to system test and rename for clarity

a476bf9

Copilot AI balanced review requested due to automatic review settings October 28, 2026 21:16

Copilot started reviewing on behalf of aelnahas October 28, 2026 21:17

View session

Copilot AI reviewed Oct 28, 2026

View reviewed changes

Comment thread internal/beatcmd/keystore_nofips.go Resolved

aelnahas added backport-8.19 Automated backport to the 8.19 branch backport-9.4 Automated backport with mergify backport-9.5 Automated backport to the 9.5 branch backport-9.6 Automated backport with mergify labels Oct 28, 2026

aelnahas added 2 commits October 29, 2026 09:50

restore TrimSpace in removeKey

63d2855

close stdin temp file so TempDir cleanup works on windows tests

c822271

Copilot AI balanced review requested due to automatic review settings October 29, 2026 14:26

Copilot started reviewing on behalf of aelnahas October 29, 2026 14:27

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

carsonip reviewed Nov 6, 2026

View reviewed changes

aelnahas removed the backport-9.4 Automated backport with mergify label Nov 6, 2026

fix code per pr review

4c11c90

- empty directory is demoted to a warn message - added updated/added count - allow both --from-dir and --stdin to be used interactively - add tests

Copilot AI balanced review requested due to automatic review settings November 9, 2026 22:17

Copilot started reviewing on behalf of aelnahas November 9, 2026 22:17

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

	if err = writableKeystore.Store(key, keyValue); err != nil {
	if err := writableKeystore.Store(key, keyValue); err != nil {

	if err = writableKeystore.Save(); err != nil {
	if err := writableKeystore.Save(); err != nil {

		if len(c.keys) == 0 {
		return fmt.Errorf("directory %q is empty", c.fromDir)

		files: map[string]string{"a": "1", "b": "new"},
		wantErr: "the settings b already exist",

		require.NoError(tb, os.WriteFile(filepath.Join(homedir, "apm-server.yml"), nil, 0644))
		return homedir

Conversation

aelnahas commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation/summary

Checklist

How to test these changes

Related issues

Uh oh!

github-actions Bot commented Oct 28, 2026

🤖 GitHub comments

Uh oh!

mergify Bot commented Oct 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

carsonip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelnahas Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

infra-vault-gh-plugin-prod Bot commented Nov 9, 2026

💚 Build Succeeded

History

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aelnahas commented Oct 28, 2026 •

edited

Loading

aelnahas Nov 6, 2026 •

edited

Loading