Remove username/password

[michel-laterman]

What is the problem this PR solves?

Remove using basic auth as credentials when contacting ES. Will only be
able to use service tokens.

PR to remove the attributes in elastic-agent: elastic/beats#29458

How does this PR solve the problem?

// Explain HOW you solved the problem in your code. It is possible that during PR reviews this changes and then this section should be updated.

Checklist

• 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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

• Relates fleet-server username/password authentication must be removed in 8.0 #978

[elasticmachine]

💚 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: 2021-12-22T18:02:58.301+0000

• Duration: 9 min 58 sec

• Commit: 9105d04

Test stats 🧪

Test	Results
Failed	0
Passed	186
Skipped	26
Total	212

🤖 GitHub comments

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

• /test : Re-trigger the build.

[michel-laterman]

Some of the integation tests are failing as they seem to do things beyond just using the fleet-server service account. For example:

=== RUN   TestMonitorEmptyIndex
monitor_integration_test.go:25: 403 - security_exception: action [indices:admin/index_template/put] is unauthorized for service account [elastic/fleet-server], this action is granted by the cluster privileges [manage_index_templates,manage,all]
--- FAIL: TestMonitorEmptyIndex (0.02s)

[michel-laterman]

@ruflin, is the service account expected to be able to do all the interactions that are currently part of the integration tests/

[ruflin]

It is not necessarily the case the fleet-server service token has all the permissions for the integration tests as these also do some additional setup. But looking at the error above, it is about setting up template and likely comes from here: https://github.com/elastic/fleet-server/blob/master/internal/pkg/testing/esutil/template.go#L52 In the early days of fleet-server, fleet-server did the setup during testing but this is now all shipped with Elasticsearch. @aleksmaus Is this part still needed? Is this to ensure that Elasticsearch has the template as expected?

Assuming we need more permissions, it would good to make all these checks with a different Elasticsearch client and not directly the one from fleet-server so username / password is only used during tests and not exposed to users.

[aleksmaus]

It is not necessarily the case the fleet-server service token has all the permissions for the integration tests as these also do some additional setup. But looking at the error above, it is about setting up template and likely comes from here: https://github.com/elastic/fleet-server/blob/master/internal/pkg/testing/esutil/template.go#L52 In the early days of fleet-server, fleet-server did the setup during testing but this is now all shipped with Elasticsearch. @aleksmaus Is this part still needed? Is this to ensure that Elasticsearch has the template as expected?

@ruflin many integration tests created artificial indices, easier to setup and guarantee that the tests from different packages do not step on each other in case if they are run in parallel.

We probably could generate a different token with wider permissions for integration testing only as a short term solution, not ideal but it will unblock this effort.

We probably should rewrite the integration tests to use the .fleet-* indices (need a proper setup/cleanup), probably add some new tests for the new .fleet system indices APIs. Make sure that the tests from different packages are not run in parallel (-p 1) if we have such a problem.
I can look into the integration tests revamp needed, assign to me.

[michel-laterman]

@aleksmaus, I've tried to change one of the integration test to use an index prefixed with .fleet-. However I still get the template management exception:

=== RUN   TestBulkCreate
{"level":"debug","cluster.addr":["localhost:9200"],"cluster.maxConnsPersHost":128,"time":"2021-12-21T09:48:44-08:00","message":"init es"}
{"level":"info","cluster.addr":["localhost:9200"],"cluster.maxConnsPersHost":128,"cluster.name":"es-docker-cluster","cluster.uuid":"PQix1rriQ3mlvzft4IUhyQ","cluster.version":"8.1.0-SNAPSHOT","time":"2021-12-21T09:48:44-08:00","message":"elasticsearch cluster info"}
{"level":"info","opts":{"flushInterval":250,"flushThresholdCnt":2048,"flushThresholdSz":1048576,"maxPending":8,"blockQueueSz":32,"apikeyMaxParallel":120},"time":"2021-12-21T09:48:44-08:00","message":"Run bulker with options"}
{"level":"info","name":".fleet-c7116v4bcv44otvnmjt0","time":"2021-12-21T09:48:44-08:00","message":"Create template"}
    bulk_integration_test.go:28: 403 - security_exception: action [indices:admin/index_template/put] is unauthorized for service account [elastic/fleet-server], this action is granted by the cluster privileges [manage_index_templates,manage,all]
--- FAIL: TestBulkCreate (0.03s)

[ruflin]

As the templates are by now always shipped in Elasticsearch I think we can start to assume these are always there. @aleksmaus Agree that we should revamp the tests quite a bit. Would be great if you could take it on. Do you need an issue for it? @jlind23 (FYI)

@michel-laterman As a quick workaround, is it possible to use a different ES client for the template actions in the tests with a username / password?

[aleksmaus]

@michel-laterman use whatever workaround is needed to unblock this PR for now. Could disable the test(s) short term as long as you verified that the fleet-server works as expected with the agents/fleet.

In some tests like this one, it tests just some basic functionality (bulker) that should work on any index and we probably can use whatever credentials or test indices to test it, doesn't have to be .fleet- index. On the other hand it would be good to test with the actual .fleet-* indices since the calls to these indices are going through the elasticsearch fleet plugin, so we can catch any potential breakage earlier.

@ruflin I created a tracker issue (#1020) and will work on revamping the integration tests so we don't have to use workarounds.

[lykkin]

👍 LGTM

[aleksmaus]

one of the issues that came up during changing of the integration tests, thought it's worth to mention.
the security token doesn't have permissions to refresh the indices. so, if we run bunch of integration tests on the same index with the test setup code deleting/creating some test data, without refresh after setup we can't guarantee the test data consistency.
so there will be a separate user/password client for:

1. tests setup
2. for some tests that don't really require .fleet-* indices (for example bulker)
   @michel-laterman @ruflin

[aleksmaus]

@michel-laterman @ruflin

btw the current code utilizes refreshes, grep for
bulk.WithRefresh()

so not sure how it works now with the service token if it was tested

[aleksmaus]

Looks like migration is broken as well.

{"log.level":"info","ecs.version":"1.6.0","service.name":"fleet-server","status":"FAILED","@timestamp":"2021-12-29T01:27:56.638Z","message":"Error - Migrate UpdateByQuery [403 Forbidden] {\"error\":{\"root_cause\":[{\"type\":\"security_exception\",\"reason\":\"action [indices:admin/refresh] is unauthorized for service account [elastic/fleet-server], this action is granted by the index privileges [maintenance,manage,all]\"}],\"type\":\"security_exception\",\"reason\":\"action [indices:admin/refresh] is unauthorized for service account [elastic/fleet-server], this action is granted by the index privileges [maintenance,manage,all]\"},\"status\":403}"}

The token doesn't have refresh permissions.

[ruflin]

Seems like @aleksmaus fixed this in an Elasticsearch PR for 8.0 but what about 7.x?

[aleksmaus]

Seems like @aleksmaus fixed this in an Elasticsearch PR for 8.0 but what about 7.x?

added labels for v7.16.3 and v7.17 to elastisearch PR, will see if it backports automagically or would have to do it manually.