ptupitsyn commented on code in PR #1639: URL: https://github.com/apache/ignite-3/pull/1639#discussion_r1101620404
########## modules/client/src/main/java/org/apache/ignite/client/ClientAuthConfiguration.java: ########## @@ -0,0 +1,23 @@ +/* + * 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.client; + +/** Ignite SSL client authentication enum. */ +public enum ClientAuthConfiguration { + NONE, OPTIONAL, REQUIRE Review Comment: Missing javadoc. Let's explain how different modes work (see https://netty.io/4.1/api/io/netty/handler/ssl/ClientAuth.html) ########## modules/client/src/main/java/org/apache/ignite/client/IgniteClient.java: ########## @@ -97,6 +97,9 @@ class Builder { /** Logger factory. */ private @Nullable LoggerFactory loggerFactory; + /** SSL configuration. */ + private SslConfiguration sslConfiguration = SslConfiguration.builder().build(); Review Comment: Why not just `null`? Let's use the same logic as `retryPolicy`, `loggerFactory`, `asyncContinuationExecutor` use. ########## modules/client/src/main/java/org/apache/ignite/client/ClientAuthConfiguration.java: ########## @@ -0,0 +1,23 @@ +/* + * 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.client; + +/** Ignite SSL client authentication enum. */ +public enum ClientAuthConfiguration { Review Comment: Let's rename to `ClientAuthenticationMode`. * `Auth` is ambiguous (authorization vs authentication). * `Configuration` is usually a class with nested members, not an enum. ########## modules/client/src/main/java/org/apache/ignite/client/SslConfiguration.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.client; + +import org.apache.ignite.internal.client.SslConfigurationBuilder; +import org.jetbrains.annotations.Nullable; + +/** Client SSL configuration. */ +public interface SslConfiguration { + Review Comment: Extra blank line ########## modules/client/src/main/java/org/apache/ignite/client/SslConfiguration.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.client; + +import org.apache.ignite.internal.client.SslConfigurationBuilder; +import org.jetbrains.annotations.Nullable; + +/** Client SSL configuration. */ +public interface SslConfiguration { + + /** If set to {@code true} then the SSL connection will be used to interact with Ignite 3 node. */ + boolean enabled(); + + /** Client authentication configuration. */ + ClientAuthConfiguration clientAuth(); Review Comment: ```suggestion ClientAuthenticationMode clientAuthenticationMode(); ``` ########## modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/ItSslClientHandlerTest.java: ########## @@ -0,0 +1,125 @@ +/* + * 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.client.handler; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.io.OutputStream; +import java.net.Socket; +import java.net.SocketException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.msgpack.core.MessagePack; + +/** SSL client integration test. */ +public class ItSslClientHandlerTest { + + /** Magic bytes. */ + private static final byte[] MAGIC = {0x49, 0x47, 0x4E, 0x49}; + + ClientHandlerModule serverModule; + + TestServer testServer; + + int serverPort; + + String password; + + String keyStorePkcs12Path; Review Comment: Can be made `private`. ########## modules/network/src/main/java/org/apache/ignite/internal/network/ssl/ClientSslContextProvider.java: ########## @@ -0,0 +1,70 @@ +/* + * 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; + +/** Client SSL context provider. */ +public class ClientSslContextProvider implements SslContextProvider { Review Comment: This interface is redundant. We never pass it anywhere, only create an instance to call one method and throw away. We can have a static class `ClientSslContextProvider` with `createServerContext` and `createClientContext` methods instead - less code, easier to follow. ########## modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/ItSslClientHandlerTest.java: ########## @@ -0,0 +1,125 @@ +/* + * 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.client.handler; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.io.OutputStream; +import java.net.Socket; +import java.net.SocketException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.msgpack.core.MessagePack; + +/** SSL client integration test. */ +public class ItSslClientHandlerTest { + + /** Magic bytes. */ + private static final byte[] MAGIC = {0x49, 0x47, 0x4E, 0x49}; + + ClientHandlerModule serverModule; + + TestServer testServer; + + int serverPort; + + String password; Review Comment: Can be a local variable. ########## modules/client/src/main/java/org/apache/ignite/client/IgniteClient.java: ########## @@ -260,6 +263,23 @@ public Builder heartbeatTimeout(long heartbeatTimeout) { return this; } + /** + * Sets the SSL configuration. + * + * @param sslConfiguration SSL configuration. + * @return This instance. + */ + public Builder sslConfiguration(@Nullable SslConfiguration sslConfiguration) { Review Comment: Should we rename this to `ssl()` to be consistent with the server-side config? ########## modules/network/src/main/java/org/apache/ignite/internal/network/netty/PipelineUtils.java: ########## @@ -30,6 +31,23 @@ public class PipelineUtils { /** {@link ChunkedWriteHandler}'s name. */ private static final String CHUNKED_WRITE_HANDLER_NAME = "chunked-write-handler"; + /** + * Sets up initial pipeline with ssl. + * + * @param pipeline Channel pipeline. + * @param serializationService Serialization service. + * @param handshakeManager Handshake manager. + * @param messageListener Message listener. + * @param sslContext Netty SSL context. + */ + public static void setup(ChannelPipeline pipeline, PerSessionSerializationService serializationService, + HandshakeManager handshakeManager, Consumer<InNetworkObject> messageListener, SslContext sslContext) { + Review Comment: Extra blank like. ########## modules/client/src/main/java/org/apache/ignite/client/IgniteClientSslException.java: ########## @@ -0,0 +1,62 @@ +/* + * 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.client; + +import static org.apache.ignite.lang.ErrorGroups.Client.CLIENT_SSL_CONFIGURATION_ERR; + +import java.util.UUID; +import org.apache.ignite.lang.IgniteException; + +/** + * Indicates invalid SSL configuration. + */ +public class IgniteClientSslException extends IgniteException { Review Comment: I don't think we need a special exception for SSL, an error code is enough. ########## modules/client/src/main/java/org/apache/ignite/internal/client/io/netty/NettyClientConnectionMultiplexer.java: ########## @@ -75,6 +90,74 @@ public void initChannel(SocketChannel ch) { } } + private void setupSsl(SocketChannel ch, IgniteClientConfiguration clientCfg) { + if (clientCfg.sslConfiguration() == null || !clientCfg.sslConfiguration().enabled()) { + return; + } + + try { + var trustStore = clientCfg.sslConfiguration().trustStore(); + if (trustStore == null) { + throw new IgniteClientSslException("SSL is enabled but no trust store is provided"); + } + if (trustStore.path() == null) { + throw new IgniteClientSslException("SSL is enabled but no path to trust store is provided"); + } Review Comment: None of the properties in the client-side `SslConfiguration` should be mandatory. We should be able to use SSL without providing anything (for example, you don't need a key store or a trust store to load `https://github.com`). ########## modules/client/src/main/java/org/apache/ignite/internal/client/SslConfigurationBuilder.java: ########## @@ -0,0 +1,114 @@ +/* + * 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.client; + +import org.apache.ignite.client.ClientAuthConfiguration; +import org.apache.ignite.client.SslConfiguration; +import org.jetbrains.annotations.Nullable; + +/** SSL configuration builder. */ +public class SslConfigurationBuilder { + private static String DEFAULT_KEYSTORE_TYPE = "PKCS12"; Review Comment: Should be `final`. ########## modules/client/src/main/java/org/apache/ignite/client/SslConfiguration.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.client; + +import org.apache.ignite.internal.client.SslConfigurationBuilder; +import org.jetbrains.annotations.Nullable; + +/** Client SSL configuration. */ +public interface SslConfiguration { + + /** If set to {@code true} then the SSL connection will be used to interact with Ignite 3 node. */ + boolean enabled(); + + /** Client authentication configuration. */ + ClientAuthConfiguration clientAuth(); + + /** Keystore configuration. */ + @Nullable KeystoreConfiguration keyStore(); Review Comment: This is not consistent with `SslConfigurationBuilder`, where `keyStore` and `trustStore` properties are not grouped. Let's remove `KeystoreConfiguration`. Using it for `TrustStore` also looks weird. ########## modules/client/src/main/java/org/apache/ignite/client/KeystoreConfiguration.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.client; + +import org.jetbrains.annotations.Nullable; + +/** Keystore configuration. */ +public interface KeystoreConfiguration { + Review Comment: Extra blank line. -- 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]
