Bug 1415 - [dnscrypt] shared secret cache
[dnscrypt] shared secret cache
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-08-26 05:24 CEST by Manu Bretelle
Modified: 2017-08-30 02:07 CEST (History)
2 users (show)

See Also:


Attachments
1_dnscrypt_free_env.patch (2.95 KB, patch)
2017-08-26 05:24 CEST, Manu Bretelle
Details | Diff
2_dnscrypt_shared_secret_cache (17.05 KB, patch)
2017-08-26 05:24 CEST, Manu Bretelle
Details | Diff
3_dnscrypt_fix_tests (31.00 KB, patch)
2017-08-26 05:25 CEST, Manu Bretelle
Details | Diff
dnscrypt tests keys and certs (1.96 KB, application/gzip)
2017-08-28 19:23 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-08-26 05:24:19 CEST
Created attachment 438 [details]
1_dnscrypt_free_env.patch

Hi,

I have a serie of patches for the dnscrypt module.

1) small patch which will free dnscrypt environment so we dont leak memory when doing reloads... patch is 1_dnscrypt_free_env.patch https://github.com/chantra/unbound/commit/e35dc30dc8fa832a8ce88132f40d7a3cdbebaf66

2) make use of a shared_secret cache 2_dnscrypt_shared_secrets_cache.patch https://github.com/chantra/unbound/commit/eb7a365af3356d4ede6cf1de408fad8533a2717a

On a server receiving only dnscrypt query, this save 20% of CPU usage from unbound as we won't be recomputing the shared secrets over and over.

3) a patch for the unittests.... Back in the days I had generated a cert for a year. It expired yesterday so the tests started failing. I added the public/secret key to the dnscrypt folder so new ephemeral keys and certs can be generated easily on the future. A small script can be used to generate them. I basically generated a new set of keys and cert and updated the test. Hopefully the regexes works on both BSD and Linux :)

Patch is 3_dnscrypt_fix_tests.patch
https://github.com/chantra/unbound/commit/976c4109f49e28af5282692f0a42584fe02ab18d

The global tests are still passing after that: https://travis-ci.org/chantra/unbound/builds/268587356

Let me know if you want some clarifications.
Tks
Comment 1 Manu Bretelle 2017-08-26 05:24:57 CEST
Created attachment 439 [details]
2_dnscrypt_shared_secret_cache
Comment 2 Manu Bretelle 2017-08-26 05:25:34 CEST
Created attachment 440 [details]
3_dnscrypt_fix_tests
Comment 3 Wouter Wijngaards 2017-08-28 09:59:43 CEST
Hi Manu,

Patch 2 contains .key files and patch complains: 
$ patch -p1 --binary < ~/Downloads/attachment.cgi 
patching file dnscrypt/testdata/gencert.sh
patching file dnscrypt/testdata/keys1/public.key
patch: **** missing line number at line 106: @@ -0,0 +1, @@ 

The --binary flag did not help parse the patch.  Otherwise the diff looks good, maybe provide the updated files in a tarball?

I think it would be a good idea to provide the certs for more than 1 year.  That escaping in the shell script is right nasty to get right.

Patch 1, applied, thank you!
Patch 2, I am reviewing the patch, looks good.

Best regards, Wouter
Comment 4 Wouter Wijngaards 2017-08-28 13:04:57 CEST
Hi Manu,

For patch 3, I made a number of edits:
1. fix comment about hash table data element
2. size of table same as message cache is just too big.  You don't need a couple Gb.  4M is nicer.  Otherwise, try to use the key_cache_size from the config_file, that is much more appropriate (both in name, and in size).
3. unused warnings removed in the sizefunc().

Thanks for the patch!  I have committed it, with these fixes:

Index: dnscrypt/dnscrypt.c
===================================================================
--- dnscrypt/dnscrypt.c	(revision 4312)
+++ dnscrypt/dnscrypt.c	(working copy)
@@ -55,7 +55,7 @@
 struct shared_secret_cache_key {
     /** the hash table key */
     uint8_t key[DNSCRYPT_SHARED_SECRET_KEY_LENGTH];
-    /** the hash table entry, data is struct reply_info* */
+    /** the hash table entry, data is uint8_t pointer of size crypto_box_BEFORENMBYTES which contains the shared secret. */
     struct lruhash_entry entry;
 };
 
@@ -786,7 +786,7 @@
     env->shared_secrets_cache = slabhash_create(
         cfg->msg_cache_slabs,
         HASH_DEFAULT_STARTARRAY,
-        cfg->msg_cache_size,
+        4000000,
         dnsc_shared_secrets_sizefunc,
         dnsc_shared_secrets_compfunc,
         dnsc_shared_secrets_delkeyfunc,
@@ -820,12 +820,13 @@
  */
 
 size_t
-dnsc_shared_secrets_sizefunc(void *k, void *d)
+dnsc_shared_secrets_sizefunc(void *k, void* ATTR_UNUSED(d))
 {
     struct shared_secret_cache_key* ssk = (struct shared_secret_cache_key*)k;
     size_t key_size = sizeof(struct shared_secret_cache_key)
         + lock_get_mem(&ssk->entry.lock);
     size_t data_size = crypto_box_BEFORENMBYTES;
+    (void)ssk; /* otherwise ssk is unused if no threading, or fixed locksize */
     return key_size + data_size;
 }
Comment 5 Manu Bretelle 2017-08-28 19:09:38 CEST
Thanks Wouter,

> 2. size of table same as message cache is just too big.  You don't need a couple Gb.  4M is nicer.  Otherwise, try to use the key_cache_size from the config_file, that is much more appropriate (both in name, and in size).

Agreed, I was going to add config options for this cache, but 4M is a good enough default.


> I think it would be a good idea to provide the certs for more than 1 year.

Yes, I made those valid until 2032. Optimally, those would be generated at test time actually... but this is good enough for now. I will provide tarballs later today.


I would like to add some metrics around the cache hit rate (and potentially size) so to get a better understanding of how the cache behave and is effective, as well as a better way to set the size of the cache. It is unclear to me what would be the best way. For the entry points in worker.c (e.g dnsc_handle_curved_request and dnsc_handle_uncurved_request), I could easily get the stats structure from worker->stats I suppose. But for util/netevent.c it seems a bit more tricky as this seems to happen out of worker context.

I suppose I should take the approach of basic_lock + stats in dnscrypt environment a la https://github.com/NLnetLabs/unbound/commit/7b18274d7e1ed0dda25669048a62ebd55cb9a11d .
Comment 6 Manu Bretelle 2017-08-28 19:23:16 CEST
Created attachment 441 [details]
dnscrypt tests keys and certs

generated with:
tar -czvf dnscrypt_tests.tgz dnscrypt/testdata testdata/dnscrypt_*/*{key,cert}
Comment 7 Wouter Wijngaards 2017-08-29 10:43:50 CEST
Hi Manu,

I get these errors after using the tarball:

When starting dnscrypt_cert_chacha.pre
[1503996054] unbound[25072:0] notice: Loaded cert 2.cert
[1503996054] unbound[25072:0] notice: Loaded cert 2_chacha.cert
[1503996054] unbound[25072:0] notice: Loaded cert 1.cert
[1503996054] unbound[25072:0] notice: Loaded key 2.key
[1503996054] unbound[25072:0] notice: Crypt public key fingerprint for 2.key: F5F3:57BF:BD5A:D8D2:7819:CCF7:ADE3:748A:12A2:7E98:FDD3:1F7A:5C02:6D35:08FE:E034
[1503996054] unbound[25072:0] notice: Crypt public key fingerprint for 2.key: F5F3:57BF:BD5A:D8D2:7819:CCF7:ADE3:748A:12A2:7E98:FDD3:1F7A:5C02:6D35:08FE:E034
[1503996054] unbound[25072:0] notice: Using X25519-XChacha20Poly1305
[1503996054] unbound[25072:0] notice: Loaded key 1.key
[1503996054] unbound[25072:0] notice: Crypt public key fingerprint for 1.key: C31B:BF32:CBB0:4410:B465:C668:887B:D873:3B53:6432:5E9A:E105:3C10:43CD:2B53:DB41
[1503996054] unbound[25072:0] fatal error: dnsc_parse_keys: could not match certificate for key 1.key. Unable to determine ES version.

Best regards, Wouter
Comment 8 Wouter Wijngaards 2017-08-29 10:48:58 CEST
Hi Manu,

Committed that result (tar xzf tarball; and then svn add dnscrypt/* stuff).  So you can see what happened to the files.

Best regards, Wouter
Comment 9 Wouter Wijngaards 2017-08-29 11:28:38 CEST
Hi Manu,

Okay, I fixed it, the .conf files needed to be changed too (from the 3.diff) and the .test files needed different grep lines in there.  I used grep -F.

Best regards, Wouter
Comment 10 Manu Bretelle 2017-08-30 02:07:35 CEST
oh yes, sorry, I only added the binary files to the tgz, not the rest of the diff. sorry