[Task Manager] Use UIAM API Key for tasks in serverless#254315
[Task Manager] Use UIAM API Key for tasks in serverless#254315ersin-erdal wants to merge 24 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/response-ops (Team:ResponseOps) |
# Conflicts: # packages/kbn-check-saved-objects-cli/src/migrations/__fixtures__/task/10.8.0.json # src/core/server/integration_tests/ci_checks/saved_objects/check_registered_types.test.ts # x-pack/platform/plugins/shared/task_manager/server/integration_tests/managed_configuration.test.ts # x-pack/platform/plugins/shared/task_manager/server/metrics/create_aggregator.test.ts # x-pack/platform/plugins/shared/task_manager/server/monitoring/configuration_statistics.test.ts # x-pack/platform/plugins/shared/task_manager/server/plugin.test.ts # x-pack/platform/plugins/shared/task_manager/server/plugin.ts # x-pack/platform/plugins/shared/task_manager/server/saved_objects/model_versions/task_model_versions.ts # x-pack/platform/plugins/shared/task_manager/server/saved_objects/schemas/task.ts # x-pack/platform/plugins/shared/task_manager/server/task_store.test.ts # x-pack/platform/plugins/shared/task_manager/server/task_store.ts
…tion_tests/ci_checks
| | ApiKeyAndUserScopeBoth; | ||
|
|
||
| export interface CreateApiKeyOptions { | ||
| shouldGrantUiam: boolean; |
There was a problem hiding this comment.
should we make it more generic and accept a list of api key types to generate?
| shouldGrantUiam: boolean; | |
| apiKeyTypes: ApiKeyType[]; |
There was a problem hiding this comment.
IMO this should stay as it is, this is to check if uiam.grant is enabled or not. we use ApiKeyType when we decide which API Key to use.
There was a problem hiding this comment.
This request led me to change the whole architecture of api key handling, had to switch to strategy pattern.
Created another PR: #259373
With this one we can add a new strategy as we have new apiKey types.
| if (apiKeyIdsToRemove.length) { | ||
| // after successful updates we should invalidate the old API keys (ES and/or UIAM) | ||
| const allSucceeded = updatedSavedObjects.every((so) => !so.error); | ||
| if (apiKeysToRemove.length && allSucceeded) { |
There was a problem hiding this comment.
The old behavior invalidated keys per-document for each individually successful update. The new code invalidates only when the entire batch succeeds. So if any from the batch failed, 0 old API keys get invalidated, and we end up with orphaned API keys.
There was a problem hiding this comment.
Good catch, fixed it.
| const { apiKey, userScope } = taskInstance; | ||
| if (apiKey && userScope) { | ||
| const { apiKey, uiamApiKey, userScope } = taskInstance; | ||
| if ((apiKey || uiamApiKey) && userScope && !userScope.apiKeyCreatedByUser) { |
There was a problem hiding this comment.
Previously when regenerateApiKey: true was passed, tasks with API keys created by user were still included and got new system-created keys (the old user key was preserved / not invalidated). Now they're excluded entirely. I agree it makes sense to exclude userScope.apiKeyCreatedByUser, but it's a (breaking) change in the behaviour, isn't it? I believe we should either document it as a fix, or preserve the old behaviour
There was a problem hiding this comment.
Agreed, removed the extra check against apiKeyCreatedByUser
| : taskInstance.apiKey; | ||
|
|
||
| if (apiKeyType === ApiKeyType.UIAM && taskInstance.apiKey && !taskInstance.uiamApiKey) { | ||
| this.logger.error( |
There was a problem hiding this comment.
I think it should a warning, similar to alerting
| this.logger.error( | |
| this.logger.warn( |
| private getFakeKibanaRequest(apiKey?: string, spaceId?: string): KibanaRequest | undefined { | ||
| if (apiKey) { | ||
| const requestHeaders: Headers = {}; | ||
| private getFakeKibanaRequest(taskInstance: ConcreteTaskInstance): KibanaRequest | undefined { |
There was a problem hiding this comment.
shall we add some unit tests?
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing exports
History
|
|
closing in favor of: #259373 |
Related https://github.com/elastic/response-ops-team/issues/527
This PR adopts the UIAM APIKeys in the Task Manager plugin.
when
security.authc?.apiKeys?.uiamis provided, we grant both the ES and UIAM APIKeys and persist in the task SO, so we can switch between two of them.On task create -with a request- we grant ES and UIAM APIKeys.
On task update we grant ES and UIAM APIKeys and invalidate the old API Keys.
On task delete we invalidate the old API Keys.
If we don't use an api key after granting it - because of an error - invalidate the unused ones.
When
xpack.task_manager.api_key_typeisuiamwe create the fakeRequest we use in the task with UIAM API Key.To verify:
Add below configs to your serverless.dev.yml
xpack.task_manager.api_key_type: 'uiam'xpack.task_manager.invalidate_api_key_task.removalDelay: '10s'xpack.task_manager.invalidate_api_key_task.interval: '1m'so we can see the invalidation task works without a problem.
Run Kibana and Elasticsearch with the below commands:
yarn es serverless --projectType observability --uiamyarn start --serverless oblt --uiam --run-examplesLogin with the
system_indices_superuseruser.Go to: http://localhost:5601/app/triggersActionsUiExample/task_manager_with_api_key
Create
task 1andtask 2Use the below query to check if the task has both ES and UIAM API Keys
Wait for a minute and do the same as above, the API Keys should be invalidated and removed from the index.
There shouldn't be any error on the console.
Add the task you removed again and do the same tests with
Remove All Taskswhich uses bulkDelete.