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/17 20:54:06 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4733: NIFI-8067: Fix 1-way SSL in GRPC processors
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