Re: RFC: ipsec(4) pseudo interface

[christos%astron.com@localhost (Christos Zoulas)]

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;

- What's the point of 'goto bad' in ioctl if there is no cleanup to be done?
- There is one more place you could use the addr_port function?
  [or perhaps abstract it in in_port_t *get_port(struct sockaddr *)?
- We should use in_port_t instead of unsigned short for ports.
- 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?

Thanks,

christos