You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "smolnar82 (via GitHub)" <gi...@apache.org> on 2023/01/24 09:07:51 UTC
[GitHub] [knox] smolnar82 opened a new pull request, #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
smolnar82 opened a new pull request, #717:
URL: https://github.com/apache/knox/pull/717
## What changes were proposed in this pull request?
There are 2 changes in this PR:
- TLS cipher suites and protocols are now customizable in CM service discovery:
- end-users can define the TLS protocol(s) and cipher suite(s) to use while creating a secure connection to a CM instance
- if none is defined, Knox will pick up the relevant supported values from Java's own `java.security` thru the acquired SSL context.
- Added a new method to indicate the TLS protocols to be included when creating SSL context in the embedded Jetty server. Prior to this change, end-users could only tell what to exclude.
## How was this patch tested?
Unit testing and running service discovery in a secure cluster with updated SSL configs.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 merged pull request #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 merged PR #717:
URL: https://github.com/apache/knox/pull/717
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
Posted by "zeroflag (via GitHub)" <gi...@apache.org>.
zeroflag commented on code in PR #717:
URL: https://github.com/apache/knox/pull/717#discussion_r1085079880
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/DiscoveryApiClient.java:
##########
@@ -157,12 +161,26 @@ private String getUsername() {
return username;
}
- private void configureTruststore(KeystoreService keystoreService) {
- SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(keystoreService);
+ private void configureSsl(GatewayConfig gatewayConfig, KeyStore trustStore) {
+ final SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(trustStore);
+
if (truststoreSSLContext != null) {
+ final ConnectionSpec.Builder connectionSpecBuilder = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS);
+ if (gatewayConfig.getIncludedSSLCiphers().isEmpty()) {
Review Comment:
Can this method `getIncludedSSLCiphers()` return null if no `ssl.include.ciphers` is configured?
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on code in PR #717:
URL: https://github.com/apache/knox/pull/717#discussion_r1086371018
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/DiscoveryApiClient.java:
##########
@@ -157,12 +161,26 @@ private String getUsername() {
return username;
}
- private void configureTruststore(KeystoreService keystoreService) {
- SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(keystoreService);
+ private void configureSsl(GatewayConfig gatewayConfig, KeyStore trustStore) {
+ final SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(trustStore);
+
if (truststoreSSLContext != null) {
+ final ConnectionSpec.Builder connectionSpecBuilder = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS);
+ if (gatewayConfig.getIncludedSSLCiphers().isEmpty()) {
Review Comment:
Fixed.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on code in PR #717:
URL: https://github.com/apache/knox/pull/717#discussion_r1086364626
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/DiscoveryApiClient.java:
##########
@@ -157,12 +161,26 @@ private String getUsername() {
return username;
}
- private void configureTruststore(KeystoreService keystoreService) {
- SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(keystoreService);
+ private void configureSsl(GatewayConfig gatewayConfig, KeyStore trustStore) {
+ final SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(trustStore);
+
if (truststoreSSLContext != null) {
+ final ConnectionSpec.Builder connectionSpecBuilder = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS);
+ if (gatewayConfig.getIncludedSSLCiphers().isEmpty()) {
Review Comment:
Ohh...and you are right. I added a new method - `getIncludedSSLProtocols` - I misread and thought your question was about that one. Let me submit a new PS soon.
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
Posted by "zeroflag (via GitHub)" <gi...@apache.org>.
zeroflag commented on code in PR #717:
URL: https://github.com/apache/knox/pull/717#discussion_r1086015378
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/DiscoveryApiClient.java:
##########
@@ -157,12 +161,26 @@ private String getUsername() {
return username;
}
- private void configureTruststore(KeystoreService keystoreService) {
- SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(keystoreService);
+ private void configureSsl(GatewayConfig gatewayConfig, KeyStore trustStore) {
+ final SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(trustStore);
+
if (truststoreSSLContext != null) {
+ final ConnectionSpec.Builder connectionSpecBuilder = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS);
+ if (gatewayConfig.getIncludedSSLCiphers().isEmpty()) {
Review Comment:
I might miss something but I don't see how is this enforced. What if there is no `SSL_INCLUDE_CIPHERS` in the config?
```
@Override
public List<String> getIncludedSSLCiphers() {
List<String> list = null;
String value = get(SSL_INCLUDE_CIPHERS);
if (value != null && !value.isEmpty() && !"none".equalsIgnoreCase(value.trim())) {
list = Arrays.asList(value.trim().split("\\s*,\\s*"));
}
return list;
}
```
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #717: KNOX-2864 - TLS cipher suites and protocols are configured for CM service discovery
Posted by "smolnar82 (via GitHub)" <gi...@apache.org>.
smolnar82 commented on code in PR #717:
URL: https://github.com/apache/knox/pull/717#discussion_r1085109397
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/DiscoveryApiClient.java:
##########
@@ -157,12 +161,26 @@ private String getUsername() {
return username;
}
- private void configureTruststore(KeystoreService keystoreService) {
- SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(keystoreService);
+ private void configureSsl(GatewayConfig gatewayConfig, KeyStore trustStore) {
+ final SSLContext truststoreSSLContext = TruststoreSSLContextUtils.getTruststoreSSLContext(trustStore);
+
if (truststoreSSLContext != null) {
+ final ConnectionSpec.Builder connectionSpecBuilder = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS);
+ if (gatewayConfig.getIncludedSSLCiphers().isEmpty()) {
Review Comment:
Nope; it cannot be null (either an empty collection or one with the declared items).
--
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: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org