-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add user information to slowlog #105621
Add user information to slowlog #105621
Conversation
a4ca9ea
to
caa59af
Compare
authContext.put("apikey.id", authenticatingSubject.getMetadata().get(AuthenticationField.API_KEY_ID_KEY).toString()); | ||
authContext.put("apikey.name", authenticatingSubject.getMetadata().get(AuthenticationField.API_KEY_NAME_KEY).toString()); | ||
} | ||
// TODO: do we want token name here for service accounts? |
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.
Do we want token name here for service accounts?
c581bbd
to
89563a4
Compare
...lowlog/src/javaRestTest/java/org/elasticsearch/xpack/security/slowlog/SecuritySlowLogIT.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Show resolved
Hide resolved
75667bc
to
325f291
Compare
Pinging @elastic/es-security (Team:Security) |
Pinging @elastic/es-docs (Team:Docs) |
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.
looking good. a couple comments
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Outdated
Show resolved
Hide resolved
...lowlog/src/javaRestTest/java/org/elasticsearch/xpack/security/slowlog/SecuritySlowLogIT.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Show resolved
Hide resolved
530c984
to
7200f76
Compare
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
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.
Looking really good, just a couple last comments w.r.t. to exactly what we log.
@@ -0,0 +1,10 @@ | |||
apply plugin: 'elasticsearch.internal-java-rest-test' |
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.
Do we need to create another QA project here? It seems we could just move SecuritySlowLogIT
into one of the existing security QA projects.
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.
Thanks for reviewing!
I created a new one because the audit log had a separate one and it felt like they're a little similar. Thinking some more about it this could also fit in with the security-basic
tests, since it's part of the basic licence. I'll move it there.
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.
it looks great, but please change the example to not use guid
also left one question
private boolean includeUserInIndexing = false; | ||
private boolean includeUserInSearch = false; | ||
|
||
public SecuritySlowLogFieldProvider(Security plugin) { |
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
is a Plugin
subclass. Are you sure we want to depend on such a high level entry point class? When do we use this constructor?
We are 'providing' this instance via SPI, so the other noarg constructor is used , right?
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.
Thanks for calling this out.
The one arg constructor is called from here, where the parameter has to be a Plugin
and the auth context that is needed is only available in the Security
plugin.
To pass "SPI validation" the provided implementation needs to be declared in module-info
and one of the validation requirements is that it has a no arg constructor, but we never actually call it. I'll add an IllegalStateException
to the no arg constructor like we do here to make sure we don't end up in that state.
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
Nice work ! This will really help with troubleshooting.
Also, don't forget to update (or log the request) to update the mappings for beats integration. (i think here)
@elasticmachine update branch |
+1 |
This PR adds two new index settings:
When enabled, depending on the authenticated user, the slow log will have a subset of these additional fields:
Example
TODO