Bug 1276 - [dnscrypt] add XChaCha20-Poly1305 cipher
[dnscrypt] add XChaCha20-Poly1305 cipher
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
1.6.2
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-02 08:57 CEST by Manu Bretelle
Modified: 2017-07-09 00:07 CEST (History)
2 users (show)

See Also:


Attachments
XChaCha20-Poly1305 cipher support for dnscrypt (28.24 KB, patch)
2017-06-02 08:57 CEST, Manu Bretelle
Details | Diff
dnscrypt_cert.tpkg (3.08 KB, application/octet-stream)
2017-06-06 18:05 CEST, Manu Bretelle
Details
dnscrypt_queries_chacha.tpkg (2.82 KB, application/octet-stream)
2017-06-06 18:06 CEST, Manu Bretelle
Details
dnscrypt_build.patch (687 bytes, patch)
2017-06-08 00:38 CEST, Manu Bretelle
Details | Diff
dnscrypt_xchacha20_m4.patch (1.98 KB, patch)
2017-06-08 00:38 CEST, Manu Bretelle
Details | Diff
dnscrypt_tests.tar (20.00 KB, application/x-tar)
2017-06-08 00:48 CEST, Manu Bretelle
Details
dnscrypt_cert_chacha test (3.32 KB, application/octet-stream)
2017-06-17 00:42 CEST, Manu Bretelle
Details
dnscrypt_cert_chacha test v2 (3.33 KB, application/octet-stream)
2017-06-23 23:51 CEST, Manu Bretelle
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Manu Bretelle 2017-06-02 08:57:34 CEST
Created attachment 400 [details]
XChaCha20-Poly1305 cipher support for dnscrypt

libsodium-1.0.12 added support for xchacha20.

This patch make unbound support it if it was compiled with a version of libsodium that includes all the helpers.

It also extended dnscrypt_cert test with a xchacha cert and added a new test to specifically test xchacha20 ES version.
Comment 1 Wouter Wijngaards 2017-06-06 14:56:47 CEST
Hi Manu,

Thank you for the patch, applied it.  Your repo is out of sync with mine for dnscrypt, because of the chroot patch and whitespace (I also removed a signed-unsigned usage warning in the code, by the way), this makes the patch fail for some parts.  Also, /usr/bin/patch reports: File testdata/dnscrypt_cert.tpkg: git binary diffs are not supported. File testdata/dnscrypt_queries_chacha.tpkg: git binary diffs are not supported.

Best regards, Wouter
Comment 2 Manu Bretelle 2017-06-06 17:52:17 CEST
Thanks Wouter,

Thanks for letting me know the change you made to the patch in order to be able to apply it.

I will upload the 2 .tpkg file directly so you can add them to the svn repo.

BTW, do you have any objections against moving out of tpkg for something based on text files? It would make tpkg test changes easier to review. In my experience, those are just a directory with text file in them, but I may be missing some historical reasons why those would be tgz-ed.

Manu
Comment 3 Manu Bretelle 2017-06-06 18:05:45 CEST
Created attachment 401 [details]
dnscrypt_cert.tpkg
Comment 4 Manu Bretelle 2017-06-06 18:06:40 CEST
Created attachment 402 [details]
dnscrypt_queries_chacha.tpkg
Comment 5 Manu Bretelle 2017-06-06 18:46:39 CEST
Here is the change to dnscrypt_cert.tpkg: https://gist.github.com/chantra/49e1c135c97b7b85aa908d836e913854 
basically adds a xchacha20 cert and verify unbound serves it.


Both dnscrypt_queries_chacha.tpkg and  dnscrypt_cert.tpkg were attached to this bug report so you can import them into the source tree.

Tks
Comment 6 Wouter Wijngaards 2017-06-07 13:47:17 CEST
Hi Manu,

Thanks for the tests.  The dnscrypt_cert test fails for two reasons.  One is that configure does not test for XCHACHA20, so that the define for it is never enabled.  The other is that dnscrypt_cert tests chacha, but without checking if chacha is present.

I guess what is missing is the configure (or ddnscrypt.m4) diffs, and maybe an 
if grep "HAVE_XCHACHA20" $PRE/config.h around that test in the tpkg so that it works.

Yes tpkg is this tar.gz for historical reasons, I don't really want to change it right now; but I agree better diffs are nice.  However, there are binary files too, on occasion, such as the cert and key files in dnscrypt.

Best regards, Wouter
Comment 7 Manu Bretelle 2017-06-07 21:26:06 CEST
Hi Wouter,

> Yes tpkg is this tar.gz for historical reasons, I don't really want to change it right now; but I agree better diffs are nice.  However, there are binary files too, on occasion, such as the cert and key files in dnscrypt.

Are you not wanting to change it because of time? or any other reasons? I think it wont take too much to port something directory based and will give it a go to see what it may take.
Comment 8 Manu Bretelle 2017-06-08 00:37:26 CEST
o'rite, so I went ahead and `sed`-ed my way through the tpkg to convert that to flat files (or directories for that matter)... https://github.com/chantra/unbound/commit/0ce986555092208334ea76f3da8ca4143add6485
is what it would look like. This is mostly a POC and I would like your input. The pro, aside from somethign which is diff-able, is also that it makes test more discoverable (grep) and easier to update.


In the meantime, as I was fixing the tests, I found an issue with the current build when dnscrypt was enabled: https://github.com/chantra/unbound/commit/9a507068f9cf635a62db454e25d84cc601a9e24e (I will upload a patch).

The detection of XCHACHA20 was totally wrong, in anycases, I moved it within dnscrypt.m4 in https://github.com/chantra/unbound/commit/418877019391fcf354674daf51b816cd27a36214 (will also submit patch)

I then modified the tests to detect if 1) dnscrypt is enabled and 2) if xchacha20 is available and adjust test accordingly (for instance if there is a xchacha20 cert, unbound will exit with an error message...) The test change is in https://github.com/chantra/unbound/commit/717845ea8ff8a1836fc9591593fa3c85d38a1d0d

I will submit all the relevant .tpkgs to this bug.
Comment 9 Manu Bretelle 2017-06-08 00:38:21 CEST
Created attachment 403 [details]
dnscrypt_build.patch
Comment 10 Manu Bretelle 2017-06-08 00:38:50 CEST
Created attachment 404 [details]
dnscrypt_xchacha20_m4.patch
Comment 11 Manu Bretelle 2017-06-08 00:48:11 CEST
Created attachment 405 [details]
dnscrypt_tests.tar
Comment 12 Manu Bretelle 2017-06-15 19:35:15 CEST
Hi Wouter,

Apparently https://www.nlnetlabs.nl/bugs-script/attachment.cgi?id=404&action=diff#b/dnscrypt/dnscrypt_config.h.in_sec1 (dnscrypt/dnscrypt_config.h.in chunk) did not make it in trunk.
Comment 13 Manu Bretelle 2017-06-17 00:41:30 CEST
Hi Wouter

Promised... this is the last ping on this bug report ;)

testdata/dnscrypt_cert_chacha.tpkg was not updated based on the last dnscrypt_tests.tar file so it is failing tests.

I will add it separately as an attachment for convenience.

tks
Comment 14 Manu Bretelle 2017-06-17 00:42:54 CEST
Created attachment 409 [details]
dnscrypt_cert_chacha test

expected md5: 82f73e7f61e7e94da2a63c21345702c0
Comment 15 Manu Bretelle 2017-06-22 22:29:55 CEST
(In reply to Manu Bretelle from comment #14)
> Created attachment 409 [details]
> dnscrypt_cert_chacha test
> 
> expected md5: 82f73e7f61e7e94da2a63c21345702c0

Hi Wouter,

friendly ping to merge this .tpkg into trunk. This should make test pass in https://github.com/chantra/unbound/commits/travis_build when compiling with/without dnscrypt and with or without xchacha20 support.

Thanks
Comment 16 Wouter Wijngaards 2017-06-23 08:57:29 CEST
Hi Manu,

Thanks, committed.  (Was busy with other bugs, i.e. that heap overflow that was reported).

Best regards, Wouter
Comment 17 Wouter Wijngaards 2017-06-23 09:06:59 CEST
Hi Manu,

That test does not work for me.  The grep statement here works for me:

    grep 'DNSC\\000\\002\\000\\000\\249\\143\;\\160H$tX\\153\\239^\\171\\160\\204`\\012mjU\\214a\\142\\138u\\161\\160W_\\012\\207x2A\\243=B+\\171X\\167tN\\202\\016\\213\\183\\012\\138\\161\\182\\204\\158\.^\\011ZQ\\003\\0214Nz\\210\\001\\142v\\190R\\193\\167\\011g\\"\\206\\210\\234|\\209\\234\\023\\216\\249eE\\163p\\143\\023)4\\149\\177}0~6\\143v\\190R\\193\\167\\011gX\\200\\231\\160X\\200\\231\\160Z\\170\\027' outfile

It has one less '\' before the ';'.

Best regards, Wouter
Comment 18 Manu Bretelle 2017-06-23 09:11:47 CEST
(In reply to Wouter Wijngaards from comment #16)
> Hi Manu,
> 
> Thanks, committed.  (Was busy with other bugs, i.e. that heap overflow that
> was reported).
> 
> Best regards, Wouter

Great stuff. Thanks. The build is back to green: https://travis-ci.org/chantra/unbound/builds/246087814
Comment 19 Manu Bretelle 2017-06-23 19:22:02 CEST
(In reply to Wouter Wijngaards from comment #17)
> Hi Manu,
> 
> That test does not work for me.  The grep statement here works for me:
> 
>     grep
> 'DNSC\\000\\002\\000\\000\\249\\143\;
> \\160H$tX\\153\\239^\\171\\160\\204`\\012mjU\\214a\\142\\138u\\161\\160W_\\01
> 2\\207x2A\\243=B+\\171X\\167tN\\202\\016\\213\\183\\012\\138\\161\\182\\204\\
> 158\.
> ^\\011ZQ\\003\\0214Nz\\210\\001\\142v\\190R\\193\\167\\011g\\"\\206\\210\\234
> |\\209\\234\\023\\216\\249eE\\163p\\143\\023)4\\149\\177}0~6\\143v\\190R\\193
> \\167\\011gX\\200\\231\\160X\\200\\231\\160Z\\170\\027' outfile
> 
> It has one less '\' before the ';'.
> 
> Best regards, Wouter


Oh, that is strange.... This is what I have in the output file:
```
; <<>> DiG 9.9.4-RedHat-9.9.4-38.el7_3.3 <<>> +tcp @127.0.0.1 -p 6263 2.dnscrypt-cert.example.com. TXT
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 26818
;; flags: qr aa rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;2.dnscrypt-cert.example.com.   IN      TXT

;; ANSWER SECTION:
2.dnscrypt-cert.example.com. 86400 IN   TXT     "DNSC\000\001\000\000\219\128\220\027\009\177\002\188\011\1524\005\213\014\210\004F8i\190\000\004bU\144\141\129bf\179\187a:\174\187\005\1596\206\005\250\247\243\242e\226\166\161\250\184\163w\224xj\134\131h\011\209R<\224\003\142v\190R\193\167\011g\"\206\210\234|\209\234\023\216\249eE\163p\143\023)4\149\177}0~6\142v\190R\193\167\011gX.\162\232X.\162\232Z\015\214h"
2.dnscrypt-cert.example.com. 86400 IN   TXT     "DNSC\000\002\000\000\249\143\;\160H$tX\153\239^\171\160\204`\012mjU\214a\142\138u\161\160W_\012\207x2A\243=B+\171X\167tN\202\016\213\183\012\138\161\182\204\158.^\011ZQ\003\0214Nz\210\001\142v\190R\193\167\011g\"\206\210\234|\209\234\023\216\249eE\163p\143\023)4\149\177}0~6\143v\190R\193\167\011gX\200\231\160X\200\231\160Z\170\027 "
2.dnscrypt-cert.example.com. 86400 IN   TXT     "DNSC\000\001\000\000+WS\171'OMF\003\240:\012`uD\029\147\\\013\027f^\169\247\231\132\001\238\004\205\221\028Z\243MpaN4\024\212l\177?\240,\129f\028\147Aj\184S\205}1\176e\226\190:\017\011O\157\007[s6q\150\128\169\016J5cD\237\242:\2500\005U\203\161\146\132\133)js./O\157\007[s6q\150W\1904\234W\1904\234Y\159hj"

;; Query time: 0 msec
;; SERVER: 127.0.0.1#6263(127.0.0.1)
;; WHEN: Fri Jun 23 09:46:11 PDT 2017
;; MSG SIZE  rcvd: 467
```


and from my Mac, using 9.10...
```
; <<>> DiG 9.10.3-P2 <<>> 2.dnscrypt-cert.example.com @::1 -p 8080 -t txt +tcp
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 9257
;; flags: qr aa rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;2.dnscrypt-cert.example.com.	IN	TXT

;; ANSWER SECTION:
2.dnscrypt-cert.example.com. 86400 IN	TXT	"DNSC\000\001\000\000\219\128\220\027\009\177\002\188\011\1524\005\213\014\210\004F8i\190\000\004bU\144\141\129bf\179\187
\174\187\005\1596\206\005\250\247\243\242e\226\166\161\250\184\163w\224xj\134\131h\011\209R<\224\003\142v\190R\193\167\011g\"\206\210\234|\209\234\023\216\249eE\163p\143
23)4\149\177}0~6\142v\190R\193\167\011gX.\162\232X.\162\232Z\015\214h"
2.dnscrypt-cert.example.com. 86400 IN	TXT	"DNSC\000\002\000\000\249\143;\160H$tX\153\239^\171\160\204`\012mjU\214a\142\138u\161\160W_\012\207x2A\243=B+\171X\167tN\
2\016\213\183\012\138\161\182\204\158.^\011ZQ\003\0214Nz\210\001\142v\190R\193\167\011g\"\206\210\234|\209\234\023\216\249eE\163p\143\023)4\149\177}0~6\143v\190R\193\167
11gX\200\231\160X\200\231\160Z\170\027 "
2.dnscrypt-cert.example.com. 86400 IN	TXT	"DNSC\000\001\000\000+WS\171'OMF\003\240:\012`uD\029\147\\\013\027f^\169\247\231\132\001\238\004\205\221\028Z\243MpaN4\02
212l\177?\240,\129f\028\147Aj\184S\205}1\176e\226\190:\017\011O\157\007[s6q\150\128\169\016J5cD\237\242:\2500\005U\203\161\146\132\133)js./O\157\007[s6q\150W\1904\234W\1
4\234Y\159hj"

;; Query time: 34 msec
;; SERVER: ::1#8080(::1)
;; WHEN: Fri Jun 23 10:16:54 PDT 2017
;; MSG SIZE  rcvd: 467
```

so 9.10 stopped escaping `;` ... I will fix the regex :s
Comment 20 Manu Bretelle 2017-06-23 19:34:24 CEST
/tmp/dnscrypt_cert_chacha.3645471 $ grep 'DNSC\\000\\002\\000\\000\\249\\143\\\?;\\160H$tX\\153\\239^\\171' outfile
2.dnscrypt-cert.example.com. 86400 IN   TXT     "DNSC\000\002\000\000\249\143\;\160H$tX\153\239^\171\160\204`\012mjU\214a\142\138u\161\160W_\012\207x2A\243=B+\171X\167tN\202\016\213\183\012\138\161\182\204\158.^\011ZQ\003\0214Nz\210\001\142v\190R\193\167\011g\"\206\210\234|\209\234\023\216\249eE\163p\143\023)4\149\177}0~6\143v\190R\193\167\011gX\200\231\160X\200\231\160Z\170\027 "
/tmp/dnscrypt_cert_chacha.3645471 $ grep 'DNSC\\000\\002\\000\\000\\249\\143\\\?;\\160H$tX\\153\\239^\\171' outfile2
2.dnscrypt-cert.example.com. 86400 IN   TXT     "DNSC\000\002\000\000\249\143;\160H$tX\153\239^\171\160\204`\012mjU\214a\142\138u\161\160W_\012\207x2A\243=B+\171X\167tN\202\016\213\183\012\138\161\182\204\158.^\011ZQ\003\0214Nz\210\001\142v\190R\193\167\011g\"\206\210\234|\209\234\023\216\249eE\163p\143\023)4\149\177}0~6\143v\190R\193\167\011gX\200\231\160X\200\231\160Z\170\027 "


seems to work.
Comment 21 Manu Bretelle 2017-06-23 23:51:09 CEST
Created attachment 414 [details]
dnscrypt_cert_chacha test v2

o'rite, applied the new regex which matches (hopefully) dig 9.9 and dig 9.10... you know want they say about regexes :s
Comment 22 Wouter Wijngaards 2017-06-26 11:07:08 CEST
Hi Manu,

Thank you!  That regex works.

Best regards, Wouter
Comment 23 Wouter Wijngaards 2017-07-06 15:43:11 CEST
Hi Manu,

The tpkg tests have been expanded into .tdir, thank you for the (initial) patch.

Best regards, Wouter
Comment 24 Manu Bretelle 2017-07-09 00:07:46 CEST
(In reply to Wouter Wijngaards from comment #23)
> Hi Manu,
> 
> The tpkg tests have been expanded into .tdir, thank you for the (initial)
> patch.
> 
> Best regards, Wouter

That's great! thanks @wouter