From 82e22a9fd0500faea0d95e9600d1e6f02373c8fa Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 16 Jul 2019 13:18:23 +0900 Subject: [PATCH 1/2] Avoid NPE when checking for CCR index privileges This commit avoids an NPE when checking for privileges to follow indices. The problem here is that in some cases we might not be able to read the authentication info from the thread context. In that case, a null user would be returned and we were not guarding against this. --- .../xpack/ccr/CcrLicenseChecker.java | 18 +++++-- .../xpack/ccr/CcrLicenseCheckerTests.java | 50 +++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java index 286d524f60b4a..82877f0c43f35 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java @@ -45,6 +45,7 @@ import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.permission.ResourcePrivileges; import org.elasticsearch.xpack.core.security.support.Exceptions; +import org.elasticsearch.xpack.core.security.user.User; import java.util.Arrays; import java.util.Collections; @@ -61,7 +62,7 @@ /** * Encapsulates licensing checking for CCR. */ -public final class CcrLicenseChecker { +public class CcrLicenseChecker { private final BooleanSupplier isCcrAllowed; private final BooleanSupplier isAuthAllowed; @@ -307,9 +308,12 @@ public void hasPrivilegesToFollowIndices(final Client remoteClient, final String return; } - ThreadContext threadContext = remoteClient.threadPool().getThreadContext(); - SecurityContext securityContext = new SecurityContext(Settings.EMPTY, threadContext); - String username = securityContext.getUser().principal(); + final User user = getUser(remoteClient); + if (user == null) { + handler.accept(new IllegalStateException("missing or unable to read authentication info on request")); + return; + } + String username = user.principal(); RoleDescriptor.IndicesPrivileges privileges = RoleDescriptor.IndicesPrivileges.builder() .indices(indices) @@ -344,6 +348,12 @@ public void hasPrivilegesToFollowIndices(final Client remoteClient, final String remoteClient.execute(HasPrivilegesAction.INSTANCE, request, ActionListener.wrap(responseHandler, handler)); } + User getUser(final Client remoteClient) { + final ThreadContext threadContext = remoteClient.threadPool().getThreadContext(); + final SecurityContext securityContext = new SecurityContext(Settings.EMPTY, threadContext); + return securityContext.getUser(); + } + public static Client wrapClient(Client client, Map headers) { if (headers.isEmpty()) { return client; diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java new file mode 100644 index 0000000000000..968e54413be37 --- /dev/null +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java @@ -0,0 +1,50 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.ccr; + +import org.elasticsearch.client.Client; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.xpack.core.security.user.User; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasToString; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assert.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class CcrLicenseCheckerTests extends ESTestCase { + + public void testNoAuthenticationInfo() { + final boolean isCcrAllowed = randomBoolean(); + final CcrLicenseChecker checker = new CcrLicenseChecker(() -> isCcrAllowed, () -> true) { + + @Override + User getUser(final Client remoteClient) { + return null; + } + + }; + final AtomicBoolean invoked = new AtomicBoolean(); + checker.hasPrivilegesToFollowIndices( + mock(Client.class), + new String[]{randomAlphaOfLength(8)}, + e -> { + invoked.set(true); + assertThat(e, instanceOf(IllegalStateException.class)); + assertThat(e, hasToString(containsString("missing or unable to read authentication info on request"))); + }); + assertTrue(invoked.get()); + } + +} From 96f93fd0ab3c0e32b5f6a887c842c8c43b7e8be8 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 16 Jul 2019 14:04:00 +0900 Subject: [PATCH 2/2] Fix imports --- .../java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java | 2 +- .../org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java index 82877f0c43f35..8adb6140be099 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/CcrLicenseChecker.java @@ -7,10 +7,10 @@ package org.elasticsearch.xpack.ccr; import org.elasticsearch.ElasticsearchStatusException; -import org.elasticsearch.action.ActionType; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionRequest; import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.action.ActionType; import org.elasticsearch.action.admin.cluster.state.ClusterStateRequest; import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse; import org.elasticsearch.action.admin.indices.stats.IndexShardStats; diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java index 968e54413be37..bf49c6ab8372f 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrLicenseCheckerTests.java @@ -7,21 +7,15 @@ package org.elasticsearch.xpack.ccr; import org.elasticsearch.client.Client; -import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.xpack.core.security.user.User; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; -import static org.junit.Assert.*; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class CcrLicenseCheckerTests extends ESTestCase {