Bugzilla – Bug 1415
[dnscrypt] shared secret cache
Last modified: 2017-08-30 02:07:35 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
Created attachment 439 [details] 2_dnscrypt_shared_secret_cache
Created attachment 440 [details] 3_dnscrypt_fix_tests
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
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; }
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 .
Created attachment 441 [details] dnscrypt tests keys and certs generated with: tar -czvf dnscrypt_tests.tgz dnscrypt/testdata testdata/dnscrypt_*/*{key,cert}
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
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
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
oh yes, sorry, I only added the binary files to the tgz, not the rest of the diff. sorry