[Synthetics] Make core API key include `read_ilm` privilege in Stateful only #178897

justinkambic · 2024-03-18T17:58:05Z

Summary

Resolves https://github.com/elastic/synthetics-dev/issues/332.

ILM is not a concept in Serverless. As such, when the Synthetics plugin requests the read_ilm permission for its core API key during bootstrapping, it's asking for a priv that will eventually not exist on Serverless, and ES will give an explicit deny for the request, which will probably cause Synthetics to crash and not be functional.

The fix is to detect the build type at startup time and enable the server plugin to determine whether it needs to include this privilege or not, based on whether Kibana is running in stateful or serverless mode.

Testing

You can easily test this in both modes. The steps are the same.

NOTE: when testing serverless, if you include the flag -E xpack.security.authz.has_privileges.strict_request_validation.enabled=true this will simulate the manner in which Elasticsearch will explicit deny the API creation request when this is enabled in the MKI environment, and thus you should include it in your testing.

Start up Kibana in serverless | stateful mode.
As an admin, navigate to Synthetics and wait for the startup flow to display.
Navigate to Kibana management's API key page. You should see the Synthetics API key. Click it.
For stateful, you should see read_ilm included under the synthetics_writer object, shown below. For serverless, you should not see this priv in the list.

Stateful API key perms

Serverless API key perms

apmmachine · 2024-03-18T17:58:18Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2024-03-18T19:06:40Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

cauemarcondes · 2024-03-19T10:46:27Z

...vability_solution/synthetics/server/synthetics_service/authentication/check_has_privilege.ts


 export const checkHasPrivileges = async (
  server: SyntheticsServerSetup,
  apiKey: { id: string; apiKey: string }
 ) => {
+  const { isServerless } = server;
+  const { indices: index, cluster } = getServiceApiKeyPrivileges(isServerless);
  return await server.coreStart.elasticsearch.client


It's not related to your changes but this await is not necessary neither the async declaration. As the server.coreStart.elasticsearch.client returns a promise the checkHasPrivileges client is the one that needs to await the response.

Totally agree, wish Kibana had a rule on unneeded async/await. I have this lint rule in the recorder and it makes the code a lot cleaner IMO.

I didn't notice it as I wasn't paying attention to the code around the bits I needed to modify.

I've also updated the function below this as it suffers from the same unneeded sugar.

635854b96805f29452cabc0a7dcb2a9a5194f6dc

cauemarcondes · 2024-03-19T10:47:35Z

x-pack/plugins/observability_solution/synthetics/server/synthetics_service/get_api_key.ts

-    },
-  ],
-  run_as: [],
+export const getServiceApiKeyPrivileges = (isServerless?: boolean) => {


I reason why to accept undefined (isServerless?:) here?

#178897 (comment)

cauemarcondes · 2024-03-19T10:52:28Z

x-pack/plugins/observability_solution/synthetics/server/types.ts

@@ -57,6 +57,7 @@ export interface SyntheticsServerSetup {
  uptimeEsClient: UptimeEsClient;
  basePath: IBasePath;
  isDev?: boolean;
+  isServerless?: boolean;


Also here why do we define it as possible undefined? I think this should be just true/false

Sure, this is fair. For flags like this that are based on config or some upstream dependency a lot of times I default to making them optional. But the code that sets this flag will always return a boolean, so it does make sense to make this field more opinionated. Just like the isDev flag above this one could also be optional.

I've also changed the assertions on this flag from general truthy/falsey to explicit boolean assertion.

8ddc360658faa4ff161b2f24829df4797ec950c2

x-pack/plugins/observability_solution/synthetics/server/plugin.ts

cauemarcondes

LGTM

jeramysoucy

Kibana security changes LGTM. Just one question on serverless integration test coverage. Also, I was unable to test manually due to lack of knowledge.
In the second step "As an admin, navigate to Synthetics and wait for the startup flow to display." I am not sure what this means. When I navigate to Synthetics I see this:

I tried creating an agent policy and location, but the API keys generated did not contain the synthetics writer section. How do I generate the synthetics API key?

jeramysoucy · 2024-03-20T15:23:34Z

x-pack/test/api_integration/apis/synthetics/synthetics_enablement.ts

@@ -74,7 +74,7 @@ export default function ({ getService }: FtrProviderContext) {
              ],
              elasticsearch: {
                cluster: [privilege],
-                indices: serviceApiKeyPrivileges.indices,
+                indices: getServiceApiKeyPrivileges(false).indices,


Should we implement equivalent serverless api integration tests where we pass true?

Yes I'm in the process of planning this with the Synthetics-on-Serverless team. I will likely do this in a separate PR that focuses just on implementing this. There will likely be other stuff that needs to change and it won't be a simple lift-and-shift. This will explode the delta for this change to many times its current size and require review burden from some additional folks who aren't on this PR already. Synthetics still isn't live in Prod, so nothing we do will impact existing early adopters, and we will likely include the API tests as acceptance criteria for switching on the plugin.

In the second step "As an admin, navigate to Synthetics and wait for the startup flow to display." I am not sure what this means. When I navigate to Synthetics I see this:

Synthetics is designed to work with our managed SaaS testing locations by default. Locally, if you haven't run an instance of this service alongside your Kibana and configured them to talk to each other (requires Minikube and some extra setup), you'll be prompted to create your own "private" location, which involves enrolling an agent in Fleet and creating some config.

I tried creating an agent policy and location, but the API keys generated did not contain the synthetics writer section. How do I generate the synthetics API key?

To see this work locally end-to-end you'd need to also set up Fleet. The way we've done this in the past is via the elastic-package's stack up command. It's not super fun to get that working locally, however. The best way to test things end-to-end would be to deploy this PR to cloud QA. LMK if you'd like me to spin one up so you can see it working. This way you'd avoid having to set these things up manually on your local system.

I do apologize, the testing steps require some knowledge of running Synthetics locally. I'll spin up a cloud instance of this patch so we can see it work in cloud.

Thanks, Justin! I don't think it's necessary to spin up a serverless instance - I trust the other reviewers have given this a thorough testing, I just wanted to understand why I wasn't seeing what I thought I should - I don't have any experience with synthetics. This makes sense to me now. I appreciate the time you took to explain.

And thank you for the follow up re:tests. Is there an open issue for this work already?

Not yet, I will make the ticket before I merge this though.

Issue is linked. I will probably merge later today, or tomorrow morning.

justinkambic · 2024-03-20T16:01:47Z

/oblt-deploy

justinkambic · 2024-03-21T19:03:14Z

@jennypavlova @cauemarcondes could you please give another review, after speaking to the Kibana team this is the way they recommend handling Elasticsearch capability-related considerations when determining if we need to adjust for Serverless or not.

cauemarcondes · 2024-03-22T09:39:38Z

...vability_solution/synthetics/server/synthetics_service/authentication/check_has_privilege.ts

  server: SyntheticsServerSetup,
  apiKey: { id: string; apiKey: string }
 ) => {
-  return await server.coreStart.elasticsearch.client
+  const { indices: index, cluster } = getServiceApiKeyPrivileges(
+    server.coreStart.elasticsearch.getCapabilities().serverless


@justinkambic could we keep exposing the isServerless in the SyntheticsServerSetup definition? this is a long chain of functions to remember to check if it serverless or not.

@cauemarcondes the Kibana team stressed that Kibana's buildFlavor and es capabilities (which is basically Elasticsearch's build flavor) should be treated as two separate things. It's a bit pedantic but the two aren't synonymous and there's no way to know if there will be important differences in the future.

I agree that this implementation isn't great because of the lengthy call chain, and it's not DRY, so we should improve it as you suggest. I can add an isEsServerless flag to the plugin class and reference it in the server object that gets injected in place of this. 👍

…eing serverless.

… flavor when determining how to interact with ES.

cauemarcondes

LGTM

kibana-ci · 2024-03-22T15:49:07Z

💚 Build Succeeded

Buildkite Build
Commit: 30ddac0

Metrics [docs]

✅ unchanged

History

💛 Build #198801 was flaky b2d94335df995f4171037782d9c7518e3006e9d9
💚 Build #198548 succeeded a26c0d1691ac94164b4dee27a9047a4d8ba5d300
💔 Build #198321 failed a88b48e8e87bf0ee4f5b876bef91768dd2d9329e
💔 Build #198300 failed 8ddc360658faa4ff161b2f24829df4797ec950c2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @justinkambic

justinkambic added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Mar 18, 2024

justinkambic self-assigned this Mar 18, 2024

justinkambic added release_note:skip Skip the PR/issue when compiling release notes v8.14.0 labels Mar 18, 2024

justinkambic force-pushed the 332/only-request-ilm-api-key-perms-for-non-serverless-builds branch 3 times, most recently from 2e33341 to 0f78371 Compare March 18, 2024 19:05

justinkambic marked this pull request as ready for review March 18, 2024 19:06

justinkambic requested a review from a team as a code owner March 18, 2024 19:06

cauemarcondes reviewed Mar 19, 2024

View reviewed changes

jennypavlova reviewed Mar 19, 2024

View reviewed changes

x-pack/plugins/observability_solution/synthetics/server/plugin.ts Outdated Show resolved Hide resolved

cauemarcondes approved these changes Mar 19, 2024

View reviewed changes

justinkambic requested a review from a team as a code owner March 19, 2024 16:06

jennypavlova approved these changes Mar 19, 2024

View reviewed changes

jeramysoucy self-requested a review March 20, 2024 11:54

justinkambic force-pushed the 332/only-request-ilm-api-key-perms-for-non-serverless-builds branch from a88b48e to a26c0d1 Compare March 20, 2024 15:31

jeramysoucy reviewed Mar 20, 2024

View reviewed changes

jeramysoucy approved these changes Mar 20, 2024

View reviewed changes

justinkambic mentioned this pull request Mar 20, 2024

[Synthetics] [Meta] Port API tests from stateful to serverless #179100

Closed

justinkambic force-pushed the 332/only-request-ilm-api-key-perms-for-non-serverless-builds branch from a26c0d1 to 49f356e Compare March 21, 2024 17:56

justinkambic requested review from cauemarcondes and jennypavlova March 21, 2024 19:02

cauemarcondes reviewed Mar 22, 2024

View reviewed changes

justinkambic added 3 commits March 22, 2024 10:09

Make read_ilm privilege request depend on Kibana build flavor not b…

7f62ddf

…eing serverless.

Reduce sugar consumption.

aae3693

Tighten type definition.

98581b5

justinkambic added 6 commits March 22, 2024 10:09

Fix API test references to updated field.

2b9ee96

Fix API tests.

49acc2d

Opt to utilize Elasticsearch capabilities API instead of Kibana build…

ae958df

… flavor when determining how to interact with ES.

Last-minute cleanup.

36cdd2f

Remove redundant logger call.

b5577d2

DRY the code off.

30ddac0

justinkambic force-pushed the 332/only-request-ilm-api-key-perms-for-non-serverless-builds branch from b2d9433 to 30ddac0 Compare March 22, 2024 14:25

cauemarcondes approved these changes Mar 22, 2024

View reviewed changes

justinkambic enabled auto-merge (squash) March 22, 2024 15:15

justinkambic merged commit 15fde36 into elastic:main Mar 22, 2024
18 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Mar 22, 2024

justinkambic deleted the 332/only-request-ilm-api-key-perms-for-non-serverless-builds branch March 22, 2024 17:15

thomheymann mentioned this pull request Mar 26, 2024

[Observability Onboarding] Link all “Add data” buttons in observability to onboarding landing page #179459

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Synthetics] Make core API key include `read_ilm` privilege in Stateful only #178897

[Synthetics] Make core API key include `read_ilm` privilege in Stateful only #178897

justinkambic commented Mar 18, 2024 •

edited

Loading

apmmachine commented Mar 18, 2024

elasticmachine commented Mar 18, 2024

cauemarcondes Mar 19, 2024

justinkambic Mar 19, 2024

cauemarcondes Mar 19, 2024

justinkambic Mar 19, 2024

cauemarcondes Mar 19, 2024

justinkambic Mar 19, 2024

cauemarcondes left a comment

jeramysoucy left a comment

jeramysoucy Mar 20, 2024

justinkambic Mar 20, 2024

justinkambic Mar 20, 2024

justinkambic Mar 20, 2024

jeramysoucy Mar 20, 2024

justinkambic Mar 20, 2024

justinkambic Mar 20, 2024

justinkambic commented Mar 20, 2024

justinkambic commented Mar 21, 2024

cauemarcondes Mar 22, 2024

justinkambic Mar 22, 2024

justinkambic Mar 22, 2024

cauemarcondes left a comment

kibana-ci commented Mar 22, 2024

[Synthetics] Make core API key include read_ilm privilege in Stateful only #178897

[Synthetics] Make core API key include read_ilm privilege in Stateful only #178897

Conversation

justinkambic commented Mar 18, 2024 • edited Loading

Summary

Testing

Stateful API key perms

Serverless API key perms

apmmachine commented Mar 18, 2024

🤖 GitHub comments

elasticmachine commented Mar 18, 2024

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

cauemarcondes left a comment

Choose a reason for hiding this comment

jeramysoucy 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

justinkambic commented Mar 20, 2024

justinkambic commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cauemarcondes left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 22, 2024

💚 Build Succeeded

Metrics [docs]

History

[Synthetics] Make core API key include `read_ilm` privilege in Stateful only #178897

[Synthetics] Make core API key include `read_ilm` privilege in Stateful only #178897

justinkambic commented Mar 18, 2024 •

edited

Loading