exceptionfactory commented on a change in pull request #4976:
URL: https://github.com/apache/nifi/pull/4976#discussion_r608137489
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/servlets/ListenHTTPServlet.java
##########
@@ -180,14 +184,22 @@ protected void doPost(final HttpServletRequest request,
final HttpServletRespons
final X509Certificate[] certs = (X509Certificate[])
request.getAttribute("javax.servlet.request.X509Certificate");
foundSubject = DEFAULT_FOUND_SUBJECT;
+ foundIssuer = DEFAULT_FOUND_ISSUER;
if (certs != null && certs.length > 0) {
for (final X509Certificate cert : certs) {
foundSubject = cert.getSubjectDN().getName();
+ foundIssuer = cert.getIssuerDN().getName();
if (authorizedPattern.matcher(foundSubject).matches()) {
- break;
+ if
(authorizedIssuerPattern.matcher(foundIssuer).matches()) {
+ break;
+ } else {
+ logger.warn("Rejecting transfer attempt from " +
foundSubject + ", Issuer " + foundIssuer + " because the Issuer is not
authorized, host=" + request.getRemoteHost());
Review comment:
Recommend adjusting the log message to use placeholders and indicating
the host first:
```suggestion
logger.warn("Access Forbidden [Issuer not
authorized] Host [{}] Subject [{}] Issuer [{}]", request.getRemoteHost(),
foundSubject, foundIssuer);
```
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/servlets/ListenHTTPServlet.java
##########
@@ -335,8 +348,9 @@ public void process(final OutputStream rawOut) throws
IOException {
}
flowFile = session.putAllAttributes(flowFile, attributes);
- flowFile = saveRequestDetailsAsAttributes(request, session,
foundSubject, flowFile);
- session.getProvenanceReporter().receive(flowFile,
request.getRequestURL().toString(), sourceSystemFlowFileIdentifier, "Remote
DN=" + foundSubject, transferMillis);
+ flowFile = saveRequestDetailsAsAttributes(request, session,
foundSubject, foundIssuer, flowFile);
+ final String details = String.format("Remote DN=%s, Issuer DN=%s",
foundSubject, foundIssuer);
+ session.getProvenanceReporter().receive(flowFile,
request.getRequestURL().toString(), sourceSystemFlowFileIdentifier, details +
foundIssuer, transferMillis);
Review comment:
It looks like the concatenation of `details` and `foundIssuer` should be
changed to just `details`:
```suggestion
session.getProvenanceReporter().receive(flowFile,
request.getRequestURL().toString(), sourceSystemFlowFileIdentifier, details,
transferMillis);
```
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenHTTP.java
##########
@@ -298,6 +299,56 @@ public void
testSecureTwoWaySslPOSTRequestsReceivedWithoutEL() throws Exception
testPOSTRequestsReceived(HttpServletResponse.SC_OK, true, true);
}
+ @Test
+ public void
testSecureTwoWaySslPOSTRequestsReceivedWithAuthorizedSubjectDn() throws
Exception {
Review comment:
Is this test method necessary in light of existing unit tests that rely
on the default Authorized DN Pattern?
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -159,11 +160,18 @@ public AllowableValue getAllowableValue() {
.build();
public static final PropertyDescriptor AUTHORIZED_DN_PATTERN = new
PropertyDescriptor.Builder()
.name("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.")
+ .description("A Regular Expression to apply against the Subject's
Distinguished Name of incoming connections. If the Pattern does not match the
Subject DN, the connection will be refused.")
Review comment:
Although the existing description says that "the connection will be
refused", recommend taking the opportunity to be more precise and write
something like "the processor will respond with a status of HTTP 403 Forbidden".
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenHTTP.java
##########
@@ -298,6 +299,56 @@ public void
testSecureTwoWaySslPOSTRequestsReceivedWithoutEL() throws Exception
testPOSTRequestsReceived(HttpServletResponse.SC_OK, true, true);
}
+ @Test
+ public void
testSecureTwoWaySslPOSTRequestsReceivedWithAuthorizedSubjectDn() throws
Exception {
+
configureProcessorSslContextService(ListenHTTP.ClientAuthentication.REQUIRED,
serverConfiguration);
+
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.AUTHORIZED_DN_PATTERN, LOCALHOST_DN);
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.assertValid();
+
+ testPOSTRequestsReceived(HttpServletResponse.SC_OK, true, true);
+ }
+
+ @Test
+ public void
testSecureTwoWaySslPOSTRequestsReceivedWithUnauthorizedSubjectDn() throws
Exception {
+
configureProcessorSslContextService(ListenHTTP.ClientAuthentication.REQUIRED,
serverConfiguration);
+
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.AUTHORIZED_DN_PATTERN, "CN=other");
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.assertValid();
+
+ testPOSTRequestsReceived(HttpServletResponse.SC_FORBIDDEN, true, true);
+ }
+
+ @Test
+ public void
testSecureTwoWaySslPOSTRequestsReceivedWithAuthorizedIssuerDn() throws
Exception {
Review comment:
Is this test method necessary since the default pattern configuration
allows access?
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/servlets/ListenHTTPServlet.java
##########
@@ -180,14 +184,22 @@ protected void doPost(final HttpServletRequest request,
final HttpServletRespons
final X509Certificate[] certs = (X509Certificate[])
request.getAttribute("javax.servlet.request.X509Certificate");
foundSubject = DEFAULT_FOUND_SUBJECT;
+ foundIssuer = DEFAULT_FOUND_ISSUER;
if (certs != null && certs.length > 0) {
for (final X509Certificate cert : certs) {
foundSubject = cert.getSubjectDN().getName();
+ foundIssuer = cert.getIssuerDN().getName();
if (authorizedPattern.matcher(foundSubject).matches()) {
- break;
+ if
(authorizedIssuerPattern.matcher(foundIssuer).matches()) {
+ break;
+ } else {
+ logger.warn("Rejecting transfer attempt from " +
foundSubject + ", Issuer " + foundIssuer + " because the Issuer is not
authorized, host=" + request.getRemoteHost());
+
response.sendError(HttpServletResponse.SC_FORBIDDEN, "not allowed based on
issuer dn");
+ return;
+ }
} else {
- logger.warn("Rejecting transfer attempt from " +
foundSubject + " because the DN is not authorized, host=" +
request.getRemoteHost());
- response.sendError(HttpServletResponse.SC_FORBIDDEN,
"not allowed based on dn");
+ logger.warn("Rejecting transfer attempt from " +
foundSubject + ", Issuer " + foundIssuer + " because the Subject is not
authorized, host=" + request.getRemoteHost());
Review comment:
Recommend taking the opportunity to reformat the warning message with
placeholders:
```suggestion
logger.warn("Access Forbidden [Subject not
authorized] Host [{}] Subject [{}] Issuer [{}]", request.getRemoteHost(),
foundSubject, foundIssuer););
```
--
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:
us...@infra.apache.org
