Re: RFC: ipsec(4) pseudo interface

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for your reviewing.

On 2017/12/20 21:08, Christos Zoulas wrote:
> In article <75925357-8e16-0f0f-b7a0-78155c8657c1%iij.ad.jp@localhost>,
> Kengo NAKAHARA  <k-nakahara%iij.ad.jp@localhost> wrote:
>> Hi,
>>
>> On 2017/12/19 2:54, Christos Zoulas wrote:
>>> In article <02c36311-2fcd-08cf-cc71-b472e7c01be9%iij.ad.jp@localhost>,
>>> Kengo NAKAHARA  <k-nakahara%iij.ad.jp@localhost> wrote:
>>>> Hi,
>>>>
>>>> We implement ipsec(4) pseudo interface for route-based VPNs. This pseudo
>>>> interface manages its security policy(SP) by itself, in particular, we do
>>>>    # ifconfig ipsec0 tunnel 10.0.0.1 10.0.0.2
>>>> the SPs "10.0.0.1 -> 10.0.0.2"(out) and "10.0.0.2 -> 10.0.0.1"(in) are
>>>> generated automatically and atomically. And then, when we do
>>>>    # ifconfig ipsec0 deletetunnel
>>>> the SPs are destroyed automatically and atomically, too.
>>>>
>>>> Here is the patches and an unified patch.
>>>>    http://netbsd.org/~knakahara/if_ipsec/if_ipsec.tgz
>>>>    http://netbsd.org/~knakahara/if_ipsec/if_ipsec-unified.patch
>>>>
>>>> By the way, I have one question. In the above patch(s), I temporarily add
>>>> manual for ipsecX pseudo interface as if_ipsec.4, because there is already
>>>> ipsec.4 for general ipsec protocol. How should I add the man of ipsec(4)
>>>> pseudo interface?
>>>>    (a) Add if_ipsec.4
>>>>    (b) move current ipsec.4(for ipsec protocol) to ipsec.9, and then
>>>>        add ipsec.4(for ipsec pseudo interface)
>>>>    (c) any other
>>>>
>>>> Could you comment the patch or the question?
>>>
>>> I've wanted this feature for a long time! Looks ok to me, but the
>>> sockaddr_copy()/port setting code, should be abstracted to a single
>>> function since it is repeated in ioctl().
>>
>> Thank you for your reviewing. I fix it in the following patch.
>>    - patch series
>>      - https://netbsd.org/~knakahara/if_ipsec/if_ipsec2.tgz
>>    - unified patch
>>      - https://netbsd.org/~knakahara/if_ipsec/if_ipsec2-unified.patch
> 
> Thanks:
> 
> +	error = var->iv_output(var, family, m);
> +	if (!error) {
> 
> - It is simpler to do (early returns):
> 	if (error)
> 		return error;
> 	...
> 	return 0;

I apply it.

> - What's the point of 'goto bad' in ioctl if there is no cleanup to be done?

Oh, indeed. I refactor cleanup processing.

> - There is one more place you could use the addr_port function?
>   [or perhaps abstract it in in_port_t *get_port(struct sockaddr *)?

Sorry, I missed it.

> - We should use in_port_t instead of unsigned short for ports.

Ah, the original code is quite old, I missed updating it.

> - I am not clear how the code interracts with ipsec endpoints created
>   from /etc/ipsec.conf and ones created via ifconfig? I.e. if I create
>   endpoints in /etc/ipsec.conf, does the cloner interface get automatically
>   constructed?

In a nutshell, it is first-come basis. If the SPs required by ipsec(4)
interface are already added by /etc/ipsec.conf or manually setkey(8),
'ifconfig ipsec0 tunnel "src" "dst"' fails, and vice versa.
Sorry, there was no information about that and SPs required by ipsec(4)
interface. I add the information to man.
Furthermore, I find a bug in handling that error case. Thank you for
your pointing out!

Here is the update patch series and unified patch.
    https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec3.tgz
    https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec3-unified.patch


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>