Quiet the one-way TLS certificate probe on MQTT connect - #38
Open
ShvaykaD wants to merge 6 commits into
Open
Conversation
MqttSslHandlerProvider sets needClientAuth(false) with wantClientAuth(true), so a client that declines the optional certificate is a supported one-way TLS connect, not an anomaly. getPeerCertificates() reports that by throwing SSLPeerUnverifiedException, which getX509Certificate() logged at warn on every such CONNECT. The warn is vestigial: 22e5771 carried it over verbatim while migrating from getPeerCertificateChain() to getPeerCertificates(), and it was never a deliberate choice for the one-way TLS case. Log at debug and add the [address][sessionId] prefix used elsewhere in the class -- the old message carried no correlation info at all.
onValidateDeviceResponse re-probed the TLS session only to decide whether to refine the CONNACK return code, so a failed one-way TLS connect threw and logged SSLPeerUnverifiedException twice. The probe is redundant: the guard asks whether this was a basic-credentials connect, which processConnect already decided. Record that branch in clientCertPresented and read the flag instead. The flag is set iff sslHandler != null && cert != null, whose negation is exactly the replaced condition, so the return codes are unchanged. volatile because processConnect runs on the Netty event loop while onValidateDeviceResponse runs on transportCallbackExecutor (see DefaultTransportService.doProcess). There is likely an incidental happens-before edge through the request template and the executor submission, but it runs through three layers of async machinery and should not be load-bearing for one read per connect. MqttTransportHandlerTest covers both branches through the CONNACK the channel receives, so the flag stays private. MQTT 5 is required: ReturnCodeResolver collapses BAD_USERNAME_OR_PASSWORD and NOT_AUTHORIZED_5 into one NOT_AUTHORIZED code for older clients, which would make the two cases indistinguishable and the assertions vacuous. The one-way TLS test also pins getPeerCertificates() to a single invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while testing the MQTT gateway API in one-way TLS mode.
Problem
MqttSslHandlerProvidersetsneedClientAuth(false)withwantClientAuth(true), so a client that declines the optional certificate still connects. JSSE reports the absent cert by throwingSSLPeerUnverifiedException, whichgetX509Certificate()logged atwarn— on an outcome the transport is configured to accept, and with no session or address to correlate it. It is vestigial:22e5771120carried thewarnover verbatim during the JDK 11 migration.It fired twice per failed connect, because
onValidateDeviceResponsere-probed the session.Approach
Log at
debug, with the[address][sessionId]prefix used elsewhere in the class.Drop the second probe. Its guard —
sslHandler == null || getX509Certificate() == null— doesn't want the certificate; it wants to know whether this was a basic-credentials connect, whichprocessConnectalready decided. AclientCertPresentedflag records that branch. It is set iffsslHandler != null && cert != null, whose negation is exactly the replaced condition, so return codes are unchanged.volatilebecauseprocessConnectruns on the Netty event loop andonValidateDeviceResponseontransportCallbackExecutor.No config, wire, or device-facing change.
Tests
Two tests in
MqttTransportHandlerTest, asserting through the CONNACK so the flag stays private. The one-way case assertsBAD_USERNAME_OR_PASSWORDand pinsgetPeerCertificates()to a single invocation; the two-way case assertsNOT_AUTHORIZED_5survives, which is what proves the flag is set. Both need MQTT 5 —ReturnCodeResolvercollapses those two codes into one for older clients, which would make the assertions vacuous.Mutation-checked: dropping the flag assignment fails the two-way test, restoring the old condition fails the one-way
times(1). The refactor is behaviour-preserving, so the return-code assertions alone would pass against the old code too.