From 54bf7d78e8e2d82a64b47a3173b7bb815ee51cf6 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 16 Aug 2017 12:54:17 +0200 Subject: [PATCH] Prevent cluster internal `ClusterState.Custom` impls to leak to a client (#26232) Today a `ClusterState.Custom` can be fetched by a transport client and leaks to the user even if the classes are private etc since the serialized bytes can be reconstructed. This change adds an option to customs to mark them as private such that our clusterstate action will never leak it. --- .../state/ClusterStateRequestBuilder.java | 9 +++ .../state/TransportClusterStateAction.java | 6 +- .../elasticsearch/cluster/ClusterState.java | 8 ++ .../cluster/SimpleClusterStateIT.java | 81 +++++++++++++++++++ 4 files changed, 103 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateRequestBuilder.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateRequestBuilder.java index 979c81c3c34ca..6ec7763e31b25 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateRequestBuilder.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/state/ClusterStateRequestBuilder.java @@ -68,6 +68,15 @@ public ClusterStateRequestBuilder setNodes(boolean filter) { return this; } + /** + * Should the cluster state result include the {@link org.elasticsearch.cluster.ClusterState.Custom}. Defaults + * to true. + */ + public ClusterStateRequestBuilder setCustoms(boolean filter) { + request.customs(filter); + return this; + } + /** * Should the cluster state result include the {@link org.elasticsearch.cluster.routing.RoutingTable}. Defaults * to true. diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java index 601c69b3189e3..9efdbfa47148b 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/state/TransportClusterStateAction.java @@ -126,7 +126,11 @@ protected void masterOperation(final ClusterStateRequest request, final ClusterS builder.metaData(mdBuilder); } if (request.customs()) { - builder.customs(currentState.customs()); + for (ObjectObjectCursor custom : currentState.customs()) { + if (custom.value.isPrivate() == false) { + builder.putCustom(custom.key, custom.value); + } + } } listener.onResponse(new ClusterStateResponse(currentState.getClusterName(), builder.build(), serializeFullClusterState(currentState, Version.CURRENT).length())); diff --git a/core/src/main/java/org/elasticsearch/cluster/ClusterState.java b/core/src/main/java/org/elasticsearch/cluster/ClusterState.java index 576fd760a08e2..3f6409cc5dfd0 100644 --- a/core/src/main/java/org/elasticsearch/cluster/ClusterState.java +++ b/core/src/main/java/org/elasticsearch/cluster/ClusterState.java @@ -92,6 +92,14 @@ public class ClusterState implements ToXContentFragment, Diffable public static final ClusterState EMPTY_STATE = builder(ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)).build(); public interface Custom extends NamedDiffable, ToXContent { + + /** + * Returns true iff this {@link Custom} is private to the cluster and should never be send to a client. + * The default is false; + */ + default boolean isPrivate() { + return false; + } } private static final NamedDiffableValueSerializer CUSTOM_VALUE_SERIALIZER = new NamedDiffableValueSerializer<>(Custom.class); diff --git a/core/src/test/java/org/elasticsearch/cluster/SimpleClusterStateIT.java b/core/src/test/java/org/elasticsearch/cluster/SimpleClusterStateIT.java index 89d33043e574f..3a4e414275b0a 100644 --- a/core/src/test/java/org/elasticsearch/cluster/SimpleClusterStateIT.java +++ b/core/src/test/java/org/elasticsearch/cluster/SimpleClusterStateIT.java @@ -28,19 +28,32 @@ import org.elasticsearch.cluster.metadata.MappingMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.routing.RoutingTable; +import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.collect.ImmutableOpenMap; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.unit.ByteSizeValue; +import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.plugins.ClusterPlugin; +import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.hamcrest.CollectionAssertions; import org.junit.Before; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.function.Supplier; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertIndexTemplateExists; @@ -54,6 +67,11 @@ */ public class SimpleClusterStateIT extends ESIntegTestCase { + @Override + protected Collection> nodePlugins() { + return Collections.singletonList(PrivateCustomPlugin.class); + } + @Before public void indexData() throws Exception { index("foo", "bar", "1", XContentFactory.jsonBuilder().startObject().field("foo", "foo").endObject()); @@ -258,4 +276,67 @@ public void testIndicesIgnoreUnavailableFalse() throws Exception { assertThat(e.getMessage(), is("no such index")); } } + + public void testPrivateCustomsAreExcluded() { + ClusterStateResponse clusterStateResponse = client().admin().cluster().prepareState().setCustoms(true).get(); + assertFalse(clusterStateResponse.getState().customs().containsKey("test")); + // just to make sure there is something + assertTrue(clusterStateResponse.getState().customs().containsKey(SnapshotDeletionsInProgress.TYPE)); + ClusterState state = internalCluster().getInstance(ClusterService.class).state(); + assertTrue(state.customs().containsKey("test")); + } + + private static class TestCustom extends AbstractNamedDiffable implements ClusterState.Custom { + + private final int value; + + TestCustom(int value) { + this.value = value; + } + + TestCustom(StreamInput in) throws IOException { + this.value = in.readInt(); + } + @Override + public String getWriteableName() { + return "test"; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeInt(value); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return builder; + } + + static NamedDiff readDiffFrom(StreamInput in) throws IOException { + return readDiffFrom(ClusterState.Custom.class, "test", in); + } + + @Override + public boolean isPrivate() { + return true; + } + } + + public static class PrivateCustomPlugin extends Plugin implements ClusterPlugin { + + public PrivateCustomPlugin() {} + + @Override + public Map> getInitialClusterStateCustomSupplier() { + return Collections.singletonMap("test", () -> new TestCustom(1)); + } + + @Override + public List getNamedWriteables() { + List entries = new ArrayList<>(); + entries.add(new NamedWriteableRegistry.Entry(ClusterState.Custom.class, "test", TestCustom::new)); + entries.add(new NamedWriteableRegistry.Entry(NamedDiff.class, "test", TestCustom::readDiffFrom)); + return entries; + } + } }