[Transform] Secondary credentials used with transforms should only require source and destination index privileges, not transform privileges

[przemekwitek]

The internal _validate action was called by PUT action using secondary headers only. This was causing authorization issues if the API key (secondary header) was lacking transform_admin role.
This PR fixes this by providing all the security headers to _validate action using ClientHelper.executeWithHeadersAsync helper method rather than raw client.execute.

Relates #93259

[elasticsearchmachine]

Hi @przemekwitek, I've created a changelog YAML for you.

[elasticsearchmachine]

Pinging @elastic/ml-core (Team:ML)

[droberts195]

I find it surprising that this works. config.setHeaders was called inside a block that switched to secondary headers. So I would have thought that here we end up calling ValidateTransformAction using the secondary headers, which should fail if those secondary headers only allow index access and not transform administration.

I can see you have a test that seems to test that this functionality works, but please can you do some extra due diligence on whether this is really working as expected, perhaps by running the test locally with extra debug, or by manually testing it with gradle run.

If this extra testing shows that the existing test is only succeeding by some fluke then I think the way to fix it would be to put this call back to how it was before, and instead switch to secondary headers (using useSecondaryAuthIfAvailable) within the subsections of TransportValidateTransformAction that access the source or destination indices.

If the code really does work correctly as you've got it now then I need to adjust my understanding of what executeWithHeadersAsync does.

[przemekwitek]

So I would have thought that here we end up calling ValidateTransformAction using the secondary headers, which should fail if those secondary headers only allow index access and not transform administration.

Yes, that was indeed the case and the scenario from Quynh I reproduced.

I can see you have a test that seems to test that this functionality works, but please can you do some extra due diligence on whether this is really working as expected, perhaps by running the test locally with extra debug, or by manually testing it with gradle run.

So, I did a little back and forth on this PR and the version you reviewed probably had the test (my new test written exactly for this case) failing in the CI which I noticed and fixed a few hours later...

The reason for this was I noticed Ben's work (#86757) solving the issue #86731 (Prefer secondary authentication headers when creating/updating transforms) and I wanted to keep his tests passing as well.

I ended up with the following (this PR is now updated):

• pass all the security headers to _validate call
• only store secondary headers in the transform config.

This way _validate call can be executed even if the secondary headers does not have transform_admin.
And at the same time we only store secondary header in the config in order to use it for data access later on (e.g: creating destination index).

I've just udpated this PR description to reflect the change: now all the security headers are passed on to _validate.

[elasticsearchmachine]

Hi @przemekwitek, I've created a changelog YAML for you.

[przemekwitek]

Some tests are now failing on CI.
So it seems this PR needs more work. I'll update it once it's ready for review again.

[przemekwitek]

Some tests are now failing on CI. So it seems this PR needs more work. I'll update it once it's ready for review again.

Ok, the tests are fixed. It was a simple NPE causing these failures.
PR is ready for review again.

[droberts195]

Unfortunately I still think there is a problem.

I think the solution is to call _validate using the internal _xpack user, as it's entirely an internal endpoint.

Then, in TransportValidateTransformAction the functions that use a Client should switch to the appropriate headers instead of relying on the caller of _validate to have done this. As far as I can see that's:

elasticsearch/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportValidateTransformAction.java

Line 130 in a8a684e

function.deduceMappings(client, config.getSource(), deduceMappingsListener);

and:

elasticsearch/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportValidateTransformAction.java

Line 139 in a8a684e

function.validateQuery(client, config.getSource(), request.timeout(), validateQueryListener);

[przemekwitek]

Unfortunately I still think there is a problem.

I think the solution is to call _validate using the internal _xpack user, as it's entirely an internal endpoint.

Done.

Then, in TransportValidateTransformAction the functions that use a Client should switch to the appropriate headers instead of relying on the caller of _validate to have done this. As far as I can see that's:

elasticsearch/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportValidateTransformAction.java

Line 130 in a8a684e

function.deduceMappings(client, config.getSource(), deduceMappingsListener);

and:

elasticsearch/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportValidateTransformAction.java

Line 139 in a8a684e

function.validateQuery(client, config.getSource(), request.timeout(), validateQueryListener);

Done.
I call these actions using executeWithHeadersAsync and provide headers stored in the config (config.getHeaders()).

[droberts195]

LGTM if you could just rename a method as per #94420 (comment) before merging.