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:
[email protected]
