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

[esp-tls] Add addr_family option to esp_tls_cfg_t (IDFGH-9620) #10967

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

mmmspatz
Copy link
Contributor

@mmmspatz mmmspatz commented Mar 11, 2023

Presently, esp-tls effectively does not support IPV6, as reported here: #9920
Basically: if DNS is able to return A records, esp-tls will always try to use them even if ipv4 connectivity is not available. However, if an A lookup fails, an AAAA lookup is attempted next, and IPV6 can work.

This behavior comes from passing AF_UNSPEC to getaddrinfo() here, which causes it to prefer IPV4 over IPV6 here. here is the spot in LwIP where an AAAA lookup is done only after an A lookup fails.

I see two options to improve this:

  1. Teach tcp_connect() to make a decision about which address family to connect to. Perhaps if the first call to connect() returns EHOSTUNREACH, do a second lookup passing AF_INET6, and then make a second connection attempt if an AAAA record is returned.
  2. Add a field to esp_tls_cfg_t that allows the user to explicitly specify whether to pass AF_UNSPEC, AF_INET, or AF_INET6 to getaddrinfo().

In this PR, I have implemented option 2. Here is how I am using it in the application I am currently working on:

#include "esp_tls.h"
#include "lwip/nd6.h"

esp_tls_cfg_t tls_cfg = {
  // insert config here
};

// Test if we have a default ipv6 route, and if so make the connection over ipv6
const ip_addr_t test_dst = IPADDR6_INIT_HOST(0x20010002, 0x0, 0x0, 0x0);
if (nd6_find_route(ip_2_ip6(&test_dst))) {
    tls_cfg.addr_family = ESP_TLS_AF_INET6;
}

esp_tls_t* tls = esp_tls_init();
esp_tls_conn_new_sync(...,  &tls_cfg, tls);

One might argue that instead of adding a field to esp_tls_cfg_t, we should add an argument to the esp_tls_conn_new*() functions. The documentation for esp_tls_conn_new_sync() suggests that the cfg field can be NULL if "If you wish to open non-TLS connection", though I'm not sure it actually handles a NULL input safely. And that is what the is_plain_tcp field is for anyways. My reason for preferring to add a cfg field is that it is backwards compatible: If the esp_tls_cfg_t is zero initialized, the field defaults to ESP_TLS_AF_UNSPEC, which gives exactly the same behavior as before.

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Mar 11, 2023
@github-actions github-actions bot changed the title [esp-tls] Add addr_family option to esp_tls_cfg_t [esp-tls] Add addr_family option to esp_tls_cfg_t (IDFGH-9620) Mar 11, 2023
@mmmspatz mmmspatz force-pushed the mspatz/esp_tls_addr_family branch from d122768 to 0abd1cb Compare March 12, 2023 21:41
@mmmspatz mmmspatz marked this pull request as ready for review March 12, 2023 21:51
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@mahavirj
Copy link
Member

sha=0abd1cb51f0a346adcda932e25182692b1f642e1

@mahavirj mahavirj added the PR-Sync-Merge Pull request sync as merge commit label Mar 23, 2023
@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Resolution: NA Issue resolution is unavailable labels Mar 24, 2023
@espressif-bot espressif-bot merged commit 8d90249 into espressif:master Mar 27, 2023
@mmmspatz mmmspatz deleted the mspatz/esp_tls_addr_family branch March 27, 2023 15:05
@AxelLin
Copy link
Contributor

AxelLin commented Apr 7, 2023

@mahavirj
Could you backport this to stable branch 5.0+?
https://www.esp32.com/viewtopic.php?f=13&t=33033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants