Hi, Here's the summary of the previous IRC meeting.
--- COMMUNITY MEETING Place: #openvpn-devel on irc.freenode.net List-Post: openvpn-devel@lists.sourceforge.net Date: Monday 2nd Feb 2015 Time: 20:00 CET (19:00 UTC) Planned meeting topics for this meeting were on this page: <https://community.openvpn.net/openvpn/wiki/Topics-2015-02-02> Next meeting has not been scheduled but will be on the same weekday and time. Your local meeting time is easy to check from services such as <http://www.timeanddate.com/worldclock> SUMMARY cron2, d12fk, lev, jamesyonan, mattock and segoon participated in this meeting. --- Discussed the "Mac OS X Keychain management client" (v3) patch: <http://thread.gmane.org/gmane.network.openvpn.devel/9421> The patch was given an ACK by jamesyonan on the condition that the changes affecting existing codebase are tested on Android. We will ask if plaisthos could help with the testing. --- Discussed the "Make OpenVPN set routes on Windows Vista and later" patch: <http://sourceforge.net/p/openvpn-gui/openvpn/ci/8195efc0d8841b52217643a43d486cda2e171fea> Jamesyonan agreed the general approach used in interactive.c is the best one. Although some potential improvements were identified, the patch was given an ACK by cron2 in order to get it into the wild in form of Git "master" Windows snapshots. Incremental improvements will be made based on user feedback and practical tests. --- Discussed the "Account for peer-id in frame size calculation" patch: <http://thread.gmane.org/gmane.network.openvpn.devel/9418> Decided to delay the link-mtu calculation until after options have received. This fixes the issue at hand and should not bring up new surprises. More discussion is needed to figure out the best long-term plan regarding protocol negotiations in general. --- Full chatlog is attached. -- Samuli Seppänen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock
(19.47.01) d12fk: mattock: yes I can stick around (20.19.45) segoon [~va...@ppp83-237-17-156.pppoe.mtu-net.ru] è entrato nella stanza. (20.19.57) segoon: hi (20.58.11) mattock: hi guys (20.58.25) mattock: and james is here also (20.58.31) cron2: ho! (21.00.38) mattock: topic list: https://community.openvpn.net/openvpn/wiki/Topics-2015-02-02 (21.00.39) vpnHelper: Title: Topics-2015-02-02 – OpenVPN Community (at community.openvpn.net) (21.00.58) mattock: is the topic order ok for everyone? (21.01.05) mattock: anything to add to the agenda? (21.01.20) cron2: wfm (21.02.35) mattock: in english? :P (21.02.41) cron2: works for me (21.02.48) mattock: ah :) (21.03.00) mattock: so first topic: "Mac OS X Keychain management client (v3)" (21.03.16) mattock: does this one get an ACK from somebody? (21.03.32) mattock: http://thread.gmane.org/gmane.network.openvpn.devel/9421 (21.03.35) vpnHelper: Title: Gmane Loom (at thread.gmane.org) (21.04.53) d12fk: hi (21.05.13) segoon: nope, and i'm afraid it got no comments (21.05.32) segoon: I hope it is kind of "no objections" :) (21.05.35) mattock: hi d12fk! (21.05.40) mattock: yeah, I hope so too (21.05.56) cron2: segoon: unfortunately, more "nobody reviewed it yet" (21.07.49) mattock: jamesyonan: any thoughts on the v3 of the MacOS X keychain patch? (21.08.13) jamesyonan: it's going to take me a while to digest this all (21.09.02) jamesyonan: but overall I think the NEED-CERTIFICATE approach is a big improvement (21.10.04) d12fk: i can stay until until 2100h, need to get sleep. superbowl took 4 hour of my night (21.10.19) mattock: d12fk: 50 minutes? (21.10.28) cron2: segoon: it's more intrusive than I expected, so I can't say "good" or "no good" on this easily. The direction looks good. (21.10.35) d12fk: mattock: roughly (21.10.37) mattock: ok (21.11.17) segoon: cron2: I tried to group all similar stuff from existing code and my code to limit code duplication (21.11.35) segoon: all changes to existing code should be rather simple (21.12.14) cron2: segoon: yes, but it's code paths that I'm absolutely not familiar with, so I do not feel comfortable judging this off-hand (21.12.24) cron2: (the networking paths, I would :) ) (21.16.20) jamesyonan: what is the rationale for buffer_list_aggregate_separator? (21.18.48) segoon: it shrinks a list to a string adding "\n" between list items (21.18.57) segoon: like "\n".join(list) (21.19.05) segoon: (I use it this way) (21.19.27) jamesyonan: why did we not need it before but we need it now? (21.21.04) segoon: I joined char** manually which is error-prone (21.21.33) segoon: I was told to use buffer API for such kind of actions (21.21.46) jamesyonan: right (21.24.30) jamesyonan: so it looks like you wrote a more general management_query_multiline_flatten and then refactored some existing functions like management_query_rsa_sig to use it (21.24.45) segoon: right (21.25.32) jamesyonan: how much testing did you do on this and buffer_list_aggregate refactor to ensure that there's no breakage of existing code that uses these methods? (21.27.38) segoon: well, I started openvpn with and without keychain, pushed data via tunnel (21.28.37) jamesyonan: did anyone test this with Android to ensure that there's no breakage caused by the refactoring? (21.28.38) segoon: I haven't tested buffer_list_aggregate specifially (21.29.26) mattock: plaisthos: ^^^ (21.32.45) mattock: jamesyonan: so android in particular could have issues? (21.32.57) cron2: mattock: plaisthos said he has no time today (21.33.05) mattock: ok (21.33.24) jamesyonan: well android uses the external PKI methods, so it uses some of the same code that was refactored (21.33.33) jamesyonan: I think it would make sense to test that (21.33.34) mattock: so it would be good test (21.33.49) mattock: we could ask plaisthos to do some testing (21.33.57) mattock: what in particular would need to be tested? (21.35.13) jamesyonan: overall the patch looks good -- the issues I had last time have been largely resolved, but I think the refactors justify more testing, especially of code that exercises the refactored functions (21.35.39) mattock: excellent! (21.36.07) mattock: so provided nothing breaks it's an ACK? (21.36.25) jamesyonan: yes (21.36.44) mattock: so some work for plaisthos and we're done :) (21.37.03) segoon: great! (21.37.06) jamesyonan: who else has acked this so far? (21.37.13) mattock: nobody (21.37.17) mattock: as a whole, at least (21.37.43) cron2: we gave segoon guidance how we want this to look like (so, feature-wise I think we agree on this), but nothing code-wise yet (21.41.53) mattock: shall we move on? (21.42.05) mattock: d12fk needs to split soon (21.44.24) cron2: d12fk: did you find time to revamp the openvpn side to use tuntap_options? (21.44.45) d12fk: nope not yet (21.45.12) d12fk: i have to notes from two week stored safly on disk =) (21.48.42) mattock: where did stop two weeks ago? (21.48.59) d12fk: right before the windows service (21.49.00) mattock: didn't we get the windows-specific bits partially reviewd? (21.49.01) mattock: ok (21.49.29) cron2: I think we agreed (three weeks ago?) on using tuntap_options for the pipe id, and I think james and d12fk did some discussion on the service architecture two weeks ago (21.50.11) cron2: but i have no idea what the next steps should be - I'm fine merging the amended openvpn code, because that's pretty self-contained (so, future-ACK here :) ) (21.50.58) d12fk: yeah the service could use some review as well (21.51.22) cron2: do we all agree on the general architecture? Jamesyonan? (21.51.36) mattock: a very brief summary of the previous meeting here: https://sourceforge.net/p/openvpn/mailman/message/33244201/ (21.51.37) vpnHelper: Title: OpenVPN / Mailing Lists (at sourceforge.net) (21.51.41) mattock: nothing technical, though (21.51.57) mattock: (gmane seems to be dropping quite a lot of messages, like that one) (21.57.07) jamesyonan: I think this is the right approach for Windows, i.e. using a Windows pipe and have the server "impersonate" the credentials of the client (22.00.46) mattock: what about the windows service code itself? (22.00.52) mattock: any comments? (22.01.26) d12fk: i personally don't like how the parametrs are passed from the gui to the service (22.03.41) cron2: if I remember right, it's 0-separated strings passed along a pipe? (22.03.47) d12fk: yes (22.04.33) cron2: can a windows filename include 0-bytes (due to character encoding)? (22.04.37) d12fk: the the command line params are just the string. I would rather have a tlv list so that only predefined options are allowed to be passed. easier to lock down (22.05.35) cron2: so that would just be "--config $there"? What other arguments are passed (maybe --setenv IV_GUI_VER ...)? (22.05.51) d12fk: cron2: no, it it's wide chars it's still 0x0041 and not 0x0000 (22.06.14) cron2: d12fk: ah, so it's *all* wide chars and you parse it as wide only (22.06.22) d12fk: yes (22.06.30) cron2: (this is what I hate about UTF8 - variable length surprises) (22.07.19) cron2: so, what options do you pass? haven't looked into the gui code yet (22.07.20) d12fk: ... but never 0 (22.07.37) cron2: ok, good :) (22.07.40) d12fk: all kinds of stuff. mgmt itf things (22.08.02) d12fk: proxy config (22.08.32) cron2: oh, so gui fetches windows system proxy settings and uses that? (22.10.10) d12fk: http://sourceforge.net/p/openvpn-gui/code/ci/master/tree/openvpn.c#l685 (22.10.12) vpnHelper: Title: OpenVPN GUI / OpenVPN-GUI / [a449f6] /openvpn.c (at sourceforge.net) (22.10.27) cron2: thanks (22.10.43) d12fk: yes, it passes the mgmt itf option so proxy info is queried during rnutime (22.12.03) d12fk: currently you could also pass --route and defeat the purpose of the service (22.12.21) d12fk: at least all the security wizardry (22.13.04) cron2: well, you can also put --route into your .ovpn (unless you lock that one down, which I understand is an option depending on installation) (22.13.39) d12fk: in general it is under program files which is read only in later windows versions (22.13.45) cron2: well, you could certainly go TLV here, it's only a handful of flags, basically the variables in line 686-692 plus managge-password (22.14.11) d12fk: yes that would make it more robust (22.15.40) cron2: I'm not sure I grok the code, though (22.15.46) cron2: you build "cmdline" (line 686) (22.16.14) cron2: *if* service pipe succeeds, you build startup_info, which only has manage info + config dir in it (22.16.31) cron2: and only startup_info is sent to the pipe (22.16.35) cron2: or am I misreading that? (22.16.58) cron2: ok, found it - "options" = "cmdline +8" (22.18.16) cron2: so, going for TLV would be more robust against local attackers trying to mess up routing via help of openvpn + openvpn service but not having a server to help them with that (22.18.36) cron2: (but having a valid .ovpn in place, because if openvpn can't connect it won't set up routes either) (22.19.10) cron2: I'd call that pretty obscure :-) - but robust is good, if we can get this actually done... So how's your available time? (22.19.31) d12fk: not so good currently (22.20.07) cron2: so maybe we should go for "merge what we have + amendments, and the incrementally make it better, while people can start testing and breaking it with git-master-on-windows snapshots"? (22.20.44) mattock: sounds good to me (22.20.59) mattock: as long the code is in snapshots and git master only, there's not that much of a risk (22.23.26) mattock: also, the "no need for admin privs on Windows" is probably good enough motivator for people to actually use the snapshots (22.23.35) mattock: usually they would not bother (22.25.44) cron2: d12fk: does that work for you? (22.31.16) mattock: did sleep deprivation due to superbowl take him down? (22.32.32) cron2: seems so :-) - let's tell him good night and continue another week (I would guess "kid woke up, dad has to go and cuddle" :) ) (22.32.39) mattock: yep (22.33.04) mattock: did everyone else drop off already? (22.33.18) lev__: how abt the mtu thing (22.33.20) cron2: jamesyonan: you still around? The next one is really for you :) (22.33.23) mattock: ah hi lev! (22.33.33) cron2: lev__: exactly this (22.33.58) jamesyonan: yes (22.34.19) cron2: ok, let me summarize the issue quickly (22.34.33) cron2: peer-id -> TPACKET_V2 increases packet overhead by 3 bytes (22.34.42) cron2: (we all knew this) (22.35.36) cron2: we overlooked to change the MTU / overhead calculation inside OpenVPN to take this into account, aka "make the buffers big enough". Syzzer discovered that uncompressed/non-compressible full-1500 byte packets would get dropped, sent a patch, I merged it. Patch here: (22.35.40) cron2: http://thread.gmane.org/gmane.network.openvpn.devel/9418 (22.35.42) vpnHelper: Title: Gmane Loom (at thread.gmane.org) (22.36.05) cron2: this is all logical and straight forward, but now something came up that we need to decide on a strategy (22.36.52) cron2: we have --tun-mtu and --link-mtu inside OpenVPN. --link-mtu can be set via command line, or, if not, is calculated as "--tun-mtu + (option-dependent) overhead" (like, compression adds +1 for the compression opcode) (22.37.28) cron2: with syzzers patch, calculated --link-mtu goes up by +3 (to be expected), but now when talking to a server with --opt-verify, OCC check blows up (22.37.44) d12fk: nope it's mother in law with windows problem =) (22.37.50) d12fk: but yeah i'm ok (22.38.42) jamesyonan: but wouldn't server need to do the same adjustment on its side so the link-mtu would be consistent? (22.38.43) cron2: because one side sends --link-mtu 1542 and the other side has --link-mtu 1545 (22.39.06) cron2: yeah... *if* you talk "peer-id capable server to peer-id capable client" (22.39.21) cron2: but if you have an old client -> new server, or new client -> old server --> boom (22.39.54) cron2: when testing old<->new compatibility, we didn't have the mtu adjustment patch, so this didn't show up (22.40.12) cron2: lev__: what did you do to your server? It's not mismatching anymore (22.40.18) jamesyonan: I see what you're saying -- opt-verify is acting too soon, it needs to wait until after options push before the parameters that affect MTU are finalized (22.40.46) lev__: cron2: let me check (22.41.22) cron2: jamesyonan: this is one way to phrase it - right now, we have always increased the MTU by +3, not depending on peer-id (22.41.57) cron2: so that would be one option to tackle it - when going to --opt-verify, use the TPACKET_V1 calculation, and adjust link-mtu +=3 if we receive a pushed option peer-id (22.42.47) jamesyonan: actually in ovpn3 I had to disable opt-verify because ovpn3 does a lot more intensive negotiation (such as negotiatiing crypto mode) and some of these negotiations alter the MTU parameters (22.43.26) jamesyonan: I think to fix these issues, opt-verify needs to be delayed (22.43.32) lev__: cron2: I have an old version now without mtu patch (22.43.55) cron2: that would work for *this* corner case (someone needs to code it and I have the suspicion that it's me), but what if a server pushes --link-mtu... oh, wait, that would actually work if we calculate this based on peer-id option set-or-not (so a new server would know what to expect, and an old server wouldn't see a difference) (22.45.47) jamesyonan: but opt-verify happens early, before the server has decided which options to push (22.45.50) cron2: jamesyonan: I don't think we can easily do this, that is, delay OCC until after push/push-reply. There is a trac bug about some OpenVPN service providers that parse the opt-verify options to *build* the push-reply options in a way that they do option negotion... (22.46.18) cron2: that's it, today some servers depend on the order of things - so turning it around might cause larger incompatibilities (22.47.26) jamesyonan: what if we have the server treat the OCC string the same way it treats the IV_ parameters, i.e. as a declaration of client capabilities (22.48.07) jamesyonan: not something that needs to strictly match between peers (22.49.16) lev__: would it make sense to send V2 packets only if they fit in MTU and V1 otherwise? (22.49.56) jamesyonan: but a lot of packets wouldn't fit in MTU if even one byte was added (22.50.32) cron2: jamesyonan: using it for negotiation seems to be what these people do, but it would break existing installations (22.51.39) cron2: as for sending TPACKET_V2 - there's always ACKs and such which are small, so that could be done, but it would make it harder to decide "no more v1 packet support" one day (22.52.43) jamesyonan: well I think that going forward, opt-verify and OCC need to evolve to deal with a higher level of negotiation. The question is how to structure a graceful upgrade path for this. (22.53.04) cron2: yeah, that's basically the options I see - "revert syzzer's +3 patch, stick to the old link-mtu and if the packet does not fit, set V1", and "use the old-calculation link-mtu for OCC, and adjust to +3 later if peer-id is pushed" (22.53.15) cron2: (and rework the whole thing long time :)) (22.53.29) cron2: I'm mostly worried about short time compatibility right now, without breaking long-term evolution (22.57.30) jamesyonan: what if we deprecate OCC but provide compatibility by generating it based on default options, i.e. don't modify the OCC string based on MTU changes caused by negotiation such as proto V2? (22.58.42) cron2: yes, this is what I had in mind - calculate "occ-transmitted" link-mtu based on default options, adjust later, use that for actual packet handling (22.59.03) jamesyonan: right (22.59.11) cron2: how do you do OCC in OpenVPN 3? "just do not send it at all"? (22.59.53) jamesyonan: I do send it, but I generate it using default options, before any negotiation occurs. (23.00.35) cron2: good, that sounds like a workable way forward. I'll do a patch for 2.x and test with lev__. (23.01.31) cron2: one more question on this: if a .ovpn config has a --link-mtu setting in it, and both client and server get upgraded to peer-id capable versions, still using the same .ovpn, all of a sudden that would reduce the effective tun-mtu by -3 (hard upper bound, 3 bytes extra overhead) (23.02.21) cron2: question 1: how common is explicitely setting --link-mtu? q2: how should we deal with it? Just silently bump up --link-mtu, or put a warning ("peer-id has been pushed, you have a static --link-mtu in your config, except mtu issues! we told you so!") (23.03.12) jamesyonan: can we not incur the 3 byte overhead of proto V2 if both client and server don't use it? (23.03.58) cron2: the current implementation is "if the client can do it, always push peer-id <x> and turn it on", so there is no "optional" here (23.04.45) lev__: so clients starting from 2.3.6 always push IV_PROTO 2 (23.05.06) jamesyonan: I sort of handle the 3-byte bloat issue with ovpn3 by only supporting proto V2 in conjunction with a bundle of other protocol enhancements that actually reduce the protocol overhead in aggregate (23.05.51) cron2: how does that work? (23.06.26) jamesyonan: ovpn3 basically supports proto v1 or v2 by negotiation (23.07.18) jamesyonan: proto v2 in ovpn3 adds 4 byte opcode, AES-GCM, and compression V2 (23.08.21) jamesyonan: but AEAD mode (i.e. AES-GCM) saves so much overhead, that even adding the 3 byte overhead still results in a net reduction (23.09.06) cron2: ah, so even if you get pushed "peer-id <x>", it won't enable v2 packets unless you also have AEAD mode (23.10.04) jamesyonan: sort of -- the server won't push peer-id <x> unless the client supports all of the V2 protocol enhancements (23.10.08) cron2: we can't do that for 2.3.x (won't receive AEAD too intrusive), but we could for 2.4... (23.10.40) cron2: now that would need more work on the 2.x server side, which is very simple right now (IV_PROTO=2 -> push peer-id) (23.11.10) cron2: and actually poses strategic questions for "how much of peer-id do we want (and can we get) into 2.3" (23.11.29) segoon ha abbandonato la stanza (quit: Quit: WeeChat 0.3.7). (23.12.45) cron2: (and it implies that having an iOS v3 client talk to a 2.4-to-be server it would find itself in the same situation - maybe no AEAD, but peer-id being pushed) (23.13.17) jamesyonan: I know we discussed this in Munich, but I've come around to the opinion that it's better to have an all-or-nothing approach with respect to protocol extensions (23.14.21) jamesyonan: rather than independently negotiate each extension (23.15.29) cron2: I can see that argument. My counterargument is that I want to see a minimal peer-id implementation (client-side only) in 2.3.x, as that will be around for a very long time and should really have it - but we won't the bigger things like AEAD or crypto negotiation in there, as that's too intrusive (23.16.02) cron2: the client-side peer-id stuff is just a few lines of code (send IV_PROTO=2, understand peer-id option, accept and generate TPACKET_v2 packets) (23.17.20) jamesyonan: agreed that AEAD or crypto negotiation shouldn't be in 2.3 (23.18.18) lev__: looks like a "small" mtu issue has grown a quite big (23.18.22) mattock: :) (23.18.29) mattock: in fact so big that I have to hit the sack (23.19.28) cron2: hehe, yes. I have some bits to code, and some thinking to do, on 2.3, 2.4, v3, interoperability, and negotiation - and we'll need to work on all the trains it seems (and come to a common understanding here) (23.19.50) mattock: so no resolution today? (23.19.57) cron2: jamesyonan: thanks, this helped, but we need to do a few more rounds on this :-) (23.20.15) mattock: I'll make a note of this in the summary (23.20.30) cron2: mattock: resolution is: for now, delay the link-mtu calculcation until after options have received (fixes the issue at hand, should not bring up new surprises) (23.20.34) mattock: a quick one: shall we remove escape dashes from the man-page? (23.20.52) ***cron2 has no opinion on this, didn't understand the issue in the first place (23.21.57) mattock: the issue was that searching the man-page failed in the past on some operating systems if, say, --win-sys was _not_ escaped like \-\-win\-sys (23.22.12) mattock: searching for that particular string failed, that is (23.22.39) mattock: I tried to reproduce the issue on Debian 7, CentOS 6 and Ubuntu 14.04 and the dashes had no effect whatsoever (23.23.18) mattock: according to anecdotal evidence the dashes were required at some point, and were added by other projects, too, but then something (groff?) was fixed and the dashes became unnecessary (23.23.35) mattock: and now we have partially escaped man-page (23.23.46) mattock: inconsistently (23.25.09) mattock: current man-page works just fine afaict, but it looks a bit silly (23.25.26) mattock: anyways, I really need to split, and this is not a deal-breaker :)
signature.asc
Description: OpenPGP digital signature
