Skip to content
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

Shield duplicates _search api audits #11710

Closed
bobbyhubbard opened this issue Jun 16, 2015 · 4 comments
Closed

Shield duplicates _search api audits #11710

bobbyhubbard opened this issue Jun 16, 2015 · 4 comments
Assignees
Labels
>bug :Search/Search Search-related issues that do not fall into other categories

Comments

@bobbyhubbard
Copy link

A standard GET by ID results in a single log entry in the audit log. Search (_search) results in two identical log entries. Every time.

  • Default ES 1.6.0 installation.
  • Added latest Shield.
  • Added "search_admin" user.

Everything works great out of the box except I get _search audits duplicated in the log. ONLY "SearchRequest" audits. I haven't found any other api yet which results in this strange behavior.

[2015-06-16 18:37:24,705] [esdev-shieldpoc01] [transport] [access_granted]      origin_type=[rest], origin_address=[/10.30.24.36:55308], principal=[search_admin], action=[indices:data/read/search], indices=[test], request=[SearchRequest]
[2015-06-16 18:37:24,705] [esdev-shieldpoc01] [transport] [access_granted]      origin_type=[rest], origin_address=[/10.30.24.36:55308], principal=[search_admin], action=[indices:data/read/search], indices=[test], request=[SearchRequest]
@clintongormley
Copy link

@jaymode please could you take a look?

@jaymode
Copy link
Member

jaymode commented Jun 19, 2015

@bobbyhubbard thanks for reporting this. This behavior isn't ideal and we'll look at if/how we can improve this. What you're seeing is a side effect of how auditing is implemented and how search requests are executed. Shield audits the individual actions that are executed by elasticsearch.

The indices:data/read/search action name corresponds to multiple actions for search. The reason that you are seeing the message twice, is the API executes a TransportSearchAction, which in turn executes a action corresponding to the search type; the default type query_then_fetch corresponds to the TransportSearchQueryThenFetchAction and the execution of this action is what causes the second log message.

@javanna
Copy link
Member

javanna commented Jun 19, 2015

This seems to have to do with the fact that the main TransportSearchAction uses an inner TransportSearchTypeAction (there is a different impl for each search type). Last time I checked I noticed some code that gets executed twice while it shouldn't (e.g. request validation), and a side effect is also the double audit log line. Maybe this inner action shouldn't be a transport action after all? The two (outer and inner) execute calls happen on the same node all the time, seems like also the transport handler registration that happens in TransportSearchTypeAction has no actual effect as it's already registered by TransportSearchAction with same name, so only used for audit logging.

@bobbyhubbard
Copy link
Author

This bit us again today when someone else in our org setup a new log drain from shield. It reported nearly double the number of rest requests as expected. Then I remembered this issue... The workaround is simple enough...to hash each message (fingerprint in logstash) and use the hash as the message id. But this WILL bite every single Shield customer who is measuring and auditing rest calls. (How many are reporting invalid results now because they dont even know about this bug like one team here almost did?)

@jaymode jaymode added discuss and removed discuss labels Nov 19, 2015
@javanna javanna added the :Search/Search Search-related issues that do not fall into other categories label Feb 17, 2016
javanna added a commit to javanna/elasticsearch that referenced this issue Feb 17, 2016
All the actions that extend TransportSearchTypeAction are subactions of the main TransportSearchAction. The main one is and should be a transport action, register request handlers, support request and response filtering etc. but the subactions shouldn't as that becomes just double work. At the moment each search request goes through validation and filters twice, one as part of the main action, and the second one as part of the subaction execution. The subactions don't need to extend TransportAction, but can be simple support classes, as they are always executed on the same node as their main action.

This commit modifies TransportSearchTypeAction to not extend TransportAction but simply AbstractComponent.

Closes elastic#11710
@javanna javanna assigned javanna and unassigned jaymode Feb 20, 2016
javanna added a commit to javanna/elasticsearch that referenced this issue Feb 29, 2016
…ype package into o.e.action.search

TransportSearchTypeAction and subclasses are not actually transport actions, but just support classes useful for their inner async actions that can easily be extracted out so that we get rid of one too many level of abstraction.

Same pattern can be applied to TransportSearchScrollQueryAndFetchAction & TransportSearchScrollQueryThenFetchAction which we could remove in favour of keeping only their inner classes named SearchScrollQueryAndFetchAsyncAction and SearchScrollQueryThenFetchAsyncAction.

Remove org.elasticsearch.action.search.type package, collapsed remaining classes into existing org.elasticsearch.action.search package

Make also ParsedScrollId ScrollIdForNode and TransportSearchHelper classes and their methods package private.

Closes elastic#11710
@javanna javanna added the >bug label Feb 29, 2016
javanna added a commit that referenced this issue Feb 29, 2016
…ype package into o.e.action.search

TransportSearchTypeAction and subclasses are not actually transport actions, but just support classes useful for their inner async actions that can easily be extracted out so that we get rid of one too many level of abstraction.

Same pattern can be applied to TransportSearchScrollQueryAndFetchAction & TransportSearchScrollQueryThenFetchAction which we could remove in favour of keeping only their inner classes named SearchScrollQueryAndFetchAsyncAction and SearchScrollQueryThenFetchAsyncAction.

Remove org.elasticsearch.action.search.type package, collapsed remaining classes into existing org.elasticsearch.action.search package

Make also ParsedScrollId ScrollIdForNode and TransportSearchHelper classes and their methods package private.

Closes #11710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

No branches or pull requests

4 participants