Hi, Here's the summary of the previous IRC meeting / sprint.
--- COMMUNITY MEETING Place: #openvpn-devel on irc.freenode.net List-Post: openvpn-devel@lists.sourceforge.net Date: Thursday, 14th July 2011 Time: 17:00 UTC Planned meeting topics for this meeting were on this page: <https://community.openvpn.net/openvpn/wiki/Topics-2011-07-14> Next meeting will be announced in advance, but will be on the same weekday and at the same time. Your local meeting time is easy to check from services such as <http://www.timeanddate.com/world clock> or with $ date -u SUMMARY andj, cron2, dazo, jamesyonan and mattock participated in this meeting. -- This meeting was sprint, where Adriaan's (andj's) PolarSSL patches were reviewed, fixed and ACKed on the fly. The sprint focused on the "OpenSSL crypto separation" patchset: <https://community.openvpn.net/openvpn/wiki/Topics-2011-07-14#OpenSSLcryptoseparation> All essential patches got an ACK, as can be seen on that page. If you have any comments regarding any of the patches, please chime in. If there are no complaints, the patches will be merged to main Git repository soon. --- Full chatlog as an attachment -- Samuli Seppänen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock
dazo 10:06:49 hey, jamesyonan jamesyonan 10:06:56 hi dazo 10:07:19 how is going with you nowadays? getting any progress on the git stuff? 10:07:32 andj 10:07:48 evening L'utente krzie si è disconnesso (Quit: Leaving) 10:08 jamesyonan 10:08:07 I working on the git stuff but I'm not quite there yet dazo got some merge conflicts to solve from the SVN tree before he can push it out 10:08 mattock 10:08:28 ok, finally andj 10:08:36 dazo: had a chance to look at a few more of the doxygen patches? mattock 10:08:49 hi guys topics here: https://community.openvpn.net/openvpn/wiki/Topics-2011-07-14 10:09:13 vpnHelper 10:09:16 Title: Topics-2011-07-14 â OpenVPN Community (at community.openvpn.net) andj 10:09:25 hi everyone dazo 10:09:25 andj: no, it's been hectic a work again ... have a new kernel to test, with 17 NIC drivers to get tested andj: but I'll try to get that squeezed in when I have some breaks 10:09:49 andj 10:09:52 ah, nice... hardware tests dazo 10:10:02 yeah, kind of andj 10:11:21 In that case, I suggest we skip the doxygen patches, and start on the openssl separation patches and let dazo check and merge doxygen as he can find the time 10:11:42 Mattock, are you taking notes on the topic tables? 10:12:00 dazo 10:12:05 yeah, jamesyonan have given a content ACK in the docs ... and I'll just verify that it's plain doc patches mattock 10:12:06 andj: I can do it I'll add links to the mailing list threads, too 10:12:13 dazo 10:12:18 andj: they're going in, I feel that's for sure andj 10:12:28 mattock: cool, that leaves me free to code dazo: yeah, I'd hoped for that 10:12:38 dazo 10:12:47 andj 10:13:22 So, first patch: https://github.com/andj/openvpn-ssl-refactoring/commit/7b829e3d539108545d47241a1a773cd2551de009 vpnHelper 10:13:23 Title: Commit 7b829e3d539108545d47241a1a773cd2551de009 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:14:27 This patch changes configure to find polarssl, if --with-ssl-type=polarssl the default is of course openssl 10:14:34 it's quiet 10:15:54 dazo 10:15:56 andj: except of the one whitespace issue on line 312/322 and on 808 ... it looks fine ... I can clean up that when merging stuff and I know it works, as I've built both with polarssl and openssl on Fedora 14 10:16:18 andj 10:16:50 dazo: that sounds good mattock 10:17:03 topic page updated, still missing a few links to gmane andj 10:17:37 mattock: at some point I didn't post them to gmane mattock 10:17:45 andj: noticed andj 10:17:46 or the mailinglist since it was on github 10:17:51 mattock 10:18:02 andj: you had a tar.gz available somewhere, right? andj 10:18:06 and I understood it wasn't really appreciated yeah, but github is probably better 10:18:15 http://thread.gmane.org/gmane.network.openvpn.devel/4788 10:19:01 vpnHelper 10:19:03 Title: Gmane Loom (at thread.gmane.org) andj 10:19:37 ok, shall we move to the next patch? dazo 10:19:37 andj: you can use 'git request-pull' ... that gives a better overview in the mail you send the ML andj 10:19:42 that's the first content one dazo 10:20:07 unless jamesyonan got something to say, I say ACK on 7b829e3d539108545d47241a1a773cd2551de009 andj 10:20:33 Next one: https://github.com/andj/openvpn-ssl-refactoring/commit/55760a88d092d511bbe9495984c7b345f981e2ec vpnHelper 10:20:34 Title: Commit 55760a88d092d511bbe9495984c7b345f981e2ec to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 10:21:41 this patch basically abstracts OpenSSL crypto stuff to a generic API, and provides the "glue" between the the new API and OpenSSL, right? andj 10:21:44 dazo: will send that after the meeting it's the first of a series that does this 10:21:55 dazo 10:21:56 andj: no stress andj 10:22:01 this one handles the random dazo 10:22:14 ahh, rand_bytes() only andj 10:22:24 yes, together with basic infrastructure Makefile.am, new files are created 10:22:42 dazo 10:23:03 yeah andj 10:23:05 Did it this way to keep things simple and readable crypto_backend.h defines the API 10:24:06 dazo 10:24:10 ACK mattock 10:24:15 updating... done 10:24:43 andj 10:25:09 https://github.com/andj/openvpn-ssl-refactoring/commit/b957e47e5cbfd98a6e39790059279edf0a9b448f vpnHelper 10:25:10 Title: Commit b957e47e5cbfd98a6e39790059279edf0a9b448f to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:25:12 is the next commit This gets rid of some of the OpenSSL specific constants 10:25:25 and lets the backends decide how to define them 10:25:33 mattock 10:26:36 added links to the ACKed patches for convenience andj 10:26:38 again, not rocket science dazo 10:26:42 ACK mattock 10:26:43 in case somebody on the ml wants to complain andj 10:26:51 mattock: sweet https://github.com/andj/openvpn-ssl-refactoring/commit/da5221f8d416a05d552dd0e09885ff7d3a677514 10:27:07 vpnHelper 10:27:08 Title: Commit da5221f8d416a05d552dd0e09885ff7d3a677514 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:27:36 changes the max key length location so it's checkable from the backend again, very simple 10:27:41 dazo 10:28:24 [diversion] - Do we really need the #if MAX_CIPHER_KEY_LENGTH < EVP_MAX_KEY_LENGTH crap? andj 10:28:38 it was there so I kept it 10:28:41 It's a safeguard against future changes 10:28:50 dazo 10:29:03 yeah, I know ... it's just something to add as a follow-up andj 10:29:28 we can add notes in mattock's format dazo 10:29:28 it's a compile time warning only well, considering we have #define MAX_CIPHER_KEY_LENGTH 64, we might keep them 10:30:08 andj 10:30:43 it doesn't harm the code, and it might protect us against a problem with some future huge-keyed algorithm dazo 10:30:51 yeah ACK on 55760a88d092d511bbe9495984c7b345f981e2ec 10:30:56 andj 10:30:59 https://github.com/andj/openvpn-ssl-refactoring/commit/8ff526f45566d235b066d70150886f96c81b986c vpnHelper 10:31:01 Title: Commit 8ff526f45566d235b066d70150886f96c81b986c to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 10:31:02 duh ACK on da5221f8d416a05d552dd0e09885ff7d3a677514, I meant 10:31:08 andj 10:31:16 Is a retty stratightforward function move +p 10:31:21 it moves the show_available_ stuff 10:31:30 Note that the cipher_ok inline function will be removed in a later patch 10:32:27 dazo 10:33:07 why is the "Workarounds for incompatibilites between OpenSSL libraries. Right now we accept OpenSSL libraries from 0.9.5 to 0.9.7." block here? andj 10:33:17 It temporarily lives in two places ssl.c and ssl_openssl.c 10:33:26 dazo 10:33:38 so it will be torn out from ssl.c in a later patch? andj 10:33:40 until all the functions have been moved to ssl_openssl.c yes 10:33:42 dazo 10:33:45 okay andj 10:34:11 (this one: https://github.com/andj/openvpn-ssl-refactoring/commit/740f3b58c7fa48dec5db37e6f6d36b0c47e30957) dazo 10:34:12 mattock: make a note to look at ssl_openssl.c later on, to remove the outdated OpenSSL version support vpnHelper 10:34:12 Title: Commit 740f3b58c7fa48dec5db37e6f6d36b0c47e30957 to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 10:34:36 goodie! mattock 10:34:38 page updated, hopefully all is still in order: https://community.openvpn.net/openvpn/wiki/Topics-2011-07-14 vpnHelper 10:34:39 Title: Topics-2011-07-14 â OpenVPN Community (at community.openvpn.net) mattock 10:34:51 updating the page is a full-time job dazo 10:34:57 heh andj 10:35:02 ") even 10:35:05 another small one: https://github.com/andj/openvpn-ssl-refactoring/commit/33efe72a9c1d9d40609ffc24fb20070f42c4018c 10:35:20 vpnHelper 10:35:21 Title: Commit 33efe72a9c1d9d40609ffc24fb20070f42c4018c to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:35:29 (if the last one was acked) This one clears error status from the crypto backend, 10:36:07 dazo 10:37:42 ACK on 8ff526f45566d235b066d70150886f96c81b986c andj 10:38:09 cool, the next one is very simple again (33efe72a9c1d9d40609ffc24fb20070f42c4018c) dazo 10:38:25 mattock: the MAX_CIPHER_KEY_LENGTH < EVP_MAX_KEY_... crap ... yes we do want it! (comment can be removed) 10:38:32 mattock 10:39:01 we missed ACK on this one, didn't we? https://github.com/andj/openvpn-ssl-refactoring/commit/33efe72a9c1d9d40609ffc24fb20070f42c4018c vpnHelper 10:39:02 Title: Commit 33efe72a9c1d9d40609ffc24fb20070f42c4018c to andj/openvpn-ssl-refactoring - GitHub (at github.com) mattock 10:39:10 oh, sorry dazo 10:39:12 ACK on 33efe72a9c1d9d40609ffc24fb20070f42c4018c andj 10:39:17 https://github.com/andj/openvpn-ssl-refactoring/commit/dd253d7934e18343193d085dfe6aff97ea104b05 10:39:26 vpnHelper 10:39:27 Title: Commit dd253d7934e18343193d085dfe6aff97ea104b05 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:39:32 this is the first big one it moves the init/uninit functions 10:39:41 dazo 10:39:49 6 files changed, 193 insertions(+), 162 deletions(-) yeah, heavier := 10:39:53 10:39:55 andj 10:39:56 "big" dazo 10:40:19 what do you basically do here? andj 10:40:21 The external interface consists of the following: crypto_init_lib 10:40:24 crypto_uninit_lib 10:40:27 crypto_init_lib_engine 10:40:30 crypto_init_dmalloc 10:40:34 mattock 10:40:43 we're making good progress here, I'm pleased andj 10:40:48 The first 2 initialise/uninitialise OpenSSL the third initialises the hardware engines 10:41:06 if there is one 10:41:12 it takes a const char *, so it's pretty generic 10:41:23 dazo 10:41:53 but is this also just a "moving from ssl.* -> ssl_openssl.*" change? andj 10:42:01 and the last one enables memory debugging for openssl... I can't get that much more generic I'm afraid... yeah, it is 10:42:05 The order changes slightly, to make things a little clearer though 10:42:29 it's all in 1 ifdef now 10:42:35 (the engine stuff) 10:42:47 well, almost 10:43:08 dazo 10:43:29 yeah, well, one thing I notice in the "old" init_crypto_lib_engine() engine_initialized = true; is always set, even if CRYPTO_ENGINE is not set 10:43:58 that does not happen in the new crypto_init_lib_engine (const char *engine_name) 10:44:19 andj 10:45:40 It isn't used outside of CRYPTO_ENGINE code snippets now dazo 10:45:59 okay andj 10:46:12 so even the definition is within #if (see line 104 in crypto_openssl.c 10:46:24 That ensures that things remain cleaner 10:46:48 dazo 10:47:23 that line is EVP_CipherUpdate (ctx, out, outl, in, inl); in my tree. ... my HEAD is 4970f1485d4d2117ccb3b1932965809fc51d8efe 10:47:41 andj 10:48:01 https://github.com/andj/openvpn-ssl-refactoring/commit/dd253d7934e18343193d085dfe6aff97ea104b05#L3R104 10:48:02 vpnHelper 10:48:02 Title: Commit dd253d7934e18343193d085dfe6aff97ea104b05 to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 10:48:27 ahh, good ACK on dd253d7934e18343193d085dfe6aff97ea104b05 10:50:11 andj 10:50:24 https://github.com/andj/openvpn-ssl-refactoring/commit/b8368cccf8b7bc448be9b1b73c83c10499473263 moves the DES key fixup/check functions vpnHelper 10:50:25 Title: Commit b8368cccf8b7bc448be9b1b73c83c10499473263 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:50:46 There was already a comment about magic numbers there see https://github.com/andj/openvpn-ssl-refactoring/commit/be63e6e86837cec71b35446a164ab158cd986ab1 10:50:58 vpnHelper 10:50:59 Title: Commit be63e6e86837cec71b35446a164ab158cd986ab1 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:51:01 for the fix on that But that patch lies far in the future 10:51:23 The three important functions are: +int key_des_num_cblocks (const cipher_kt_t *kt); 108 10:52:08 bool key_des_check (uint8_t *key, int key_len, int ndc); 10:52:16 and void key_des_fixup (uint8_t *key, int key_len, int ndc); 10:52:21 dazo 10:52:44 yeah, and they are identical afaics andj 10:53:10 That's the beauty of a good refactoring session, the code doesn't change (much) dazo 10:53:20 absolutely 10:53:24 andj 10:54:08 Next patch gets more interesting though, the bread and butter of the crypto lib dazo 10:54:14 what was the patch which fixes the issues you mentioned? andj 10:54:25 https://github.com/andj/openvpn-ssl-refactoring/commit/be63e6e86837cec71b35446a164ab158cd986ab1 vpnHelper 10:54:27 Title: Commit be63e6e86837cec71b35446a164ab158cd986ab1 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 10:54:30 that one dazo 10:54:33 I see stuff like "+typedef EVP_CIPHER cipher_kt_t;" ... that kind of surprises me a bit mattock 10:54:45 ACK on b8368? andj 10:55:19 dazo: you need a generic cipher type and a generic md type that's what those add 10:55:24 mattock 10:55:40 or still in progress? 10:55:47 andj 10:55:49 how you define them is up to you, could be a struct/a library, anything in progress I think 10:55:54 not a library, I meant an integer 10:56:06 dazo 10:56:09 yeah, fair enough .... but you use then +key_des_num_cblocks (const EVP_CIPHER *kt) ... why not use cipher_kt_t directly? andj 10:56:25 It's the openssl specific part of the library so it's ok to use the openssl-specific construct 10:56:43 makes the openssl code a little clearer, but doesn't hurt the backend API 10:57:07 dazo 10:57:35 yeah, okay ... I can see that point ... I'd like to have a second opinion on b8368cccf8b7bc448be9b1b73c83c10499473263 andj 10:57:49 dazo: changing that shouldn't be a problem though mattock 10:58:05 jamesyonan: what's your take on https://github.com/andj/openvpn-ssl-refactoring/commit/b8368cccf8b7bc448be9b1b73c83c10499473263 ? vpnHelper 10:58:06 Title: Commit b8368cccf8b7bc448be9b1b73c83c10499473263 to andj/openvpn-ssl-refactoring - GitHub (at github.com) L'utente krzee è entrato nella stanza 10:58 dazo 10:58:34 no, not at all ... it's just that I feel unsure about it, without really knowing why ... and so I'd like to then get a pair of extra eyes on it, just in case andj 10:58:51 dazo: we can add it as a note, and move on to the next patch for now dazo 10:58:59 that's fine mattock 10:59:02 let's move on jamesyonan 10:59:31 seems reasonable to me dazo 10:59:31 yup mattock 10:59:41 ok, addings ACKs from dazo and jamesyonan dazo 10:59:43 okay, then ACK by james and me andj 10:59:47 https://github.com/andj/openvpn-ssl-refactoring/commit/d86e00908abc3f98e90d589daadfc07caac4f2c7 vpnHelper 10:59:48 Title: Commit d86e00908abc3f98e90d589daadfc07caac4f2c7 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:00:02 is the next one, had the same criticism about magic numbers 11:00:10 and again solved in the future patch 11:00:18 dazo 11:00:27 yeah, I saw that here you basically simplify two function calls, done often with one function? cipher_des_encrypt_ecb() 11:01:32 andj 11:01:34 This one just simplifies things a little yeah, exactly 11:01:42 dazo 11:01:58 ACK on d86e00908abc3f98e90d589daadfc07caac4f2c7 andj 11:02:18 ok, bread and butter time https://github.com/andj/openvpn-ssl-refactoring/commit/21e946650fae0b777f7e0f1811213f167fb0648a 11:02:19 vpnHelper 11:02:20 Title: Commit 21e946650fae0b777f7e0f1811213f167fb0648a to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:02:31 this one moves the message digest type functions const md_kt_t * md_kt_get (const char *digest); 11:03:03 const char * md_kt_name (const md_kt_t *kt); 11:03:09 int md_kt_size (const md_kt_t *kt); 11:03:16 are the new backend functions 11:03:21 they allow a digest type to be chosen, and some information about them to be printed 11:04:02 and retrieved 11:04:10 Again, it's just a rename/moe 11:06:24 move 11:06:26 dazo 11:06:27 ACK on 21e946650fae0b777f7e0f1811213f167fb0648a andj 11:06:40 https://github.com/andj/openvpn-ssl-refactoring/commit/9bacf964d13a357c3d9d1b6a14121c3694477a24 vpnHelper 11:06:41 Title: Commit 9bacf964d13a357c3d9d1b6a14121c3694477a24 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:06:46 Now that the MD type has moved dazo 11:06:49 yeah, didn't notice where init_key_type() went andj 11:06:53 this adds the actual functions I'm trying to keep naming within a backend consistent 11:07:30 There is a subtle change here: the MD5_* functions aren't called anymore 11:08:20 the md_ctx_* generic functions are called instead 11:08:32 dazo 11:08:38 mm andj 11:09:55 That was done to keep the interface simple, and more generic dazo 11:10:00 ACK on 9bacf964d13a357c3d9d1b6a14121c3694477a24 andj 11:10:13 https://github.com/andj/openvpn-ssl-refactoring/commit/f331011e2ab7aa59f5493907a2b1d98925fa7f97 vpnHelper 11:10:14 Title: Commit f331011e2ab7aa59f5493907a2b1d98925fa7f97 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:10:19 same thing, but for HMAC dazo 11:10:35 mattock: are we going to sprint the whole time today, or did you have any other plans as well? dazo can manage 50 more minutes, but believe his head is about to explode at that point 11:11 andj 11:11:19 Let's try to get the crypto separation patches done today, if possible dazo 11:11:25 yeah, agreed (8 more patches, incl HMAC) 11:11:54 andj 11:11:59 Just move milestone by milestone, and perhaps go for an extra session next week... or not, depending on when the alpha needs to be released 11:12:16 mattock 11:12:54 dazo: no other plans if there were, they'd be on the topic page 11:13:10 dazo 11:13:12 goodie let's get done with the crypto stuff today then andj: you add some extra checks in hmac_ctx_init() here? 11:14:57 + ASSERT(NULL != kt && NULL != ctx); 11:15:13 + 11:15:14 + CLEAR(*ctx); 11:15:14 I presume this is new 11:15:17 andj 11:15:20 yeah, sorry, force of habit could remove them, but I don't think they hurt 11:15:44 dazo 11:15:52 no, not at all I don't mind them here, just wanted to be sure 11:15:59 but here, more things happens as well 11:16:07 struct key *key is replaced with const uint8_t *key 11:16:31 so the whole API is getting rid of the struct as well? 11:17:05 andj 11:17:17 Inside the backend API, yes dazo 11:17:21 (whole == hmac) L'utente krzee si è disconnesso (Quit: This computer has gone to sleep) 11:17 andj 11:17:38 to keep the backend API generic struct key contains both the hmac and the cipher key 11:18:26 dazo 11:18:27 ahh! I see, the struct is used in init_key_hmac() in crypto.c instead of passing them the whole way through andj 11:18:33 if I'm not mistaken exactly 11:18:38 and only the hmac key is passed to the backend api 11:18:52 dazo 11:19:02 yeah, which makes a lot more sense as well a "need to know" basis on the functions 11:19:15 andj 11:19:24 so  init_hmac (ctx->hmac, kt->digest, key, kt, prefix); 11:19:24 becomes  hmac_ctx_init (ctx->hmac, key->hmac, kt->hmac_length, kt->digest, 522+    prefix); 11:19:33 (ignoring the 522+ copy-paste error) 11:19:58 L'utente psha è entrato nella stanza 11:20 dazo 11:20:43 why do you add + if (prefix) on the msg() in this function? andj 11:21:30 Hmm, let me think dazo 11:21:42 in fact you do it later on twice again, where even a curly brace could caught those two andj 11:21:48 ah, right... to ensure that you can call it quietly dazo 11:22:28 but why hook it to the prefix variable? ahh, I see now 11:22:50 the prefix is only used when printing stuff, otherwise it would print (null) instead, which would be nonsense 11:23:21 andj 11:23:28 I'm actually not entirely sure that I'm happy with that bit myself in the sense that it might not be best to print the information from the backend 11:23:47 dazo 11:23:59 good point andj 11:24:01 but that the printing should be in the front end perhaps how about we add a note to try to factor that out in a future patch? 11:24:19 dazo 11:24:30 agreed, that makes a lot more sense .... esp. if it is generic stuff it prints .... if it is backend specific, then it's fine where it is agreed 11:24:40 ACK on f331011e2ab7aa59f5493907a2b1d98925fa7f97 11:25:15 andj 11:25:29 https://github.com/andj/openvpn-ssl-refactoring/commit/5b8f6dbaf96b0b82a50e4b3c9acb4bbe6f5bf968 vpnHelper 11:25:30 Title: Commit 5b8f6dbaf96b0b82a50e4b3c9acb4bbe6f5bf968 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:25:33 key types like md types, but for ciphers 11:25:41 mattock 11:25:50 could someone write a short sentence explaining what part of f331011 should be fixed later? a summary, if you will 11:25:58 dazo is back 11:26 andj 11:26:45 "The hmac_ctx_init function should not print generic information" mattock 11:26:47 ok adding 11:26:49 dazo 11:26:52 yeah andj 11:28:33 so this one adds static cipher information which can be used during ... ciphering 11:28:42 dazo 11:32:18 In the old implementation, kt_key_size() does check kt->cipher_length ... in addition, and may return kt->cipher_length * 8 while the new one only uses EVP_CIPHER_key_length() ... no matter what 11:32:37 any reason for that change? 11:32:42 andj 11:33:07 let me check dazo 11:34:51 kt_key_size() -> cipher_kt_key_size(), if I didn't miss anything andj 11:34:59 I think it's because of this +      kt->cipher_length = cipher_kt_key_size (kt->cipher); so it basically gets initialised from there, and never changed after that as far as I can tell 11:35:26 So it's unnecessary to return kt->cipher_length 11:35:49 which is good, since we then don't need kt in the backend module 11:36:01 struct key_type 11:36:18 is similar to struct key in that sense 11:36:26 dazo 11:36:32 true ... but it's some of the logic which is then lost where - if (kt->cipher_length) 11:36:42 - return kt->cipher_length * 8; 11:36:42 it's basically 8-doubling the cipher_length in some cases 11:37:07 andj 11:37:12 in the sens that cipher_length changes? yeah, that's true 11:37:32 dazo 11:37:32 this part of the logic is nowhere now ... and that makes me a bit worried mattock 11:37:51 jamesyonan: any comments on ^^^ dazo 11:37:56 - else if (kt->cipher) - return EVP_CIPHER_key_length (kt->cipher) * 8; 11:37:56 and this multiplication is also lost 11:38:03 would it be correct then add * 8 in crypto.c where cipher_kt_key_size() is called? 11:38:50 jamesyonan 11:38:59 thinking about this... the * 8 is just to convert cipher length in bytes to bits 11:40:58 dazo 11:41:47 mm jamesyonan 11:41:47 it's mostly used for reporting purposes so you can say BF-CBC 128 bits 11:42:20 andj 11:43:02 The old kt_key_size function where the *8 was used 11:43:07 is only used in options.c 11:43:21 as far as I can tell at the moment 11:43:30 dazo 11:43:32 well, but I see kt->cipher_length is used many places ... and I'm worried that if that's now 1/8 of what it was that we're doing something wrong andj 11:43:44 It's still the same, right? dazo 11:44:12 yeah ... it's used quite some places in crypto.c ... struct key_type andj 11:44:41 kt->cipher_length = in bytes and so is  EVP_CIPHER_key_length (kt->cipher) 11:44:44 so the only difference is in options.c 11:45:58 - buf_printf (&out, ",keysize %d", kt_key_size (&kt)); 11:46:08 +  buf_printf (&out, ",keysize %d", kt.cipher_length); 11:46:08 so that should be multiplied by 8 perhaps 11:46:20 the value of cipher_length doesn't change 11:47:34 just the value used in the options string 11:47:42 jamesyonan 11:47:43 yeah, you don't really want to store the *8 value. Internally, we always represent the key size in bytes. andj 11:48:03 nothing changes, except for the options string, which I'll fix now jamesyonan 11:48:09 the *8 should only be for reporting the key siz e in bits dazo 11:50:46 okay, this is actually an "accidental fix" then ... right? (except that options.c now need the *8) 11:51:07 andj 11:51:20 https://github.com/andj/openvpn-ssl-refactoring/commit/de00fa7e30d7a68528f1ce7338f4f4e83d665090 vpnHelper 11:51:23 Title: Commit de00fa7e30d7a68528f1ce7338f4f4e83d665090 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:51:27 is the patch that fixes the options dazo 11:51:58 ACK on de00fa7e30d7a68528f1ce7338f4f4e83d665090 11:52:03 andj 11:52:09 wheeee almost there 11:52:18 https://github.com/andj/openvpn-ssl-refactoring/commit/5b8f6dbaf96b0b82a50e4b3c9acb4bbe6f5bf968#L4R2807 11:52:19 vpnHelper 11:52:20 Title: Commit 5b8f6dbaf96b0b82a50e4b3c9acb4bbe6f5bf968 to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 11:52:23 (you forgot signed-off) andj 11:52:34 That might have happened a few times dazo 11:52:49 yeah, it happens easily I might cherry-pick your patches one-by-one, fixing such stuff along the way 11:53:10 andj 11:53:23 Would hurt my tree, but ok dazo 11:53:46 well, as long as jamesyonan finds 5b8f6dbaf96b0b82a50e4b3c9acb4bbe6f5bf968 okay, I'm fine with that one andj 11:54:26 cool, https://github.com/andj/openvpn-ssl-refactoring/commit/05c901305e658d188e2bf5020a425bace935a8a2 vpnHelper 11:54:28 Title: Commit 05c901305e658d188e2bf5020a425bace935a8a2 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 11:54:36 adds cipher operations I'd like to immediately add the comment about cipher_ctx_init 11:55:24 which is similar to hmac_ctx_init 11:55:33 in that it contains a msg 11:55:40 so that'll get fixed 11:55:48 jamesyonan 11:57:18 I'm okay with 5b8f6dbaf96b0b82a50e4b3c9acb4bbe6f5bf968 assuming that the * 8 issue is resolved andj 11:57:41 yeah, I pushed a patch for that just now dazo 11:58:05 jamesyonan: * 8 is solved by doing it in options.c, where it is printed jamesyonan 11:58:28 sure, that's reasonable mattock 11:59:28 a few more dazo 11:59:55 andj: cipher_ctx_init () is basically using exactly the same approach as the hmac function ... here it prints no matter what, related to prefix ... so moving that out of backend and into frontend makes sense andj 12:00:06 yup, will do that dazo 12:01:57 ACK on 05c901305e658d188e2bf5020a425bace935a8a2 andj 12:02:36 doxygen additions: https://github.com/andj/openvpn-ssl-refactoring/commit/81fd3eda9299a19ed5dce7ac89f8638a41dcc2b3 vpnHelper 12:02:38 Title: Commit 81fd3eda9299a19ed5dce7ac89f8638a41dcc2b3 to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 12:03:19 ACK on 81fd3eda9299a19ed5dce7ac89f8638a41dcc2b3 andj 12:03:19 BTW, we've done the hardcore patches now rest should be plain sailing 12:03:26 https://github.com/andj/openvpn-ssl-refactoring/commit/07b4cdb87c866059dee207a77faf4f76bfb9d43f 12:03:27 dazo 12:03:27 whoooh vpnHelper 12:03:28 Title: Commit 07b4cdb87c866059dee207a77faf4f76bfb9d43f to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 12:03:29 andj 12:03:41 move a function dazo 12:04:02 just out of curiosity .... why? andj 12:04:15 keeps the MD* functions close together dazo 12:04:21 fair enough ACK on 07b4cdb87c866059dee207a77faf4f76bfb9d43f 12:04:28 andj 12:04:28 call it OCD dazo 12:04:33 andj 12:04:42 https://github.com/andj/openvpn-ssl-refactoring/commit/740f3b58c7fa48dec5db37e6f6d36b0c47e30957 vpnHelper 12:04:43 Title: Commit 740f3b58c7fa48dec5db37e6f6d36b0c47e30957 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 12:04:49 the promised "get rid of old stuff" patch 12:04:51 which does what it says on the tin 12:05:11 basically gets rid of all openssl-specific stuff (which was moved in previous patches) 12:05:45 from the crypto.c/h files 12:05:53 dazo 12:06:18 yeah ... I'll just do a compile with openssl, just to be 100% sure andj 12:06:43 and last of all there's https://github.com/andj/openvpn-ssl-refactoring/commit/fe519cd6f0c382e5b75945e8cac9b825a6e3625f 12:06:47 vpnHelper 12:06:48 Title: Commit fe519cd6f0c382e5b75945e8cac9b825a6e3625f to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 12:06:51 which is another OCD patch doesn't change any code, just whitespace 12:07:25 dazo 12:07:25 ACK on 740f3b58c7fa48dec5db37e6f6d36b0c47e30957 jamesyonan 12:09:25 I'm not really a big fan of patches that change style mattock 12:09:37 jamesyonan: which one? andj 12:09:39 jamesyonan: it moves it to the rest of openvpn style but it can be safely ignored 12:09:47 so we can just leave it 12:09:54 dazo 12:10:11 such patches are tricky, it's easy to slip in things ... but I do like a uniform coding style, though mattock 12:10:13 ah, whitespace andj 12:10:49 It's in a separate patch because I didn't want to have a row about whitespace mattock 12:11:17 jamesyonan: ACK or NACK? either would be good jamesyonan 12:11:42 I'd say NACK on fe519 if it's just about whitespace dazo 12:11:43 it's not easy to do code review on this patch ... as the change set is so big, with big blocks mattock 12:11:55 ok, NACK then, leave as is dazo 12:11:57 because of that, I tend to a NACK as well andj 12:12:08 It does move it to the OpenVPN code style, but let's just nack it dazo 12:12:30 heh Yeah, I'm torn ... as I'd like a pure unified style however, I'd need a better tool to verify line-by-line 12:12:52 andj 12:13:06 it's just been pulled through an indenter jamesyonan 12:13:17 well the problem with patches like these is you are guaranteed to get merge conflicts if any pre-style patches are applied to post-style code dazo 12:13:57 yeah, even though, 'git apply' have --whitespace=fix .... which really can do miracles in such cases jamesyonan 12:14:24 right, that's cool dazo 12:15:10 My only concern is that the review task is heavy ... I woldn't mind ACKing it, if I could just verify it easily that it only changes whitespaces jamesyonan 12:15:25 yeah, exactly andj 12:15:30 just throw it through an indenter yourself dazo 12:15:36 andj: lets' put this one on hold for now ... and when we're through everything else, have things pushed, we can pick up this one again 12:16:11 mattock 12:16:17 sounds good still one more, right? "Added a check for Openssl or PolarSSL defines" 12:16:23 andj 12:16:33 yeah, cool oh, oops 12:16:56 https://github.com/andj/openvpn-ssl-refactoring/commit/f7843d14f10c3755c96f28d96ed02eeae20d0e56 12:16:57 vpnHelper 12:16:58 Title: Commit f7843d14f10c3755c96f28d96ed02eeae20d0e56 to andj/openvpn-ssl-refactoring - GitHub (at github.com) dazo 12:17:23 ACK on f7843d14f10c3755c96f28d96ed02eeae20d0e56 jamesyonan 12:17:58 one is misspelled mattock 12:18:06 do guys still have energy to review the 6 remaining doxygen patches for "no functionality changes"? dazo 12:18:33 jamesyonan: which one? dazo must be blind ... 12:18 dazo 12:18:52 ahh, in the commit message? mattock: the comment on "Refactored maximum cipher and hmac length constants" can be removed ... we agreed it makes sense 12:19:06 andj 12:20:01 Here's the first fix for the cipher doc stuff: https://github.com/andj/openvpn-ssl-refactoring/commit/89e31cfb9c6c5fd33600a76c77e645c24dd0663b vpnHelper 12:20:03 Title: Commit 89e31cfb9c6c5fd33600a76c77e645c24dd0663b to andj/openvpn-ssl-refactoring - GitHub (at github.com) mattock 12:20:23 dazo: ok, fixing dazo 12:20:28 thx! mattock 12:20:52 done dazo 12:22:11 ACK on 89e31cfb9c6c5fd33600a76c77e645c24dd0663b jamesyonan 12:22:42 +# error "At least on of OpenSSL or PolarSSL needs to be defined." dazo 12:22:49 duh! now I see it 12:22:53 andj 12:23:00 doh can you take that along with the patch merge dazo? 12:23:09 dazo 12:23:27 mattock: make a note on that patch, that need to fix spelling "on -> one" please 12:23:31 12:23:32 cron2 12:23:33 folks, you're so impressive... (I've been watching you with half an eye, but had "customer is waiting, time is due" work to do) dazo 12:24:06 cron2: next week is your chance mattock 12:24:14 89e31cfb is not on the list, adding to a random place dazo 12:24:28 mattock: at the end of crypto stuff we did now andj 12:24:33 It's a fix to an issue cron2 12:24:44 dazo: I smell a portability/cleanup sprint coming up, but "not next week" (mom's birthday) dazo 12:24:55 andj 12:25:04 I'm working on a hardcore windows fixing sprint at work dazo 12:25:23 andj: I don't envy you mattock 12:25:50 mkay, done dazo 12:25:58 thx! cron2 gets to replay a 15-year-old zmodem implementation at work. That's actually fun 12:26 mattock 12:26:18 andj: fixing Windows? you sure set your aim high dazo 12:26:24 that sounds fun, in an odd way andj 12:26:59 haha dazo 12:27:13 re: next weeks ... I'm in serious need of some holiday, so even though I will only have ~1 week real holiday ... I'd like to excuse myself for the next 2 weeks andj 12:27:28 ok, what shall we do review/ack-wise? mattock 12:27:48 dazo: get some rest, we need you in one piece dazo 12:27:54 on the other hand, I'll try to get the doxygen patches applied + the patches we've ACKed today next Thursday ... I really need to get ahead of my TODO list mattock: thx 12:28:13 I trust cron2 to be a valuable reviewer as well ... so if he finds time (but obviously not next week), I have confidence there 12:28:53 unless, jamesyonan steps in as well 12:29:19 cron2 12:29:36 what did you leave to be ACKed? dazo 12:29:51 SSL library separatoin *separation 12:29:55 cron2 12:30:05 oh dazo 12:30:06 and Verification functions cron2 has no idea about crypto stuff 12:30 cron2 12:30:25 give me some good network related changes dazo 12:30:25 well, the separation patches are doable andj has no idea about network stuff in OpenVPN 12:30 dazo 12:30:45 crypto separation is what we did today ... and that was fairly fine to review andj 12:30:45 ssl_separation is similar 12:31:02 dazo 12:31:29 yeah, that's what I kind of hoped for andj 12:31:54 verification is a little tougher mattock 12:32:06 cron2, jamesyonan: up for "SSL library separation" patchset review next Thursday at 17:00 UTC? andj 12:32:08 because some big cleanup happened there dazo 12:32:18 ahh jamesyonan 12:32:34 works for me andj 12:32:40 works for me dazo 12:32:55 then there'll be some progress next week as well mattock 12:33:02 and I can be the Trac slave / secretary andj 12:33:07 I'm very happy about today's session mattock 12:33:12 me too, we made great progress cron2 12:33:13 mattock: no, mom's birthday andj 12:33:24 other day next week? dazo 12:33:26 andj: jamesyonan: As the verification stuff is heavier, and might require more background knowledge ... maybe you could do that and then save SSL library separation for cron2 and/or me? 12:33:42 mattock 12:33:50 hmm, might be a good idea andj 12:34:14 That's fine too jamesyonan 12:34:45 yes, I can help review SSL verification stuff dazo 12:34:56 goodie andj 12:34:56 cool, thanks last requested patch is ready, pushing in a sec 12:35:13 mattock 12:35:14 nice, SSL verification shall be our target next week andj 12:36:44 https://github.com/andj/openvpn-ssl-refactoring/commit/7f009fd01788dd5787facd953fe260491ac62b44 dazo 12:36:45 andj: let me know when pushed vpnHelper 12:36:46 Title: Commit 7f009fd01788dd5787facd953fe260491ac62b44 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 12:36:47 last one dazo 12:36:47 heh dazo fetches 12:36 andj 12:37:22 That looks a lot better dazo 12:37:44 ACK on 7f009fd01788dd5787facd953fe260491ac62b44 mattock: ^^ last patch to today's session 12:37:55 mattock 12:38:00 nice which patch does this fix/affect? 12:38:18 dazo 12:38:27 mattock: if you make a note to fix whitespace issues here ... there are 3 lines which needs to be fixed mattock 12:38:28 I'll make a note of it andj 12:38:30 the HMAC one mattock 12:38:32 ok oh, obviously 12:38:40 dazo 12:38:47 heh, yeah andj 12:39:14 wow, really happy about that dazo too 12:39 andj 12:39:30 took some work, but it's in mattock 12:39:40 fixes the "do not print generic stuff" issue, I presume dazo 12:39:40 yeah andj 12:39:48 yeah, exactly dazo 12:40:22 andj: btw ... this whole session have gone through an openvpn + polarssl compile cron2 12:40:34 using IPv6, I hope (but otherwise: cool) 12:40:53 dazo 12:40:59 unfortunately, no IPv6 on the work VPN infrastructure andj 12:41:15 oh yeah, just a heads up: dazo 12:41:16 but I use IPv6 tunnel when being in the office .... mattock 12:41:21 final version (rev40) of the topic page: https://community.openvpn.net/openvpn/wiki/Topics-2011-07-14 vpnHelper 12:41:22 Title: Topics-2011-07-14 â OpenVPN Community (at community.openvpn.net) andj 12:41:27 I'm playing with CMake at work to ease our windows build a little there 12:41:39 Noticing some advantages and some disadvantages 12:42:02 mattock: great work 12:42:31 dazo 12:42:34 yeah, CMake makes some things very nice and easy .... other things very awkward andj 12:43:10 But I just loved the way I could use cmake to generate visual studio stuff and nmake stuff for that matter 12:43:22 when I was building PolarSSL 12:43:29 so I thought I'd give it a try 12:43:35 to learn as well 12:43:38 dazo 12:43:41 I believe there's a lot of benefits when combining cmake with Windows andj 12:44:11 Not necessarily for packaging, as I don't think it supports signing (not sure about that) but for building it's great 12:44:18 dazo 12:44:23 one thing I'm struggling with cmake though, is parallel building (make -jX) ... that can sometimes fail badly andj 12:44:38 hmm, interesting to have a look at dazo 12:45:05 I'm using CMake in eurpehia .... and make -jX fails 8 of 10 times (well, last time I tried was with 2.6 .... it might have improved in 2.8) 12:45:26 andj 12:46:03 yeah, 2.8 improved quite a few things I heard packaging support as well 12:46:12 dazo 12:46:13 there's some dependency tracking which is lacking probably packaging I've not looked into yet, use it only for building 12:46:30 for now, at least 12:46:34 mattock 12:47:15 I assume the official part is over, so I'll write the summary now andj 12:47:24 yup, I think so dazo 12:47:36 mattock: thx a lot for keeping track of the review process! andj 12:47:42 Anyway, I'm going to take a break from the computer, thanks everyone dazo 12:48:03 andj: thank you, you too! andj 12:48:10 spend some time with the wife and all that mattock 12:48:37 good sprint/meeting guys!
