IGNITE-18828 Add ciphers support to SSL (jdbc, client, scalecube) #1779
Conversation
valepakh
force-pushed
the
IGNITE-18828
branch
2 times, most recently
from
92d4a6c
to
9dad12c
Compare
| + " password: " + trustStorePassword + "\n" | ||
| + " }\n" | ||
| + " }\n" | ||
| + " }\n" | ||
| + "}"; | ||
| } | ||
|
|
||
| private static String getResourcePath(String resource) { | ||
| /** Converts URL gotten from classloader to proper file system path and escape backslashes so it could be used in the config. */ |
There was a problem hiding this comment.
The part of the comment about escaping is no longer valid.
...ain/java/org/apache/ignite/internal/network/configuration/SslConfigurationValidatorImpl.java
Show resolved
Hide resolved
modules/runner/src/integrationTest/java/org/apache/ignite/internal/ssl/ItSslTest.java
Outdated
Show resolved
Hide resolved
They are negotiated automatically from the list of enabled ciphers, which is what we are configuring here. |
As a user, why would I want to configure a custom list of ciphers? I've dealt with a lot of network/web applications and never had to do that. Am I missing something? |
ptupitsyn
changed the title
This is mostly for the national-level ciphers where national agencies require that only some specific subset of the supported cipher suites is enabled. |
|
|
||
| // First node will initialize the cluster with single node successfully since the second node can't connect to it. | ||
| assertThat(node1, willCompleteSuccessfully()); | ||
| assertThat(node2, willTimeoutIn(1, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
Are we sure 100% that node1 will always be the first node that is started? It there a chance that node2 will be the first and will start successfully? I think this place is the potential fluky test. I would ask you to do the following:
CompletableFuture<IgniteImpl> node1 = cluster.startClusterNode(0, sslEnabledWithCipher1BoostrapConfig);
assertThat(node1, willCompleteSuccessfully());
CompletableFuture<IgniteImpl> node2 = cluster.startClusterNode(1, sslEnabledWithCipher2BoostrapConfig);There was a problem hiding this comment.
This will not work because the future completes only after the cluster is initialized. Well, maybe we could initialize the cluster consisting of one node and then add the second node.
Start second node after the cluster initialization
| @@ -72,6 +76,11 @@ public ClientAuthenticationMode clientAuthenticationMode() { | |||
| return clientAuth; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Is it really necessary to add inheritDoc everywhere? I think it works just fine automatically if I override the documented method.
| Set<String> ciphers = Arrays.stream(ssl.ciphers().split(",")) | ||
| .map(String::strip) | ||
| .collect(Collectors.toSet()); | ||
| if (!supported.containsAll(ciphers)) { |
There was a problem hiding this comment.
It is possible different JVM (or ssl libs) may have different ciphers support.
With current approach, user may be forced to rewrite config for different environments.
I guess at least one supported cipher is enough for establishing a connection.
However, it make sense to log unsupported ciphers.
Will it be more user friendly, if we will filter out unsupported ciphers from given list and fail with error only when no valid cipher found?
There was a problem hiding this comment.
Yes, I agree, I'll create a follow-up ticket.
There was a problem hiding this comment.
Hmm, cannot this result into a situation when one just makes a typo in the name of a most preferred cipher and thus a less preferred cipher is selected?
There was a problem hiding this comment.
I think it's still better than to fail if any configured cipher happened to be incompatible?
There was a problem hiding this comment.
Would you like to review it here?
#1792
https://issues.apache.org/jira/browse/IGNITE-18828