feat: captive portal dns (+lwip apis) #20

ForsakenHarmony · 2021-10-26T02:38:55Z

Implemented the captive portal dns from the esp idf example

To use it:

Start wifi in ap mode
Start captive portal DNS
Start http server with 404 handler (feat: http server error handler #11), which redirects to / (important!)

The captive portal should show up when you log into wifi

Example gist with working code

Depends on #15 (and in turn #13)

ivmarkov · 2021-10-31T08:11:18Z

src/dns.rs

@@ -0,0 +1,305 @@
+use core::mem;


I must say, I LOVE the idea of having a Captive Portal!
What I don't love that much:

Overall, the code feels very low level (as if we are coding in C, via Rust) - I'll comment more below

It is part of esp-idf-svc but I'm not sure what it is doing here? This is a generic DNS server, which can - just as well - be coded against Rust's STD APIs (correct me if I'm wrong!) - or better yet - against the embedded-nal so that it does have a no_std story as well. (embedded-nal traits do have STD implementations too, and if these are lacking, we can implement / patch / upstream our own changes.). So I'm very much in favor of moving this into embedded-svc (which depends on no-std-net already anyway) - possibly behind a feature toggle.

As for what would be the use-case of that? Well, I might want to use a captive dns portal on the Raspberry Pi too?

The code is indeed C converted to Rust, but not quite at the point where I'm super happy with the "Rustyness" of it yet

Sure, it would be nice to have it generic.
But my use case wasn't quite as generic, so I implemented it specifically for the esp right now.

embedded-nal seems nice, hadn't heard of it before.

I feel like someone else must have already implemented a captive portal dns in rust that's more generic, though. (not that I checked and not that I'm opposed to make this more generic)

This PR also doesn't have to be merged, it was more of an "are you interested in having this here", because I had already built it

ivmarkov · 2021-10-31T08:18:57Z

src/dns.rs

+
+impl CaptivePortalDns {
+    pub fn new() -> Self {
+        CaptivePortalDns { task_handle: None }


I guess the desire to stay no_std compatible is what brought you into exposing bits and pieces for FreeRTOS? How about we take a simple trait instead, that would allow us to spawn a thread? We can even provide an implementation of the trait when Rust STD is enabled, and provide additional constructor which does not take the trait, for STD mode.

I guess the desire to stay no_std compatible is what brought you into exposing bits and pieces for FreeRTOS?
Yes

How about we take a simple trait instead, that would allow us to spawn a thread? We can even provide an implementation of the trait when Rust STD is enabled, and provide additional constructor which does not take the trait, for STD mode.

As in, implement it with std::thread when we're using std and otherwise add an impl with FreeRTOS?

As in implement it with std::thread when we are using std and don't provide an implementation at all for no_std. With no_std, user would be on their own to provide an implementation in whatever way they feel like.

It boils down to the following: we have to keep the esp-idf-svc surface small, clean and easy to explain by exposing Rust-safe, self-contained pieces of functionality, - ideally - behind generic traits. The emphasis being on safe and self-contained. We cannot just go there and expose random bits and pieces of all the raw level unsafe APIs that are out there in the ESP-IDF. For example, in your "basic freertos task api" what you have exposed from FreeRTOS is just a way to start a task (and that is always pinned to a core, if I'm not mistaken). Is that API generic enough so that folks can use it without calling other, unsafe Freertos APIs? Well, no. For one, the queueing and synchronization primitives of freertos are not exposed as safe APIs, because you did not need them for your particular case. But very likely people will need these to do something meaningful with freertos. So, how are we evolving your Freertos code? Is it going to - at some point in time - reach a feature parity with the unsafe freertos APIs?

If yes, then we are basically creating a whole Freertos wrapper, and shouldn't we then instead work towards making some of the Freertos Rust wrappers interop-ing with esp-idf-svc interop-ing with esp-idf-sys?

If no, aren't we in a better position by saying to folks that there are NO freertos safe APIs, and they should just use the unsafe bindings which are out there in esp-idf-sys?

ivmarkov · 2021-10-31T08:21:59Z

src/dns.rs

+    ip_addr: u32,
+}
+
+fn parse_dns_name(raw_name: *mut u8, parsed_name: &mut [u8]) -> *mut u8 {


This and the next function are littered with unsafe statements. Would it be possible to stay in Rust safe-land for parsing this stuff? Only if possible?

ivmarkov · 2021-10-31T08:23:28Z

src/dns.rs

+            answer.class = htons(qd_class);
+            answer.ttl = htonl(ANS_TTL_SEC);
+
+            let mut ip_info = esp_netif_ip_info_t::default();


In an implementation which is independent of ESP-IDF, we would want the IP address to be abstracted away, via a trait, or passed at construction/start time.

ivmarkov · 2021-11-13T17:05:02Z

OK so the real trickiness of all this code is in parse_dns_request. The socket stuff is just a 3-liner that receives DNS requests and replies in a loop. Everybody can write these 3 lines.

Now, the nice thing is - everything in parse_dns_request is ESP-IDF-agnostic (that is, aside from it always taking and serving the ESP-IDF Wifi gateway IP).

I still don't like it that there are so many unsafe statements in the above function, but my little mind cannot figure out for now how to go from byte buffers to Rust structures and back with less unsafe, so the unsafes will stay intact (for now).

My plan is the following:

I'll take your parse_dns_request almost verbatim and move it into embedded-svc, as a pub static function in a dns::CaptivePortalDns structure. It will be generalized just a bit, so that it can "ask" - via a closure - what IP to return for a particular domain name, but other than that the signature (and the code) would stay intact.
The socket 3-liner I'll implement only when the code is compiled against STD. In no_std, users would have to write the socket 3-liner themselves, and then call the magic parse_dns_request to process the incoming UDP packet.

Any glaring holes in this plan that you might spot?

ForsakenHarmony · 2021-11-15T11:28:32Z

I still don't like it that there are so many unsafe statements in the above function, but my little mind cannot figure out for now how to go from byte buffers to Rust structures and back with less unsafe, so the unsafes will stay intact (for now).

Should be possible to use an iterator I think, there were more pointers in there I converted

Just for reference, this is the example I converted https://github.com/espressif/esp-idf/blob/4a011f318377ab717b9dc459019c1055602c93ff/examples/protocols/http_server/captive_portal/main/dns_server.c

I'll take your parse_dns_request almost verbatim and move it into embedded-svc, as a pub static function in a dns::CaptivePortalDns structure. It will be generalized just a bit, so that it can "ask" - via a closure - what IP to return for a particular domain name, but other than that the signature (and the code) would stay intact.

Could also pass in an IP when you start the dns server, it's probably unlikely to change

The socket 3-liner I'll implement only when the code is compiled against STD. In no_std, users would have to write the socket 3-liner themselves, and then call the magic parse_dns_request to process the incoming UDP packet.

Would be great to have an impl with embedded-nal I suppose, but that should do for now

ivmarkov · 2021-11-15T13:55:52Z

I still don't like it that there are so many unsafe statements in the above function, but my little mind cannot figure out for now how to go from byte buffers to Rust structures and back with less unsafe, so the unsafes will stay intact (for now).

Should be possible to use an iterator I think, there were more pointers in there I converted

Just for reference, this is the example I converted https://github.com/espressif/esp-idf/blob/4a011f318377ab717b9dc459019c1055602c93ff/examples/protocols/http_server/captive_portal/main/dns_server.c

I'll take your parse_dns_request almost verbatim and move it into embedded-svc, as a pub static function in a dns::CaptivePortalDns structure. It will be generalized just a bit, so that it can "ask" - via a closure - what IP to return for a particular domain name, but other than that the signature (and the code) would stay intact.

Could also pass in an IP when you start the dns server, it's probably unlikely to change

The socket 3-liner I'll implement only when the code is compiled against STD. In no_std, users would have to write the socket 3-liner themselves, and then call the magic parse_dns_request to process the incoming UDP packet.

Would be great to have an impl with embedded-nal I suppose, but that should do for now

I was halfway there in removing the unsafes over the weekend when I found this. To be expected of course that such a thing exists in the first place. I'm switching to it instead.

ivmarkov · 2021-11-16T17:51:19Z

Closing in favor of esp-rs/embedded-svc@759b996

WIP and not tested yet, but I hope to get it working over the weekend.

ForsakenHarmony added 3 commits October 26, 2021 03:33

feat: waiter and some small wrappers

599c9b0

feat: basic rtos task api

badca9e

feat: captive portal dns (+lwip apis)

f2886d5

ivmarkov added the hacktoberfest-accepted label Oct 31, 2021

ivmarkov requested changes Oct 31, 2021

View reviewed changes

ivmarkov mentioned this pull request Oct 31, 2021

feat: basic rtos task api #15

Closed

ivmarkov closed this Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: captive portal dns (+lwip apis) #20

feat: captive portal dns (+lwip apis) #20

ForsakenHarmony commented Oct 26, 2021 •

edited

Loading

ivmarkov Oct 31, 2021

ForsakenHarmony Nov 4, 2021 •

edited

Loading

ivmarkov Oct 31, 2021

ForsakenHarmony Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov Oct 31, 2021

ivmarkov Oct 31, 2021

ivmarkov commented Nov 13, 2021

ForsakenHarmony commented Nov 15, 2021 •

edited

Loading

ivmarkov commented Nov 15, 2021

ivmarkov commented Nov 16, 2021

feat: captive portal dns (+lwip apis) #20

feat: captive portal dns (+lwip apis) #20

Conversation

ForsakenHarmony commented Oct 26, 2021 • edited Loading

ivmarkov Oct 31, 2021

Choose a reason for hiding this comment

ForsakenHarmony Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

ivmarkov Oct 31, 2021

Choose a reason for hiding this comment

ForsakenHarmony Nov 4, 2021

Choose a reason for hiding this comment

ivmarkov Nov 4, 2021

Choose a reason for hiding this comment

ivmarkov Oct 31, 2021

Choose a reason for hiding this comment

ivmarkov Oct 31, 2021

Choose a reason for hiding this comment

ivmarkov commented Nov 13, 2021

ForsakenHarmony commented Nov 15, 2021 • edited Loading

ivmarkov commented Nov 15, 2021

ivmarkov commented Nov 16, 2021

ForsakenHarmony commented Oct 26, 2021 •

edited

Loading

ForsakenHarmony Nov 4, 2021 •

edited

Loading

ForsakenHarmony commented Nov 15, 2021 •

edited

Loading