You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/12/16 22:48:50 UTC
[GitHub] [nifi] turcsanyip opened a new pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
turcsanyip opened a new pull request #4733:
URL: https://github.com/apache/nifi/pull/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 `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on a change in pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#discussion_r551455310
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -85,18 +89,20 @@
.build();
public static final PropertyDescriptor PROP_USE_SECURE = new PropertyDescriptor.Builder()
.name("Use TLS")
- .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.")
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#issuecomment-754246017
@turcsanyip Thanks for making the changes, everything looks good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4733:
URL: https://github.com/apache/nifi/pull/4733
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on a change in pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#discussion_r551451724
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -153,70 +162,83 @@
return RELATIONSHIPS;
}
-
@Override
protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return PROPERTIES;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext context) {
+ List<ValidationResult> results = new ArrayList<>(super.customValidate(context));
+
+ final boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
+ final boolean sslContextServiceConfigured = context.getProperty(PROP_SSL_CONTEXT_SERVICE).isSet();
+
+ if (useSecure && !sslContextServiceConfigured) {
+ results.add(new ValidationResult.Builder()
+ .subject(PROP_SSL_CONTEXT_SERVICE.getDisplayName())
+ .valid(false)
+ .explanation(String.format("'%s' must be configured when '%s' is true", PROP_SSL_CONTEXT_SERVICE.getDisplayName(), PROP_USE_SECURE.getDisplayName()))
+ .build());
+ }
+
+ return results;
+ }
@OnScheduled
public void startServer(final ProcessContext context) throws NoSuchAlgorithmException, IOException, KeyStoreException, CertificateException, UnrecoverableKeyException {
final ComponentLog logger = getLogger();
// gather configured properties
final Integer port = context.getProperty(PROP_SERVICE_PORT).asInteger();
final Boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
- final Integer flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
- final Integer maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
+ final int flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
+ final int maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
final SSLContextService sslContextService = context.getProperty(PROP_SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
- final SSLContext sslContext = sslContextService == null ? null : sslContextService.createSSLContext(org.apache.nifi.security.util.ClientAuth.NONE);
final Pattern authorizedDnPattern = Pattern.compile(context.getProperty(PROP_AUTHORIZED_DN_PATTERN).getValue());
final FlowFileIngestServiceInterceptor callInterceptor = new FlowFileIngestServiceInterceptor(getLogger());
callInterceptor.enforceDNPattern(authorizedDnPattern);
final FlowFileIngestService flowFileIngestService = new FlowFileIngestService(getLogger(),
sessionFactoryReference,
context);
- NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
+ final NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
.addService(ServerInterceptors.intercept(flowFileIngestService, callInterceptor))
// default (de)compressor registries handle both plaintext and gzip compressed messages
.compressorRegistry(CompressorRegistry.getDefaultInstance())
.decompressorRegistry(DecompressorRegistry.getDefaultInstance())
.flowControlWindow(flowControlWindow)
- .maxMessageSize(maxMessageSize);
+ .maxInboundMessageSize(maxMessageSize);
- if (useSecure && sslContext != null) {
+ if (useSecure) {
// 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());
final KeyStore keyStore = KeyStore.getInstance(sslContextService.getKeyStoreType());
try (final InputStream is = new FileInputStream(sslContextService.getKeyStoreFile())) {
keyStore.load(is, sslContextService.getKeyStorePassword().toCharArray());
}
keyManagerFactory.init(keyStore, sslContextService.getKeyStorePassword().toCharArray());
- SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);
+ final SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);
// if the trust store is configured, then client auth is required.
if (StringUtils.isNotBlank(sslContextService.getTrustStoreFile())) {
- final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm(),
- sslContext.getProvider());
+ final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
final KeyStore trustStore = KeyStore.getInstance(sslContextService.getTrustStoreType());
try (final InputStream is = new FileInputStream(sslContextService.getTrustStoreFile())) {
trustStore.load(is, sslContextService.getTrustStorePassword().toCharArray());
}
trustManagerFactory.init(trustStore);
- sslContextBuilder = sslContextBuilder.trustManager(trustManagerFactory);
- sslContextBuilder = sslContextBuilder.clientAuth(io.netty.handler.ssl.ClientAuth.REQUIRE);
+ sslContextBuilder.trustManager(trustManagerFactory);
+ sslContextBuilder.clientAuth(ClientAuth.REQUIRE);
} else {
- sslContextBuilder = sslContextBuilder.clientAuth(io.netty.handler.ssl.ClientAuth.NONE);
+ sslContextBuilder.clientAuth(ClientAuth.NONE);
}
- sslContextBuilder = GrpcSslContexts.configure(sslContextBuilder);
- serverBuilder = serverBuilder.sslContext(sslContextBuilder.build());
+ GrpcSslContexts.configure(sslContextBuilder);
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on a change in pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#discussion_r551448672
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/InvokeGRPC.java
##########
@@ -240,13 +262,11 @@ public void initializeClient(final ProcessContext context) throws Exception {
// configure whether or not we're using secure comms
final boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
final SSLContextService sslContextService = context.getProperty(PROP_SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
- final SSLContext sslContext = sslContextService == null ? null : sslContextService.createSSLContext(ClientAuth.NONE);
- if (useSecure && sslContext != null) {
+ if (useSecure) {
SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
- if(StringUtils.isNotBlank(sslContextService.getKeyStoreFile())) {
- final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm(),
- sslContext.getProvider());
+ if (StringUtils.isNotBlank(sslContextService.getKeyStoreFile())) {
+ final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
Review comment:
I did not know about those util methods. Much better in this way. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#discussion_r551593919
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -153,70 +162,83 @@
return RELATIONSHIPS;
}
-
@Override
protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return PROPERTIES;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext context) {
+ List<ValidationResult> results = new ArrayList<>(super.customValidate(context));
+
+ final boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
+ final boolean sslContextServiceConfigured = context.getProperty(PROP_SSL_CONTEXT_SERVICE).isSet();
+
+ if (useSecure && !sslContextServiceConfigured) {
+ results.add(new ValidationResult.Builder()
+ .subject(PROP_SSL_CONTEXT_SERVICE.getDisplayName())
+ .valid(false)
+ .explanation(String.format("'%s' must be configured when '%s' is true", PROP_SSL_CONTEXT_SERVICE.getDisplayName(), PROP_USE_SECURE.getDisplayName()))
+ .build());
+ }
+
+ return results;
+ }
@OnScheduled
public void startServer(final ProcessContext context) throws NoSuchAlgorithmException, IOException, KeyStoreException, CertificateException, UnrecoverableKeyException {
final ComponentLog logger = getLogger();
// gather configured properties
final Integer port = context.getProperty(PROP_SERVICE_PORT).asInteger();
final Boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
- final Integer flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
- final Integer maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
+ final int flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
+ final int maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
final SSLContextService sslContextService = context.getProperty(PROP_SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
- final SSLContext sslContext = sslContextService == null ? null : sslContextService.createSSLContext(org.apache.nifi.security.util.ClientAuth.NONE);
final Pattern authorizedDnPattern = Pattern.compile(context.getProperty(PROP_AUTHORIZED_DN_PATTERN).getValue());
final FlowFileIngestServiceInterceptor callInterceptor = new FlowFileIngestServiceInterceptor(getLogger());
callInterceptor.enforceDNPattern(authorizedDnPattern);
final FlowFileIngestService flowFileIngestService = new FlowFileIngestService(getLogger(),
sessionFactoryReference,
context);
- NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
+ final NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
.addService(ServerInterceptors.intercept(flowFileIngestService, callInterceptor))
// default (de)compressor registries handle both plaintext and gzip compressed messages
.compressorRegistry(CompressorRegistry.getDefaultInstance())
.decompressorRegistry(DecompressorRegistry.getDefaultInstance())
.flowControlWindow(flowControlWindow)
- .maxMessageSize(maxMessageSize);
+ .maxInboundMessageSize(maxMessageSize);
- if (useSecure && sslContext != null) {
+ if (useSecure) {
// 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());
final KeyStore keyStore = KeyStore.getInstance(sslContextService.getKeyStoreType());
try (final InputStream is = new FileInputStream(sslContextService.getKeyStoreFile())) {
keyStore.load(is, sslContextService.getKeyStorePassword().toCharArray());
}
keyManagerFactory.init(keyStore, sslContextService.getKeyStorePassword().toCharArray());
- SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);
+ final SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);
// if the trust store is configured, then client auth is required.
if (StringUtils.isNotBlank(sslContextService.getTrustStoreFile())) {
- final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm(),
- sslContext.getProvider());
+ final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
final KeyStore trustStore = KeyStore.getInstance(sslContextService.getTrustStoreType());
try (final InputStream is = new FileInputStream(sslContextService.getTrustStoreFile())) {
trustStore.load(is, sslContextService.getTrustStorePassword().toCharArray());
}
trustManagerFactory.init(trustStore);
- sslContextBuilder = sslContextBuilder.trustManager(trustManagerFactory);
- sslContextBuilder = sslContextBuilder.clientAuth(io.netty.handler.ssl.ClientAuth.REQUIRE);
+ sslContextBuilder.trustManager(trustManagerFactory);
+ sslContextBuilder.clientAuth(ClientAuth.REQUIRE);
} else {
- sslContextBuilder = sslContextBuilder.clientAuth(io.netty.handler.ssl.ClientAuth.NONE);
+ sslContextBuilder.clientAuth(ClientAuth.NONE);
}
- sslContextBuilder = GrpcSslContexts.configure(sslContextBuilder);
- serverBuilder = serverBuilder.sslContext(sslContextBuilder.build());
+ GrpcSslContexts.configure(sslContextBuilder);
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on a change in pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#discussion_r551454345
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -85,18 +89,20 @@
.build();
public static final PropertyDescriptor PROP_USE_SECURE = new PropertyDescriptor.Builder()
.name("Use TLS")
- .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.")
.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).")
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#discussion_r545390131
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/InvokeGRPC.java
##########
@@ -240,13 +262,11 @@ public void initializeClient(final ProcessContext context) throws Exception {
// configure whether or not we're using secure comms
final boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
final SSLContextService sslContextService = context.getProperty(PROP_SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
- final SSLContext sslContext = sslContextService == null ? null : sslContextService.createSSLContext(ClientAuth.NONE);
- if (useSecure && sslContext != null) {
+ if (useSecure) {
SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
- if(StringUtils.isNotBlank(sslContextService.getKeyStoreFile())) {
- final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm(),
- sslContext.getProvider());
+ if (StringUtils.isNotBlank(sslContextService.getKeyStoreFile())) {
+ final KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
Review comment:
`KeyManagerFactory` construction and initialization could be streamlined using `KeyStoreUtils`
```suggestion
final TlsConfiguration tlsConfiguration = sslContextService.createTlsConfiguration();
final KeyManagerFactory keyManagerFactory = KeyStoreUtils.loadKeyManagerFactory(tlsConfiguration);
```
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/InvokeGRPC.java
##########
@@ -255,9 +275,8 @@ public void initializeClient(final ProcessContext context) throws Exception {
sslContextBuilder.keyManager(keyManagerFactory);
}
- if(StringUtils.isNotBlank(sslContextService.getTrustStoreFile())) {
- final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm(),
- sslContext.getProvider());
+ if (StringUtils.isNotBlank(sslContextService.getTrustStoreFile())) {
+ final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
Review comment:
`KeyStoreUtils` can also be used to streamline initialization of `TrustManagerFactory`
```suggestion
final TrustManagerFactory trustManagerFactory = KeyStoreUtis.loadTrustManagerFactory(tlsConfiguration);
```
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -153,70 +162,83 @@
return RELATIONSHIPS;
}
-
@Override
protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return PROPERTIES;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext context) {
+ List<ValidationResult> results = new ArrayList<>(super.customValidate(context));
+
+ final boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
+ final boolean sslContextServiceConfigured = context.getProperty(PROP_SSL_CONTEXT_SERVICE).isSet();
+
+ if (useSecure && !sslContextServiceConfigured) {
+ results.add(new ValidationResult.Builder()
+ .subject(PROP_SSL_CONTEXT_SERVICE.getDisplayName())
+ .valid(false)
+ .explanation(String.format("'%s' must be configured when '%s' is true", PROP_SSL_CONTEXT_SERVICE.getDisplayName(), PROP_USE_SECURE.getDisplayName()))
+ .build());
+ }
+
+ return results;
+ }
@OnScheduled
public void startServer(final ProcessContext context) throws NoSuchAlgorithmException, IOException, KeyStoreException, CertificateException, UnrecoverableKeyException {
final ComponentLog logger = getLogger();
// gather configured properties
final Integer port = context.getProperty(PROP_SERVICE_PORT).asInteger();
final Boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
- final Integer flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
- final Integer maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
+ final int flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
+ final int maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
final SSLContextService sslContextService = context.getProperty(PROP_SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
- final SSLContext sslContext = sslContextService == null ? null : sslContextService.createSSLContext(org.apache.nifi.security.util.ClientAuth.NONE);
final Pattern authorizedDnPattern = Pattern.compile(context.getProperty(PROP_AUTHORIZED_DN_PATTERN).getValue());
final FlowFileIngestServiceInterceptor callInterceptor = new FlowFileIngestServiceInterceptor(getLogger());
callInterceptor.enforceDNPattern(authorizedDnPattern);
final FlowFileIngestService flowFileIngestService = new FlowFileIngestService(getLogger(),
sessionFactoryReference,
context);
- NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
+ final NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
.addService(ServerInterceptors.intercept(flowFileIngestService, callInterceptor))
// default (de)compressor registries handle both plaintext and gzip compressed messages
.compressorRegistry(CompressorRegistry.getDefaultInstance())
.decompressorRegistry(DecompressorRegistry.getDefaultInstance())
.flowControlWindow(flowControlWindow)
- .maxMessageSize(maxMessageSize);
+ .maxInboundMessageSize(maxMessageSize);
- if (useSecure && sslContext != null) {
+ if (useSecure) {
// 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());
Review comment:
See comments on leveraging `KeyStoreUtils`
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -85,18 +89,20 @@
.build();
public static final PropertyDescriptor PROP_USE_SECURE = new PropertyDescriptor.Builder()
.name("Use TLS")
- .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.")
Review 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.
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -121,18 +127,21 @@
public static final PropertyDescriptor PROP_AUTHORIZED_DN_PATTERN = new PropertyDescriptor.Builder()
.name("Authorized DN Pattern")
.displayName("Authorized DN Pattern")
- .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() + "," +
Review comment:
Recommend replacing `2-way SSL` with `Mutual TLS` to reflect modern protocol versions.
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -85,18 +89,20 @@
.build();
public static final PropertyDescriptor PROP_USE_SECURE = new PropertyDescriptor.Builder()
.name("Use TLS")
- .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.")
.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).")
Review comment:
Recommend replacing `2-way SSL` with `Mutual TLS` to reflect modern protocol versions.
##########
File path: nifi-nar-bundles/nifi-grpc-bundle/nifi-grpc-processors/src/main/java/org/apache/nifi/processors/grpc/ListenGRPC.java
##########
@@ -153,70 +162,83 @@
return RELATIONSHIPS;
}
-
@Override
protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return PROPERTIES;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext context) {
+ List<ValidationResult> results = new ArrayList<>(super.customValidate(context));
+
+ final boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
+ final boolean sslContextServiceConfigured = context.getProperty(PROP_SSL_CONTEXT_SERVICE).isSet();
+
+ if (useSecure && !sslContextServiceConfigured) {
+ results.add(new ValidationResult.Builder()
+ .subject(PROP_SSL_CONTEXT_SERVICE.getDisplayName())
+ .valid(false)
+ .explanation(String.format("'%s' must be configured when '%s' is true", PROP_SSL_CONTEXT_SERVICE.getDisplayName(), PROP_USE_SECURE.getDisplayName()))
+ .build());
+ }
+
+ return results;
+ }
@OnScheduled
public void startServer(final ProcessContext context) throws NoSuchAlgorithmException, IOException, KeyStoreException, CertificateException, UnrecoverableKeyException {
final ComponentLog logger = getLogger();
// gather configured properties
final Integer port = context.getProperty(PROP_SERVICE_PORT).asInteger();
final Boolean useSecure = context.getProperty(PROP_USE_SECURE).asBoolean();
- final Integer flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
- final Integer maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
+ final int flowControlWindow = context.getProperty(PROP_FLOW_CONTROL_WINDOW).asDataSize(DataUnit.B).intValue();
+ final int maxMessageSize = context.getProperty(PROP_MAX_MESSAGE_SIZE).asDataSize(DataUnit.B).intValue();
final SSLContextService sslContextService = context.getProperty(PROP_SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
- final SSLContext sslContext = sslContextService == null ? null : sslContextService.createSSLContext(org.apache.nifi.security.util.ClientAuth.NONE);
final Pattern authorizedDnPattern = Pattern.compile(context.getProperty(PROP_AUTHORIZED_DN_PATTERN).getValue());
final FlowFileIngestServiceInterceptor callInterceptor = new FlowFileIngestServiceInterceptor(getLogger());
callInterceptor.enforceDNPattern(authorizedDnPattern);
final FlowFileIngestService flowFileIngestService = new FlowFileIngestService(getLogger(),
sessionFactoryReference,
context);
- NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
+ final NettyServerBuilder serverBuilder = NettyServerBuilder.forPort(port)
.addService(ServerInterceptors.intercept(flowFileIngestService, callInterceptor))
// default (de)compressor registries handle both plaintext and gzip compressed messages
.compressorRegistry(CompressorRegistry.getDefaultInstance())
.decompressorRegistry(DecompressorRegistry.getDefaultInstance())
.flowControlWindow(flowControlWindow)
- .maxMessageSize(maxMessageSize);
+ .maxInboundMessageSize(maxMessageSize);
- if (useSecure && sslContext != null) {
+ if (useSecure) {
// 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());
final KeyStore keyStore = KeyStore.getInstance(sslContextService.getKeyStoreType());
try (final InputStream is = new FileInputStream(sslContextService.getKeyStoreFile())) {
keyStore.load(is, sslContextService.getKeyStorePassword().toCharArray());
}
keyManagerFactory.init(keyStore, sslContextService.getKeyStorePassword().toCharArray());
- SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);
+ final SslContextBuilder sslContextBuilder = SslContextBuilder.forServer(keyManagerFactory);
// if the trust store is configured, then client auth is required.
if (StringUtils.isNotBlank(sslContextService.getTrustStoreFile())) {
- final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm(),
- sslContext.getProvider());
+ final TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
final KeyStore trustStore = KeyStore.getInstance(sslContextService.getTrustStoreType());
try (final InputStream is = new FileInputStream(sslContextService.getTrustStoreFile())) {
trustStore.load(is, sslContextService.getTrustStorePassword().toCharArray());
}
trustManagerFactory.init(trustStore);
- sslContextBuilder = sslContextBuilder.trustManager(trustManagerFactory);
- sslContextBuilder = sslContextBuilder.clientAuth(io.netty.handler.ssl.ClientAuth.REQUIRE);
+ sslContextBuilder.trustManager(trustManagerFactory);
+ sslContextBuilder.clientAuth(ClientAuth.REQUIRE);
} else {
- sslContextBuilder = sslContextBuilder.clientAuth(io.netty.handler.ssl.ClientAuth.NONE);
+ sslContextBuilder.clientAuth(ClientAuth.NONE);
}
- sslContextBuilder = GrpcSslContexts.configure(sslContextBuilder);
- serverBuilder = serverBuilder.sslContext(sslContextBuilder.build());
+ GrpcSslContexts.configure(sslContextBuilder);
Review comment:
This line can be removed since `GrpcSslContexts.configure()` is a helper method to return a new `SslContextBuilder`, which is not used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] pvillard31 commented on pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#issuecomment-754551608
Merged to main following previous comments. Thanks @turcsanyip @exceptionfactory
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] turcsanyip commented on pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on pull request #4733:
URL: https://github.com/apache/nifi/pull/4733#issuecomment-754105034
@exceptionfactory Thanks for your thorough review. Committed the changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org