add user authentication test for ILM #32826

talevy · 2018-08-13T22:31:42Z

These are some user-auth scenarios I think may be worth testing. For some reason, the tests hang on the read-only action. Still not sure why.

elasticmachine · 2018-08-13T22:31:44Z

Pinging @elastic/es-core-infra

dakrone · 2018-08-14T22:34:56Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+            pollIntervalEntity.startObject("transient");
+            {
+                pollIntervalEntity.field(LifecycleSettings.LIFECYCLE_POLL_INTERVAL, "1s");
+            }pollIntervalEntity.endObject();


I think the newline is messed up here?

I was trying out a new formatting, I am missing a space there. but no like? I am happy to push it down a line

I'm not a fan of this style of formatting, I think it looks confusing and somewhat ugly since it does not fit in with the code style of the rest of the code. I don't mind the style we have elsewhere sometimes of indenting lines which are inside objects but since here the JSON we are building is quite straight forward I also think its easy to follow formatted normally

dakrone · 2018-08-14T22:37:32Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+                assertThat(indexExplain.get("managed"), equalTo(true));
+                assertThat(indexExplain.get("step"), equalTo("error"));
+                assertThat(indexExplain.get("failed_step"), equalTo("readonly"));
+                assertThat(indexExplain.get("step_info"), equalTo("permissionsss!"));


wat?

Do we really assert that the explanation for the step is "permissionsss!" 🐍 ?

haha, yes!

This is technically WIP since the test does not pass... but I have no idea why. I was hoping to at least get early feedback on the types of tests... as to why things are stuck on readonly. still a mystery to me and I am debugging it

I can help look into this tomorrow if you are still having trouble finding the cause of the failure

colings86 · 2018-08-15T08:21:47Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+            pollIntervalEntity.startObject("transient");
+            {
+                pollIntervalEntity.field(LifecycleSettings.LIFECYCLE_POLL_INTERVAL, "1s");
+            }pollIntervalEntity.endObject();


I'm not a fan of this style of formatting, I think it looks confusing and somewhat ugly since it does not fit in with the code style of the rest of the code. I don't mind the style we have elsewhere sometimes of indenting lines which are inside objects but since here the JSON we are building is quite straight forward I also think its easy to follow formatted normally

colings86 · 2018-08-15T08:22:27Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
+import static org.hamcrest.Matchers.equalTo;
+
+public class PermissionsIT extends ESRestTestCase {


Could you add a JavaDoc here explaining what this test class aims to test?

colings86 · 2018-08-15T08:25:00Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+    public void testCanManageIndexAndPolicyDifferentUsers() throws Exception {
+        String index = "ilm-00001";
+        createIndexAsAdmin(index, indexSettingsWithPolicy, "");
+        assertBusy(() -> assertFalse(indexExists(index)));


Is this actually testing anything ILM related? I think we need to test that the policy actually progresses here to ensure the ILM side is doing the permissions correctly?

the index is deleted by ILM?

theoretically. plan to continue debugging the lack of progress in the system today

Could you add a javadoc for this test to explain what its doing? At first glance it looks a bit confusing because it just creates an index and then checks it doesn't exist.

colings86 · 2018-08-15T08:25:34Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+                assertThat(indexExplain.get("managed"), equalTo(true));
+                assertThat(indexExplain.get("step"), equalTo("error"));
+                assertThat(indexExplain.get("failed_step"), equalTo("readonly"));
+                assertThat(indexExplain.get("step_info"), equalTo("permissionsss!"));


I can help look into this tomorrow if you are still having trouble finding the cause of the failure

colings86 · 2018-08-20T12:06:44Z

Took a while to track down but it seems that the problem with the policy not progressing is due to the update settings request in the read only step being dropped by the SecurityActionFilter. This is because the ClientHelper.INDEX_LIFECYCLE_ORIGIN is not present in the switch statement in AuthorizationUtils. Adding this makes the policy progress.

Additionally I think there is a bug in the SecurityActionFilter.apply() because if AuthorizationUtils.switchUserBasedOnActionOriginAndExecute() throws an IllegalArgumentException there is no catch surrounding it in SecurityActionFilter.apply() to then call listener.OnFailure() so the request gets silently dropped. We should probably fix this bug too but I'll leave it up to you whether we do this in this PR or in a separate change. /cc @jaymode

jasontedor · 2018-08-20T13:23:35Z

I am not seeing what you're seeing @colings86. Current master looks like this:

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java

Lines 94 to 118 in a883e7d

    
           try { 
        
               if (useSystemUser) { 
        
                   securityContext.executeAsUser(SystemUser.INSTANCE, (original) -> { 
        
                       try { 
        
                           applyInternal(action, request, authenticatedListener); 
        
                       } catch (IOException e) { 
        
                           listener.onFailure(e); 
        
                       } 
        
                   }, Version.CURRENT); 
        
               } else if (AuthorizationUtils.shouldSetUserBasedOnActionOrigin(threadContext)) { 
        
                   AuthorizationUtils.switchUserBasedOnActionOriginAndExecute(threadContext, securityContext, (original) -> { 
        
                       try { 
        
                           applyInternal(action, request, authenticatedListener); 
        
                       } catch (IOException e) { 
        
                           listener.onFailure(e); 
        
                       } 
        
                   }); 
        
               } else { 
        
                   try (ThreadContext.StoredContext ignore = threadContext.newStoredContext(true)) { 
        
                       applyInternal(action, request, authenticatedListener); 
        
                   } 
        
               } 
        
           } catch (Exception e) { 
        
               listener.onFailure(e); 
        
           }

The whole block is wrapped in a try-catch. And anyway, higher up we have this wrapped too, which is always our last resort against issues like this:

elasticsearch/server/src/main/java/org/elasticsearch/action/support/TransportAction.java

Lines 136 to 153 in a883e7d

    
               @Override 
        
               public void proceed(Task task, String actionName, Request request, ActionListener<Response> listener) { 
        
                   int i = index.getAndIncrement(); 
        
                   try { 
        
                       if (i < this.action.filters.length) { 
        
                           this.action.filters[i].apply(task, actionName, request, listener, this); 
        
                       } else if (i == this.action.filters.length) { 
        
                           this.action.doExecute(task, request, listener); 
        
                       } else { 
        
                           listener.onFailure(new IllegalStateException("proceed was called too many times")); 
        
                       } 
        
                   } catch(Exception e) { 
        
                       logger.trace("Error during transport action execution.", e); 
        
                       listener.onFailure(e); 
        
                   } 
        
               } 
        
           }

colings86 · 2018-08-20T14:39:08Z

@jasontedor hmm good point with the surrounding try-catches. I'm not sure why the request was getting dropped then but I added log lines to all the listener.onFailure() calls and did not see any output from them. AuthorizationUtils.switchUserBasedOnActionOriginAndExecute() was definitely throwing an IllegalArgumentException though so I think the fix to add ilm to the case statement is correct even if we don't need to fix the action filter itself?

jasontedor · 2018-08-20T15:19:08Z

@colings86 I think that you're running with assertions enabled? That would lead to the assertion tripping, throwing an AssertionError which we do not catch, and that would lead to a hung listener? Are you sure that an IllegalArugmentException is being thrown? I don't even see how that method could throw one. I do see an IllegalStateException but that would not be thrown if it is indeed the assertion that it is tripping, which I think is what is happening here?

colings86 · 2018-08-20T15:27:28Z

@jasontedor ah yes you are right, since I'm running in a test from gradle ./gradlew :x-pack:plugin:ilm:qa:withsecurity:integtest it will have assertions enabled so you are right that it won't be caught. My certainty that the IllegalStateException was being thrown (i mixed up IllegalStateException and IllegalArgumentException was based on adding logging before this line and seeing it output in the logs but still forgetting that assertions would be on. The fix to add ClientHelper.INDEX_LIFECYCLE_ORIGIN to the switch statement I still think is the right fix though.

It is a bit unfortunately that the asserts don't really show up clearly in the logs though, this made this bug quite difficult to track down without digging into it. I wonder if the assertion is actually worth it here since if the IllegalStateException had been thrown it would have highlighted the bug quicker I think?

jaymode · 2018-08-20T15:28:07Z

AuthorizationUtils.switchUserBasedOnActionOriginAndExecute() was definitely throwing an IllegalArgumentException though so I think the fix to add ilm to the case statement is correct even if we don't need to fix the action filter itself?

That is the right fix for getting the user added to the request.

jasontedor · 2018-08-20T15:37:07Z

The fix to add ClientHelper.INDEX_LIFECYCLE_ORIGIN to the switch statement I still think is the right fix though.

I am not arguing against the fix, the fix is clearly correct. I am arguing that there is not any problem with SecurityActionFilter#apply.

jasontedor · 2018-08-20T19:23:08Z

I wonder if the assertion is actually worth it here since if the IllegalStateException had been thrown it would have highlighted the bug quicker I think?

The problem here is that the assertion error was getting caught by the JDK, set as the outcome on a future task, and lost. We need to ensure that errors thrown from triggered listeners do not get lost. I opened #32998.

talevy · 2018-08-20T22:12:23Z

thanks for the assistance here everyone. I think I caught up with changes necessary to make these tests fly!

that being said, the qa:with-security tests passed without the adding of the INDEX_LIFECYCLE_ORIGIN

colings86

I left a couple of comments but I think this is close now

colings86 · 2018-08-21T07:42:35Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+        String token = basicAuthHeaderValue("test_admin", new SecureString("x-pack-test-password".toCharArray()));
+        return Settings.builder()
+            .put(ThreadContext.PREFIX + ".Authorization", token)
+            .build();


Is this going to be safe? As I understand it, the rest test case uses the admin settings to reset the cluster to a clean state between tests, but here this admin user only has access to the ilm-* indices so I'm not sure if it will be able to clean up the the not-ilm index?

test_admin has full administrator authority. the client() is running as the ILM-specific, test_ilm, user. This is why wipeCluster() has not been throwing an exception and failing the tests when running as adminClient().

colings86 · 2018-08-21T07:44:52Z

...lugin/ilm/qa/with-security/src/test/java/org/elasticsearch/xpack/security/PermissionsIT.java

+    public void testCanManageIndexAndPolicyDifferentUsers() throws Exception {
+        String index = "ilm-00001";
+        createIndexAsAdmin(index, indexSettingsWithPolicy, "");
+        assertBusy(() -> assertFalse(indexExists(index)));


Could you add a javadoc for this test to explain what its doing? At first glance it looks a bit confusing because it just creates an index and then checks it doesn't exist.

colings86 · 2018-08-21T07:46:05Z

...src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java

+    protected void masterOperation(Request request, ClusterState state, ActionListener<Response> listener) {
+        Map<String, String> filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream()
+            .filter(e -> ClientHelper.SECURITY_HEADER_FILTERS.contains(e.getKey()))
+            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));


This might be worth a comment explaining why this needs to be here and not in the task so someone doesn't move it into the task without realising it will break things?

colings86

LGTM

talevy added WIP :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Aug 13, 2018

talevy force-pushed the ilm-security-user-test branch from a75c831 to a3414eb Compare August 13, 2018 22:49

elasticmachine mentioned this pull request Aug 13, 2018

[meta] Index Lifecycle Management Plan #29823

Closed

add user authentication test for ILM

b58b30e

talevy force-pushed the ilm-security-user-test branch from a3414eb to b58b30e Compare August 14, 2018 22:17

talevy removed the WIP label Aug 14, 2018

talevy requested review from colings86 and dakrone August 14, 2018 22:19

dakrone reviewed Aug 14, 2018

View reviewed changes

colings86 reviewed Aug 15, 2018

View reviewed changes

Merge branch 'index-lifecycle' into ilm-security-user-test

1cfe5f1

fix test

1af0dad

Merge branch 'index-lifecycle' into ilm-security-user-test

d9a5b86

colings86 requested changes Aug 21, 2018

View reviewed changes

add comments

dfaa600

talevy requested a review from colings86 August 21, 2018 13:42

colings86 approved these changes Aug 21, 2018

View reviewed changes

rename role to ilm

595cdad

talevy merged commit 6780ab9 into elastic:index-lifecycle Aug 21, 2018

talevy deleted the ilm-security-user-test branch August 21, 2018 19:27

talevy added a commit that referenced this pull request Aug 21, 2018

add user authentication test for ILM (#32826)

aa072e6

add user authentication test for ILM #32826

add user authentication test for ILM #32826

Uh oh!

Conversation

talevy commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Aug 20, 2018

Uh oh!

colings86 commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Aug 20, 2018

Uh oh!

colings86 commented Aug 20, 2018

Uh oh!

jaymode commented Aug 20, 2018

Uh oh!

jasontedor commented Aug 20, 2018

Uh oh!

jasontedor commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talevy commented Aug 20, 2018

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talevy Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

talevy commented Aug 13, 2018 •

edited

Loading

colings86 commented Aug 20, 2018 •

edited

Loading

colings86 commented Aug 20, 2018 •

edited

Loading

jasontedor commented Aug 20, 2018 •

edited

Loading

talevy Aug 21, 2018 •

edited

Loading