sashapolo commented on code in PR #1659: URL: https://github.com/apache/ignite-3/pull/1659#discussion_r1102361488
########## modules/network/src/main/java/org/apache/ignite/internal/network/configuration/AbstractSslConfigurationSchema.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.ignite.internal.network.configuration; + +import org.apache.ignite.configuration.annotation.AbstractConfiguration; +import org.apache.ignite.configuration.annotation.ConfigValue; +import org.apache.ignite.configuration.annotation.Value; +import org.apache.ignite.configuration.validation.OneOf; + +/** SSL configuration schema. */ +@AbstractConfiguration +public class AbstractSslConfigurationSchema { Review Comment: why do you need this abstract schema? Are there going to be more inheritors apart from `SslConfigurationSchema`? ########## modules/network/src/main/java/org/apache/ignite/internal/network/ssl/SslContextProvider.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.ignite.internal.network.ssl; + +import io.netty.handler.ssl.ClientAuth; +import io.netty.handler.ssl.SslContext; +import io.netty.handler.ssl.SslContextBuilder; +import java.io.IOException; +import java.nio.file.NoSuchFileException; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.UnrecoverableKeyException; +import java.security.cert.CertificateException; +import javax.net.ssl.KeyManagerFactory; +import javax.net.ssl.TrustManagerFactory; +import org.apache.ignite.internal.network.configuration.SslView; +import org.apache.ignite.lang.ErrorGroups.Common; +import org.apache.ignite.lang.IgniteException; + +/** SSL context provider. */ +public final class SslContextProvider { + + private SslContextProvider() { + throw new IllegalStateException("SslContextProvider is an utility class and should not be instantiated"); Review Comment: Same here about throwing an exception ########## modules/network/src/main/java/org/apache/ignite/internal/network/ssl/KeystoreLoader.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.ignite.internal.network.ssl; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.cert.CertificateException; +import org.apache.ignite.internal.network.configuration.KeyStoreView; + +/** Java keystore loader. */ +public final class KeystoreLoader { + + private KeystoreLoader() { + throw new IllegalStateException("KeystoreLoader is an utility class and should not be instantiated"); + } + + /** Initialize and load keystore with provided configuration. */ + public static KeyStore load(KeyStoreView keyStoreConfiguration) + throws KeyStoreException, IOException, CertificateException, NoSuchAlgorithmException { Review Comment: Do we really need to throw all these exceptions? Maybe we should wrap them into `IgniteException` here? ########## modules/network/src/main/java/org/apache/ignite/internal/network/ssl/KeystoreLoader.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.ignite.internal.network.ssl; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.cert.CertificateException; +import org.apache.ignite.internal.network.configuration.KeyStoreView; + +/** Java keystore loader. */ +public final class KeystoreLoader { + + private KeystoreLoader() { + throw new IllegalStateException("KeystoreLoader is an utility class and should not be instantiated"); + } + + /** Initialize and load keystore with provided configuration. */ + public static KeyStore load(KeyStoreView keyStoreConfiguration) + throws KeyStoreException, IOException, CertificateException, NoSuchAlgorithmException { + + KeyStore ks = KeyStore.getInstance(keyStoreConfiguration.type()); + ks.load( + Files.newInputStream(Path.of(keyStoreConfiguration.path())), Review Comment: `Files.newInputStream` should be used with try-with-resources ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/ssl/ItSslTest.java: ########## @@ -0,0 +1,193 @@ +/* + * 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.ignite.internal.ssl; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +import java.nio.file.Path; +import org.apache.ignite.internal.Cluster; +import org.apache.ignite.internal.testframework.WorkDirectory; +import org.apache.ignite.internal.testframework.WorkDirectoryExtension; +import org.intellij.lang.annotations.Language; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.api.TestInstance.Lifecycle; +import org.junit.jupiter.api.extension.ExtendWith; + +/** SSL support integration test. */ +@ExtendWith(WorkDirectoryExtension.class) +public class ItSslTest { + + static String password; Review Comment: all these fields can be `private` ########## modules/network/src/test/java/org/apache/ignite/internal/network/ssl/SslContextProviderTest.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.ignite.internal.network.ssl; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertThrows; + +import io.netty.handler.ssl.SslContext; +import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension; +import org.apache.ignite.internal.configuration.testframework.InjectConfiguration; +import org.apache.ignite.internal.network.configuration.SslConfiguration; +import org.apache.ignite.lang.ErrorGroups.Common; +import org.apache.ignite.lang.IgniteException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +@ExtendWith(ConfigurationExtension.class) +class SslContextProviderTest { + + @InjectConfiguration + SslConfiguration configuration; + + String password; + + String keyStorePkcs12Path; + + String trustStoreJks12Path; + + @BeforeEach + void setUp() { + password = "changeit"; + keyStorePkcs12Path = SslContextProviderTest.class.getClassLoader().getResource("ssl/keystore.p12").getPath(); Review Comment: I have a question regarding these keystore and truststore files: can they be generated automatically and not placed into the repository? We could do it either manually or using a Junit extension ########## modules/network/src/main/java/org/apache/ignite/internal/network/configuration/AbstractSslConfigurationSchema.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.ignite.internal.network.configuration; + +import org.apache.ignite.configuration.annotation.AbstractConfiguration; +import org.apache.ignite.configuration.annotation.ConfigValue; +import org.apache.ignite.configuration.annotation.Value; +import org.apache.ignite.configuration.validation.OneOf; + +/** SSL configuration schema. */ +@AbstractConfiguration +public class AbstractSslConfigurationSchema { + /** Enable/disable SSL. */ + @Value(hasDefault = true) + public final boolean enabled = false; + + /** Client authentication. */ + @OneOf({"none", "optional", "require"}) Review Comment: Please update the javadoc with a description about what these options mean ########## modules/network/src/main/java/org/apache/ignite/internal/network/netty/NettyClient.java: ########## @@ -48,44 +51,50 @@ public class NettyClient { /** Destination address. */ private final SocketAddress address; - /** Future that resolves when the client finished the handshake. */ - @Nullable - private volatile OrderingFuture<NettySender> senderFuture = null; - /** Future that resolves when the client channel is opened. */ private final CompletableFuture<Void> channelFuture = new CompletableFuture<>(); - /** Client channel. */ - @Nullable - private volatile Channel channel = null; - /** Message listener. */ private final Consumer<InNetworkObject> messageListener; /** Handshake manager. */ private final HandshakeManager handshakeManager; + /** SSL configuration. */ + private final SslView configuration; Review Comment: let's rename this field to `sslConfiguration` ########## modules/network/src/main/java/org/apache/ignite/internal/network/ssl/KeystoreLoader.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.ignite.internal.network.ssl; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.cert.CertificateException; +import org.apache.ignite.internal.network.configuration.KeyStoreView; + +/** Java keystore loader. */ +public final class KeystoreLoader { + + private KeystoreLoader() { + throw new IllegalStateException("KeystoreLoader is an utility class and should not be instantiated"); Review Comment: I think that throwing an exception here is redundant, having a private constructor is enough ########## modules/network/src/test/java/org/apache/ignite/internal/network/ssl/SslContextProviderTest.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.ignite.internal.network.ssl; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertThrows; + +import io.netty.handler.ssl.SslContext; +import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension; +import org.apache.ignite.internal.configuration.testframework.InjectConfiguration; +import org.apache.ignite.internal.network.configuration.SslConfiguration; +import org.apache.ignite.lang.ErrorGroups.Common; +import org.apache.ignite.lang.IgniteException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +@ExtendWith(ConfigurationExtension.class) +class SslContextProviderTest { + + @InjectConfiguration + SslConfiguration configuration; Review Comment: All these fields can be `final` ########## modules/network/src/test/java/org/apache/ignite/network/DefaultMessagingServiceTest.java: ########## @@ -168,6 +169,9 @@ private static void configureNetworkDefaults( lenient().when(networkConfigView.portRange()).thenReturn(0); lenient().when(networkConfigView.outbound()).thenReturn(outboundConfig); lenient().when(networkConfigView.inbound()).thenReturn(inboundConfig); + SslView ssl = mock(SslView.class); + lenient().when(ssl.enabled()).thenReturn(false); Review Comment: I think these mocks should be rewritten using `@InjectConfiguration` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
