Skip to content

Commit ccb7f72

Browse files
zoewanggzoewangg
committed
Correctly select the cipher suites based on the HTTP protocol and use SystemPropertyTlsKeyManagersProvider if no KeyManger is provided.
1 parent 34bcb56 commit ccb7f72

File tree

6 files changed

+152
-23
lines changed

6 files changed

+152
-23
lines changed

‎.changes/next-release/bugfix-NettyNIOHTTPClient-dd43cf4.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"category": "Netty NIO HTTP Client",
3+
"contributor": "",
4+
"type": "bugfix",
5+
"description": "Use `SystemPropretyTlsKeyManagersProvider` if no `KeyManger` is provided."
6+
}

‎.changes/next-release/feature-NettyNIOHTTPClient-e7d2844.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"category": "Netty NIO HTTP Client",
3+
"contributor": "",
4+
"type": "bugfix",
5+
"description": "Correctly select the cipher suites based on the HTTP protocol. See [#2159](https://github.com/aws/aws-sdk-java-v2/issues/2159)"
6+
}

‎http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/AwaitCloseChannelPoolMap.java

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,12 @@
2121
import io.netty.channel.Channel;
2222
import io.netty.channel.pool.ChannelPool;
2323
import io.netty.channel.pool.ChannelPoolHandler;
24-
import io.netty.handler.codec.http2.Http2SecurityUtil;
2524
import io.netty.handler.ssl.SslContext;
26-
import io.netty.handler.ssl.SslContextBuilder;
2725
import io.netty.handler.ssl.SslProvider;
28-
import io.netty.handler.ssl.SupportedCipherSuiteFilter;
29-
import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
3026
import java.net.URI;
3127
import java.net.URISyntaxException;
3228
import java.time.Duration;
3329
import java.util.Collection;
34-
import java.util.List;
3530
import java.util.Map;
3631
import java.util.concurrent.CompletableFuture;
3732
import java.util.concurrent.ConcurrentHashMap;
@@ -40,18 +35,13 @@
4035
import java.util.concurrent.TimeoutException;
4136
import java.util.concurrent.atomic.AtomicReference;
4237
import java.util.function.Function;
43-
import javax.net.ssl.KeyManager;
44-
import javax.net.ssl.KeyManagerFactory;
45-
import javax.net.ssl.SSLException;
46-
import javax.net.ssl.TrustManagerFactory;
4738
import software.amazon.awssdk.annotations.SdkInternalApi;
4839
import software.amazon.awssdk.annotations.SdkTestInternalApi;
4940
import software.amazon.awssdk.http.Protocol;
5041
import software.amazon.awssdk.http.nio.netty.ProxyConfiguration;
5142
import software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup;
5243
import software.amazon.awssdk.http.nio.netty.internal.http2.HttpOrHttp2ChannelPool;
5344
import software.amazon.awssdk.utils.Logger;
54-
import software.amazon.awssdk.utils.Validate;
5545

5646
/**
5747
* Implementation of {@link SdkChannelPoolMap} that awaits channel pools to be closed upon closing.
@@ -126,10 +116,7 @@ public static Builder builder() {
126116

127117
@Override
128118
protected SimpleChannelPoolAwareChannelPool newPool(URI key) {
129-
SslContext sslContext = null;
130-
if (needSslContext(key)) {
131-
sslContext = sslContextProvider.sslContext();
132-
}
119+
SslContext sslContext = needSslContext(key) ? sslContextProvider.sslContext() : null;
133120

134121
Bootstrap bootstrap = createBootstrap(key);
135122

‎http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/SslContextProvider.java

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,15 @@
2121
import io.netty.handler.ssl.SslProvider;
2222
import io.netty.handler.ssl.SupportedCipherSuiteFilter;
2323
import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
24+
import java.util.List;
2425
import javax.net.ssl.KeyManager;
2526
import javax.net.ssl.KeyManagerFactory;
2627
import javax.net.ssl.SSLException;
2728
import javax.net.ssl.TrustManagerFactory;
2829
import software.amazon.awssdk.annotations.SdkInternalApi;
2930
import software.amazon.awssdk.http.Protocol;
31+
import software.amazon.awssdk.http.SystemPropertyTlsKeyManagersProvider;
32+
import software.amazon.awssdk.http.TlsTrustManagersProvider;
3033
import software.amazon.awssdk.utils.Logger;
3134
import software.amazon.awssdk.utils.Validate;
3235

@@ -49,7 +52,7 @@ public SslContext sslContext() {
4952
try {
5053
return SslContextBuilder.forClient()
5154
.sslProvider(sslProvider)
52-
.ciphers(Http2SecurityUtil.CIPHERS, SupportedCipherSuiteFilter.INSTANCE)
55+
.ciphers(getCiphers(), SupportedCipherSuiteFilter.INSTANCE)
5356
.trustManager(trustManagerFactory)
5457
.keyManager(keyManagerFactory)
5558
.build();
@@ -58,12 +61,25 @@ public SslContext sslContext() {
5861
}
5962
}
6063

64+
/**
65+
* HTTP/2: per Rfc7540, there is a blocked list of cipher suites for HTTP/2, so setting
66+
* the recommended cipher suites directly here
67+
*
68+
* HTTP/1.1: return null so that the default ciphers suites will be used
69+
* https://github.com/netty/netty/blob/0dc246eb129796313b58c1dbdd674aa289f72cad/handler/src/main/java/io/netty/handler/ssl
70+
* /SslUtils.java
71+
*/
72+
private List<String> getCiphers() {
73+
return protocol.equals(Protocol.HTTP2) ? Http2SecurityUtil.CIPHERS : null;
74+
}
75+
6176
private TrustManagerFactory getTrustManager(NettyConfiguration configuration) {
62-
Validate.isTrue(configuration.tlsTrustManagersProvider() == null || !configuration.trustAllCertificates(),
77+
TlsTrustManagersProvider tlsTrustManagersProvider = configuration.tlsTrustManagersProvider();
78+
Validate.isTrue(tlsTrustManagersProvider == null || !configuration.trustAllCertificates(),
6379
"A TlsTrustManagerProvider can't be provided if TrustAllCertificates is also set");
6480

65-
if (configuration.tlsTrustManagersProvider() != null) {
66-
return StaticTrustManagerFactory.create(configuration.tlsTrustManagersProvider().trustManagers());
81+
if (tlsTrustManagersProvider != null) {
82+
return StaticTrustManagerFactory.create(tlsTrustManagersProvider.trustManagers());
6783
}
6884

6985
if (configuration.trustAllCertificates()) {
@@ -72,6 +88,7 @@ private TrustManagerFactory getTrustManager(NettyConfiguration configuration) {
7288
return InsecureTrustManagerFactory.INSTANCE;
7389
}
7490

91+
// return null so that the system default trust manager will be used
7592
return null;
7693
}
7794

@@ -82,6 +99,8 @@ private KeyManagerFactory getKeyManager(NettyConfiguration configuration) {
8299
return StaticKeyManagerFactory.create(keyManagers);
83100
}
84101
}
85-
return null;
102+
103+
KeyManager[] systemPropertyKeyManagers = SystemPropertyTlsKeyManagersProvider.create().keyManagers();
104+
return systemPropertyKeyManagers == null ? null : StaticKeyManagerFactory.create(systemPropertyKeyManagers);
86105
}
87106
}

‎http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/AwaitCloseChannelPoolMapTest.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,11 @@ public void usesProvidedKeyManagersProvider() {
224224
.build();
225225

226226
channelPoolMap = AwaitCloseChannelPoolMap.builder()
227-
.sdkChannelOptions(new SdkChannelOptions())
228-
.sdkEventLoopGroup(SdkEventLoopGroup.builder().build())
229-
.configuration(new NettyConfiguration(config.merge(GLOBAL_HTTP_DEFAULTS)))
230-
.build();
227+
.sdkChannelOptions(new SdkChannelOptions())
228+
.sdkEventLoopGroup(SdkEventLoopGroup.builder().build())
229+
.protocol(Protocol.HTTP1_1)
230+
.configuration(new NettyConfiguration(config.merge(GLOBAL_HTTP_DEFAULTS)))
231+
.build();
231232

232233
ChannelPool channelPool = channelPoolMap.newPool(URI.create("https://localhost:" + mockProxy.port()));
233234
channelPool.acquire().awaitUninterruptibly();

‎http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/SslContextProviderTest.java

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.http.nio.netty.internal;
17+
18+
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
20+
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TLS_KEY_MANAGERS_PROVIDER;
21+
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TLS_TRUST_MANAGERS_PROVIDER;
22+
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES;
23+
24+
import io.netty.handler.codec.http2.Http2SecurityUtil;
25+
import io.netty.handler.ssl.SslProvider;
26+
import javax.net.ssl.TrustManager;
27+
import org.junit.Test;
28+
import org.mockito.Mockito;
29+
import software.amazon.awssdk.http.Protocol;
30+
import software.amazon.awssdk.http.SdkHttpConfigurationOption;
31+
import software.amazon.awssdk.http.TlsKeyManagersProvider;
32+
import software.amazon.awssdk.http.TlsTrustManagersProvider;
33+
import software.amazon.awssdk.utils.AttributeMap;
34+
35+
public class SslContextProviderTest {
36+
37+
@Test
38+
public void sslContext_h2WithJdk_h2CiphersShouldBeUsed() {
39+
SslContextProvider sslContextProvider = new SslContextProvider(new NettyConfiguration(SdkHttpConfigurationOption.GLOBAL_HTTP_DEFAULTS),
40+
Protocol.HTTP2,
41+
SslProvider.JDK);
42+
43+
assertThat(sslContextProvider.sslContext().cipherSuites()).isSubsetOf(Http2SecurityUtil.CIPHERS);
44+
}
45+
46+
@Test
47+
public void sslContext_h2WithOpenSsl_h2CiphersShouldBeUsed() {
48+
SslContextProvider sslContextProvider = new SslContextProvider(new NettyConfiguration(SdkHttpConfigurationOption.GLOBAL_HTTP_DEFAULTS),
49+
Protocol.HTTP2,
50+
SslProvider.OPENSSL);
51+
52+
assertThat(sslContextProvider.sslContext().cipherSuites()).isSubsetOf(Http2SecurityUtil.CIPHERS);
53+
}
54+
55+
@Test
56+
public void sslContext_h1_defaultCipherShouldBeUsed() {
57+
SslContextProvider sslContextProvider = new SslContextProvider(new NettyConfiguration(SdkHttpConfigurationOption.GLOBAL_HTTP_DEFAULTS),
58+
Protocol.HTTP1_1,
59+
SslProvider.JDK);
60+
61+
assertThat(sslContextProvider.sslContext().cipherSuites()).isNotIn(Http2SecurityUtil.CIPHERS);
62+
}
63+
64+
@Test
65+
public void customizedKeyManagerPresent_shouldUseCustomized() {
66+
TlsKeyManagersProvider mockProvider = Mockito.mock(TlsKeyManagersProvider.class);
67+
SslContextProvider sslContextProvider = new SslContextProvider(new NettyConfiguration(AttributeMap.builder()
68+
.put(TRUST_ALL_CERTIFICATES, false)
69+
.put(TLS_KEY_MANAGERS_PROVIDER, mockProvider)
70+
.build()),
71+
Protocol.HTTP1_1,
72+
SslProvider.JDK);
73+
74+
sslContextProvider.sslContext();
75+
Mockito.verify(mockProvider).keyManagers();
76+
}
77+
78+
@Test
79+
public void customizedTrustManagerPresent_shouldUseCustomized() {
80+
TlsTrustManagersProvider mockProvider = Mockito.mock(TlsTrustManagersProvider.class);
81+
TrustManager mockTrustManager = Mockito.mock(TrustManager.class);
82+
Mockito.when(mockProvider.trustManagers()).thenReturn(new TrustManager[] {mockTrustManager});
83+
SslContextProvider sslContextProvider = new SslContextProvider(new NettyConfiguration(AttributeMap.builder()
84+
.put(TRUST_ALL_CERTIFICATES, false)
85+
.put(TLS_TRUST_MANAGERS_PROVIDER, mockProvider)
86+
.build()),
87+
Protocol.HTTP1_1,
88+
SslProvider.JDK);
89+
90+
sslContextProvider.sslContext();
91+
Mockito.verify(mockProvider).trustManagers();
92+
}
93+
94+
@Test
95+
public void TlsTrustManagerAndTrustAllCertificates_shouldThrowException() {
96+
TlsTrustManagersProvider mockProvider = Mockito.mock(TlsTrustManagersProvider.class);
97+
assertThatThrownBy(() -> new SslContextProvider(new NettyConfiguration(AttributeMap.builder()
98+
.put(TRUST_ALL_CERTIFICATES, true)
99+
.put(TLS_TRUST_MANAGERS_PROVIDER,
100+
mockProvider)
101+
.build()),
102+
Protocol.HTTP1_1,
103+
SslProvider.JDK)).isInstanceOf(IllegalArgumentException.class)
104+
.hasMessageContaining("A TlsTrustManagerProvider can't"
105+
+ " be provided if "
106+
+ "TrustAllCertificates is also "
107+
+ "set");
108+
109+
}
110+
}

0 commit comments

Comments
 (0)