Change for ALPN support. #5
Conversation
Add property of ALPN protocol names, and add constructor for use ALPN.
M2Mqtt/Net/MqttNetworkChannel.cs
Outdated
| try | ||
| { | ||
| this.sslStream.AuthenticateAsClientAsync(authOptions).Wait(); | ||
| } | ||
| catch(Exception ex) | ||
| { | ||
| throw ex; | ||
| } |
There was a problem hiding this comment.
What is the purpose of this try-catch if you want to re-throw the exception?
There was a problem hiding this comment.
It's useless try-catch. Sorry, I was checking AuthenticateAsClientAsync's Exception with "ex" on error case and missed to delete.
I delete this try-catch.
M2Mqtt/Net/MqttNetworkChannel.cs
Outdated
| /// <param name="userCertificateValidationCallback">A LocalCertificateSelectionCallback delegate responsible for selecting the certificate used for authentication</param> | ||
| public MqttNetworkChannel(string remoteHostName, int remotePort, bool secure, X509Certificate caCert, X509Certificate clientCert, MqttSslProtocols sslProtocol, | ||
| RemoteCertificateValidationCallback userCertificateValidationCallback, | ||
| LocalCertificateSelectionCallback userCertificateSelectionCallback, List<string> alpnProtocols) |
There was a problem hiding this comment.
Please make the parameter optional or introduce an overload. And align all usages with null for this param.
There was a problem hiding this comment.
I'm very sorry I'm not sure what you mean in this comment.
This constructor which I add is added as new another one, so it's already an overload constructor.
Some MqttNetworkChannel constructor existing already, so I thought I should not change exsisting constructors and I added another.
You mean, I should set to parameter "aplnProtocols" a default value null?
... userCertificateSelectionCallback, List<string> alpnProtocols = null)
Please tell me more detail you mean.
(It's very helpful for me if you show some code snippet or something)
There was a problem hiding this comment.
Please make your constructor default one and previous one calling this one. I think this would be cleaner and all code concentrated on one place. Or remove yours and add optional parameter to the default one.
|
Thank you for your checking for pull request. thank you |
M2Mqtt/Net/MqttNetworkChannel.cs
Outdated
| /// <param name="userCertificateValidationCallback">A LocalCertificateSelectionCallback delegate responsible for selecting the certificate used for authentication</param> | ||
| public MqttNetworkChannel(string remoteHostName, int remotePort, bool secure, X509Certificate caCert, X509Certificate clientCert, MqttSslProtocols sslProtocol, | ||
| RemoteCertificateValidationCallback userCertificateValidationCallback, | ||
| LocalCertificateSelectionCallback userCertificateSelectionCallback, List<string> alpnProtocols) |
There was a problem hiding this comment.
Please make your constructor default one and previous one calling this one. I think this would be cleaner and all code concentrated on one place. Or remove yours and add optional parameter to the default one.
|
I understand your comments. And I commented about "alpnProtocols" parameter a little in README.md . |
|
Thank you for your review and merge! |
Add property of ALPN protocol names, and add constructor for use ALPN.
summary
Hi, I'm simotin13, sorry for sending you a sudden pull request.
I'm using M2MqttDotnetCore for my application(.Net Core app).
In my Application, I need to access to AWS IoT Core MQTT Broker And It's support ALPN.
https://aws.amazon.com/about-aws/whats-new/2018/02/aws-iot-core-now-supports-mqtt-connections-with-certificate-based-client-authentication-on-port-443/?nc1=h_ls
I had to use ALPN, So I added ALPN feature to this repository.
https://github.com/simotin13/paho.mqtt.m2mqtt/tree/features/add_ALPN_support
I want merge my changes into repository if It's OK.
If my changes have any problems, I will fix them.
Changes
2 files changed,
MqttNetworkChannel.cs
I add member property "alpnProtocols" for ALPN protocol names, And if alpnProtocols isn't empty connect to host using ALPN.
Also Add Constructor of MqttNetworkChannel, add argument for ALPN protocol names.
MqttClient.cs
I changed Init method, add argument for for ALPN protocol names.
sample and test
I create a sample project for checking my changes, and I tested that I able to use AWS ALPN.
https://github.com/simotin13/test_m2mqtt
The AWS ALPN Protocol Name is "x-amzn-mqtt-ca".
About AWS ALPN,
https://aws.amazon.com/blogs/iot/mqtt-with-tls-client-authentication-on-port-443-why-it-is-useful-and-how-it-works/
Also tested connet to local mosquitto broker for checking I don't brake repository code.
I tested without SSL/TLS connect(1883 port), with user/password connect, and SSL/TLS(8883 port) and It works and looks good to me.
Thank you.