Skip to content

gcoap_dns: initial import of a DNS over CoAP (DoC) client#16705

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
miri64:gcoap_dns/feat/initial
Jul 13, 2022
Merged

gcoap_dns: initial import of a DNS over CoAP (DoC) client#16705
miri64 merged 2 commits intoRIOT-OS:masterfrom
miri64:gcoap_dns/feat/initial

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Aug 3, 2021 •
edited
Loading

Contribution description

This provides a DNS-over-CoAPS (DoC) client prototype implementation using gcoap. The protocol design itself is heavily inspired by DoH, but has some additional features (such as FETCH support) that I got after a short brainstorming session with @chrysn. The main use case is encrypted DNS communication, so only coaps:// resources are allowed as a target for now. Long-term plans to add OSCORE support exist as well.

This PR serves merely to show an implementation prototype of said protocol. For discussions on protocol design, please refer to the repository for the DoC draft.

TODO:

  • block-wise transfer
  • proxy support
  • Kconfig all the options
  • automate tests

Testing procedure

Just run

make -C tests/gcoap_dns flash test

for any board you like.

If you want to test CoAPS behavior or more complex setups (e.g. with proxies), install https://github.com/anr-bmbf-pivot/aiodnsprox/

Run e.g.

sudo dist/tools/tapsetup/tapsetup
sudo ip addr add 2001:db8::1 dev tapbr0
sudo ip route add 2001:db8::/64 via "<native link-local>" dev tapbr0
./aiodns-proxy -c 2001:db8::1 -U 8.8.8.8 --dtls-credentials client_identity secretPSK

and in a different shell

$ make -C tests/gcoap_dns/ term
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gcoap_dns'
/home/mlenders/Repositories/RIOT-OS/RIOT/tests/gcoap_dns/bin/native/tests_gcoap_dns.elf tap0 
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2022.07-devel-654-g0761c-gcoap_dns/feat/initial)
> ifconfig 7 add 2001:db8::2
nib route add 7 default "<tapbr0 link local>"
uri coaps://[2001:db8::1]/dns
creds 1 client_identity secretPSK
ifconfig 7 add 2001:db8::2
success: added 2001:db8::2/64 to interface 7
> nib route add 7 default fe80::dc1a:a8ff:fe09:45b3
> uri coaps://[2001:db8::1]/dns
Successfully added URI coaps://[2001:db8::1]/dns
> creds 1 client_identity secretPSK
Successfully added creds: 1, client_identity, secretPSK
> query example.org
query example.org
Hostname example.org resolves to 2606:2800:220:1:248:1893:25c8:1946 (IPv6)

(for non-native/non-Ethernet boards you also will need a border router)

One should also be able to run the tests without CoAPS support by setting COAPS=0 in the environment variables at compile time.

Issues/PRs references

Depends on PRs #16669, #16695 (merged), #16702, #16709, #16712 (merged).

@miri64 miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 3, 2021
@miri64 miri64 requested a review from cgundogan August 3, 2021 13:28
@github-actions github-actions bot added Area: build system Area: Build system Area: CoAP Area: Constrained Application Protocol implementations Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 3, 2021
@miri64 miri64 requested a review from chrysn August 3, 2021 13:28
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch 2 times, most recently from b3150a7 to e8ae2d6 Compare August 4, 2021 11:57
@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 4, 2021
@miri64 miri64 mentioned this pull request Aug 4, 2021
Merged
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch 2 times, most recently from fbc59c5 to 1a09ec0 Compare August 5, 2021 15:10
@miri64 miri64 mentioned this pull request Aug 5, 2021
Merged
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch from ddc08da to 484306f Compare August 5, 2021 18:07
@miri64 miri64 mentioned this pull request Aug 6, 2021
Merged
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch from 35b3c13 to d71474e Compare August 10, 2021 10:35
@miri64 miri64 mentioned this pull request Aug 11, 2021
Merged
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch from d71474e to f25f272 Compare August 11, 2021 10:46
This was referenced Sep 16, 2021
Merged
Closed
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Sep 17, 2021

Rebased since #16702 got merged. No longer waiting for other PRs (for the moment ;-))

@miri64 miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 17, 2021
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch from f25f272 to 292cc4c Compare September 17, 2021 12:15
@miri64 miri64 force-pushed the gcoap_dns/feat/initial branch from 20caff0 to a14dea1 Compare October 8, 2021 10:06
@miri64 miri64 requested a review from leandrolanzieri as a code owner June 2, 2022 17:29
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jun 2, 2022

This is ready for review now.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jun 2, 2022

Also addressed #16705 (comment) now.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jun 3, 2022

Anyone interested in my experimental OSCORE support may go here: https://github.com/miri64/RIOT/tree/gcoap_dns/enh/oscore :-)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jun 28, 2022

Squashed by offline request.

chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

(Flushing out a round of initial comments while going through the code to encourage interactive handling.)

chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

(More comments)

sys/net/application_layer/gcoap/dns.c

if ((remote->port != _remote.port) ||
(remote->family != _remote.family) ||
(!ipv6_addr_is_unspecified((ipv6_addr_t *)_remote.addr.ipv6) &&
Copy link
Copy Markdown
Member

@chrysn chrysn Jun 28, 2022

Choose a reason for hiding this comment

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

Is there any test case you can put in a report, to have a good issue number to go both in the PR notes and into the code (so that when the issue is finally resolved, we can remove the check)?

chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Looks good over-all, see comments (for many of which anything more than "because." can be a good answer).

I didn't check the credentials part too carefully, but it looks plausible enough. (It can probably be simplified if the credman API becomes easier to use; iterating through an array to find an empty slot is what I'd expect credman to do).

I didn't do a detailed check on the tests, but they look good from cursory reading, and do pass on native.

I'm running a few more practical tests, and expect to be done with them by the time the rest of the comments is through.

chrysn
chrysn reviewed Jun 28, 2022
View reviewed changes
@chrysn
Copy link
Copy Markdown
Member

chrysn commented Jun 28, 2022

I also did some testing now, both on native and microbit-v2 via 6lowpan.

> coap get demo.coap.amsuess.com 5683 /time
gcoap_cli: sending msg ID 36691, 11 bytes
> gcoap: response Success, code 2.05, 16 bytes
00000000  32  30  32  32  2D  30  36  2D  32  38  20  32  31  3A  33  37

after applying roughly

diff --git a/examples/gcoap/Makefile b/examples/gcoap/Makefile
index bcb3c4731e..648dbd4ca9 100644
--- a/examples/gcoap/Makefile
+++ b/examples/gcoap/Makefile
@@ -40,0 +41,2 @@ USEMODULE += ps
+USEMODULE += gcoap_dns
+
diff --git a/examples/gcoap/client.c b/examples/gcoap/client.c
index d22c62eac0..b483acfb58 100644
--- a/examples/gcoap/client.c
+++ b/examples/gcoap/client.c
@@ -30,0 +31 @@
+#include "net/gcoap/dns.h"
@@ -148,0 +150,2 @@ static bool _parse_endpoint(sock_udp_ep_t *remote,
+    gcoap_dns_server_uri_set("coap://[2a02:b18:c13b:8010::907]/dns");
+
@@ -151,2 +154,3 @@ static bool _parse_endpoint(sock_udp_ep_t *remote,
-        puts("gcoap_cli: unable to parse destination address");
-        return false;
+        if (gcoap_dns_query(addr_str, &remote->addr, AF_INET6) < 0 ) {
+            return false;
+        }

and running aiodnsprox locally.

chrysn
chrysn approved these changes Jun 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

There is only one unresolved comment left, in #16705 (comment), but that's really more about a to-be-opened unrelated issue, so it can stay unresolved.

I consider this ready. Thanks for adding this, please finalize.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jun 29, 2022

Squashed.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 1, 2022

Mphff again needing to account for Python 3.5 (or outdated python libs) oddities:

make: Leaving directory '/tmp/dwq.0.356379192510457/e8d17f859edfdaad3f899117c9a3ee12/tests/gcoap_dns'
make: Entering directory '/tmp/dwq.0.356379192510457/e8d17f859edfdaad3f899117c9a3ee12/tests/gcoap_dns'
WARNING: No route found for IPv6 destination :: (no default route?). This affects only IPv6
/usr/lib/python3/dist-packages/scapy/layers/dot11.py:19: CryptographyDeprecationWarning: Python 3.5 support will be dropped in the next release of cryptography. Please upgrade your Python.
  from cryptography.hazmat.backends import default_backend
Traceback (most recent call last):
  File "/tmp/dwq.0.356379192510457/e8d17f859edfdaad3f899117c9a3ee12/tests/gcoap_dns/tests/01-run.py", line 13, in <module>
    from scapy.all import DNS, DNSQR, DNSRR, raw
ImportError: cannot import name 'raw'

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 1, 2022

With Python 3.10, using bytes instead of raw works. Let's see if this is also the case for python 3.5.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 1, 2022

(also confirmed with python 3.7, older versions I am too lazy to test :P)

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Jul 2, 2022

The Python change looks good (why was there a raw in the first place -- but anyway...), leaving 3 failures:

  • The run_test/tests/pkg_fff/esp32-wroom-32:gnu is WIP in another PR (cpu/esp32: enforce MAXTHREADS is at least 3 #18210)
  • The run_test/tests/gcoap_dns/esp32-wroom-32:gnu ... looks weird, what even is a LoadProhibitedCause?
  • run_test/tests/gcoap_dns/samr21-xpro:gnu could be yet another Python version issue (if the SAMR21 test runs on a slightly different runner, maybe?)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 2, 2022

That older scapy version seems to be in-consistent in itself... I pass a string as rdata, but it complains that it is not a string when handed to inet_pton. @kaspar030 would it be possible to at least update scapy on the pyfleet?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 12, 2022 •
edited
Loading

Pre-encoded the rdata AN section now, this should hopefully fix the issue with the older scapy version as a hotfix. I rather want to get this in for the release.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 12, 2022

Can reproduce the remaining test error. Will check.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 12, 2022

Oops, that actually spotted an off-by-one error in the test application. Fixed with the latest force push. Curious that that only was an issue on ESP32... probably because its atoi or argv behaves a little bit differently (the other platforms probably read bogus from argv and ran into usage that way, ESP32 just crashed properly).

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 13, 2022

Thanks for the review!

@benpicco benpicco mentioned this pull request Jul 17, 2022
Merged
This was referenced Jul 19, 2022
Merged
Merged
@miri64 miri64 mentioned this pull request Jul 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrysn chrysn chrysn approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@maribu maribu Awaiting requested review from maribu

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: CoAP Area: Constrained Application Protocol implementations Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2022.07

Development

Successfully merging this pull request may close these issues.

2 participants

@miri64 @chrysn