Azure / azure-iot-sdk-csharp

A C# SDK for connecting devices to Microsoft Azure IoT services
Other
464 stars 493 forks source link

Remove default remote certificate callback #3329

Closed timtay-microsoft closed 1 year ago

timtay-microsoft commented 1 year ago

The ClientWebSocket instance used by our MQTT/AMQP libraries already have a default remote certificate validation callback, so there is no need for us to define it as well. Simply leaving this field as null will make these libraries use their default implementation.

In addition, now that we aren't passing in a default value for this field every time, we don't have to worry about the .NET 472 case that #3328 had to work around.

IoT hub device client already had this behavior, but now the provisioning device client and the IoT hub service client do as well. This is why we only saw these .NET 472 test failures in our DPS tests (since no Hub service client operations use MQTT)

timtay-microsoft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
abhipsaMisra commented 1 year ago

Should we remove the check that was added in #3328 ?

drwill-ms commented 1 year ago

The ClientWebSocket instance used by our MQTT/AMQP libraries already have a default remote certificate validation callback, so there is no need for us to define it as well. Simply leaving this field as null will make these libraries use their default implementation.

In addition, now that we aren't passing in a default value for this field every time, we don't have to worry about the .NET 472 case that #3328 had to work around.

IoT hub device client already had this behavior, but now the provisioning device client and the IoT hub service client do as well. This is why we only saw these .NET 472 test failures in our DPS tests (since no Hub service client operations use MQTT)

What about HTTP calls?

timtay-microsoft commented 1 year ago

The ClientWebSocket instance used by our MQTT/AMQP libraries already have a default remote certificate validation callback, so there is no need for us to define it as well. Simply leaving this field as null will make these libraries use their default implementation. In addition, now that we aren't passing in a default value for this field every time, we don't have to worry about the .NET 472 case that #3328 had to work around. IoT hub device client already had this behavior, but now the provisioning device client and the IoT hub service client do as well. This is why we only saw these .NET 472 test failures in our DPS tests (since no Hub service client operations use MQTT)

What about HTTP calls?

Good question. The same behavior from the transport library is there ("if you don't set this callback, we have a default implementation for you"), but it looks like we are currently assuming we have an implementation, so I'll fix that

Should we remove the check that was added in #3328 ?

Yes. Looks like I forgot to commit that, so I've added that change now

timtay-microsoft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
timtay-microsoft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).