Hi, Thank you for your reviewing.
On 2017/12/20 21:08, Christos Zoulas wrote: > In article <75925357-8e16-0f0f-b7a0-78155c865...@iij.ad.jp>, > Kengo NAKAHARA <k-nakah...@iij.ad.jp> wrote: >> Hi, >> >> On 2017/12/19 2:54, Christos Zoulas wrote: >>> In article <02c36311-2fcd-08cf-cc71-b472e7c01...@iij.ad.jp>, >>> Kengo NAKAHARA <k-nakah...@iij.ad.jp> 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-nakah...@iij.ad.jp>
