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

feat(common): Add ESP DNS module with support for UDP, DoT, and DoH protocols #781

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

espressif-abhikroy
Copy link
Collaborator

@espressif-abhikroy espressif-abhikroy commented Mar 14, 2025

This commit introduces a custom DNS module for ESP32, enabling DNS resolution capabilities over various protocols including traditional UDP, TCP, DNS over TLS (DoT), and DNS over HTTPS (DoH). The module includes initialization, cleanup, and error handling functionalities, along with protocol-specific implementations for each DNS type.

Description

Related

Testing


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

Sorry, something went wrong.

@espressif-abhikroy espressif-abhikroy self-assigned this Mar 14, 2025
@espressif-abhikroy espressif-abhikroy force-pushed the component/esp_dns branch 5 times, most recently from 5a245b4 to f575341 Compare March 21, 2025 12:57
…rotocols

This commit introduces a custom DNS module for ESP32, enabling DNS resolution capabilities
over various protocols including traditional UDP, DNS over TLS (DoT), and DNS over HTTPS (DoH).
The module includes initialization, cleanup, and error handling functionalities,
along with protocol-specific implementations for each DNS type.
Comment on lines +1 to +2
| Supported Targets | ESP32 | ESP32-C2 | ESP32-C3 | ESP32-C5 | ESP32-C6 | ESP32-C61 | ESP32-H2 | ESP32-P4 | ESP32-S2 | ESP32-S3 |
| ----------------- | ----- | -------- | -------- | -------- | -------- | --------- | -------- | -------- | -------- | -------- |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually place this in example README's -- I'd suggest removing from component's docs as this DNS module should support all targets


- **Flexible Configuration**: Easily configure DNS servers, ports, timeouts, and protocol-specific options.

- **LWIP Integration**: Seamlessly integrates with the ESP-IDF networking stack through LWIP hooks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about coexistence with Thread lib (or any other user of that hook)?


Once you add this component to your project, it will replace the default LWIP DNS resolution automatically.

**⚠️ Warning:** This component cannot work alongside other components that use the CONFIG_LWIP_HOOK_NETCONN_EXT_RESOLVE_CUSTOM hook, such as the OpenThread component.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see 👍


### 1. Add Component to Your Project

Add the ESP DNS component to your project's `components` directory or include it as a dependency in your project's `idf_component.yml` file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • the most common way: idf.py add-dependency

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this README also appear in the component manager so this information is already presented to user. This section is useful only if the user is reading it out of the component manager page.

.port = 443, // Optional, default is 443
.timeout_ms = 5000, // Optional, default is 5000
.tls_config = {
.crt_bundle_attach = esp_crt_bundle_attach, // Optional, default is NULL
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esp_crt_bundle_attach is not optional, right, if it defaults to NULL?



/* Global DNS handle instance */
static esp_dns_handle_t g_dns_handle = NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static esp_dns_handle_t g_dns_handle = NULL;
static esp_dns_handle_t s_dns_handle = NULL;

since declared as static

Comment on lines +51 to +52
static struct dns_handle instance;
static bool initialized = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Could we remove the static modifiers so the component could be used with multiple handles? e.g. one DoH another DoT?


/* ========================= LWIP HOOK FUNCTIONS ========================= */

#if defined(CONFIG_LWIP_HOOK_NETCONN_EXT_RESOLVE_CUSTOM)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move it to some lwip specific module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the hook to lwip specific file.

Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful component, generally LGTM, just left a few nitpicks. Nice work!

Comment on lines +15 to +19
#define DEFAULT_TCP_PORT 53 /* Default TCP port */
#define DEFAULT_DOT_PORT 853 /* Default TLS port */
#define DEFAULT_DOH_PORT 443 /* Default HTTPS port */

#define DEFAULT_TIMEOUT_MS 5000
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest namespacing with ESP_ or ESP_DNS (since this is a public header and DEFAULT_TCP_PORT is a just a common name)

#endif /* ENABLE_TCP */

#if ENABLE_DOT_CERT
/*---------- DNS over TLS Test with server cert ----------*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these sections look very similar, maybe adding a function with config parameter could reduce code duplicaiton?

Comment on lines +80 to +83
size_t create_dns_query(uint8_t *buffer, size_t buffer_size, const char *hostname, int addrtype, uint16_t *id_o);

/* Function to parse a DNS answer for an A and AAAA records */
void parse_dns_response(uint8_t *buffer, size_t response_size, dns_response_t *dns_response);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would also suggest namespacing these. Even if this is a private header, the symbol would still reside in global symbol name and may cause collision.

uint8_t offset = 0;

/* Loop through each part of the name, handling labels and compression pointers */
while (ptr[offset] != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also check if wer'e not past the parsed data? (you can receive anything from the network, e.g. a malicious packet)?

Comment on lines +12 to +15
#include "lwip/prot/dns.h"
#include "lwip/api.h"
#include "lwip/opt.h"
#include "lwip/dns.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove lwip dependency here?

*
* SPDX-License-Identifier: Apache-2.0
*/
#include "freertos/FreeRTOS.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also , do we need freertos in this module?

Comment on lines +302 to +307
case DNS_PROTOCOL_TCP:
*err = dns_resolve_tcp(g_dns_handle, name, addr, rrtype);
break;
case DNS_PROTOCOL_DOT:
*err = dns_resolve_dot(g_dns_handle, name, addr, rrtype);
break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, can we use the factory design pattern here? (e.g. if a user wants DNS over TCP, they don't need to pull mbedtls into the link dependency)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expose all the init function and remove the common init.

&handle->response_buffer.dns_response);
} else {
ESP_LOGE(TAG, "HTTP Error: %d", esp_http_client_get_status_code(evt->client));
ESP_LOG_BUFFER_HEXDUMP(TAG, handle->response_buffer.buffer, handle->response_buffer.length, ESP_LOG_ERROR);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question: In case of error response should the handle->response_buffer.buffer and handle->response_buffer.length be checked for correctness before print the buffer here?

typedef struct dns_handle* esp_dns_handle_t;


esp_dns_handle_t esp_dns_init(const esp_dns_config_t *config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation.

PRIV_INCLUDE_DIRS "."
PRIV_REQUIRES nvs_flash lwip esp_event esp-tls esp_http_client esp-tls tcp_transport)

if(CONFIG_LWIP_HOOK_NETCONN_EXT_RESOLVE_CUSTOM)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this required for this component to work? We should add an error in case it is not enabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a warning that the hook is not enabled.

Comment on lines +9 to +18
- [Features](#features)
- [How It Works](#how-it-works)
- [Certificate Options](#certificate-options)
- [How to Use](#how-to-use)
- [Configuration](#configuration)
- [Common DNS Servers](#common-dns-servers)
- [Requirements](#requirements)
- [Limitations](#limitations)
- [Performance Considerations](#performance-considerations)
- [Troubleshooting](#troubleshooting)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [Features](#features)
- [How It Works](#how-it-works)
- [Certificate Options](#certificate-options)
- [How to Use](#how-to-use)
- [Configuration](#configuration)
- [Common DNS Servers](#common-dns-servers)
- [Requirements](#requirements)
- [Limitations](#limitations)
- [Performance Considerations](#performance-considerations)
- [Troubleshooting](#troubleshooting)
- [Features](#features)
- [Requirements](#requirements)
- [How to Use](#how-to-use)
- [Configuration](#configuration)
- [Certificate Options](#certificate-options)
- [Common DNS Servers](#common-dns-servers)
- [Limitations](#limitations)
- [Performance Considerations](#performance-considerations)
- [How It Works](#how-it-works)
- [Troubleshooting](#troubleshooting)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reorder the outline for a better reading flow.


### 1. Add Component to Your Project

Add the ESP DNS component to your project's `components` directory or include it as a dependency in your project's `idf_component.yml` file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this README also appear in the component manager so this information is already presented to user. This section is useful only if the user is reading it out of the component manager page.

#include "freertos/semphr.h"
#include "freertos/task.h"
#include "esp_log.h"
//#include "esp_netif.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean up.

if (handle->config.tls_config.crt_bundle_attach) {
esp_transport_ssl_crt_bundle_attach(transport, handle->config.tls_config.crt_bundle_attach);
} else {
if (handle->config.tls_config.cert_pem == NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No DER?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add support for DER

}

/* Send DNS query */
len = esp_transport_write(transport,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we iterate to guarantee that the whole buffer was sent?

sizeof(dot_buffer),
timeout_ms);
if (len > 0) {
/* Skip the 2-byte length field that prepends DNS messages as required by RFC 7858 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 bytes encode the size of the answer. It would be better to use it to guide the parsing and allow processing of bigger answer and future memory usage control.

Comment on lines +47 to +62
/* Cache management. TBD: Not implemented yet */
struct {
void *cache; /* DNS cache structure */
uint64_t last_cleanup; /* Timestamp of last cache cleanup */
} cache_state;

/* Statistics. TBD: Not implemented yet */
struct {
uint64_t queries_sent; /* Total number of queries sent */
uint64_t successful_responses; /* Total number of successful responses */
uint64_t timeouts; /* Number of query timeouts */
uint64_t errors; /* Number of query errors */
uint64_t fallbacks; /* Number of protocol fallbacks */
uint64_t cache_hits; /* Number of cache hits */
double avg_response_time_ms; /* Average response time in milliseconds */
} stats;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not implemented, we should remove it from the MR and create a task to track it.

/* DNS resolution functions for different transport types */
err_t dns_resolve_udp(const esp_dns_handle_t handle, const char *name, ip_addr_t *addr, u8_t rrtype)
{
// TODO: Implement UDP DNS resolution
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you add it in this MR? If not, it should be removed from the feature list in the README.

0x00, 0x01 // QCLASS: IN (internet)
*/

dns_header_t *header = (dns_header_t *)buffer;
Copy link
Collaborator

@alisitsyn alisitsyn Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below: Is it better to check pointers before using?

#include "esp_crt_bundle.h"
#endif

#define ENABLE_NATIVE_UDP 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these values be defined in kconfig?


## Configuration

###Setting Up the ESP DNS Component
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: add a space before Setting to show it properly

def test_examples_getaddrinfo(dut):
dut.expect('esp>', timeout=30)
dut.write('getaddrinfo www.google.com')
dut.expect(r'getaddrinfo', timeout=30)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking if it is better to catch more information about resolved addresses using regular expressions?

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.

None yet

4 participants