Skip to content

Commit

Permalink
Fix wrong TLS usage (#1833)
Browse files Browse the repository at this point in the history
* Fix wrong usage of TLS options

* Fix public broker tests

* Update ReleaseNotes.md
  • Loading branch information
chkr1011 authored Sep 6, 2023
1 parent cae7206 commit 6fdee9e
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 16 deletions.
1 change: 1 addition & 0 deletions .github/workflows/ReleaseNotes.md
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
* [Client] Fixed wrong TLS options handling (#1830).
* [Client] Fixed NullReferenceExeption when performing a Ping when the client is not connected (#1831).
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ public Task ConnectAsync(CancellationToken cancellationToken)
var uri = _webSocketOptions.Uri;
if (!uri.StartsWith("ws://", StringComparison.OrdinalIgnoreCase) && !uri.StartsWith("wss://", StringComparison.OrdinalIgnoreCase))
{
if (_webSocketOptions.TlsOptions?.UseTls == false)
if (_webSocketOptions.TlsOptions?.UseTls == true)
{
uri = "ws://" + uri;
uri = "wss://" + uri;
}
else
{
uri = "wss://" + uri;
uri = "ws://" + uri;
}
}

Expand Down
22 changes: 13 additions & 9 deletions Source/MQTTnet.TestApp/PublicBrokerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ public static async Task RunAsync()
UseTls = true,
SslProtocol = SslProtocols.Tls13,
// Don't use this in production code. This handler simply allows any invalid certificate to work.
CertificateValidationHandler = w => true
AllowUntrustedCertificates = true,
IgnoreCertificateChainErrors = true,
CertificateValidationHandler = _ => true
};
#endif
// Also defining TLS12 for servers that don't seem no to support TLS13.
Expand All @@ -34,7 +36,9 @@ public static async Task RunAsync()
UseTls = true,
SslProtocol = SslProtocols.Tls12,
// Don't use this in production code. This handler simply allows any invalid certificate to work.
CertificateValidationHandler = w => true
AllowUntrustedCertificates = true,
IgnoreCertificateChainErrors = true,
CertificateValidationHandler = _ => true
};

// mqtt.eclipseprojects.io
Expand Down Expand Up @@ -97,10 +101,10 @@ await ExecuteTestAsync(
"test.mosquitto.org WS TLS12",
new MqttClientOptionsBuilder().WithWebSocketServer(o => o.WithUri("test.mosquitto.org:8081/mqtt")).WithProtocolVersion(MqttProtocolVersion.V311).WithTlsOptions(unsafeTls12).Build());

// await ExecuteTestAsync(
// "test.mosquitto.org WS TLS12 (WebSocket4Net)",
// new MqttClientOptionsBuilder().WithWebSocketServer("test.mosquitto.org:8081/mqtt").WithProtocolVersion(MqttProtocolVersion.V311).WithTls(unsafeTls12).Build(),
// true);
await ExecuteTestAsync(
"test.mosquitto.org WS TLS12 (WebSocket4Net)",
new MqttClientOptionsBuilder().WithWebSocketServer(o => o.WithUri("test.mosquitto.org:8081/mqtt")).WithProtocolVersion(MqttProtocolVersion.V311).WithTlsOptions(unsafeTls12).Build(),
true);

// broker.emqx.io
await ExecuteTestAsync(
Expand Down Expand Up @@ -150,7 +154,6 @@ await ExecuteTestAsync(
true);

// mqtt.swifitch.cz: Does not seem to operate any more

// cloudmqtt.com: Cannot test because it does not offer a free plan any more.

Write("Finished.", ConsoleColor.White);
Expand Down Expand Up @@ -197,9 +200,10 @@ static async Task ExecuteTestAsync(string name, MqttClientOptions options, bool

Write("[OK]\n", ConsoleColor.Green);
}
catch (Exception e)
catch (Exception exception)
{
Write("[FAILED] " + e.Message + "\n", ConsoleColor.Red);
Write("[FAILED]" + Environment.NewLine, ConsoleColor.Red);
Write(exception + Environment.NewLine, ConsoleColor.Red);
}
}

Expand Down
7 changes: 6 additions & 1 deletion Source/MQTTnet/Client/Options/MqttClientOptionsBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ public MqttClientOptions Build()
throw new InvalidOperationException("A channel must be set.");
}

var tlsOptions = _tlsOptions;
// The user can specify the TCP options with already configured TLS options
// or start with TLS settings not knowing which transport will be used (depending
// on the order of called methods from the builder).
// The builder prefers the explicitly set TLS options!
var tlsOptions = _tlsOptions ?? _tcpOptions?.TlsOptions;

if (_tlsParameters != null)

Check warning on line 41 in Source/MQTTnet/Client/Options/MqttClientOptionsBuilder.cs

View workflow job for this annotation

GitHub Actions / build

'MqttClientOptionsBuilder._tlsParameters' is obsolete

Check warning on line 41 in Source/MQTTnet/Client/Options/MqttClientOptionsBuilder.cs

View workflow job for this annotation

GitHub Actions / build

'MqttClientOptionsBuilder._tlsParameters' is obsolete
{
if (_tlsParameters?.UseTls == true)

Check warning on line 43 in Source/MQTTnet/Client/Options/MqttClientOptionsBuilder.cs

View workflow job for this annotation

GitHub Actions / build

'MqttClientOptionsBuilder._tlsParameters' is obsolete

Check warning on line 43 in Source/MQTTnet/Client/Options/MqttClientOptionsBuilder.cs

View workflow job for this annotation

GitHub Actions / build

'MqttClientOptionsBuilder._tlsParameters' is obsolete
Expand Down
6 changes: 3 additions & 3 deletions Source/MQTTnet/Implementations/MqttWebSocketChannel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ public async Task ConnectAsync(CancellationToken cancellationToken)
var uri = _options.Uri;
if (!uri.StartsWith("ws://", StringComparison.OrdinalIgnoreCase) && !uri.StartsWith("wss://", StringComparison.OrdinalIgnoreCase))
{
if (_options.TlsOptions?.UseTls == false)
if (_options.TlsOptions?.UseTls == true)
{
uri = "ws://" + uri;
uri = "wss://" + uri;
}
else
{
uri = "wss://" + uri;
uri = "ws://" + uri;
}
}

Expand Down

0 comments on commit 6fdee9e

Please sign in to comment.