Bug 1217 - support for DNSCrypt
support for DNSCrypt
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-09 22:37 CET by Manu Bretelle
Modified: 2017-04-03 13:29 CEST (History)
3 users (show)

See Also:


Attachments
dnscrypt_cert.tpkg (2.76 KB, application/octet-stream)
2017-03-20 16:34 CET, Manu Bretelle
Details
dnscrypt_queries.tpkg (2.54 KB, application/octet-stream)
2017-03-20 16:35 CET, Manu Bretelle
Details
dnscrypt metrics (6.07 KB, patch)
2017-03-24 06:15 CET, Manu Bretelle
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manu Bretelle 2017-02-09 22:37:40 CET
Hi,

Testing water here.

Currently, the only way to run DNSCrypt (https://dnscrypt.org/) in front of unbound is to use DNSCryptWrapper (https://github.com/cofyc/dnscrypt-wrapper).

While this work out, one of the main issue is that we then lose client IP information, which can be useful for multiple reasons (acl, logging, potentially ECS (#1208)...) as all requests are now coming from the wrapper IP (likely localhost).

I have a POC that works by creating a separate interface with @port and decapsulating/encapsulating dnscrypt for the traffic from interfaces using this specific @port.

I know the effort is going toward DNS over TLS (hence the testing water). Happy to have this gated behind #defines
Comment 1 Wouter Wijngaards 2017-02-10 09:02:04 CET
Hi Manu,

This was not really on our roadmap.  But the patch could certainly end up in contrib/ , so that other users can conveniently find it?

Best regards, Wouter
Comment 2 Manu Bretelle 2017-02-13 19:14:28 CET
Thanks Wouter,

Sounds like a good enough start. I will publish it on github when I get some time to fix a few rough edges.

Wouter, there is chances that I am doing things in a clowny way and that there may be a better solution. I will appreciate your feedback on that later when I post the diff.

I am happy to gate the feature behind `#ifdef`s, that being said, if there is no option this ever make it into master, I may just avoid the ifdef mess to simplify code. My current POC has them (to some extend), so I will leave them for now.

Thanks
Comment 3 Wouter Wijngaards 2017-02-14 09:30:40 CET
Hi Manu,

I'd be happy to review your patch (and fix it better-er, if possible :-) )  And if it doesn't touch too many parts to do the crypto stuff, it could be mainline... And if not that, we can host it as a user-contributed diff in the contrib folder of the source distribution.

I understand that the option-config stuff touches a bunch of files, that is not a problem, of course.

Best regards, Wouter
Comment 4 Manu Bretelle 2017-03-03 22:16:57 CET
Hi Wouter,

Allright, so I have something up. You can check it there: https://github.com/chantra/unbound/pull/1/files for a side by side diff. There is a big initial commit which I iterated upon. I will happily squash everything or split the diffs differently if needed. In anycase, the GH PR should be a good start to check the diff. It is rebased on the latest SVN rev 4031 as of today.
Looking forward for your feedbacks.


I have tried to limit the number of entry points in unbound core as much as I could. If there is opportunities to make this patch with even less code in core, let me know and I will get it done. 

Some places where it seems needed though are notably all the `comm_point_{tcp,udp,raw}..` functions. I got somehow lost with all of them, hopefully, this is correct.

In worker.c, I tried to abstract most of it behind some dnscrypt helper functions, but because dnscrypt needs to be able to parse plain request for qtype TXT, qname providername, this bit is left in worker.c to avoid duplicating a bunch of logic/functions readily available in worker.c. I could maybe move that bunch of validation in a separate function though.

cfgr will be available with or without `--enable-dnscrypt` activated but only used when `--enable-dnscrypt` was used. If `--enable-dnscrypt` is not enabled and one specifies `dnscrypt-enable: yes` unbound will terminate with a fatal error.

The original implementation was using the main buffer, but in order to keep backward compatibility with dnstap and reply logging, it now uses a separate buffer. The issue is that by the time the answer was logged, the buffer would have been encrypted. Using a separate buffer fixes this. The dnscrypt buffer is only allocated when the worker is running in dnscrypt context, otherwise, dnscrypt buffer points to the standard buffer.


Serving certificates piggy backs on local-data/local-zone. Some of the default values are arbitrary but an option could make this adjustable.

I have added the .travis.yml so I could confirm it still build and pass existing tests with and without --enable-dnscrypt.


It is not possible to generate keys/certs with unbound tooling currently, but dnscrypt-wrapper can do that just fine for us.


Diff: https://patch-diff.githubusercontent.com/raw/chantra/unbound/pull/1.diff
Patch: https://patch-diff.githubusercontent.com/raw/chantra/unbound/pull/1.patch
Comment 5 Manu Bretelle 2017-03-06 18:36:25 CET
Hi Wouter,

I have added dnscrypt_queries.tpkg and dnscrypt_cert.tpkg to perform some e2e validation.
How up to date are the tests ran by `testcode/do-tests.sh`? Running off master, I hit a  fair amount of FAILED, not sure I miss a few things or the tests are just deprecated/old and not really expected to run anymore.
Comment 6 Manu Bretelle 2017-03-07 21:37:53 CET
Hi Wouter,

I have been fuzzing the dnscrypt stack using https://github.com/chantra/dnscrypt-wrapper/blob/fuzzer/tests/dnscrypt-fuzzer.py a few times.

This does test a variety of bit flipping, truncation, byte injection and byte removal as well as inner payload (DNS) corruption only.

No issue detected so far.
Comment 7 Manu Bretelle 2017-03-14 01:11:44 CET
Hi Wouter,

I was wondering if you had time to check the diff. Do you prefer the diff to be uploaded to the bugzilla ticket? or is github fine?

Let me know if there is anything that you would like to address and if whether or not there would be more clever ways to do what I am doing here.

Thanks
Comment 8 Wouter Wijngaards 2017-03-14 09:05:06 CET
Hi Manu,

Haven't had time to look at your patch yet, but I am about to.

Best regards, Wouter
Comment 9 Manu Bretelle 2017-03-14 16:33:39 CET
Thanks Wouter,

Very much appreciated.
Comment 10 Wouter Wijngaards 2017-03-20 16:10:52 CET
Hi Manu,

I have applied the patch, I think the code is very good.  And done some small fixups.  But the two tpkg files arrived as empty for me; could you email them to me? (or attach them?)  Then I can test that it works.

Best regards, Wouter
Comment 11 Manu Bretelle 2017-03-20 16:14:39 CET
Hi Wouter!

Great news, thanks! I will get those tpkg sorted in a bit.

I will also follow up with some patches to add monitoring. Libsodium has also added support for xchacha20 and I have a WIP patch to support it. https://github.com/chantra/unbound/commit/f46a79041b0e9dbd686d37f373d4619d0f22b6ea
Comment 12 Wouter Wijngaards 2017-03-20 16:20:27 CET
Hi,

Yes that'd be nice to be able to test this in the test set.  The patch looks good, let me know when it is out of WIP state and ready to go!

Best regards, Wouter
Comment 13 Manu Bretelle 2017-03-20 16:34:24 CET
Created attachment 385 [details]
dnscrypt_cert.tpkg

test dnscrypt certificate distribution
Comment 14 Manu Bretelle 2017-03-20 16:35:35 CET
Created attachment 386 [details]
dnscrypt_queries.tpkg

Test dnscrypt queries flows using dnscrypt-proxy
Comment 15 Wouter Wijngaards 2017-03-20 16:52:22 CET
Hi Manu,

Thank you for the tests.  They succeed.  I have added them to the unit test set.

Best regards, Wouter
Comment 16 Manu Bretelle 2017-03-24 06:15:40 CET
Created attachment 389 [details]
dnscrypt metrics

Add metrics to unbound-control interface showing crypted, cert request, plaintext and malformed queries.
Comment 17 Manu Bretelle 2017-03-31 18:16:01 CEST
Hi Wouter,

not sure if you saw but I added `dnscrypt metrics` patch.

Tks
Comment 18 Wouter Wijngaards 2017-04-03 13:29:56 CEST
Hi Manu,

Yes but I hadn't reviewed it yet.  Now I have, and it is committed.  Thank you for the neat code!

Best regards, Wouter