Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove default remote certificate callback #3329

Merged
merged 4 commits into from
May 5, 2023

Conversation

timtay-microsoft
Copy link
Member

@timtay-microsoft timtay-microsoft commented May 5, 2023

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)

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.

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
@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@abhipsaMisra
Copy link
Member

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

@drwill-ms
Copy link
Contributor

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
Copy link
Member Author

timtay-microsoft commented May 5, 2023

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
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This reverts commit c40179d.
@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft timtay-microsoft merged commit 8289847 into previews/v2 May 5, 2023
@timtay-microsoft timtay-microsoft deleted the timtay/defaultCallback branch May 5, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants