Hi, On Mon, Jun 13, 2016 at 11:16:08PM +0200, Steffan Karger wrote: > Based on the 'IV_NCP=2' mechanism described in > http://permalink.gmane.org/gmane.network.openvpn.devel/9385. > > This is the first patch of a set that adds support for cipher negotiation. > Follow-up patches will add ways to restrict or disable the mechanism, and > add server-side support. > > v2: > * Account for crypto overhead through struct frame. This is less > transparant, but the code has been built to work this way. The > previous approach didn't work with TCP mode (or --port-share). > * Calculate the link-mtu sent in the options string based on the crypto > parameters specified in the config file (prevents link-mtu warnings in > older peers when connecting). > > v3: > * Use existing max_int() function, instead of new MAX() macro. > * Fix typo in comment. > * Do not regenerate keys if the server sends a second push msg. > * Only advertise IV_NCP=2 if we're a pull-client (and thus can do NCP).
There might be surprises lurking here regarding explicitly configured
--link-mtu, so this will need detailed testing in as many possible option
variants as possible.
"Logic wise" this looks reasonable to me, so I'm willing to give an ACK
to v4
"Memory wise", to address James' concerns, I do not see anything obvious
where this would lead to memory leaks, given that it's mostly just moving
code around that is run once before and afterwards. I think we're going
to waste a handful of bytes on the frame overhead (like, "100"-ish) and
resulting maximum buffer size, but that seems impossible to avoid without
much more intrusive changes.
One small thing I did not like, but that's "style":
> - /*
> - * key_id increments to KEY_ID_MASK then recycles back to 1.
> - * This way you know that if key_id is 0, it is the first key.
> - */
> ++session->key_id;
> session->key_id &= P_KEY_ID_MASK;
> if (!session->key_id)
I think we should leave this comment in - I saw you've added a more
detailed comment to the associated .h file, but poor folks like me,
without a decent "explain in a second window what the docs say about
the line of code I'm staring at" IDE might benefit from this comment...
> +int
> +tls_peer_info_ncp_ver(const char *peer_info)
> +{
> + const char *ncpstr = peer_info ? strstr (peer_info, "IV_NCP=") : NULL;
> + if (ncpstr)
> + {
> + int ncp = 0;
> + int r = sscanf(ncpstr, "IV_NCP=%d", &ncp);
> + if (r == 1)
> + return ncp;
> + }
> + return 0;
> +}
> +
This one should better go to 5/5 :-) - but you know that already.
thanks,
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025 g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
