[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6032: NIFI-10010: ListenTCP adds client certificate's Subject and Issuer DN…

[GitBox <gi...@apache.org>]

exceptionfactory commented on code in PR #6032:
URL: https://github.com/apache/nifi/pull/6032#discussion_r869556379


##########
nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/main/java/org/apache/nifi/event/transport/SslSessionStatus.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.event.transport;
+
+public class SslSessionStatus {
+    private final String subjectDN;
+    private final String issuerDN;

Review Comment:
   Recommend using `X500Principal` instead of `String` and renaming these properties to just `subject` and `issuer` in order to provide better fidelity with the `X509Certificate` source of the information.



##########
nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/main/java/org/apache/nifi/event/transport/netty/codec/SocketByteArrayMessageDecoder.java:
##########
@@ -38,7 +47,38 @@ public class SocketByteArrayMessageDecoder extends MessageToMessageDecoder<byte[
     protected void decode(final ChannelHandlerContext channelHandlerContext, final byte[] bytes, final List<Object> decoded) {
         final InetSocketAddress remoteAddress = (InetSocketAddress) channelHandlerContext.channel().remoteAddress();
         final String address = remoteAddress.getHostString();
-        final ByteArrayMessage message = new ByteArrayMessage(bytes, address);
+
+        ByteArrayMessage message = getMessageWithSslSessionStatus(channelHandlerContext, bytes, address);
+        if (message == null) {
+            message = new ByteArrayMessage(bytes, address);
+        }

Review Comment:
   Rather than returning a `ByteArrayMessage` that could be `null`, recommend restructuring the approach.  Creating a method called `getSslSessionStatus()` that takes the `ChannelHandlerContext` could return an `SslSessionStatus` object, which could be `null`, and then that could be passed to the `ByteArrayMessage` constructor without the need for null-checking.



##########
nifi-nar-bundles/nifi-extension-utils/nifi-event-transport/src/main/java/org/apache/nifi/event/transport/netty/codec/SocketByteArrayMessageDecoder.java:
##########
@@ -38,7 +47,38 @@ public class SocketByteArrayMessageDecoder extends MessageToMessageDecoder<byte[
     protected void decode(final ChannelHandlerContext channelHandlerContext, final byte[] bytes, final List<Object> decoded) {
         final InetSocketAddress remoteAddress = (InetSocketAddress) channelHandlerContext.channel().remoteAddress();
         final String address = remoteAddress.getHostString();
-        final ByteArrayMessage message = new ByteArrayMessage(bytes, address);
+
+        ByteArrayMessage message = getMessageWithSslSessionStatus(channelHandlerContext, bytes, address);
+        if (message == null) {
+            message = new ByteArrayMessage(bytes, address);
+        }
         decoded.add(message);
     }
+
+    private ByteArrayMessage getMessageWithSslSessionStatus(final ChannelHandlerContext channelHandlerContext, final byte[] bytes, final String address) {
+        Iterator<Map.Entry<String, ChannelHandler>> iterator = channelHandlerContext.channel().pipeline().iterator();
+        while (iterator.hasNext()) {
+            final ChannelHandler channelHandler = iterator.next().getValue();
+            if (channelHandler instanceof SslHandler) {
+                return createMessageWithSslSessionStatus((SslHandler)channelHandler, bytes, address);
+            }
+        }
+        return null;
+    }
+
+    private ByteArrayMessage createMessageWithSslSessionStatus(final SslHandler sslHandler, final byte[] bytes, final String address) {
+        final SSLSession sslSession = sslHandler.engine().getSession();
+        try {
+            final Certificate[] certificates = sslSession.getPeerCertificates();
+            if (certificates.length > 0) {
+                final X509Certificate certificate = (X509Certificate) certificates[0];
+                final String subjectDN = certificate.getSubjectDN().toString();
+                final String issuerDN = certificate.getIssuerDN().toString();

Review Comment:
   Following the comment to change the `SslSessionStatus` property types, recommend replacing `getSubjectDN()` and `getIssuerDN()` with `getSubjectX500Principal()` and `getIssuerX500Principal()` since the DN methods are denigrated according to the Java 8 documentation:
   
   https://docs.oracle.com/javase/8/docs/api/java/security/cert/X509Certificate.html#getIssuerDN--



##########
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenTCP.java:
##########
@@ -291,4 +305,15 @@ protected String getBatchKey(ByteArrayMessage event) {
         }
         return eventBatcher;
     }
+
+    private void addClientCertificateAttributes(final Map<String, String> attributes, final ByteArrayMessage event) {
+        final SslSessionStatus sslSessionStatus = event.getSslSessionStatus();
+        if (sslSessionStatus != null) {
+            attributes.put(CLIENT_CERTIFICATE_SUBJECT_DN_ATTRIBUTE, sslSessionStatus.getSubjectDN());
+            attributes.put(CLIENT_CERTIFICATE_ISSUER_DN_ATTRIBUTE, sslSessionStatus.getIssuerDN());
+        } else {
+            getLogger().debug("Remote Peer [{}] not verified: client certificates not provided",
+                    event.getSender());
+        }

Review Comment:
   Having the debug log seems unnecessary since the absence of the attributes in outgoing FlowFiles indicates the same fact of missing client certificate information.
   ```suggestion
           }
   ```



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org