NIFI-8067: Fix 1-way SSL in GRPC processors #4733
Conversation
| final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm(), | ||
| sslContext.getProvider()); | ||
| if (StringUtils.isNotBlank(sslContextService.getKeyStoreFile())) { | ||
| final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); |
There was a problem hiding this comment.
KeyManagerFactory construction and initialization could be streamlined using KeyStoreUtils
| final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); | |
| final TlsConfiguration tlsConfiguration = sslContextService.createTlsConfiguration(); | |
| final KeyManagerFactory keyManagerFactory = KeyStoreUtils.loadKeyManagerFactory(tlsConfiguration); |
There was a problem hiding this comment.
I did not know about those util methods. Much better in this way. Thanks!
| final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm(), | ||
| sslContext.getProvider()); | ||
| if (StringUtils.isNotBlank(sslContextService.getTrustStoreFile())) { | ||
| final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); |
There was a problem hiding this comment.
KeyStoreUtils can also be used to streamline initialization of TrustManagerFactory
| final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); | |
| final TrustManagerFactory trustManagerFactory = KeyStoreUtis.loadTrustManagerFactory(tlsConfiguration); |
| .displayName("Use TLS") | ||
| .description("Whether or not to use TLS to send the contents of the gRPC messages.") | ||
| .displayName("Use SSL/TLS") | ||
| .description("Whether or not to use SSL/TLS to receive the contents of the gRPC messages.") |
There was a problem hiding this comment.
Recommend leaving the existing display name and description since SSLv3 is no longer available for most Java Virtual Machines. Unfortunately, classes still contain the SSL prefix even though only TLS is allowed.
There was a problem hiding this comment.
Done.
My original intent was to keep some kind of consistency with the name of the 'SSL Context Service' property.
But it is better to go ahead and use TLS where possible.
| .required(false) | ||
| .defaultValue("false") | ||
| .allowableValues("true", "false") | ||
| .build(); | ||
| public static final PropertyDescriptor PROP_SSL_CONTEXT_SERVICE = new PropertyDescriptor.Builder() | ||
| .name("SSL Context Service") | ||
| .displayName("SSL Context Service") | ||
| .description("The SSL Context Service used to provide client certificate information for TLS (https) connections.") | ||
| .description("The SSL Context Service used to provide server certificate information for SSL/TLS (https) connections. Keystore must be configured on the service." + | ||
| " If truststore is also configured, it will turn on and require client certificate authentication (2-way SSL).") |
There was a problem hiding this comment.
Recommend replacing 2-way SSL with Mutual TLS to reflect modern protocol versions.
| .description("A Regular Expression to apply against the Distinguished Name of incoming connections. If the Pattern does not match the DN, the connection will be refused.") | ||
| .required(true) | ||
| .description("A Regular Expression to apply against the Distinguished Name of incoming connections. If the Pattern does not match the DN, the connection will be refused." + | ||
| " The property will only be used if client certificate authentication (2-way SSL) has been configured on " + PROP_SSL_CONTEXT_SERVICE.getDisplayName() + "," + |
There was a problem hiding this comment.
Recommend replacing 2-way SSL with Mutual TLS to reflect modern protocol versions.
| // construct key manager | ||
| if (StringUtils.isBlank(sslContextService.getKeyStoreFile())) { | ||
| throw new IllegalStateException("SSL is enabled, but no keystore has been configured. You must configure a keystore."); | ||
| } | ||
|
|
||
| final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm(), | ||
| sslContext.getProvider()); | ||
| final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); |
There was a problem hiding this comment.
See comments on leveraging KeyStoreUtils
| } | ||
| sslContextBuilder = GrpcSslContexts.configure(sslContextBuilder); | ||
| serverBuilder = serverBuilder.sslContext(sslContextBuilder.build()); | ||
| GrpcSslContexts.configure(sslContextBuilder); |
There was a problem hiding this comment.
This line can be removed since GrpcSslContexts.configure() is a helper method to return a new SslContextBuilder, which is not used.
There was a problem hiding this comment.
Not sure. GrpcSslContexts.configure() returns the SslContextBuilder instance passed in (like a builder).
The interesting part that it also configures some things on SslContextBuilder. That's why I don't think it can be removed.
There was a problem hiding this comment.
You are correct, after looking more closely at GrpcSslContexts.configure(), it does appear to be setting some protocol options based on the runtime platform.
|
@exceptionfactory Thanks for your thorough review. Committed the changes. |
|
@turcsanyip Thanks for making the changes, everything looks good. |
|
Merged to main following previous comments. Thanks @turcsanyip @exceptionfactory |
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#4733.
Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#4733.
https://issues.apache.org/jira/browse/NIFI-8067
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically
main)?Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.