pvillard31 commented on code in PR #11273:
URL: https://github.com/apache/nifi/pull/11273#discussion_r3287162146
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/connector/ProxyHeaderValidatorCustomizer.java:
##########
@@ -33,6 +37,8 @@
public class ProxyHeaderValidatorCustomizer implements
HttpConfiguration.Customizer {
private static final String MISDIRECTED_REQUEST_REASON = "Invalid Proxy
Host Requested";
+ private static final String REQUEST_REPLICATED_HEADER =
"request-replicated";
Review Comment:
Could we reference `RequestReplicationHeader.REQUEST_REPLICATED.getHeader()`
(possibly after moving that enum, or just this single constant, into
`nifi-web-servlet-shared` next to `ProxyHeader`) so the header value is defined
in exactly one place?
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/connector/ProxyHeaderValidatorCustomizer.java:
##########
@@ -65,6 +71,24 @@ public Request customize(final Request request, final
HttpFields.Mutable respons
}
private Request customizeSecureRequest(final Request request) {
+ final X509Certificate peerCertificate = findPeerCertificate(request);
+
+ // Requests not authenticated with Client Certificates require header
validation
+ if (peerCertificate == null) {
+ processProxyHostHeaders(request);
+ } else {
Review Comment:
Should this branch also require the peer certificate to match a known
cluster node identity (or come from the cluster-only trust store), so that a
non-node user holding a valid client certificate cannot bypass proxy host
validation simply by adding the `request-replicated` header?
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/test/java/org/apache/nifi/web/server/StandardServerProviderTest.java:
##########
@@ -82,6 +82,8 @@ class StandardServerProviderTest {
private static final String HOST_HEADER = "Host";
+ private static final String REQUEST_REPLICATED_HEADER =
"request-replicated";
Review Comment:
Same question on the test side: can `REQUEST_REPLICATED_HEADER` here share
the canonical constant rather than redefining the literal?
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/connector/ProxyHeaderValidatorCustomizer.java:
##########
@@ -85,7 +109,24 @@ private Request customizeSecureRequest(final Request
request) {
throw new
HttpException.RuntimeException(HttpStatus.MISDIRECTED_REQUEST_421,
MISDIRECTED_REQUEST_REASON);
}
+ }
- return request;
+ private X509Certificate findPeerCertificate(final Request request) {
+ final X509Certificate peerCertificate;
+ final ConnectionMetaData connectionMetaData =
request.getConnectionMetaData();
+ final Connection connection = connectionMetaData.getConnection();
+ final EndPoint endPoint = connection.getEndPoint();
+ final EndPoint.SslSessionData sslSessionData =
endPoint.getSslSessionData();
Review Comment:
Is there any code path under a successful `isSecure()` where
`connection.getEndPoint()` or `endPoint.getSslSessionData()` could be null (for
example during a protocol upgrade), and if so should we guard the chain rather
than risk an `NullPointerException` inside the customizer?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
