-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Security usage data #110548
Security usage data #110548
Conversation
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.
Author's notes for reviewers
auditLoggingEnabled: boolean; | ||
auditLoggingType?: 'ecs' | 'legacy'; |
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.
Since we already have auditLoggingEnabled
and we can't change the existing mapping for that field, I added a second optional field for auditLoggingType
. The vast majority of clusters do not have audit logging enabled.
@@ -391,11 +391,18 @@ export function createConfig( | |||
function getSessionConfig(session: RawConfigType['session'], providers: ProvidersConfigType) { | |||
return { | |||
cleanupInterval: session.cleanupInterval, | |||
getExpirationTimeouts({ type, name }: AuthenticationProvider) { | |||
getExpirationTimeouts(provider?: AuthenticationProvider) { |
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're missing unit tests for this change in config.test.ts
. The existing tests all assume that this parameter will be defined
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.
Ah good point. I think I'll change it to provider: AuthenticationProvider | undefined
too to prevent accidental misuse. We only need to use this for usage data collection.
Edit: done in a9959d1.
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.
Telemetry and core-related changes LGTM.
"principal": { | ||
"type": "keyword", | ||
"_meta": { | ||
"description": "Indicates what elasticsearch user or service account is configured, if any." |
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.
nit: It would help us a lot if we knew what was grouped under other
without potentially exposing too much information in the payload. However, IIRC we strip the description
out from the usage data before shipping it off to the telemetry cluster so it should be safe enough to add a little more in the description.
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.
Okay, I'll change this description to:
Indicates how Kibana authenticates itself to Elasticsearch. If elasticsearch.username is configured, this can be any of: "elastic_user", "kibana_user", "kibana_system_user", or "other_user". Otherwise, if elasticsearch.serviceAccountToken is configured, this will be "kibana_service_account". Otherwise, this value will be "unknown", because some other principal might be used to authenticate Kibana to Elasticsearch (such as an x509 certificate), or authentication may be skipped altogether.
I think "unknown" is better than "other", plus it's less likely to be confused with "other_user".
Edit: done in 0317d33. Unfortunately I could not use a string template or string concatenation to break this up, as it caused the telemetry_check.js
script to fail. So I had to write this description in one line 😅
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
# Conflicts: # x-pack/plugins/security/server/config.test.ts # x-pack/plugins/security/server/config.ts
# Conflicts: # x-pack/plugins/security/server/config.test.ts # x-pack/plugins/security/server/config.ts
* Security usage data (#110548) # Conflicts: # x-pack/plugins/security/server/config.test.ts # x-pack/plugins/security/server/config.ts * Fix bad merge * FIX TESTS AGAIN Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Resolves #110532.
Adds these five fields: