-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Used SO for saving the API key IDs that should be deleted #82211
Used SO for saving the API key IDs that should be deleted #82211
Conversation
…a configuration option where can set an execution interval for a TM task which will get the data from this SO and remove marked for delete keys.
…-api-keys-using-task
…-api-keys-using-task # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature is going to be sooo great to have! We've had a lot of flaky tests because of this and added a lot of code to work around it. This will allow us to clean some of that up in the future ❤️ .
I have a bunch of optional nits, questions and comments. I will do a final pass on the tests and functionality later (in case some core changes are made).
x-pack/plugins/alerts/server/invalidate_pending_api_keys/add_to_invalidate_api_keys.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/add_to_invalidate_api_keys.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
…-api-keys-using-task # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! 👍 Just one thing on the hasApiKeysPendingInvalidation
variable, the for loop for invalidating keys and a few nits.
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/routes.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/alert_types.ts
Outdated
Show resolved
Hide resolved
…-api-keys-using-task # Conflicts: # x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but left some questions and comments. I'm curious about the api key ids, and whether they need to be encrypted. Also noted we should get someone in security to review the overall approach.
if (!apiKey) { | ||
return; | ||
} | ||
const apiKeyId = Buffer.from(apiKey, 'base64').toString().split(':')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh - it looks like the security plugin expects the api key id
to be passed on deletion, I'm surprised to find that the id is a base64 encoding of the key itself! Seems like this code could be fragile if that changed, but perhaps we aren't otherwise storing the id separately because they are related like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just thought, if this is the apiKey itself, but in base64, then we really need to use encrypted saved objects here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we should have someone from security review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last piece is the important one .split(':')[0];
. An API key is a combination of id
and key
, separated by :
and encoded in base64. So this code does the reverse to get the id
only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhhh! Seems comment-worthy then, won't scare me next time I see it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be encrypting this, even though we're only storing the id
, which isn't the "secret" part. Someone with access to the .kibana
index shouldn't be able to enumerate these keys, even if they're going to be invalidated in the near future.
Access to API Key information is currently governed by the manage_api_keys
and manage_own_api_keys
cluster privileges, and storing the ids in plaintext feels like we'd be violating this expectation.
Additional nit: Should we move this line inside the try
block to prevent this from throwing an error if the API Key is somehow malformed?
x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts
Outdated
Show resolved
Hide resolved
securityPluginSetup?: SecurityPluginSetup | ||
) { | ||
let totalInvalidated = 0; | ||
await Promise.all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means we sending all the invalidation requests at once? I'm worried about overwhelming ES here - should be breaking this into something like 10 or 20 to run at once, as a set of batches?
We should also ask security about a batch version of invalidating these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a debug log call here. Probably just one for all the keys, just printing totalInvalidated
, at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security team has an opened issue for the batch invalidation by Ids array #79714, so I will open a follow up issue and add the dependency on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to chunk this out as well, regardless of #79714. Using Promise.all
with a large input array will negatively impact the Kibana server's performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it currently restricted by the page size of 100 in the const apiKeysToInvalidate = await savedObjectsClient.find<InvalidatePendingApiKey>
? I think having 100 maximum won't impact Kibana so hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having 100 maximum won't impact Kibana so hard.
I would hope not, and in practice, you're probably right. We haven't benchmarked this one way or another to know for sure, but we all have to keep in mind that the Kibana server is a shared resource, where any number of operations could be happening simultaneously. The starting tier on ESS only provides a single GB of ram, which is a fairly constrained environment considering everything that Kibana is capable of doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may reduce the number in twice - make it 50 :-)
…-api-keys-using-task # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
if (!apiKey) { | ||
return; | ||
} | ||
const apiKeyId = Buffer.from(apiKey, 'base64').toString().split(':')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be encrypting this, even though we're only storing the id
, which isn't the "secret" part. Someone with access to the .kibana
index shouldn't be able to enumerate these keys, even if they're going to be invalidated in the near future.
Access to API Key information is currently governed by the manage_api_keys
and manage_own_api_keys
cluster privileges, and storing the ids in plaintext feels like we'd be violating this expectation.
Additional nit: Should we move this line inside the try
block to prevent this from throwing an error if the API Key is somehow malformed?
securityPluginSetup?: SecurityPluginSetup | ||
) { | ||
let totalInvalidated = 0; | ||
await Promise.all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to chunk this out as well, regardless of #79714. Using Promise.all
with a large input array will negatively impact the Kibana server's performance.
x-pack/plugins/alerts/server/invalidate_pending_api_keys/mark_api_key_for_invalidation.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerts/server/invalidate_pending_api_keys/mark_api_key_for_invalidation.ts
Outdated
Show resolved
Hide resolved
securityPluginSetup?: SecurityPluginSetup | ||
) { | ||
let totalInvalidated = 0; | ||
await Promise.all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having 100 maximum won't impact Kibana so hard.
I would hope not, and in practice, you're probably right. We haven't benchmarked this one way or another to know for sure, but we all have to keep in mind that the Kibana server is a shared resource, where any number of operations could be happening simultaneously. The starting tier on ESS only provides a single GB of ram, which is a fairly constrained environment considering everything that Kibana is capable of doing.
x-pack/plugins/alerts/server/invalidate_pending_api_keys/mark_api_key_for_invalidation.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from the security side of things - thanks for the edits
) * Used SO for saving the API key IDs that should be deleted and create a configuration option where can set an execution interval for a TM task which will get the data from this SO and remove marked for delete keys. * removed invalidateApiKey from AlertsClient * Fixed type checks * Fixed jest tests * Removed test code * Changed SO name * fixed type cheks * Moved invalidate logic out of alerts client * fixed type check * Added functional tests * Fixed due to comments * added configurable delay for invalidation task * added interval to the task response * Fixed jest tests * Fixed due to comments * Fixed task * fixed paging * Fixed date filter * Fixed jest tests * fixed due to comments * fixed due to comments * Fixed e2e test * Fixed e2e test * Fixed due to comments. Changed api key invalidation task to use SavedObjectClient * Use encryptedSavedObjectClient * set back flaky test comment
…83547) * Used SO for saving the API key IDs that should be deleted and create a configuration option where can set an execution interval for a TM task which will get the data from this SO and remove marked for delete keys. * removed invalidateApiKey from AlertsClient * Fixed type checks * Fixed jest tests * Removed test code * Changed SO name * fixed type cheks * Moved invalidate logic out of alerts client * fixed type check * Added functional tests * Fixed due to comments * added configurable delay for invalidation task * added interval to the task response * Fixed jest tests * Fixed due to comments * Fixed task * fixed paging * Fixed date filter * Fixed jest tests * fixed due to comments * fixed due to comments * Fixed e2e test * Fixed e2e test * Fixed due to comments. Changed api key invalidation task to use SavedObjectClient * Use encryptedSavedObjectClient * set back flaky test comment
* master: (51 commits) [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439) adds xpack.security.authc.selector.enabled setting (elastic#83551) skip flaky suite (elastic#77279) [ML] Improve support for script and aggregation fields in anomaly detection jobs (elastic#81923) [Workplace Search] Migrate SourcesLogic from ent-search (elastic#83544) [ML] Add UI test for feature importance features (elastic#82677) [Maps] Improve icons for all layer types (elastic#83503) Replace experimental badge with Beta (elastic#83468) [Fleet][EPM] Unified install and archive (elastic#83384) Move src/legacy/server/keystore to src/cli (elastic#83483) Used SO for saving the API key IDs that should be deleted (elastic#82211) [Uptime] Mock implementation to account for math flakiness test (elastic#83535) [Workplace Search] Enable check for org context based on URL (elastic#83487) [App Search] Added all Document related routes and logic (elastic#83324) [Alerting UI] Fix console error when setting connector params (elastic#83333) [Discover] Allow custom name for fields via index pattern field management (elastic#70039) [Uptime] Fix monitor list down histogram (elastic#83411) remove headers timeout hack, rely on nodejs timeouts (elastic#83419) [ML] Update console autocomplete for ML data frame evaluate API (elastic#83151) [Lens] Color in dimension trigger (elastic#76871) ...
💔 Build Failed
Failed CI Steps
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/maps.maps app "before all" hook in "maps app"Standard Out
Stack Trace
Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/maps.maps app "before all" hook in "maps app"Standard Out
Stack Trace
Firefox XPack UI Functional Tests.x-pack/test/functional/apps/canvas.Canvas app "before all" hook in "Canvas app"Standard Out
Stack Trace
and 9 more failures, only showing the first 3. Metrics [docs]Distributable file count
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
Used SO for saving the API key IDs that should be deleted and create a configuration option where can set an execution interval for a TM task which will get the data from this SO and remove marked for delete keys.
Resolve #53868
Note for docs
Two new configuration optons:
xpack.alerts.invalidateApiKeysTask.interval: '5m'
xpack.alerts.invalidateApiKeysTask.removalDelay: '5m'