-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
net: mqtt: ALPN Support for socket mqtt #69531
Conversation
048510f
to
da0b628
Compare
Could you add a commit to this PR that adds the support to the AWS sample? |
include/zephyr/net/mqtt.h
Outdated
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove empty lines.
subsys/net/lib/mqtt/Kconfig
Outdated
@@ -29,6 +29,11 @@ config MQTT_LIB_TLS | |||
help | |||
Enable TLS support for socket MQTT Library | |||
|
|||
config MQTT_LIB_TLS_USE_ALPN | |||
bool "ALPN support for mqtt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add depends on MQTT_LIB_TLS
@@ -69,6 +69,20 @@ int mqtt_client_tls_connect(struct mqtt_client *client) | |||
} | |||
} | |||
|
|||
#if defined(CONFIG_MQTT_LIB_TLS_USE_ALPN) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove empty lines around if block
#if defined(CONFIG_MQTT_LIB_TLS_USE_ALPN) | ||
|
||
if (tls_config->alpn_protocol_name_list != NULL && | ||
tls_config->alpn_protocol_name_count > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please align to the opening bracket.
@ZYNQHRONIZE Please squash the four commits you added to the first one as we do not use fixup commits in zephyr. |
da0b628
to
4411c68
Compare
Implement the ALPN Support for Mqtt Library allow mqtt to have ability to utilize ALPN for connect to server that support ALPN, such as AWS IoT Core Signed-off-by: sukrit buddeewong <[email protected]>
4411c68
to
3856874
Compare
fb13292
to
f3a1cfa
Compare
|
||
config AWS_USE_MQTT_OVER_PORT_443 | ||
bool "enable to use port 443 for MQTT protocol" | ||
default n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options are off by default so this line can be removed.
@@ -73,6 +73,12 @@ config AWS_EXPONENTIAL_BACKOFF | |||
help | |||
Enable AWS exponential backoff for reconnecting to AWS MQTT broker. | |||
|
|||
|
|||
config AWS_USE_MQTT_OVER_PORT_443 | |||
bool "enable to use port 443 for MQTT protocol" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not look correct if we have a config option that forces to use one port value.
Perhaps divide this to two settings
config AWS_USE_MQTT_OVER_TLS
bool "Use MQTT over TLS"
....
config AWS_MQTT_TLS_PORT
int "MQTT TLS port"
default 443
range 1 65535
depends on AWS_USE_MQTT_OVER_TLS
....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, however I'm don't think the first config would actually be needed, as we always use TLS with AWS.
The difference here is, whether to use TLS ALPN extension or not. @ZYNQHRONIZE I assume that connecting over port 8883 w/o ALPN is still a valid method to connect to AWS? Is that correct? Perhaps we should keep it as a default, and add
config AWS_USE_TLS_ALPN
bool "Use TLS ALPN extension when connecting to AWS"
And then, as Jukka suggested, another config for the port value. But instead, default to 8883, and optionally default 443 if AWS_USE_TLS_ALPN
. WDYT?
We could also, instead of changing the default config file, add an overlay config that would enable ALPN support in the sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes if you want to connect with 8883, ALPN its not required. But if you want to use MQTT over port 443, it's require as shown in table below. about if AWS_USE_TLS_ALPN ALPN was set then port will set to 443 by default, I think it's ok if we didn't have a plant to support AWS's Custom authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done that way as well, ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds ok to me.
@jukkar @rlubos
Port 443
|
7dadd43
to
a09a356
Compare
CI complains:
Recent IPv4 address changes in net_if requires some changes in the sample
|
@jukkar can you provide me more details about Recent IPv4 address changes. |
This
needs to be changed to
For the |
@jukkar seems ok now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! Overall changes look good, just one minor issue spotted that needs fixing.
@@ -54,6 +54,10 @@ static uint32_t messages_received_counter; | |||
static bool do_publish; /* Trigger client to publish */ | |||
static bool do_subscribe; /* Trigger client to subscribe */ | |||
|
|||
#if (CONFIG_AWS_MQTT_PORT == 443 && !CONFIG_MQTT_LIB_WEBSOCKET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if !CONFIG_MQTT_LIB_WEBSOCKET
would be a correct syntax, the symbol might be undefined. I think it would be better to use:
#if (CONFIG_AWS_MQTT_PORT == 443) && !defined(CONFIG_MQTT_LIB_WEBSOCKET)
@@ -266,6 +270,10 @@ static void aws_client_setup(void) | |||
tls_config->sec_tag_count = ARRAY_SIZE(sec_tls_tags); | |||
tls_config->hostname = CONFIG_AWS_ENDPOINT; | |||
tls_config->cert_nocopy = TLS_CERT_NOCOPY_NONE; | |||
#if (CONFIG_AWS_MQTT_PORT == 443 && !CONFIG_MQTT_LIB_WEBSOCKET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
due to MQTT now have ALPN support the example code of using ALPN to connect MQTT over port 443 need to be added Signed-off-by: sukrit buddeewong <[email protected]>
ALPN Support For MQTT Library
introduction
Implement the ALPN Support for MQTT Library allow MQTT to have
ability to utilize ALPN for connect to server that support ALPN such as AWS IoT Core.
With ALPN AWS IoT Core will allow device to establish MQTT connection over port 443
Testing
This code was test on modified AWS IoT Sample.
prj.conf
CONFIG_MBEDTLS_SSL_ALPN=y
CONFIG_MQTT_LIB_TLS_USE_ALPN=y
in KConfig
In code
Result
The System was able to Sub and pub data over MQTT on port 443 as shown below