On 18/12/16 17:40, Steffan Karger wrote: > Our internal options digest uses MD5 hashes to store the state, instead of > storing the full options string. There's nothing wrong with that, but it > would still be better to use SHA256 because: > * That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5) > * We don't have to explain anymore that MD5 is fine too > > The slightly less bytes for the digest (16 instead of 32) and operations > per connection setup are not worth sticking to MD5. > > Signed-off-by: Steffan Karger <stef...@karger.me> > --- > > This patch is meant for the master branch only.
I started looking at this patch .... and I think it needs some rework,
as it does a couple of unexpected things ...
[...snip...]
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index b6e12e1..a1733b0 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -677,23 +677,17 @@ process_incoming_push_request(struct context *c)
> #endif /* if P2MP_SERVER */
>
> static void
> -push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options
> *opt)
> +push_update_digest(md_ctx_t *ctx, struct buffer *buf)
> {
> char line[OPTION_PARM_SIZE];
> while (buf_parse(buf, ',', line, sizeof(line)))
> {
> /* peer-id might change on restart and this should not trigger
> reopening tun */
> - if (strprefix(line, "peer-id "))
> + if (strstr(line, "peer-id ") != line)
> {
> - continue;
> - }
> - /* tun reopen only needed if cipher change can change tun MTU */
> - if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
> - {
> - continue;
> + md_ctx_update(ctx, (const uint8_t *) line, strlen(line));
> }
> }
> - md_ctx_update(ctx, (const uint8_t *) line, strlen(line)+1);
> }
This actually tries to revert commit ec4dff3bbdcc9fedf7844 ... which is
quite surprising.
[...snip...]
> @@ -736,15 +730,14 @@ process_incoming_push_msg(struct context *c,
> option_types_found,
> c->c2.es))
> {
> - push_update_digest(&c->c2.pulled_options_state, &buf_orig,
> - &c->options);
> + push_update_digest(&c->c2.pulled_options_state, &buf_orig);
And this too is also a revert of the same commit as above.
Had it been just a simple rebase, I'd be willing to tackle that
on-the-fly. But as it tries to do some really unexpected reverts, there
might be a few other issues I'm not able to spot instantly.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
