From 4c2f3778d759f16f358f8a21025714ec5ce5fb38 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 11 Nov 2019 10:10:14 +0100 Subject: [PATCH 1/3] DATAREDIS-1060 - Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 8e149f902c..58f8ab1a9e 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 2.3.0.BUILD-SNAPSHOT + 2.3.0.DATAREDIS-1060-SNAPSHOT Spring Data Redis From fab9e38c557670d6b01978c67241d6167870e1a2 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 11 Nov 2019 15:40:29 +0100 Subject: [PATCH 2/3] DATAREDIS-1060 - Redis password should not automatically be applied to Sentinel. --- src/main/asciidoc/reference/redis.adoc | 3 +- .../redis/connection/RedisConfiguration.java | 73 +++++++++++++++++ .../RedisSentinelConfiguration.java | 77 +++++++++++++++--- .../lettuce/LettuceConnectionFactory.java | 1 - .../connection/lettuce/LettuceConverters.java | 25 ++++-- .../RedisSentinelConfigurationUnitTests.java | 65 ++++++++++++++- .../LettuceConnectionFactoryUnitTests.java | 79 +++++++++++++++++-- 7 files changed, 292 insertions(+), 31 deletions(-) diff --git a/src/main/asciidoc/reference/redis.adoc b/src/main/asciidoc/reference/redis.adoc index 4ffbc75c3e..96be9c7ee1 100644 --- a/src/main/asciidoc/reference/redis.adoc +++ b/src/main/asciidoc/reference/redis.adoc @@ -174,13 +174,14 @@ public RedisConnectionFactory lettuceConnectionFactory() { .Configuration Properties * `spring.redis.sentinel.master`: name of the master node. * `spring.redis.sentinel.nodes`: Comma delimited list of host:port pairs. +* `spring.redis.sentinel.password`: The password to apply when authenticating with Redis Sentinel ==== Sometimes, direct interaction with one of the Sentinels is required. Using `RedisConnectionFactory.getSentinelConnection()` or `RedisConnection.getSentinelCommands()` gives you access to the first active Sentinel configured. [NOTE] ==== -Configuration options like password, timeouts, SSL... also get applied to Redis Sentinel when using https://lettuce.io/[Lettuce]. +Sentinel authentication is only available using https://lettuce.io/[Lettuce]. ==== [[redis:template]] diff --git a/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java b/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java index 0af18b1b4d..e04f118abe 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java @@ -337,6 +337,79 @@ default void setMaster(final String name) { * @return {@link Set} of sentinels. Never {@literal null}. */ Set getSentinels(); + + /** + * Get the {@link RedisPassword} used when authenticating with a Redis Server. + * + * @return never {@literal null}. + */ + default RedisPassword getDataNodePassword() { + return getPassword(); + } + + /** + * Create and set a {@link RedisPassword} to be used when authenticating with Sentinel from the given {@link String} + * + * @param password can be {@literal null}. + * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} + * should be used for authenticating with Redis Sentinel. + * @since 2.2.2 + */ + default void setSentinelPassword(@Nullable String password) { + setSentinelPassword(RedisPassword.of(password)); + } + + /** + * Create and set a {@link RedisPassword} to be used when authenticating with Sentinel from the given + * {@link Character} sequence. + * + * @param password can be {@literal null}. + * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} + * should be used for authenticating with Redis Sentinel. + * @since 2.2.2 + */ + default void setSentinelPassword(@Nullable char[] password) { + setSentinelPassword(RedisPassword.of(password)); + } + + /** + * Set a {@link RedisPassword} to be used when authenticating with Sentinel. + * + * @param password must not be {@literal null} use {@link RedisPassword#none()} instead. + * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} + * should be used for authenticating with Redis Sentinel. + * @since 2.2.2 + */ + void setSentinelPassword(RedisPassword password); + + /** + * Get the {@link RedisPassword} to use when connecting to a Redis Sentinel.
+ * This can be the password explicitly set via {@link #setSentinelPassword(RedisPassword)}, or the + * {@link #getDataNodePassword() Data Node password} if it should be also used for + * {@link #getUseDataNodeAuthenticationForSentinel() sentinel}, or {@link RedisPassword#none()} if no password has + * been set. + * + * @return the {@link RedisPassword} for authenticating with Sentinel. + * @since 2.2.2 + */ + RedisPassword getSentinelPassword(); + + /** + * Use the {@link #getDataNodePassword() RedisPassword} also for authentication with Redis Sentinel. + * + * @param useDataNodeAuthenticationForSentinel + * @throws IllegalStateException if a {@link #getSentinelPassword() Sentinel Password} is already set. + * @since 2.2.2 + */ + void useDataNodeAuthenticationForSentinel(boolean useDataNodeAuthenticationForSentinel); + + /** + * Use the {@link #getDataNodePassword() RedisPassword} also for authentication with Redis Sentinel. + * + * @return + * @since 2.2.2 + */ + boolean getUseDataNodeAuthenticationForSentinel(); } /** diff --git a/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java b/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java index a5d34227ab..e29b6f1c94 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java @@ -15,8 +15,6 @@ */ package org.springframework.data.redis.connection; -import static org.springframework.util.Assert.*; -import static org.springframework.util.Assert.hasText; import static org.springframework.util.StringUtils.*; import java.util.Collections; @@ -46,11 +44,15 @@ public class RedisSentinelConfiguration implements RedisConfiguration, SentinelC private static final String REDIS_SENTINEL_MASTER_CONFIG_PROPERTY = "spring.redis.sentinel.master"; private static final String REDIS_SENTINEL_NODES_CONFIG_PROPERTY = "spring.redis.sentinel.nodes"; + private static final String REDIS_SENTINEL_PASSWORD_CONFIG_PROPERTY = "spring.redis.sentinel.password"; private @Nullable NamedNode master; private Set sentinels; private int database; - private RedisPassword password = RedisPassword.none(); + + private RedisPassword dataNodePassword = RedisPassword.none(); + private RedisPassword sentinelPassword = RedisPassword.none(); + private boolean useDataNodePasswordForSentinel = false; /** * Creates new {@link RedisSentinelConfiguration}. @@ -89,7 +91,7 @@ public RedisSentinelConfiguration(String master, Set sentinelHostAndPort */ public RedisSentinelConfiguration(PropertySource propertySource) { - notNull(propertySource, "PropertySource must not be null!"); + Assert.notNull(propertySource, "PropertySource must not be null!"); this.sentinels = new LinkedHashSet<>(); @@ -101,6 +103,10 @@ public RedisSentinelConfiguration(PropertySource propertySource) { appendSentinels( commaDelimitedListToSet(propertySource.getProperty(REDIS_SENTINEL_NODES_CONFIG_PROPERTY).toString())); } + + if (propertySource.containsProperty(REDIS_SENTINEL_PASSWORD_CONFIG_PROPERTY)) { + this.setSentinelPassword(propertySource.getProperty(REDIS_SENTINEL_PASSWORD_CONFIG_PROPERTY).toString()); + } } /** @@ -110,7 +116,7 @@ public RedisSentinelConfiguration(PropertySource propertySource) { */ public void setSentinels(Iterable sentinels) { - notNull(sentinels, "Cannot set sentinels to 'null'."); + Assert.notNull(sentinels, "Cannot set sentinels to 'null'."); this.sentinels.clear(); @@ -134,7 +140,7 @@ public Set getSentinels() { */ public void addSentinel(RedisNode sentinel) { - notNull(sentinel, "Sentinel must not be 'null'."); + Assert.notNull(sentinel, "Sentinel must not be 'null'."); this.sentinels.add(sentinel); } @@ -144,7 +150,7 @@ public void addSentinel(RedisNode sentinel) { */ public void setMaster(NamedNode master) { - notNull(master, "Sentinel master node must not be 'null'."); + Assert.notNull(master, "Sentinel master node must not be 'null'."); this.master = master; } @@ -230,7 +236,7 @@ public void setDatabase(int index) { */ @Override public RedisPassword getPassword() { - return password; + return dataNodePassword; } /* @@ -242,15 +248,60 @@ public void setPassword(RedisPassword password) { Assert.notNull(password, "RedisPassword must not be null!"); - this.password = password; + this.dataNodePassword = password; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#setSentinelPassword(org.springframework.data.redis.connection.RedisPassword) + */ + public void setSentinelPassword(RedisPassword sentinelPassword) { + + Assert.state(!useDataNodePasswordForSentinel, + "Configuration uses Redis Data Node password for authenticating with Sentinel. Please set 'RedisSentinelConfiguration.useDataNodeAuthenticationForSentinel(false)' before using this option."); + Assert.notNull(sentinelPassword, "SentinelPassword must not be null!"); + this.sentinelPassword = sentinelPassword; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#setSentinelPassword() + */ + @Override + public RedisPassword getSentinelPassword() { + return getUseDataNodeAuthenticationForSentinel() ? this.dataNodePassword : sentinelPassword; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#useDataNodeAuthenticationForSentinel(boolean) + */ + @Override + public void useDataNodeAuthenticationForSentinel(boolean useDataNodeAuthenticationForSentinel) { + + if (useDataNodeAuthenticationForSentinel) { + Assert.state(!this.sentinelPassword.isPresent(), + "Configuration already defines a password for authenticating with Sentinel. Please use 'RedisSentinelConfiguration.setSentinelPassword(RedisPassword.none())' remove the password."); + } + + this.useDataNodePasswordForSentinel = useDataNodeAuthenticationForSentinel; + } + + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#getUseDataNodeAuthenticationForSentinel() + */ + @Override + public boolean getUseDataNodeAuthenticationForSentinel() { + return this.useDataNodePasswordForSentinel; } private RedisNode readHostAndPortFromString(String hostAndPort) { String[] args = split(hostAndPort, ":"); - notNull(args, "HostAndPort need to be seperated by ':'."); - isTrue(args.length == 2, "Host and Port String needs to specified as host:port"); + Assert.notNull(args, "HostAndPort need to be seperated by ':'."); + Assert.isTrue(args.length == 2, "Host and Port String needs to specified as host:port"); return new RedisNode(args[0], Integer.valueOf(args[1]).intValue()); } @@ -261,8 +312,8 @@ private RedisNode readHostAndPortFromString(String hostAndPort) { */ private static Map asMap(String master, Set sentinelHostAndPorts) { - hasText(master, "Master address must not be null or empty!"); - notNull(sentinelHostAndPorts, "SentinelHostAndPorts must not be null!"); + Assert.hasText(master, "Master address must not be null or empty!"); + Assert.notNull(sentinelHostAndPorts, "SentinelHostAndPorts must not be null!"); Map map = new HashMap<>(); map.put(REDIS_SENTINEL_MASTER_CONFIG_PROPERTY, master); diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java index eaa1875e37..07eb48d2f2 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java @@ -1080,7 +1080,6 @@ private RedisURI getSentinelRedisURI() { applyToAll(redisUri, it -> { - getRedisPassword().toOptional().ifPresent(it::setPassword); clientConfiguration.getClientName().ifPresent(it::setClientName); it.setSsl(clientConfiguration.isUseSsl()); diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java index 1a6fb1af6a..b7b8c22cc0 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java @@ -50,6 +50,7 @@ import org.springframework.data.redis.connection.RedisListCommands.Position; import org.springframework.data.redis.connection.RedisNode; import org.springframework.data.redis.connection.RedisNode.NodeType; +import org.springframework.data.redis.connection.RedisPassword; import org.springframework.data.redis.connection.RedisSentinelConfiguration; import org.springframework.data.redis.connection.RedisServer; import org.springframework.data.redis.connection.RedisStringCommands.SetOption; @@ -198,7 +199,8 @@ abstract public class LettuceConverters extends Converters { }; SCORED_VALUE_TO_TUPLE = source -> source != null - ? new DefaultTuple(source.getValue(), Double.valueOf(source.getScore())) : null; + ? new DefaultTuple(source.getValue(), Double.valueOf(source.getScore())) + : null; BYTES_LIST_TO_TUPLE_LIST_CONVERTER = source -> { @@ -297,7 +299,8 @@ private Set parseFlags(Set source) { }; GEO_COORDINATE_TO_POINT_CONVERTER = geoCoordinate -> geoCoordinate != null - ? new Point(geoCoordinate.getX().doubleValue(), geoCoordinate.getY().doubleValue()) : null; + ? new Point(geoCoordinate.getX().doubleValue(), geoCoordinate.getY().doubleValue()) + : null; GEO_COORDINATE_LIST_TO_POINT_LIST_CONVERTER = new ListConverter<>(GEO_COORDINATE_TO_POINT_CONVERTER); KEY_VALUE_UNWRAPPER = source -> source.getValueOrElse(null); @@ -637,15 +640,21 @@ public static RedisURI sentinelConfigurationToRedisURI(RedisSentinelConfiguratio Assert.notNull(sentinelConfiguration, "RedisSentinelConfiguration is required"); Set sentinels = sentinelConfiguration.getSentinels(); - RedisURI.Builder builder = null; + RedisURI.Builder builder = RedisURI.builder(); for (RedisNode sentinel : sentinels) { - if (builder == null) { - builder = RedisURI.Builder.sentinel(sentinel.getHost(), sentinel.getPort(), - sentinelConfiguration.getMaster().getName()); - } else { - builder.withSentinel(sentinel.getHost(), sentinel.getPort()); + RedisURI.Builder uri = RedisURI.Builder.sentinel(sentinel.getHost(), sentinel.getPort(), + sentinelConfiguration.getMaster().getName()); + + if (sentinelConfiguration.getSentinelPassword().isPresent()) { + uri.withPassword(sentinelConfiguration.getSentinelPassword().get()); } + builder.withSentinel(uri.build()); + } + + RedisPassword password = sentinelConfiguration.getPassword(); + if (password.isPresent()) { + builder.withPassword(password.get()); } return builder.build(); diff --git a/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java b/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java index 95aabc3eb9..db2848bab1 100644 --- a/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java @@ -22,7 +22,6 @@ import java.util.HashSet; import org.junit.Test; - import org.springframework.core.env.PropertySource; import org.springframework.mock.env.MockPropertySource; import org.springframework.util.StringUtils; @@ -51,8 +50,7 @@ public void shouldCreateRedisSentinelConfigurationCorrectlyGivenMasterAndSingleH public void shouldCreateRedisSentinelConfigurationCorrectlyGivenMasterAndMultipleHostAndPortStrings() { RedisSentinelConfiguration config = new RedisSentinelConfiguration("mymaster", - new HashSet<>(Arrays.asList( - HOST_AND_PORT_1, HOST_AND_PORT_2, HOST_AND_PORT_3))); + new HashSet<>(Arrays.asList(HOST_AND_PORT_1, HOST_AND_PORT_2, HOST_AND_PORT_3))); assertThat(config.getSentinels().size()).isEqualTo(3); assertThat(config.getSentinels()).contains(new RedisNode("127.0.0.1", 123), new RedisNode("localhost", 456), @@ -121,4 +119,65 @@ public void shouldBeCreatedCorrecltyGivenValidPropertySourceWithMasterAndMultipl assertThat(config.getSentinels()).contains(new RedisNode("127.0.0.1", 123), new RedisNode("localhost", 456), new RedisNode("localhost", 789)); } + + @Test // DATAREDIS-1060 + public void throwsExceptionOnSentinelPasswordAlreadySetWhenTryingToReuseDataNodePassword() { + + RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", + Collections.singleton(HOST_AND_PORT_1)); + configuration.setSentinelPassword(RedisPassword.of("so-secret-you'll-never-guess-123")); + + assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy(() -> configuration.useDataNodeAuthenticationForSentinel(true)); + } + + @Test // DATAREDIS-1060 + public void throwsExceptionOnSettingSentinelPasswordWhenAlreadyReusingDataNodePassword() { + + RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", + Collections.singleton(HOST_AND_PORT_1)); + configuration.setPassword(RedisPassword.of("qwerty")); + configuration.useDataNodeAuthenticationForSentinel(true); + + assertThatExceptionOfType(IllegalStateException.class) + .isThrownBy(() -> configuration.setSentinelPassword(RedisPassword.of("who-needs-security-anyway"))); + } + + @Test // DATAREDIS-1060 + public void settingSentinelPasswordReturnsDataNodePasswordIfUseDataNodeAuthIsTrue() { + + RedisPassword password = RedisPassword.of("monkey-dragon->yeah-getting-better-combining-trivial-ones"); + RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", + Collections.singleton(HOST_AND_PORT_1)); + configuration.setPassword(password); + configuration.useDataNodeAuthenticationForSentinel(true); + + assertThat(configuration.getSentinelPassword()).isEqualTo(password); + } + + @Test // DATAREDIS-1060 + public void dataNodePasswordDoesNotAffectSentinelPassword() { + + RedisPassword password = RedisPassword.of("88888888-8x8-getting-creative-now"); + RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", + Collections.singleton(HOST_AND_PORT_1)); + configuration.setPassword(password); + + assertThat(configuration.getSentinelPassword()).isEqualTo(RedisPassword.none()); + } + + @Test // DATAREDIS-1060 + public void readSentinelPasswordFromConfigProperty() { + + MockPropertySource propertySource = new MockPropertySource(); + propertySource.setProperty("spring.redis.sentinel.master", "myMaster"); + propertySource.setProperty("spring.redis.sentinel.nodes", HOST_AND_PORT_1); + propertySource.setProperty("spring.redis.sentinel.password", "computer-says-no"); + + RedisSentinelConfiguration config = new RedisSentinelConfiguration(propertySource); + + assertThat(config.getSentinelPassword()).isEqualTo(RedisPassword.of("computer-says-no")); + assertThat(config.getSentinels().size()).isEqualTo(1); + assertThat(config.getSentinels()).contains(new RedisNode("127.0.0.1", 123)); + } } diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java index 15fe5a61e9..6e25b8ce04 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java @@ -46,7 +46,6 @@ import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentMatchers; - import org.springframework.beans.DirectFieldAccessor; import org.springframework.beans.factory.DisposableBean; import org.springframework.data.redis.ConnectionFactoryTracker; @@ -179,8 +178,8 @@ public void passwordShouldBeSetCorrectlyOnClusterClient() { } } - @Test // DATAREDIS-524, DATAREDIS-1045 - public void passwordShouldBeSetCorrectlyOnSentinelClient() { + @Test // DATAREDIS-524, DATAREDIS-1045, DATAREDIS-1060 + public void passwordShouldNotBeSetOnSentinelClient() { LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory( new RedisSentinelConfiguration("mymaster", Collections.singleton("host:1234"))); @@ -197,8 +196,78 @@ public void passwordShouldBeSetCorrectlyOnSentinelClient() { assertThat(redisUri.getPassword()).isEqualTo(connectionFactory.getPassword().toCharArray()); for (RedisURI sentinel : redisUri.getSentinels()) { - assertThat(sentinel.getPassword()) - .isEqualTo(connectionFactory.getPassword().toCharArray()); + assertThat(sentinel.getPassword()).isNull(); + } + } + + @Test // DATAREDIS-1060 + public void sentinelPasswordShouldBeSetOnSentinelClient() { + + RedisSentinelConfiguration config = new RedisSentinelConfiguration("mymaster", Collections.singleton("host:1234")); + config.setSentinelPassword("sentinel-pwd"); + + LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(config); + connectionFactory.setClientResources(getSharedClientResources()); + connectionFactory.setPassword("o_O"); + connectionFactory.afterPropertiesSet(); + ConnectionFactoryTracker.add(connectionFactory); + + AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client"); + assertThat(client).isInstanceOf(RedisClient.class); + + RedisURI redisUri = (RedisURI) getField(client, "redisURI"); + + assertThat(redisUri.getPassword()).isEqualTo(connectionFactory.getPassword().toCharArray()); + + for (RedisURI sentinel : redisUri.getSentinels()) { + assertThat(sentinel.getPassword()).isEqualTo("sentinel-pwd".toCharArray()); + } + } + + @Test // DATAREDIS-1060 + public void redisPasswordShouldBeSetOnSentinelClientIfItShouldBeReused() { + + RedisSentinelConfiguration config = new RedisSentinelConfiguration("mymaster", Collections.singleton("host:1234")); + config.useDataNodeAuthenticationForSentinel(true); + + LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(config); + connectionFactory.setClientResources(getSharedClientResources()); + connectionFactory.setPassword("o_O"); + connectionFactory.afterPropertiesSet(); + ConnectionFactoryTracker.add(connectionFactory); + + AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client"); + assertThat(client).isInstanceOf(RedisClient.class); + + RedisURI redisUri = (RedisURI) getField(client, "redisURI"); + + assertThat(redisUri.getPassword()).isEqualTo(connectionFactory.getPassword().toCharArray()); + + for (RedisURI sentinel : redisUri.getSentinels()) { + assertThat(sentinel.getPassword()).isEqualTo("o_O".toCharArray()); + } + } + + @Test // DATAREDIS-1060 + public void sentinelPasswordShouldNotLeakIntoDataNodeClient() { + + RedisSentinelConfiguration config = new RedisSentinelConfiguration("mymaster", Collections.singleton("host:1234")); + config.setSentinelPassword("sentinel-pwd"); + + LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(config); + connectionFactory.setClientResources(getSharedClientResources()); + connectionFactory.afterPropertiesSet(); + ConnectionFactoryTracker.add(connectionFactory); + + AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client"); + assertThat(client).isInstanceOf(RedisClient.class); + + RedisURI redisUri = (RedisURI) getField(client, "redisURI"); + + assertThat(redisUri.getPassword()).isNull(); + + for (RedisURI sentinel : redisUri.getSentinels()) { + assertThat(sentinel.getPassword()).isEqualTo("sentinel-pwd".toCharArray()); } } From e27783d5a11b270a66ddcbf248b583bbace3b45c Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 12 Nov 2019 15:21:24 +0100 Subject: [PATCH 3/3] DATAREDIS-1060 - Polishing. Remove data node password reuse for sentinel as we favor explicit configuration over implicit. Use RedisURI builder and not sentinel URI builder to configure the SentinelURI correctly. Associate masterId with outermost RedisURI. --- .../redis/connection/RedisConfiguration.java | 43 +++++-------------- .../RedisSentinelConfiguration.java | 33 ++------------ .../connection/lettuce/LettuceConverters.java | 12 +++--- .../RedisSentinelConfigurationUnitTests.java | 38 +--------------- .../LettuceConnectionFactoryUnitTests.java | 24 ----------- 5 files changed, 21 insertions(+), 129 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java b/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java index e04f118abe..90a8a27121 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java @@ -339,20 +339,20 @@ default void setMaster(final String name) { Set getSentinels(); /** - * Get the {@link RedisPassword} used when authenticating with a Redis Server. - * + * Get the {@link RedisPassword} used when authenticating with a Redis Server.. + * * @return never {@literal null}. + * @since 2.2.2 */ default RedisPassword getDataNodePassword() { return getPassword(); } /** - * Create and set a {@link RedisPassword} to be used when authenticating with Sentinel from the given {@link String} + * Create and set a {@link RedisPassword} to be used when authenticating with Redis Sentinel from the given + * {@link String}. * * @param password can be {@literal null}. - * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} - * should be used for authenticating with Redis Sentinel. * @since 2.2.2 */ default void setSentinelPassword(@Nullable String password) { @@ -360,12 +360,10 @@ default void setSentinelPassword(@Nullable String password) { } /** - * Create and set a {@link RedisPassword} to be used when authenticating with Sentinel from the given + * Create and set a {@link RedisPassword} to be used when authenticating with Redis Sentinel from the given * {@link Character} sequence. * * @param password can be {@literal null}. - * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} - * should be used for authenticating with Redis Sentinel. * @since 2.2.2 */ default void setSentinelPassword(@Nullable char[] password) { @@ -373,43 +371,22 @@ default void setSentinelPassword(@Nullable char[] password) { } /** - * Set a {@link RedisPassword} to be used when authenticating with Sentinel. + * Set a {@link RedisPassword} to be used when authenticating with Redis Sentinel. * * @param password must not be {@literal null} use {@link RedisPassword#none()} instead. - * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} - * should be used for authenticating with Redis Sentinel. * @since 2.2.2 */ void setSentinelPassword(RedisPassword password); /** - * Get the {@link RedisPassword} to use when connecting to a Redis Sentinel.
- * This can be the password explicitly set via {@link #setSentinelPassword(RedisPassword)}, or the - * {@link #getDataNodePassword() Data Node password} if it should be also used for - * {@link #getUseDataNodeAuthenticationForSentinel() sentinel}, or {@link RedisPassword#none()} if no password has + * Returns the {@link RedisPassword} to use when connecting to a Redis Sentinel.
+ * Can be set via {@link #setSentinelPassword(RedisPassword)} or {@link RedisPassword#none()} if no password has * been set. * - * @return the {@link RedisPassword} for authenticating with Sentinel. + * @return the {@link RedisPassword} for authenticating with Redis Sentinel. * @since 2.2.2 */ RedisPassword getSentinelPassword(); - - /** - * Use the {@link #getDataNodePassword() RedisPassword} also for authentication with Redis Sentinel. - * - * @param useDataNodeAuthenticationForSentinel - * @throws IllegalStateException if a {@link #getSentinelPassword() Sentinel Password} is already set. - * @since 2.2.2 - */ - void useDataNodeAuthenticationForSentinel(boolean useDataNodeAuthenticationForSentinel); - - /** - * Use the {@link #getDataNodePassword() RedisPassword} also for authentication with Redis Sentinel. - * - * @return - * @since 2.2.2 - */ - boolean getUseDataNodeAuthenticationForSentinel(); } /** diff --git a/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java b/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java index e29b6f1c94..a28b4d2129 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java @@ -52,7 +52,6 @@ public class RedisSentinelConfiguration implements RedisConfiguration, SentinelC private RedisPassword dataNodePassword = RedisPassword.none(); private RedisPassword sentinelPassword = RedisPassword.none(); - private boolean useDataNodePasswordForSentinel = false; /** * Creates new {@link RedisSentinelConfiguration}. @@ -253,47 +252,21 @@ public void setPassword(RedisPassword password) { /* * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#setSentinelPassword(org.springframework.data.redis.connection.RedisPassword) + * @see org.springframework.data.redis.connection.RedisConfiguration.SentinelConfiguration#setSentinelPassword(org.springframework.data.redis.connection.RedisPassword) */ public void setSentinelPassword(RedisPassword sentinelPassword) { - Assert.state(!useDataNodePasswordForSentinel, - "Configuration uses Redis Data Node password for authenticating with Sentinel. Please set 'RedisSentinelConfiguration.useDataNodeAuthenticationForSentinel(false)' before using this option."); Assert.notNull(sentinelPassword, "SentinelPassword must not be null!"); this.sentinelPassword = sentinelPassword; } /* * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#setSentinelPassword() + * @see org.springframework.data.redis.connection.RedisConfiguration.SentinelConfiguration#setSentinelPassword() */ @Override public RedisPassword getSentinelPassword() { - return getUseDataNodeAuthenticationForSentinel() ? this.dataNodePassword : sentinelPassword; - } - - /* - * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#useDataNodeAuthenticationForSentinel(boolean) - */ - @Override - public void useDataNodeAuthenticationForSentinel(boolean useDataNodeAuthenticationForSentinel) { - - if (useDataNodeAuthenticationForSentinel) { - Assert.state(!this.sentinelPassword.isPresent(), - "Configuration already defines a password for authenticating with Sentinel. Please use 'RedisSentinelConfiguration.setSentinelPassword(RedisPassword.none())' remove the password."); - } - - this.useDataNodePasswordForSentinel = useDataNodeAuthenticationForSentinel; - } - - /* - * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#getUseDataNodeAuthenticationForSentinel() - */ - @Override - public boolean getUseDataNodeAuthenticationForSentinel() { - return this.useDataNodePasswordForSentinel; + return sentinelPassword; } private RedisNode readHostAndPortFromString(String hostAndPort) { diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java index b7b8c22cc0..7ff1a37b2b 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java @@ -640,16 +640,16 @@ public static RedisURI sentinelConfigurationToRedisURI(RedisSentinelConfiguratio Assert.notNull(sentinelConfiguration, "RedisSentinelConfiguration is required"); Set sentinels = sentinelConfiguration.getSentinels(); + RedisPassword sentinelPassword = sentinelConfiguration.getSentinelPassword(); RedisURI.Builder builder = RedisURI.builder(); for (RedisNode sentinel : sentinels) { - RedisURI.Builder uri = RedisURI.Builder.sentinel(sentinel.getHost(), sentinel.getPort(), - sentinelConfiguration.getMaster().getName()); + RedisURI.Builder sentinelBuilder = RedisURI.Builder.redis(sentinel.getHost(), sentinel.getPort()); - if (sentinelConfiguration.getSentinelPassword().isPresent()) { - uri.withPassword(sentinelConfiguration.getSentinelPassword().get()); + if (sentinelPassword.isPresent()) { + sentinelBuilder.withPassword(sentinelPassword.get()); } - builder.withSentinel(uri.build()); + builder.withSentinel(sentinelBuilder.build()); } RedisPassword password = sentinelConfiguration.getPassword(); @@ -657,6 +657,8 @@ public static RedisURI sentinelConfigurationToRedisURI(RedisSentinelConfiguratio builder.withPassword(password.get()); } + builder.withSentinelMasterId(sentinelConfiguration.getMaster().getName()); + return builder.build(); } diff --git a/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java b/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java index db2848bab1..35100ea25d 100644 --- a/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java @@ -120,41 +120,6 @@ public void shouldBeCreatedCorrecltyGivenValidPropertySourceWithMasterAndMultipl new RedisNode("localhost", 789)); } - @Test // DATAREDIS-1060 - public void throwsExceptionOnSentinelPasswordAlreadySetWhenTryingToReuseDataNodePassword() { - - RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", - Collections.singleton(HOST_AND_PORT_1)); - configuration.setSentinelPassword(RedisPassword.of("so-secret-you'll-never-guess-123")); - - assertThatExceptionOfType(IllegalStateException.class) - .isThrownBy(() -> configuration.useDataNodeAuthenticationForSentinel(true)); - } - - @Test // DATAREDIS-1060 - public void throwsExceptionOnSettingSentinelPasswordWhenAlreadyReusingDataNodePassword() { - - RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", - Collections.singleton(HOST_AND_PORT_1)); - configuration.setPassword(RedisPassword.of("qwerty")); - configuration.useDataNodeAuthenticationForSentinel(true); - - assertThatExceptionOfType(IllegalStateException.class) - .isThrownBy(() -> configuration.setSentinelPassword(RedisPassword.of("who-needs-security-anyway"))); - } - - @Test // DATAREDIS-1060 - public void settingSentinelPasswordReturnsDataNodePasswordIfUseDataNodeAuthIsTrue() { - - RedisPassword password = RedisPassword.of("monkey-dragon->yeah-getting-better-combining-trivial-ones"); - RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", - Collections.singleton(HOST_AND_PORT_1)); - configuration.setPassword(password); - configuration.useDataNodeAuthenticationForSentinel(true); - - assertThat(configuration.getSentinelPassword()).isEqualTo(password); - } - @Test // DATAREDIS-1060 public void dataNodePasswordDoesNotAffectSentinelPassword() { @@ -177,7 +142,6 @@ public void readSentinelPasswordFromConfigProperty() { RedisSentinelConfiguration config = new RedisSentinelConfiguration(propertySource); assertThat(config.getSentinelPassword()).isEqualTo(RedisPassword.of("computer-says-no")); - assertThat(config.getSentinels().size()).isEqualTo(1); - assertThat(config.getSentinels()).contains(new RedisNode("127.0.0.1", 123)); + assertThat(config.getSentinels()).hasSize(1).contains(new RedisNode("127.0.0.1", 123)); } } diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java index 6e25b8ce04..04e7ef0cae 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java @@ -224,30 +224,6 @@ public void sentinelPasswordShouldBeSetOnSentinelClient() { } } - @Test // DATAREDIS-1060 - public void redisPasswordShouldBeSetOnSentinelClientIfItShouldBeReused() { - - RedisSentinelConfiguration config = new RedisSentinelConfiguration("mymaster", Collections.singleton("host:1234")); - config.useDataNodeAuthenticationForSentinel(true); - - LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(config); - connectionFactory.setClientResources(getSharedClientResources()); - connectionFactory.setPassword("o_O"); - connectionFactory.afterPropertiesSet(); - ConnectionFactoryTracker.add(connectionFactory); - - AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client"); - assertThat(client).isInstanceOf(RedisClient.class); - - RedisURI redisUri = (RedisURI) getField(client, "redisURI"); - - assertThat(redisUri.getPassword()).isEqualTo(connectionFactory.getPassword().toCharArray()); - - for (RedisURI sentinel : redisUri.getSentinels()) { - assertThat(sentinel.getPassword()).isEqualTo("o_O".toCharArray()); - } - } - @Test // DATAREDIS-1060 public void sentinelPasswordShouldNotLeakIntoDataNodeClient() {