pivotal-jbarrett commented on a change in pull request #7449: URL: https://github.com/apache/geode/pull/7449#discussion_r836751929
########## File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java ########## @@ -70,28 +77,74 @@ @RunWith(GeodeParamsRunner.class) public class P2PMessagingConcurrencyDUnitTest { - // how many messages will each sender generate? - private static final int MESSAGES_PER_SENDER = 1_000; + // how many sending member JVMs + private static final int SENDERS = 1; - // number of concurrent (sending) tasks to run - private static final int SENDER_COUNT = 10; + // number of concurrent (sending) tasks to run in each sending JVM + private static final int TASKS_PER_SENDER = 10; - // (exclusive) upper bound of random message size, in bytes - private static final int LARGEST_MESSAGE_BOUND = 32 * 1024 + 2; // 32KiB + 2 + // how many messages will each sending task generate? + private static final int MESSAGES_PER_SENDING_TASK = 1_000; + + /* + * Upper bound (exclusive) of random message size, in bytes. + * The magnitude is chosen to give us a mix of messages around + * 16KB. I want a number that results in a largest message that + * is not a power of two since I believe, for no particular reason, + * that it might catch more bugs than a power of two would, + * so I add 2. + */ + private static final int LARGEST_MESSAGE_BOUND = 32 * 1024 + 2; + + /* + * Non-random payload content makes debugging easier when a NULL cipher is used. + * Null ciphers are not allowed on TLSv1.3 though. + */ + private static boolean RANDOMIZE_PAYLOAD_CONTENT = true; + + private static final byte[] NON_RANDOM_PAYLOAD_PATTERN = + "EVERYGOODBOYDOESFINE".getBytes(StandardCharsets.UTF_8); // random seed private static final int RANDOM_SEED = 1234; + /* + * On TLSv1.3 using a GCM-based cipher, a KeyUpdate TLS message will be sent + * after a preconfigured number of bytes have been encrypted by an SSLEngine. + * Setting this to true causes generation of KeyUpdate TLS messages. + * See NioSslEngineKeyUpdateTest for more details. + * + * Leave this turned off for CI since ENCRYPTED_BYTES_LIMIT is + * performance-sensitive. Turning it on in CI can make this test flaky. + */ + private static final boolean TEST_WITH_TLS_KEY_UPDATE_MESSAGE_PROCESSING = false; Review comment: I fail to see how this can't be made deterministic. Having a test that we have to manually tune and run isn't all that useful. ########## File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java ########## @@ -70,28 +77,74 @@ @RunWith(GeodeParamsRunner.class) public class P2PMessagingConcurrencyDUnitTest { - // how many messages will each sender generate? - private static final int MESSAGES_PER_SENDER = 1_000; + // how many sending member JVMs + private static final int SENDERS = 1; - // number of concurrent (sending) tasks to run - private static final int SENDER_COUNT = 10; + // number of concurrent (sending) tasks to run in each sending JVM + private static final int TASKS_PER_SENDER = 10; - // (exclusive) upper bound of random message size, in bytes - private static final int LARGEST_MESSAGE_BOUND = 32 * 1024 + 2; // 32KiB + 2 + // how many messages will each sending task generate? + private static final int MESSAGES_PER_SENDING_TASK = 1_000; + + /* + * Upper bound (exclusive) of random message size, in bytes. + * The magnitude is chosen to give us a mix of messages around + * 16KB. I want a number that results in a largest message that + * is not a power of two since I believe, for no particular reason, + * that it might catch more bugs than a power of two would, + * so I add 2. + */ + private static final int LARGEST_MESSAGE_BOUND = 32 * 1024 + 2; + + /* + * Non-random payload content makes debugging easier when a NULL cipher is used. + * Null ciphers are not allowed on TLSv1.3 though. + */ + private static boolean RANDOMIZE_PAYLOAD_CONTENT = true; Review comment: What is the point of this flag? Should this not be made into a separate test or test parameter? ########## File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java ########## @@ -248,13 +338,18 @@ private static ClusterDistributionManager getCDM() { * Left the field here in case it comes in handy later. */ private volatile int messageId; - private final Random random; - - TestMessage(final InternalDistributedMember receiver, - final Random random, final int messageId) { + private volatile Random random; Review comment: Use `ThreadLocalRandom` for multithreaded `Random` access. ########## File path: geode-core/src/integrationTest/java/org/apache/geode/internal/net/NioSslEngineKeyUpdateTest.java ########## @@ -0,0 +1,459 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.internal.net; + +import static java.util.concurrent.CompletableFuture.supplyAsync; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_PASSWORD; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import java.io.FileInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.nio.ByteBuffer; +import java.nio.channels.ServerSocketChannel; +import java.nio.channels.SocketChannel; +import java.security.GeneralSecurityException; +import java.security.KeyManagementException; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.security.Security; +import java.security.UnrecoverableKeyException; +import java.util.Arrays; +import java.util.Properties; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.TrustManagerFactory; + +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.geode.cache.ssl.CertStores; +import org.apache.geode.cache.ssl.CertificateBuilder; +import org.apache.geode.cache.ssl.CertificateMaterial; +import org.apache.geode.distributed.internal.DMStats; + +/** + * In TLSv1.3, when a GCM-based cipher is used, there is a limit on the number + * of bytes that may be encoded with a key. When that limit is reached, a TLS + * KeyUpdate message is generated (by the SSLEngine). That message causes a new + * key to be negotiated between the peer SSLEngines. + * + * Geode's {@link NioSslEngine} class (subclass of {@link NioFilter}) encapsulates + * Java's {@link SSLEngine}. Geode's MsgStreamer, Connection, and ConnectionTable + * classes interact with NioSslEngine to accomplish peer-to-peer (P2P) messaging. + * + * This test constructs a pair of SSLEngine's and wraps them in NioSslEngine. + * Rather than relying on MsgStreamer, Connection, or ConnectionTable classes + * (which themselves have a lot of dependencies on other classes), this test + * implements simplified logic for driving the sending and receiving of data, + * i.e. calling NioSslEngine.wrap() and NioSslEngine.unwrap(). See + * {@link #send(int, NioSslEngine, SocketChannel, int)} and + * {@link #receive(int, NioSslEngine, SocketChannel, ByteBuffer)}. + * + * The {@link #secureDataTransferTest()} arranges for the encrypted bytes limit + * to be reached very quickly (see {@link #ENCRYPTED_BYTES_LIMIT}). + * The test verifies data transfer continues correctly, after the limit is reached. + * This indirectly verifies that the KeyUpdate protocol initiated by the sending + * SSLEngine is correctly handled by all the components involved. + */ +public class NioSslEngineKeyUpdateTest { + + private static final String TLS_PROTOCOL = "TLSv1.3"; + private static final String TLS_CIPHER_SUITE = "TLS_AES_256_GCM_SHA384"; + + // number of bytes the GCM cipher can encrypt before initiating a KeyUpdate + private static final int ENCRYPTED_BYTES_LIMIT = 1; + + /* + * KeyUpdate messages don't happen in the handshake, they happen afterward. + * This is the number of bytes we'll transfer after the TLS handshake. + */ + private static final int BYTES_TO_TRANSFER_AFTER_HANDSHAKE = 2; + + { + assert IsPowerOfTwo(ENCRYPTED_BYTES_LIMIT) && ENCRYPTED_BYTES_LIMIT != 0; + + // The bytes will be sent using two calls to wrap so the number must be even + assert BYTES_TO_TRANSFER_AFTER_HANDSHAKE % 2 == 0; + + assert BYTES_TO_TRANSFER_AFTER_HANDSHAKE > ENCRYPTED_BYTES_LIMIT; + + Security.setProperty("jdk.tls.keyLimits", + "AES/GCM/NoPadding KeyUpdate " + ENCRYPTED_BYTES_LIMIT); + } + + private static BufferPool bufferPool; + private static SSLContext sslContext; + private static KeyStore keystore; + private static char[] keystorePassword; + private static KeyStore truststore; + + private SSLEngine clientEngine; + private SSLEngine serverEngine; + private int packetBufferSize; + + @BeforeClass + public static void beforeClass() throws GeneralSecurityException, IOException { + DMStats mockStats = mock(DMStats.class); + bufferPool = new BufferPool(mockStats); + + final Properties securityProperties = createKeystoreAndTruststore(); + + keystore = KeyStore.getInstance("JKS"); + keystorePassword = securityProperties.getProperty(SSL_KEYSTORE_PASSWORD).toCharArray(); + keystore.load(new FileInputStream(securityProperties.getProperty(SSL_KEYSTORE)), + keystorePassword); + + truststore = KeyStore.getInstance("JKS"); + final char[] truststorePassword = + securityProperties.getProperty(SSL_TRUSTSTORE_PASSWORD).toCharArray(); + truststore.load(new FileInputStream(securityProperties.getProperty(SSL_TRUSTSTORE)), + truststorePassword); + } + + @Before + public void before() throws NoSuchAlgorithmException, UnrecoverableKeyException, + KeyStoreException, KeyManagementException { + final KeyManagerFactory kmf = KeyManagerFactory.getInstance("PKIX"); + kmf.init(keystore, keystorePassword); + final TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); + tmf.init(truststore); + + sslContext = SSLContext.getInstance("TLS"); + final SecureRandom random = new SecureRandom(new byte[] {1, 2, 3}); + sslContext.init(kmf.getKeyManagers(), tmf.getTrustManagers(), random); + + final SSLParameters defaultParameters = sslContext.getDefaultSSLParameters(); + final String[] protocols = defaultParameters.getProtocols(); + final String[] cipherSuites = defaultParameters.getCipherSuites(); + System.out.println( + String.format("TLS settings (default) before handshake: Protocols: %s, Cipher Suites: %s", + Arrays.toString(protocols), Arrays.toString(cipherSuites))); + + clientEngine = createSSLEngine("server-host", true, sslContext); + packetBufferSize = clientEngine.getSession().getPacketBufferSize(); + + serverEngine = createSSLEngine("client-host", false, sslContext); + } + + @Test + public void handshakeTest() { + clientServerTest( + (channel, filter, peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Client:"); + return true; + }, + (channel, filter, peerNetData1) -> { + handshakeTLS(channel, filter, peerNetData1, + "Server:"); + return true; + }); + } + + @Test + public void secureDataTransferTest() { + clientServerTest( + (final SocketChannel channel, + final NioSslEngine filter, + final ByteBuffer peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Client:"); + /* + * if we call send() only once, it seems that jdk.tls.keyLimits + * is not evaluated by wrap(). Calling it twice seems to fix that. + */ + // send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE, filter, channel); Review comment: Delete commented code. ########## File path: geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java ########## @@ -248,12 +248,24 @@ public ByteBufferSharing wrap(ByteBuffer appData) throws IOException { SSLEngineResult wrapResult = engine.wrap(appData, myNetData); - if (wrapResult.getHandshakeStatus() == NEED_TASK) { - handleBlockingTasks(); + switch (wrapResult.getStatus()) { + case BUFFER_UNDERFLOW: + throw new SSLException("Error encrypting data: " + wrapResult); + case BUFFER_OVERFLOW: + write(channel, myNetData); Review comment: Why do we `write` on a buffer overflow? Shouldn't the results of `wrap` be such that there is nothing to `write` since it couldn't put a whole message in the provided buffer? ########## File path: geode-core/src/main/java/org/apache/geode/internal/net/NioFilter.java ########## @@ -35,7 +35,7 @@ * Be sure to call close() on the returned {@link ByteBufferSharing}. The best way to do that is * to call this method in a try-with-resources statement. */ - ByteBufferSharing wrap(ByteBuffer buffer) throws IOException; + ByteBufferSharing wrap(ByteBuffer buffer, final SocketChannel channel) throws IOException; Review comment: Isn't the `NioFilter` unique to each `Socket`/`SocketChannel`, why isn't this a member? ########## File path: geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java ########## @@ -402,6 +436,10 @@ public synchronized void close(SocketChannel socketChannel) { socketChannel.write(myNetData); } } + if (result != null && result.getStatus() != CLOSED) { + throw new SSLHandshakeException( + "Error closing SSL session. Status=" + result.getStatus()); + } Review comment: I don't see why the rearrangement is necessary from the comment. I think you need to clarify. Is it because the write additional net data after close is required but if we exit the loop not in a closed state we are borked? ########## File path: geode-core/src/integrationTest/java/org/apache/geode/internal/net/NioSslEngineKeyUpdateTest.java ########## @@ -0,0 +1,459 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.internal.net; + +import static java.util.concurrent.CompletableFuture.supplyAsync; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_PASSWORD; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import java.io.FileInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.nio.ByteBuffer; +import java.nio.channels.ServerSocketChannel; +import java.nio.channels.SocketChannel; +import java.security.GeneralSecurityException; +import java.security.KeyManagementException; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.security.Security; +import java.security.UnrecoverableKeyException; +import java.util.Arrays; +import java.util.Properties; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.TrustManagerFactory; + +import org.junit.Before; Review comment: Use JUnit 5. ########## File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java ########## @@ -341,6 +450,10 @@ private static Properties gemFireConfiguration( */ props.setProperty("conserve-sockets", String.valueOf(conserveSockets)); + if (TEST_WITH_TLS_KEY_UPDATE_MESSAGE_PROCESSING) { Review comment: Make a separate test or parameter to this test. ########## File path: geode-core/src/integrationTest/java/org/apache/geode/internal/net/NioSslEngineKeyUpdateTest.java ########## @@ -0,0 +1,459 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.internal.net; + +import static java.util.concurrent.CompletableFuture.supplyAsync; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_PASSWORD; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import java.io.FileInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.nio.ByteBuffer; +import java.nio.channels.ServerSocketChannel; +import java.nio.channels.SocketChannel; +import java.security.GeneralSecurityException; +import java.security.KeyManagementException; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.security.Security; +import java.security.UnrecoverableKeyException; +import java.util.Arrays; +import java.util.Properties; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.TrustManagerFactory; + +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.geode.cache.ssl.CertStores; +import org.apache.geode.cache.ssl.CertificateBuilder; +import org.apache.geode.cache.ssl.CertificateMaterial; +import org.apache.geode.distributed.internal.DMStats; + +/** + * In TLSv1.3, when a GCM-based cipher is used, there is a limit on the number + * of bytes that may be encoded with a key. When that limit is reached, a TLS + * KeyUpdate message is generated (by the SSLEngine). That message causes a new + * key to be negotiated between the peer SSLEngines. + * + * Geode's {@link NioSslEngine} class (subclass of {@link NioFilter}) encapsulates + * Java's {@link SSLEngine}. Geode's MsgStreamer, Connection, and ConnectionTable + * classes interact with NioSslEngine to accomplish peer-to-peer (P2P) messaging. + * + * This test constructs a pair of SSLEngine's and wraps them in NioSslEngine. + * Rather than relying on MsgStreamer, Connection, or ConnectionTable classes + * (which themselves have a lot of dependencies on other classes), this test + * implements simplified logic for driving the sending and receiving of data, + * i.e. calling NioSslEngine.wrap() and NioSslEngine.unwrap(). See + * {@link #send(int, NioSslEngine, SocketChannel, int)} and + * {@link #receive(int, NioSslEngine, SocketChannel, ByteBuffer)}. + * + * The {@link #secureDataTransferTest()} arranges for the encrypted bytes limit + * to be reached very quickly (see {@link #ENCRYPTED_BYTES_LIMIT}). + * The test verifies data transfer continues correctly, after the limit is reached. + * This indirectly verifies that the KeyUpdate protocol initiated by the sending + * SSLEngine is correctly handled by all the components involved. + */ +public class NioSslEngineKeyUpdateTest { + + private static final String TLS_PROTOCOL = "TLSv1.3"; + private static final String TLS_CIPHER_SUITE = "TLS_AES_256_GCM_SHA384"; + + // number of bytes the GCM cipher can encrypt before initiating a KeyUpdate + private static final int ENCRYPTED_BYTES_LIMIT = 1; + + /* + * KeyUpdate messages don't happen in the handshake, they happen afterward. + * This is the number of bytes we'll transfer after the TLS handshake. + */ + private static final int BYTES_TO_TRANSFER_AFTER_HANDSHAKE = 2; + + { + assert IsPowerOfTwo(ENCRYPTED_BYTES_LIMIT) && ENCRYPTED_BYTES_LIMIT != 0; + + // The bytes will be sent using two calls to wrap so the number must be even + assert BYTES_TO_TRANSFER_AFTER_HANDSHAKE % 2 == 0; + + assert BYTES_TO_TRANSFER_AFTER_HANDSHAKE > ENCRYPTED_BYTES_LIMIT; + + Security.setProperty("jdk.tls.keyLimits", + "AES/GCM/NoPadding KeyUpdate " + ENCRYPTED_BYTES_LIMIT); + } + + private static BufferPool bufferPool; + private static SSLContext sslContext; + private static KeyStore keystore; + private static char[] keystorePassword; + private static KeyStore truststore; + + private SSLEngine clientEngine; + private SSLEngine serverEngine; + private int packetBufferSize; + + @BeforeClass + public static void beforeClass() throws GeneralSecurityException, IOException { + DMStats mockStats = mock(DMStats.class); + bufferPool = new BufferPool(mockStats); + + final Properties securityProperties = createKeystoreAndTruststore(); + + keystore = KeyStore.getInstance("JKS"); + keystorePassword = securityProperties.getProperty(SSL_KEYSTORE_PASSWORD).toCharArray(); + keystore.load(new FileInputStream(securityProperties.getProperty(SSL_KEYSTORE)), + keystorePassword); + + truststore = KeyStore.getInstance("JKS"); + final char[] truststorePassword = + securityProperties.getProperty(SSL_TRUSTSTORE_PASSWORD).toCharArray(); + truststore.load(new FileInputStream(securityProperties.getProperty(SSL_TRUSTSTORE)), + truststorePassword); + } + + @Before + public void before() throws NoSuchAlgorithmException, UnrecoverableKeyException, + KeyStoreException, KeyManagementException { + final KeyManagerFactory kmf = KeyManagerFactory.getInstance("PKIX"); + kmf.init(keystore, keystorePassword); + final TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); + tmf.init(truststore); + + sslContext = SSLContext.getInstance("TLS"); + final SecureRandom random = new SecureRandom(new byte[] {1, 2, 3}); + sslContext.init(kmf.getKeyManagers(), tmf.getTrustManagers(), random); + + final SSLParameters defaultParameters = sslContext.getDefaultSSLParameters(); + final String[] protocols = defaultParameters.getProtocols(); + final String[] cipherSuites = defaultParameters.getCipherSuites(); + System.out.println( + String.format("TLS settings (default) before handshake: Protocols: %s, Cipher Suites: %s", + Arrays.toString(protocols), Arrays.toString(cipherSuites))); + + clientEngine = createSSLEngine("server-host", true, sslContext); + packetBufferSize = clientEngine.getSession().getPacketBufferSize(); + + serverEngine = createSSLEngine("client-host", false, sslContext); + } + + @Test + public void handshakeTest() { + clientServerTest( + (channel, filter, peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Client:"); + return true; + }, + (channel, filter, peerNetData1) -> { + handshakeTLS(channel, filter, peerNetData1, + "Server:"); + return true; + }); + } + + @Test + public void secureDataTransferTest() { + clientServerTest( + (final SocketChannel channel, + final NioSslEngine filter, + final ByteBuffer peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Client:"); + /* + * if we call send() only once, it seems that jdk.tls.keyLimits + * is not evaluated by wrap(). Calling it twice seems to fix that. + */ + // send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE, filter, channel); + send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2, filter, channel, 0); + send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2, filter, channel, + BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2); + return true; + }, + (final SocketChannel channel, + final NioSslEngine filter, + final ByteBuffer peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Server:"); + /* + * Call receive() twice (like we did for send()) just to test that receive() is + * leaving buffers in the correct readable/writable state when it returns. + */ + for (int i = 0; i < 2; i++) { + final byte[] received = + receive(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2, filter, channel, peerNetData); + assertThat(received).hasSize(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2); + for (int j = i * BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2; j < (i + 1) + * BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2; j++) { + assertThat(received[j % received.length]).isEqualTo((byte) j); + } + } + return true; + }); + } + + private static SSLEngine createSSLEngine(final String peerHost, final boolean useClientMode, + final SSLContext sslContext) { + final SSLEngine engine = sslContext.createSSLEngine(peerHost, 10001); + engine.setEnabledProtocols(new String[] {TLS_PROTOCOL}); + engine.setEnabledCipherSuites(new String[] {TLS_CIPHER_SUITE}); + engine.setUseClientMode(useClientMode); + return engine; + } + + private void clientServerTest(final PeerAction clientAction, final PeerAction serverAction) { + final ExecutorService executorService = Executors.newFixedThreadPool(2); + + final CompletableFuture<SocketAddress> boundAddress = new CompletableFuture<>(); + + final CountDownLatch serversWaiting = new CountDownLatch(1); + + final CompletableFuture<Boolean> serverHandshakeFuture = + supplyAsync( + () -> server(boundAddress, packetBufferSize, + serverAction, serversWaiting, serverEngine), + executorService); + + final CompletableFuture<Boolean> clientHandshakeFuture = + supplyAsync( + () -> client(boundAddress, packetBufferSize, + clientAction, serversWaiting, clientEngine), + executorService); + + CompletableFuture.allOf(serverHandshakeFuture, clientHandshakeFuture) + .join(); + } + + /* + * An action taken on a client or server after the SocketChannel has been established. + */ + private interface PeerAction { + boolean apply(final SocketChannel acceptedChannel, + final NioSslEngine filter, + final ByteBuffer peerNetData) throws IOException; + } + + private static boolean client( + final CompletableFuture<SocketAddress> boundAddress, + final int packetBufferSize, + final PeerAction peerAction, + final CountDownLatch serversWaiting, + final SSLEngine engine) { + try { + try (final SocketChannel connectedChannel = SocketChannel.open()) { + connectedChannel.connect(boundAddress.get()); + final ByteBuffer peerNetData = + ByteBuffer.allocateDirect(packetBufferSize); + + final NioSslEngine filter = new NioSslEngine(engine, bufferPool); + + final boolean result = + peerAction.apply(connectedChannel, filter, peerNetData); + + serversWaiting.await(); // wait for last server to give up before closing our socket + + filter.close(connectedChannel); + + return result; + } + } catch (IOException | InterruptedException | ExecutionException e) { + printException("In client:", e); + throw new RuntimeException(e); + } + } + + private static boolean server( + final CompletableFuture<SocketAddress> boundAddress, + final int packetBufferSize, + final PeerAction peerAction, + final CountDownLatch serversWaiting, + final SSLEngine engine) { + try (final ServerSocketChannel boundChannel = ServerSocketChannel.open()) { + final InetSocketAddress bindAddress = + new InetSocketAddress(InetAddress.getLoopbackAddress(), 0); + boundChannel.bind(bindAddress); + boundAddress.complete(boundChannel.getLocalAddress()); + try (final SocketChannel acceptedChannel = boundChannel.accept()) { + final ByteBuffer peerNetData = + ByteBuffer.allocateDirect(packetBufferSize); + + final NioSslEngine filter = new NioSslEngine(engine, bufferPool); + + final boolean result = + peerAction.apply(acceptedChannel, filter, peerNetData); + + filter.close(acceptedChannel); + + return result; + } + } catch (IOException e) { + printException("In server:", e); + throw new RuntimeException(e); + } finally { + serversWaiting.countDown(); + } + } + + private static void printException(final String context, final Exception e) { + System.out.println(context + "\n"); + e.printStackTrace(); + } + + private static Properties createKeystoreAndTruststore() + throws GeneralSecurityException, IOException { + final CertificateMaterial ca = new CertificateBuilder() + .commonName("Test CA") + .isCA() + .generate(); + + CertificateMaterial serverCertificate = new CertificateBuilder() + .commonName("server") + .issuedBy(ca) + .generate(); + + final CertStores serverStore = CertStores.serverStore(); + serverStore.withCertificate("server", serverCertificate); + serverStore.trust("ca", ca); + return serverStore.propertiesWith("all", true, false); Review comment: Use the method that takes the file locations and use `@TempDir` annotation to initialize `File` objects. The current usage will leave files around each run. Using the annotations will cleanup the files at the end of the test. ########## File path: geode-core/src/distributedTest/java/org/apache/geode/distributed/internal/P2PMessagingConcurrencyDUnitTest.java ########## @@ -283,8 +383,11 @@ public void toData(final DataOutput out, final SerializationContext context) out.writeInt(length); final byte[] payload = new byte[length]; - random.nextBytes(payload); - + if (RANDOMIZE_PAYLOAD_CONTENT) { Review comment: Make a separate test or parameter to this test. ########## File path: geode-core/src/main/java/org/apache/geode/internal/net/NioSslEngine.java ########## @@ -248,12 +248,24 @@ public ByteBufferSharing wrap(ByteBuffer appData) throws IOException { SSLEngineResult wrapResult = engine.wrap(appData, myNetData); - if (wrapResult.getHandshakeStatus() == NEED_TASK) { - handleBlockingTasks(); + switch (wrapResult.getStatus()) { + case BUFFER_UNDERFLOW: + throw new SSLException("Error encrypting data: " + wrapResult); + case BUFFER_OVERFLOW: + write(channel, myNetData); + break; + case OK: + break; + case CLOSED: + throw new SSLException("Error encrypting data: " + wrapResult); } - if (wrapResult.getStatus() != OK) { - throw new SSLException("Error encrypting data: " + wrapResult); + switch (wrapResult.getHandshakeStatus()) { Review comment: Seems odd to evaluate this after some of the status above. Some of those should probably be returning rather than breaking. Could we end up with a `BUFFER_OVERFLOW` and `NEED_TASK`? ########## File path: geode-core/src/integrationTest/java/org/apache/geode/internal/net/NioSslEngineKeyUpdateTest.java ########## @@ -0,0 +1,459 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.internal.net; + +import static java.util.concurrent.CompletableFuture.supplyAsync; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_KEYSTORE_PASSWORD; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE; +import static org.apache.geode.distributed.ConfigurationProperties.SSL_TRUSTSTORE_PASSWORD; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import java.io.FileInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.nio.ByteBuffer; +import java.nio.channels.ServerSocketChannel; +import java.nio.channels.SocketChannel; +import java.security.GeneralSecurityException; +import java.security.KeyManagementException; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.security.Security; +import java.security.UnrecoverableKeyException; +import java.util.Arrays; +import java.util.Properties; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.TrustManagerFactory; + +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.geode.cache.ssl.CertStores; +import org.apache.geode.cache.ssl.CertificateBuilder; +import org.apache.geode.cache.ssl.CertificateMaterial; +import org.apache.geode.distributed.internal.DMStats; + +/** + * In TLSv1.3, when a GCM-based cipher is used, there is a limit on the number + * of bytes that may be encoded with a key. When that limit is reached, a TLS + * KeyUpdate message is generated (by the SSLEngine). That message causes a new + * key to be negotiated between the peer SSLEngines. + * + * Geode's {@link NioSslEngine} class (subclass of {@link NioFilter}) encapsulates + * Java's {@link SSLEngine}. Geode's MsgStreamer, Connection, and ConnectionTable + * classes interact with NioSslEngine to accomplish peer-to-peer (P2P) messaging. + * + * This test constructs a pair of SSLEngine's and wraps them in NioSslEngine. + * Rather than relying on MsgStreamer, Connection, or ConnectionTable classes + * (which themselves have a lot of dependencies on other classes), this test + * implements simplified logic for driving the sending and receiving of data, + * i.e. calling NioSslEngine.wrap() and NioSslEngine.unwrap(). See + * {@link #send(int, NioSslEngine, SocketChannel, int)} and + * {@link #receive(int, NioSslEngine, SocketChannel, ByteBuffer)}. + * + * The {@link #secureDataTransferTest()} arranges for the encrypted bytes limit + * to be reached very quickly (see {@link #ENCRYPTED_BYTES_LIMIT}). + * The test verifies data transfer continues correctly, after the limit is reached. + * This indirectly verifies that the KeyUpdate protocol initiated by the sending + * SSLEngine is correctly handled by all the components involved. + */ +public class NioSslEngineKeyUpdateTest { + + private static final String TLS_PROTOCOL = "TLSv1.3"; + private static final String TLS_CIPHER_SUITE = "TLS_AES_256_GCM_SHA384"; + + // number of bytes the GCM cipher can encrypt before initiating a KeyUpdate + private static final int ENCRYPTED_BYTES_LIMIT = 1; + + /* + * KeyUpdate messages don't happen in the handshake, they happen afterward. + * This is the number of bytes we'll transfer after the TLS handshake. + */ + private static final int BYTES_TO_TRANSFER_AFTER_HANDSHAKE = 2; + + { + assert IsPowerOfTwo(ENCRYPTED_BYTES_LIMIT) && ENCRYPTED_BYTES_LIMIT != 0; + + // The bytes will be sent using two calls to wrap so the number must be even + assert BYTES_TO_TRANSFER_AFTER_HANDSHAKE % 2 == 0; + + assert BYTES_TO_TRANSFER_AFTER_HANDSHAKE > ENCRYPTED_BYTES_LIMIT; + + Security.setProperty("jdk.tls.keyLimits", + "AES/GCM/NoPadding KeyUpdate " + ENCRYPTED_BYTES_LIMIT); + } + + private static BufferPool bufferPool; + private static SSLContext sslContext; + private static KeyStore keystore; + private static char[] keystorePassword; + private static KeyStore truststore; + + private SSLEngine clientEngine; + private SSLEngine serverEngine; + private int packetBufferSize; + + @BeforeClass + public static void beforeClass() throws GeneralSecurityException, IOException { + DMStats mockStats = mock(DMStats.class); + bufferPool = new BufferPool(mockStats); + + final Properties securityProperties = createKeystoreAndTruststore(); + + keystore = KeyStore.getInstance("JKS"); + keystorePassword = securityProperties.getProperty(SSL_KEYSTORE_PASSWORD).toCharArray(); + keystore.load(new FileInputStream(securityProperties.getProperty(SSL_KEYSTORE)), + keystorePassword); + + truststore = KeyStore.getInstance("JKS"); + final char[] truststorePassword = + securityProperties.getProperty(SSL_TRUSTSTORE_PASSWORD).toCharArray(); + truststore.load(new FileInputStream(securityProperties.getProperty(SSL_TRUSTSTORE)), + truststorePassword); + } + + @Before + public void before() throws NoSuchAlgorithmException, UnrecoverableKeyException, + KeyStoreException, KeyManagementException { + final KeyManagerFactory kmf = KeyManagerFactory.getInstance("PKIX"); + kmf.init(keystore, keystorePassword); + final TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX"); + tmf.init(truststore); + + sslContext = SSLContext.getInstance("TLS"); + final SecureRandom random = new SecureRandom(new byte[] {1, 2, 3}); + sslContext.init(kmf.getKeyManagers(), tmf.getTrustManagers(), random); + + final SSLParameters defaultParameters = sslContext.getDefaultSSLParameters(); + final String[] protocols = defaultParameters.getProtocols(); + final String[] cipherSuites = defaultParameters.getCipherSuites(); + System.out.println( + String.format("TLS settings (default) before handshake: Protocols: %s, Cipher Suites: %s", + Arrays.toString(protocols), Arrays.toString(cipherSuites))); + + clientEngine = createSSLEngine("server-host", true, sslContext); + packetBufferSize = clientEngine.getSession().getPacketBufferSize(); + + serverEngine = createSSLEngine("client-host", false, sslContext); + } + + @Test + public void handshakeTest() { + clientServerTest( + (channel, filter, peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Client:"); + return true; + }, + (channel, filter, peerNetData1) -> { + handshakeTLS(channel, filter, peerNetData1, + "Server:"); + return true; + }); + } + + @Test + public void secureDataTransferTest() { + clientServerTest( + (final SocketChannel channel, + final NioSslEngine filter, + final ByteBuffer peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Client:"); + /* + * if we call send() only once, it seems that jdk.tls.keyLimits + * is not evaluated by wrap(). Calling it twice seems to fix that. + */ + // send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE, filter, channel); + send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2, filter, channel, 0); + send(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2, filter, channel, + BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2); + return true; + }, + (final SocketChannel channel, + final NioSslEngine filter, + final ByteBuffer peerNetData) -> { + handshakeTLS(channel, filter, peerNetData, "Server:"); + /* + * Call receive() twice (like we did for send()) just to test that receive() is + * leaving buffers in the correct readable/writable state when it returns. + */ + for (int i = 0; i < 2; i++) { + final byte[] received = + receive(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2, filter, channel, peerNetData); + assertThat(received).hasSize(BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2); + for (int j = i * BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2; j < (i + 1) + * BYTES_TO_TRANSFER_AFTER_HANDSHAKE / 2; j++) { + assertThat(received[j % received.length]).isEqualTo((byte) j); + } + } + return true; + }); + } + + private static SSLEngine createSSLEngine(final String peerHost, final boolean useClientMode, + final SSLContext sslContext) { + final SSLEngine engine = sslContext.createSSLEngine(peerHost, 10001); + engine.setEnabledProtocols(new String[] {TLS_PROTOCOL}); + engine.setEnabledCipherSuites(new String[] {TLS_CIPHER_SUITE}); + engine.setUseClientMode(useClientMode); + return engine; + } + + private void clientServerTest(final PeerAction clientAction, final PeerAction serverAction) { + final ExecutorService executorService = Executors.newFixedThreadPool(2); + + final CompletableFuture<SocketAddress> boundAddress = new CompletableFuture<>(); + + final CountDownLatch serversWaiting = new CountDownLatch(1); + + final CompletableFuture<Boolean> serverHandshakeFuture = + supplyAsync( + () -> server(boundAddress, packetBufferSize, + serverAction, serversWaiting, serverEngine), + executorService); + + final CompletableFuture<Boolean> clientHandshakeFuture = + supplyAsync( + () -> client(boundAddress, packetBufferSize, + clientAction, serversWaiting, clientEngine), + executorService); + + CompletableFuture.allOf(serverHandshakeFuture, clientHandshakeFuture) + .join(); + } + + /* + * An action taken on a client or server after the SocketChannel has been established. + */ + private interface PeerAction { + boolean apply(final SocketChannel acceptedChannel, + final NioSslEngine filter, + final ByteBuffer peerNetData) throws IOException; + } + + private static boolean client( + final CompletableFuture<SocketAddress> boundAddress, + final int packetBufferSize, + final PeerAction peerAction, + final CountDownLatch serversWaiting, + final SSLEngine engine) { + try { + try (final SocketChannel connectedChannel = SocketChannel.open()) { + connectedChannel.connect(boundAddress.get()); + final ByteBuffer peerNetData = + ByteBuffer.allocateDirect(packetBufferSize); + + final NioSslEngine filter = new NioSslEngine(engine, bufferPool); + + final boolean result = + peerAction.apply(connectedChannel, filter, peerNetData); + + serversWaiting.await(); // wait for last server to give up before closing our socket + + filter.close(connectedChannel); + + return result; + } + } catch (IOException | InterruptedException | ExecutionException e) { + printException("In client:", e); + throw new RuntimeException(e); + } + } + + private static boolean server( + final CompletableFuture<SocketAddress> boundAddress, + final int packetBufferSize, + final PeerAction peerAction, + final CountDownLatch serversWaiting, + final SSLEngine engine) { + try (final ServerSocketChannel boundChannel = ServerSocketChannel.open()) { + final InetSocketAddress bindAddress = + new InetSocketAddress(InetAddress.getLoopbackAddress(), 0); + boundChannel.bind(bindAddress); + boundAddress.complete(boundChannel.getLocalAddress()); + try (final SocketChannel acceptedChannel = boundChannel.accept()) { + final ByteBuffer peerNetData = + ByteBuffer.allocateDirect(packetBufferSize); + + final NioSslEngine filter = new NioSslEngine(engine, bufferPool); + + final boolean result = + peerAction.apply(acceptedChannel, filter, peerNetData); + + filter.close(acceptedChannel); + + return result; + } + } catch (IOException e) { + printException("In server:", e); + throw new RuntimeException(e); + } finally { + serversWaiting.countDown(); + } + } + + private static void printException(final String context, final Exception e) { + System.out.println(context + "\n"); + e.printStackTrace(); + } + + private static Properties createKeystoreAndTruststore() + throws GeneralSecurityException, IOException { + final CertificateMaterial ca = new CertificateBuilder() + .commonName("Test CA") + .isCA() + .generate(); + + CertificateMaterial serverCertificate = new CertificateBuilder() + .commonName("server") + .issuedBy(ca) + .generate(); + + final CertStores serverStore = CertStores.serverStore(); + serverStore.withCertificate("server", serverCertificate); + serverStore.trust("ca", ca); + return serverStore.propertiesWith("all", true, false); + } + + /** + * @param filter is a newly constructed and intitialized object; it has not been used + * for handshakes previously. + * @param peerNetData on entry: don't care about read/write state or contents + * since all contents will be cleared by the filter's handshake(); + * on return: the buffer will be in write mode and may contain + */ + private static boolean handshakeTLS(final SocketChannel channel, Review comment: I am concerned that we re-implemented portions of the production code to test this. This smells funny. Is there any way to refactor the production code to be more inline with this so we can utilize it? If not, then I think we need some good documentation here as to why not. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org