-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
add sni support (IDFGH-8363) #9833
Conversation
I believe PRs are supposed to be made against the master branch. |
For some odd reason I cannot get current master to compile, so I can't port this to the current master, and there are merge conflicts, so I can't confidently resolve them. |
@axos88 Thanks for adding the support of the feature in ESP-IDF. Just to note here, We are using |
@AdityaHPatwardhan, I am using esp-rs/esp-idf-sys to build esp-idf, but I get the following error on master (works on latest stable though):
|
95af1bd
to
b1c5f53
Compare
fixed build issue, rebased onto master. |
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.
@axos88 Thanks for updating the changes.
I have reviewed them in detail. I have left some comments and suggestions.
I also wanted to request you, if possible could you keep a separate commit for the fix that you have added? I think we may have to backport it.
components/esp-tls/esp_tls_mbedtls.c
Outdated
#if defined(CONFIG_ESP_TLS_SERVER_SNI_HOOK) | ||
if (cfg->sni_callback != NULL) { | ||
ESP_LOGI(TAG, "Initializing server side SNI callback"); | ||
mbedtls_ssl_conf_sni(&tls->conf, cfg->sni_callback, cfg->sni_callback_p_info); | ||
} | ||
#endif |
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.
This part should come under set_server_config
API since this part also does server configuration.
I think we can keep this conf_sni
part in the else
condition added above.
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 this is the case. The set_server_config is called when a new server side socket is created, while server_session_create is called each time a client connection is accepted. It is at that point that SNI has to be initialized, per incoming socket.
Adding it to set_server_config did not actually activate SNI as far as I remember - that's what I tried first.
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.
@axos88 Sorry for the delayed reply, I wanted to test this first at my end before commenting.
I will do that now and give my comments in some time.
Do you have a MWE which I could use to test out this feature ?
Thanks!
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.
Not currently, and I won't be able to provide you one until next week (I'm currently away from home).
My project is written in rust though with the esp-rs/esp-idf-sys
wrapper, so I'm not sure it's of much help to you if you want to do the testing directly.
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.
Hi @axos88 I was able to test the SNI feature locally.
I can confirm that with current approach if servercert
and server key
are not supplied and we configure the sni_callback using mbedtls_ssl_conf_sni
in the ser_server_config
function. Then the sni callback is set at once and is used for every connection formed by that specific context. There is no need to set the sni callback each time in the esp_mbedtls_server_session_create
. I will update my comments based on that.
@AdityaHPatwardhan let me know if you have outstanding concerns. |
components/esp-tls/esp_tls_mbedtls.c
Outdated
@@ -566,7 +566,16 @@ esp_err_t set_server_config(esp_tls_cfg_server_t *cfg, esp_tls_t *tls) | |||
return esp_ret; | |||
} | |||
} else { | |||
#if defined(CONFIG_ESP_TLS_SERVER_SNI_HOOK) | |||
if (cfg->sni_callback == NULL) { |
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 think the SNI callback needs to be configured in this function ( but outside this if-else condition.)
I think only the check if sni_callback is present needs to be in the else
part.
We should allow users to set the sni_callback along with providing server cert and keys. I think the sni callback would only be triggered when the ClientHello message would contain the ServerName
extension. Otherwise, the normal certs would be used.
free((void *) cfg->servercert_buf); | ||
free((void *) cfg->cacert_buf); | ||
free(cfg); | ||
free(ssl_ctx); | ||
return NULL; |
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 think this part is duplicated 3 times, can we have a goto:
statement for this so that the code would jump to this in case of error.
@axos88 Sorry for the delayed review, Thanks again for your contribution for adding the sni feature in esp-tls. |
Thanks, I'll update the pr next week, when I'm back |
…since it has access to more information, so the application can make a more informed decision on which certificate to serve (such as alpn value, server certificate type, etc.)
355e718
to
39a6ec0
Compare
Updated per your comments. Also discovered the certificate selection hook in mbedtls Mbed-TLS/mbedtls#5430, so moved to use that one instead of the SNI hook, since it has guaranteed access to all TLS extensions received in the ClientHello. This means that the application can make a better decision on which certificate to send, based on the ALPN, or the server certificate type, or any other TLS extension received. This callback does not take a void* unfortunately, so it is a bit more circumstancial to use IMHO, so I have added support for setting the user_data of the ssl_conf as well, so one can supply their inputs there. |
@axos88 Thanks for updating the MR. Adding the option for Also I was also curious as to what the reply for Mbed-TLS/mbedtls#5454 (comment) would be from mbedTLS side, I think that would help me better understand the feature and its application here. |
No, if the cert_callback does not set the private key and the ca_cert, the ones provided will be used by default, so it does make sense to allocate memory if the user wants a default. The cert_selection callback can be used for setitng other parameters, such as verification mode, client ca bundle, etc. |
Yeah I'm curious too, but as far as I have seen this is a strict superset. Just made that comment to have an official confirmation on this. As far as https://mbed-tls.readthedocs.io/en/latest/kb/how-to/use-sni/ is concerned, the certificate_selection callback can call all of these functions described there, but has access to more information. |
@axos88 Thank you very much for the PR. I will see to it that these changes are merged in our internal code-base, after that they should be available on GitHub in a few days. |
So do I understand correctly that GitHub is a mirror of some interior repository you use? Saw some strange comments in some PRS with shasums, that would explain them |
@AdityaHPatwardhan, do you have an estimate of how long until this can be merged? |
Hi @axos88 The MR should be merged and should be available on GitHub by next week. |
… access to required information so that the application can make a more informed decision on which certificate to serve (such as alpn value, server certificate type, etc.) Closes #9833 Signed-off-by: Aditya Patwardhan <[email protected]>
This PR adds full server side SNI support.