Integrates security with asynchronous search #11

anshul291995 · 2020-12-16T05:54:25Z

Integrates security with asynchronous search. Enforce access to search results through user assigned backend roles.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Bukhtawar

Will take a deeper look in a while

Bukhtawar · 2020-12-16T06:18:20Z

build.gradle

@@ -56,6 +56,7 @@ dependencies {
    testCompile "org.elasticsearch.plugin:reindex-client:${es_version}"
    compileOnly "org.elasticsearch.plugin:transport-netty4-client:${es_version}"
    compileOnly "org.elasticsearch:elasticsearch:${es_version}"
+    compile "com.amazon.opendistroforelasticsearch:common-utils:1.11.0.1"


Lets not hardcode the version

+1 Probably like this? https://github.com/opendistro-for-elasticsearch/alerting/blob/master/core/build.gradle#L28

There is new version of common-utils: 1.11.1.0 for 7.9.1.

Changed the version

Bukhtawar · 2020-12-16T06:18:48Z

src/main/java/com/amazon/opendistroforelasticsearch/search/async/UserAuthUtils.java

+        if(userDetails == null) {
+            return null;
+        }
+        String name = "";


Name is the only part of user object which can't be null. Hence initialised with empty string. Should I use new String() instead ?

Bukhtawar · 2020-12-16T06:19:04Z

.../amazon/opendistroforelasticsearch/search/async/context/active/AsyncSearchActiveContext.java


    public AsyncSearchActiveContext(AsyncSearchContextId asyncSearchContextId, String nodeId,
                                    TimeValue keepAlive, boolean keepOnCompletion,
                                    ThreadPool threadPool, LongSupplier currentTimeSupplier,
                                    AsyncSearchProgressListener searchProgressActionListener,
-                                    AsyncSearchContextListener asyncSearchContextListener) {
+                                    AsyncSearchContextListener asyncSearchContextListener, User user) {


Bukhtawar · 2020-12-16T06:20:17Z

...opendistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceModel.java

-    public AsyncSearchPersistenceModel(long startTimeMillis, long expirationTimeMillis, String response, String error) {
+    public AsyncSearchPersistenceModel(long startTimeMillis, long expirationTimeMillis, String response, String error, User user) {


Can we retain the two constructors one for SearchResponse one for Exception

Only one constructor was being used in the main code. This is after we made the state machine changes and this is called from BeginPersistChanges. The two functions that I removed were only being used by test cases.

Bukhtawar · 2020-12-16T06:22:37Z

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java

+        if(user == null) {
+            client.delete(new DeleteRequest(ASYNC_SEARCH_RESPONSE_INDEX, id), ActionListener.wrap(deleteResponse -> {
+                if (deleteResponse.getResult() == DocWriteResponse.Result.DELETED) {
+                    logger.warn("Delete async search {} successful. Returned result {}", id, deleteResponse.getResult());
+                    listener.onResponse(true);
+                } else {
+                    logger.debug("Delete async search {} unsuccessful. Returned result {}", id, deleteResponse.getResult());
+                    listener.onResponse(false);
+                }
+            }, e -> {
+                final Throwable cause = ExceptionsHelper.unwrapCause(e);
+                if (cause instanceof DocumentMissingException) {
+                    logger.warn(() -> new ParameterizedMessage("Async search response doc already deleted {}", id), e);
+                    listener.onResponse(false);
+                } else {
+                    logger.warn(() -> new ParameterizedMessage("Failed to delete async search for id {}", id), e);
+                    listener.onFailure(cause instanceof Exception ? (Exception) cause : new NotSerializableExceptionWrapper(cause));
+                }
+            }));
+        }
+        else {
+            UpdateRequest updateRequest = new UpdateRequest(ASYNC_SEARCH_RESPONSE_INDEX, id);
+            String scriptCode = "if (ctx._source.user == null || ctx._source.user.backend_roles == null || " +
+                    "( params.backend_roles!=null && params.backend_roles.containsAll(ctx._source.user.backend_roles))) " +
+                    "{ ctx.op = 'delete' } else { ctx.op = 'none' }";
+            Map<String, Object> params = new HashMap<>();
+            params.put("backend_roles", user.getBackendRoles());
+            Script deleteConditionallyScript = new Script(ScriptType.INLINE, "painless", scriptCode, params);
+            updateRequest.script(deleteConditionallyScript);
+            client.update(updateRequest, ActionListener.wrap(deleteResponse -> {
+                switch (deleteResponse.getResult()) {
+                    case UPDATED:


Redundant code. Can we abstract out

Abstracted the exception consumer.

skkosuri-amzn · 2020-12-18T02:41:41Z

...azon/opendistroforelasticsearch/search/async/transport/TransportSubmitAsyncSearchAction.java

@@ -71,10 +73,12 @@ public TransportSubmitAsyncSearchAction(ThreadPool threadPool, TransportService
    @Override
    protected void doExecute(Task task, SubmitAsyncSearchRequest request, ActionListener<AsyncSearchResponse> listener) {
        AsyncSearchContext asyncSearchContext = null;
+        String userStr = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER_AND_ROLES);


OPENDISTRO_SECURITY_USER_AND_ROLES name got changed in 1.11.1.0 . We renamed it to more generic name and included tenantid also.

Changed to 1.11.1.0

skkosuri-amzn · 2020-12-18T02:48:25Z

...zon/opendistroforelasticsearch/search/async/transport/TransportAsyncSearchRoutingAction.java

+            ThreadContext threadContext = threadPool.getThreadContext();
+            String userStr =  threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER_AND_ROLES);
+            User user = User.parse(userStr);
+            threadContext.stashContext();


stashContext returns an auto-closable. You want want to this way:
try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { ....do my work }

skkosuri-amzn

Make sure to move to common-utils-1.11.1.0

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java

Bukhtawar · 2020-12-23T08:51:46Z

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java

+                    case UPDATED:
+                        listener.onFailure(new IllegalStateException("Document updated when requesting delete for async search id "+ id));
+                        break;


Why should this be IllegalStateException?
Concurrent update and deletes are still possible

Bukhtawar · 2020-12-23T08:52:54Z

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java

        UpdateRequest updateRequest = new UpdateRequest(ASYNC_SEARCH_RESPONSE_INDEX, id);
-        updateRequest.doc(source, XContentType.JSON);
-        updateRequest.retryOnConflict(5);


Concurrent updates can cause a conflict. Lets retry on conflict

Added retries.

Bukhtawar · 2020-12-24T09:22:39Z

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java

-                listener.onResponse(false);
-            }
-        }, e -> {
+        Consumer<Exception> onFailure = e -> {
            final Throwable cause = ExceptionsHelper.unwrapCause(e);
            if (cause instanceof DocumentMissingException) {


Is VersionConflictingEngineException possible with concurrent updates and deletes. Lets write a test to veify

Bukhtawar · 2020-12-24T09:23:00Z

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java

+            Map<String, Object> params = new HashMap<>();
+            params.put("backend_roles", user.getBackendRoles());
+            Script deleteConditionallyScript = new Script(ScriptType.INLINE, "painless", scriptCode, params);
+            updateRequest.script(deleteConditionallyScript);


Do we need to retry on conflicts?

Bukhtawar · 2020-12-24T10:18:04Z

...zon/opendistroforelasticsearch/search/async/transport/TransportAsyncSearchRoutingAction.java

+            }
+        }
+
+        private void sendLocalRequest(AsyncSearchId asyncSearchId, Request request, ActionListener<Response> listener)


Rename to executeLocally

Bukhtawar

Looks good minor comments

Anshul Agarwal added 2 commits December 15, 2020 15:30

Integrates security with async search plugin

62c88df

Minor fixes

5d6865b

Bukhtawar reviewed Dec 16, 2020

View reviewed changes

Bukhtawar requested a review from skkosuri-amzn December 16, 2020 06:28

Added tests and corresponding fixes

189fac3

skkosuri-amzn reviewed Dec 18, 2020

View reviewed changes

Bukhtawar mentioned this pull request Dec 21, 2020

Asynchronous Search component breakdown #3

Closed

8 tasks

Anshul Agarwal added 2 commits December 21, 2020 21:22

Merge conflicts

08fe7cd

Added test cases and adressed comments

87217a3

skkosuri-amzn approved these changes Dec 23, 2020

View reviewed changes

Changed opendistro version

b417d1f

Bukhtawar reviewed Dec 23, 2020

View reviewed changes

...endistroforelasticsearch/search/async/context/persistence/AsyncSearchPersistenceService.java Show resolved Hide resolved

Bukhtawar reviewed Dec 23, 2020

View reviewed changes

Anshul Agarwal added 4 commits December 23, 2020 22:11

Added test cases and changed elasticsearch version to 7.9.1

fff13da

Fixed test

1789a03

Added retries on update

eb97045

Fixed tests

5e48770

Bukhtawar changed the title ~~Integrates security with async search plugin~~ Integrates security with asynchronous search Dec 24, 2020

Bukhtawar reviewed Dec 24, 2020

View reviewed changes

Bukhtawar approved these changes Dec 24, 2020

View reviewed changes

Bukhtawar merged commit 3f6ea1c into master Dec 24, 2020

Bukhtawar deleted the security branch January 2, 2021 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrates security with asynchronous search #11

Integrates security with asynchronous search #11

anshul291995 commented Dec 16, 2020 •

edited by Bukhtawar

Loading

Bukhtawar left a comment

Bukhtawar Dec 16, 2020

skkosuri-amzn Dec 18, 2020

skkosuri-amzn Dec 18, 2020

anshul291995 Dec 23, 2020

Bukhtawar Dec 16, 2020

anshul291995 Dec 21, 2020

Bukhtawar Dec 16, 2020

anshul291995 Dec 21, 2020

Bukhtawar Dec 16, 2020

anshul291995 Dec 21, 2020

Bukhtawar Dec 16, 2020

anshul291995 Dec 21, 2020

skkosuri-amzn Dec 18, 2020

anshul291995 Dec 23, 2020

skkosuri-amzn Dec 18, 2020

anshul291995 Dec 21, 2020

skkosuri-amzn left a comment

Bukhtawar Dec 23, 2020

Bukhtawar Dec 23, 2020

anshul291995 Dec 23, 2020

Bukhtawar Dec 24, 2020

Bukhtawar Dec 24, 2020

Bukhtawar Dec 24, 2020

Bukhtawar left a comment

		public AsyncSearchPersistenceModel(long startTimeMillis, long expirationTimeMillis, String response, String error) {
		public AsyncSearchPersistenceModel(long startTimeMillis, long expirationTimeMillis, String response, String error, User user) {

Integrates security with asynchronous search #11

Integrates security with asynchronous search #11

Conversation

anshul291995 commented Dec 16, 2020 • edited by Bukhtawar Loading

Bukhtawar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skkosuri-amzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar left a comment

Choose a reason for hiding this comment

anshul291995 commented Dec 16, 2020 •

edited by Bukhtawar

Loading