Internal Kafka clients should do TLS hostname verification except Nodeports #7526

see-quick · 2022-10-26T12:30:52Z

Signed-off-by: see-quick [email protected]

Type of change

Bugfix

Description

Currently, our internal Kafka clients do not perform TLS hostname verification on all possible Kubernetes listeners. This PR solves #7518 and enables TLS hostname verification and also disables it in situations where we do not want to execute it (e.g., when we use Nodeport listener, because it does not support TLS hostname verification on ports).

Checklist

Make sure all tests pass

…cation except Nodeports Signed-off-by: see-quick <[email protected]>

see-quick · 2022-10-26T12:45:28Z

/azp run regression

azure-pipelines · 2022-10-26T12:45:38Z

Azure Pipelines successfully started running 1 pipeline(s).

systemtest/src/test/java/io/strimzi/systemtest/bridge/HttpBridgeKafkaExternalListenersST.java

scholzj · 2022-10-26T12:59:19Z

systemtest/src/test/java/io/strimzi/systemtest/bridge/HttpBridgeKafkaExternalListenersST.java

+            // we disable ssl.endpoint.identification.algorithm for external listener (i.e., Nodeport),
+            // because TLS hostname verification is not supported on such listener type.
+            .withAdditionalConfig("ssl.endpoint.identification.algorithm=\n")


Same here ... Why does this use Node port listener here?

The producer fails on this error:

eadyOps=0] SSL Handshake failed javax.net.ssl.SSLHandshakeException: No subject alternative names matching IP address 192.168.0.226 found at sun.security.ssl.Alert.createSSLException(Alert.java:131) ~[?:?] at sun.security.ssl.TransportContext.fatal(TransportContext.java:353) ~[?:?] at sun.security.ssl.TransportContext.fatal(TransportContext.java:296) ~[?:?] at sun.security.ssl.TransportContext.fatal(TransportContext.java:291) ~[?:?] at sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1357) ~[?:?] at sun.security.ssl.CertificateMessage$T13CertificateConsumer.onConsumeCertificate(CertificateMessage.java:1232) ~[?:?] at sun.security.ssl.CertificateMessage$T13CertificateConsumer.consume(CertificateMessage.java:1175) ~[?:?] at sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392) ~[?:?] at sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:443) ~[?:?] at sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(SSLEngineImpl.java:1074) ~[?:?] at sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(SSLEngineImpl.java:1061) ~[?:?] at java.security.AccessController.doPrivileged(Native Method) ~[?:?] at sun.security.ssl.SSLEngineImpl$DelegatedTask.run(SSLEngineImpl.java:1008) ~[?:?]

I'm not wondering about the disabling of it. I'm wondering why does this use node-ports in the first place as it seems unnecessary.

I am not sure...you mean we should rather produce messages to the internal listener instead of producing them via external NodePort?

Because these test cases currently do the following:

Create a bridge consumer to receive messages from the external listener (NodePort)

Create a producer to produce messages via the same listener (NodePort)

So should we modify the second point to use an internal listener instead of an external one?

Yeah, I think we should think about what the purpose of the test is. i mean I can approve this and we can merge it. But we should look into that in some separate PR - what exactly does this bring us.

IIRC reason why we are using external producer etc. was because at the time I was writing those "weird username tests", there were no possibilities to use internal clients - as they didn't allow any username to be longer than some number of characters and it shouldn't contain any special characters.

External clients are allowing it.

TBH I don't remember the exact reason why we went this way - maybe there was a different reason, but I guess we can rewrite those test to use our test-clients.

But the rewriting itself should be for a different PR.

Yeah, I think we should think about what the purpose of the test is.

I agree, we definitely could do an investigation into such test cases and find out if they are necessary. :) I can create an issue for that.

see-quick · 2022-10-27T09:55:28Z

btw it seems that a few test cases are failing in ListenersST due to the TLS hostname verification...with following errror:

Caused by: javax.net.ssl.SSLHandshakeException: No subject alternative DNS name matching my-cluster-82bc9931-kafka-bootstrap found.
	at sun.security.ssl.Alert.createSSLException(Alert.java:131) ~[?:?]
	at sun.security.ssl.TransportContext.fatal(TransportContext.java:353) ~[?:?]
	at sun.security.ssl.TransportContext.fatal(TransportContext.java:296) ~[?:?]
	at sun.security.ssl.TransportContext.fatal(TransportContext.java:291) ~[?:?]
	at sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1357) ~[?:?]
	at sun.security.ssl.CertificateMessage$T13CertificateConsumer.onConsumeCertificate(CertificateMessage.java:1232) ~[?:?]
	at sun.security.ssl.CertificateMessage$T13CertificateConsumer.consume(CertificateMessage.java:1175) ~[?:?]
	at sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392) ~[?:?]
	at sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:443) ~[?:?]
	at sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(SSLEngineImpl.java:1074) ~[?:?]
	at sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(SSLEngineImpl.java:1061) ~[?:?]
	at java.security.AccessC

I am surprised about that because the client (producer) is configured to use an internal TLS listener and not Nodeport...

Signed-off-by: see-quick <[email protected]>

see-quick · 2022-10-27T12:34:14Z

Update: I have managed to solve issue with the latest commit :)

see-quick · 2022-10-27T13:01:09Z

/azp run regression

azure-pipelines · 2022-10-27T13:01:18Z

Azure Pipelines successfully started running 1 pipeline(s).

see-quick · 2022-11-03T14:56:17Z

/azp run regression

azure-pipelines · 2022-11-03T14:56:31Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: see-quick <[email protected]>

see-quick · 2022-11-04T11:44:01Z

/azp run regression

azure-pipelines · 2022-11-04T11:44:11Z

Azure Pipelines successfully started running 1 pipeline(s).

systemtest/src/main/java/io/strimzi/systemtest/security/SystemTestCertManager.java

systemtest/src/test/java/io/strimzi/systemtest/bridge/HttpBridgeKafkaExternalListenersST.java

Signed-off-by: see-quick <[email protected]>

see-quick · 2022-11-10T10:58:16Z

/azp run regression

azure-pipelines · 2022-11-10T10:58:27Z

Azure Pipelines successfully started running 1 pipeline(s).

see-quick · 2022-11-11T10:45:07Z

I noticed that the test case testCaRenewalBreakInMiddle is failing for some reason. (but it is not related to my PR). I can create issue for fixing that test case :).

see-quick · 2022-11-11T10:45:14Z

/azp run regression

azure-pipelines · 2022-11-11T10:45:26Z

Azure Pipelines successfully started running 1 pipeline(s).

[system test] -> internal Kafka clients should do TLS hostname verifi…

fc20eb7

…cation except Nodeports Signed-off-by: see-quick <[email protected]>

see-quick added this to the 0.32.0 milestone Oct 26, 2022

see-quick added the System tests label Oct 26, 2022

see-quick self-assigned this Oct 26, 2022

scholzj reviewed Oct 26, 2022

View reviewed changes

add SANs Kafka brokers in ListenersST test suite to fix problem

6a0ee25

Signed-off-by: see-quick <[email protected]>

see-quick requested a review from Frawless October 27, 2022 13:11

scholzj modified the milestones: 0.32.0, 0.33.0 Oct 28, 2022

see-quick added 2 commits November 4, 2022 12:38

fix testCustomCertNodePortAndTlsRollingUpdate test case

de80b47

Signed-off-by: see-quick <[email protected]>

let messages count

08904ef

Signed-off-by: see-quick <[email protected]>

see-quick added the needs review label Nov 4, 2022

Frawless approved these changes Nov 4, 2022

View reviewed changes

im-konge approved these changes Nov 4, 2022

View reviewed changes

scholzj reviewed Nov 7, 2022

View reviewed changes

systemtest/src/main/java/io/strimzi/systemtest/security/SystemTestCertManager.java Outdated Show resolved Hide resolved

systemtest/src/test/java/io/strimzi/systemtest/bridge/HttpBridgeKafkaExternalListenersST.java Show resolved Hide resolved

remove sans from inter

df449a8

Signed-off-by: see-quick <[email protected]>

scholzj approved these changes Nov 10, 2022

View reviewed changes

see-quick mentioned this pull request Nov 10, 2022

Remove un-necessary NodePorts from system test. #7608

Closed

see-quick removed the needs review label Nov 10, 2022

see-quick added the ready for merge Label for PRs which are ready for merge label Nov 15, 2022

see-quick merged commit 115dd4e into strimzi:main Nov 15, 2022

im-konge mentioned this pull request Nov 18, 2022

[ST] Kafka clients used in system tests should do TLS hostname verification #7518

Closed

im-konge mentioned this pull request Feb 21, 2023

[systemtest] Fix routes tests in ListenersST and add route tag #8138

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal Kafka clients should do TLS hostname verification except Nodeports #7526

Internal Kafka clients should do TLS hostname verification except Nodeports #7526

see-quick commented Oct 26, 2022 •

edited

Loading

see-quick commented Oct 26, 2022

azure-pipelines bot commented Oct 26, 2022

scholzj Oct 26, 2022

see-quick Oct 26, 2022

scholzj Oct 26, 2022

see-quick Oct 26, 2022

scholzj Oct 26, 2022

im-konge Oct 26, 2022 •

edited

Loading

see-quick Oct 27, 2022

see-quick commented Oct 27, 2022 •

edited

Loading

see-quick commented Oct 27, 2022

see-quick commented Oct 27, 2022

azure-pipelines bot commented Oct 27, 2022

see-quick commented Nov 3, 2022

azure-pipelines bot commented Nov 3, 2022

see-quick commented Nov 4, 2022

azure-pipelines bot commented Nov 4, 2022

see-quick commented Nov 10, 2022

azure-pipelines bot commented Nov 10, 2022

see-quick commented Nov 11, 2022

see-quick commented Nov 11, 2022

azure-pipelines bot commented Nov 11, 2022

Internal Kafka clients should do TLS hostname verification except Nodeports #7526

Internal Kafka clients should do TLS hostname verification except Nodeports #7526

Conversation

see-quick commented Oct 26, 2022 • edited Loading

Type of change

Description

Checklist

see-quick commented Oct 26, 2022

azure-pipelines bot commented Oct 26, 2022

scholzj Oct 26, 2022

Choose a reason for hiding this comment

see-quick Oct 26, 2022

Choose a reason for hiding this comment

scholzj Oct 26, 2022

Choose a reason for hiding this comment

see-quick Oct 26, 2022

Choose a reason for hiding this comment

scholzj Oct 26, 2022

Choose a reason for hiding this comment

im-konge Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

see-quick Oct 27, 2022

Choose a reason for hiding this comment

see-quick commented Oct 27, 2022 • edited Loading

see-quick commented Oct 27, 2022

see-quick commented Oct 27, 2022

azure-pipelines bot commented Oct 27, 2022

see-quick commented Nov 3, 2022

azure-pipelines bot commented Nov 3, 2022

see-quick commented Nov 4, 2022

azure-pipelines bot commented Nov 4, 2022

see-quick commented Nov 10, 2022

azure-pipelines bot commented Nov 10, 2022

see-quick commented Nov 11, 2022

see-quick commented Nov 11, 2022

azure-pipelines bot commented Nov 11, 2022

see-quick commented Oct 26, 2022 •

edited

Loading

im-konge Oct 26, 2022 •

edited

Loading

see-quick commented Oct 27, 2022 •

edited

Loading