[strongSwan-dev] [PATCH] charon: per connection replay window configuration

[Christophe Gouault]

Hi Martin,

On 05/22/2014 12:25 PM, Martin Willi wrote:
> Christophe,
>
>> Make replay window configurable per connection.
>
> Thanks for your patch, I finally could take a look at it. Some notes:
>
>> +++ b/src/libcharon/config/child_cfg.c
>> +		.replay_window = -1,
>
> I think it would make sense to initialize the default value directly to
> ->get_int("charon.replay_window"), and remove that get_int() from the
> kernel interface. Any kernel backend could make use of it, and...
>
>> +++ b/src/libcharon/plugins/stroke/stroke_config.c
>> +	child_cfg->set_replay_window(child_cfg, msg->add_conn.replay_window);
>
> ... you could have that -1 magic value check just specific to stroke,
> and not across the whole code base. Just omit set_replay_window() in
> that case to use the default.

I agree this makes sense and sounds better.

>> +++ b/src/libhydra/kernel/kernel_ipsec.h
>> @@ -115,7 +116,8 @@ struct kernel_ipsec_t {
>> -						ipsec_mode_t mode, u_int16_t ipcomp, u_int16_t cpi,
>> +						ipsec_mode_t mode, u_int16_t ipcomp,
>> +						u_int32_t replay_window, u_int16_t cpi,
>
> There are some additional implementations of kernel_ipsec_t that need to
> get updated to the new method signature, namely:
>
>> $ git grep -l "^[[:space:]]*kernel_ipsec_t \w*;" -- '*.h'
>> src/charon-tkm/src/tkm/tkm_kernel_ipsec.h
>> src/frontends/android/jni/libandroidbridge/kernel/android_ipsec.h
>> src/libcharon/plugins/kernel_libipsec/kernel_libipsec_ipsec.h
>> src/libcharon/plugins/load_tester/load_tester_ipsec.h
>> src/libhydra/plugins/kernel_klips/kernel_klips_ipsec.h
>> src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.h
>> src/libhydra/plugins/kernel_pfkey/kernel_pfkey_ipsec.h

Right! Sorry, we missed these ones.

>> +++ b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
>> +	if (replay_window == UINT32_C(-1) ||
>> +		replay_window == this->replay_window)
>> +	{
>> +		/* default replay_window parameters */
>> +		replay_window = this->replay_window;
>> +		replay_bmp = this->replay_bmp;
>> +	}
>> +	else
>> +	{
>> +		/* sa-specific replay_window parameters */
>> +		replay_bmp = replay_bmp_len(replay_window);
>> +	}
>
> this->replay_* gets obsolete, and we'd just need a single code path
> here.

Agreed.

> All other changes look fine.

Great :) We'll provide a v2 including your remarks.

Best regards,
Christophe

> Best regards
> Martin
>