NIFI-1981 Cluster communication without client certificate #508
Conversation
| @Test | ||
| void testShouldDetermineClientAuthStatusFromSocket() { | ||
| // Arrange | ||
| SSLSocket needSocket = [getNeedClientAuth: { -> true }] as SSLSocket |
There was a problem hiding this comment.
I'm seeing some Java 8 stuff (Lambdas, e.g) but this PR is against 0.x, which needs Java 7 language target
There was a problem hiding this comment.
nevermind, I should've noticed these were Groovy coercions.
<-- embarrassed
There was a problem hiding this comment.
Discussed with Matt offline, but these are standard Groovy map coercions, not lambdas. Running a local check against Java 7 to ensure no Java 8 features were introduced.
|
+1 LGTM, built and ran with Java 7 and 8 on 0.x branch. Mind squashing the commits? Then I'll merge to 0.x and master, thanks! |
|
Squashing and will push. Thanks @mattyb149 . |
| </dependency> | ||
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcpkix-jdk15on</artifactId> |
There was a problem hiding this comment.
Missed this the first time, a duplicate definition of this artifact. Gives the following warning upon build:
[WARNING] Some problems were encountered while building the effective model for org.apache.nifi:nifi-security-utils🫙1.0.0-SNAPSHOT
[WARNING] 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.bouncycastle:bcpkix-jdk15on:jar -> duplicate declaration of version (?) @ org.apache.nifi:nifi-security-utils:[unknown-version], /Users/mburgess/git-apache/nifi/nifi-commons/nifi-security-utils/pom.xml, line 43, column 21
There was a problem hiding this comment.
Great catch Matt. Fixed, squashed, and pushed.
…ertificates would fail even if needClientAuth set to false. Fixed IDE setting for import wildcarding on Groovy files. (+4 squashed commits) Squashed commits: [4c3b174] NIFI-1981 Lowered logging level of client auth setting on cluster connection receive. [b50f473] NIFI-1981 Finished logic to suppress exception on missing client certificates when clientAuth is set to WANT. Added unit tests for CertificateUtil methods. [ace35a2] NIFI-1981 Added test scope dependency on BouncyCastle and BC PKIX modules for CertificateUtils tests. [2c463d1] NIFI-1981 Added ClientAuth enum and CertificateUtil methods to extract this setting from an SSLSocket. Added logic to compare X509Certificate DNs regardless of RDN element order. Added logic to suppress peer certificate exceptions when client authentication is not required. Removed duplicate dependency in pom.xml.
|
@mattyb149 merged to |
2 participants
This PR resolves an issue where cluster communications were failing when the node sent a heartbeat without a client certificate chain because
nifi.security.needClientAuthwas set to false on the NCM. Even though the TLS negotiation did not require or expect a client certificate and the handshake was successful, the NiFi code inSocketProtocolListenerimmediately attempted to determine the incoming node's DN from the certificate, which was missing, and thusSSLSocketImplthrew anSSLPeerNotVerifiedExceptionsaying "peer not authenticated".With this patch, the NCM will now respect the
needClientAuthsetting when attempting to extract the node DN.This PR is targeted to the
0.xbranch as this is where the issue was discovered and does not merge cleanly with themasterbranch. These changes will need to be selectively merged to themasterbranch.