Re: RFC: ipsec(4) pseudo interface
Hi, On 2017/12/26 17:31, Kengo NAKAHARA wrote: > Hi, > > On 2017/12/25 23:14, Christos Zoulas wrote: >> On Dec 25, 4:54pm, k-nakah...@iij.ad.jp (Kengo NAKAHARA) wrote: >> -- Subject: Re: RFC: ipsec(4) pseudo interface >> >> | Here is the updated patch series and unified patch. >> | - https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4.tgz >> | - https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4-unified.patch >> >> Thanks, LGTM! > > Thank you very much for your many helpful pointing out! > > Does anyone else have any comments? It seems net/ipsec ATFs are stable now, so I commit it. Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
Hi, On 2017/12/25 23:14, Christos Zoulas wrote: > On Dec 25, 4:54pm, k-nakah...@iij.ad.jp (Kengo NAKAHARA) wrote: > -- Subject: Re: RFC: ipsec(4) pseudo interface > > | Here is the updated patch series and unified patch. > | - https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4.tgz > | - https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4-unified.patch > > Thanks, LGTM! Thank you very much for your many helpful pointing out! Does anyone else have any comments? Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
On Dec 25, 4:54pm, k-nakah...@iij.ad.jp (Kengo NAKAHARA) wrote: -- Subject: Re: RFC: ipsec(4) pseudo interface | Here is the updated patch series and unified patch. | - https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4.tgz | - https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4-unified.patch Thanks, LGTM! christos
Re: RFC: ipsec(4) pseudo interface
Hi,
On 2017/12/23 9:05, Christos Zoulas wrote:
> In article ,
> Kengo NAKAHARA wrote:
>> Hi,
>>
>> Thank you for your reviewing.
>
>
> Thanks for fixing; more nit-picking:
> 1. there is a variable called err instead of error why (all the rest
>are called error)?
Oh, good catch. I fix it(ipsecif4_fragout()).
> 2. I prefer fewer lines of code, fewer variables for all the copies
>of those similar functions, like:
>
> +static int
> +if_ipsec_encap_detach(struct ipsec_variant *var)
> +{
> +
> + KASSERT(var != NULL);
> + KASSERT(if_ipsec_variant_is_configured(var));
> +
> + switch (var->iv_psrc->sa_family) {
> +#ifdef INET
> + case AF_INET:
> + return ipsecif4_detach(var);
> +#endif /* INET */
> +#ifdef INET6
> + case AF_INET6:
> + return ipsecif6_detach(var);
> + break;
> +#endif /* INET6 */
> + default:
> + return EINVAL;
> + }
> +}
Exactly. I fix the following duplications.
+ if_ipsec_encap_attach() and if_ipsec_encap_detach()
+ if_ipsec_gather_addr_port() and if_ipsec_set_addr_port()
- remove if_ipsec_gather_addr_port()
+ sa_len checking in if_ipsec_ioctl()
- functionize it as if_ipsec_check_salen()
Here is the updated patch series and unified patch.
- https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4.tgz
- https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4-unified.patch
Thanks,
--
//
Internet Initiative Japan Inc.
Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit
Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
In article ,
Kengo NAKAHARA wrote:
>Hi,
>
>Thank you for your reviewing.
Thanks for fixing; more nit-picking:
1. there is a variable called err instead of error why (all the rest
are called error)?
2. I prefer fewer lines of code, fewer variables for all the copies
of those similar functions, like:
+static int
+if_ipsec_encap_detach(struct ipsec_variant *var)
+{
+
+ KASSERT(var != NULL);
+ KASSERT(if_ipsec_variant_is_configured(var));
+
+ switch (var->iv_psrc->sa_family) {
+#ifdef INET
+ case AF_INET:
+ return ipsecif4_detach(var);
+#endif /* INET */
+#ifdef INET6
+ case AF_INET6:
+ return ipsecif6_detach(var);
+ break;
+#endif /* INET6 */
+ default:
+ return EINVAL;
+ }
+}
Best,
christos
Re: RFC: ipsec(4) pseudo interface
Hi, I'm sorry, I send mail while editing by mistake. On 2017/12/20 22:40, Thor Lancelot Simon wrote: > On Mon, Dec 18, 2017 at 06:49:44PM +0900, Kengo NAKAHARA 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. > > Do you have IKE daemon changes to use this? No, I don't. Because ipsec(4) interface send the same PF_KEY message as adding transport mode security policy manually. That is the behavior to use existing IKE daemon. Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
Hi, On 2017/12/20 22:40, Thor Lancelot Simon wrote: > On Mon, Dec 18, 2017 at 06:49:44PM +0900, Kengo NAKAHARA 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. > > Do you have IKE daemon changes to use this? No, I don't. -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
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 wrote:
>> Hi,
>>
>> On 2017/12/19 2:54, Christos Zoulas wrote:
>>> In article <02c36311-2fcd-08cf-cc71-b472e7c01...@iij.ad.jp>,
>>> Kengo NAKAHARA 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
Re: RFC: ipsec(4) pseudo interface
On Mon, Dec 18, 2017 at 06:49:44PM +0900, Kengo NAKAHARA 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. Do you have IKE daemon changes to use this? Thor
Re: RFC: ipsec(4) pseudo interface
In article <75925357-8e16-0f0f-b7a0-78155c865...@iij.ad.jp>,
Kengo NAKAHARA wrote:
>Hi,
>
>On 2017/12/19 2:54, Christos Zoulas wrote:
>> In article <02c36311-2fcd-08cf-cc71-b472e7c01...@iij.ad.jp>,
>> Kengo NAKAHARA 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
Re: RFC: ipsec(4) pseudo interface
Hi, On 2017/12/19 11:07, Christos Zoulas wrote: > In article <20171218184400.ga27...@britannica.bec.de>, > Joerg Sonnenberger wrote: >> On Mon, Dec 18, 2017 at 06:49:44PM +0900, Kengo NAKAHARA wrote: >>> (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 >> >> I'd call it either ifipsec(4) or ipsecif(4). Thank you for comments. I use ipsecif(4) as it is easy to sort in alphabetical with ipsec(4). :) > Traditionally the names of devices drivers and protocols go to section 4. > So we have a conflict here, use one of the names Joerg suggested and cross > reference it on the top of the ipsec.4 page: > > .Pp > This manual pages describes the IPSEC. > For the network device driver please see > .Xr ifipsec 4 . Thank you for your suggestion. I add the reference to ipsec(4). As a result, here is the updated patch. - patch series - https://netbsd.org/~knakahara/if_ipsec/if_ipsec2.tgz - in paticular, manual is added in 0004-add-ipsec-4-I-F-man-as-ipsecif.4.patch - unified patch - https://netbsd.org/~knakahara/if_ipsec/if_ipsec2-unified.patch # include christos@n.o's pointing out Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
Hi, On 2017/12/19 2:54, Christos Zoulas wrote: > In article <02c36311-2fcd-08cf-cc71-b472e7c01...@iij.ad.jp>, > Kengo NAKAHARA 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 # include changing man entry, I will describe about it in a later mail Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
Re: RFC: ipsec(4) pseudo interface
In article <20171218184400.ga27...@britannica.bec.de>, Joerg Sonnenberger wrote: >On Mon, Dec 18, 2017 at 06:49:44PM +0900, Kengo NAKAHARA wrote: >> (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 > >I'd call it either ifipsec(4) or ipsecif(4). Traditionally the names of devices drivers and protocols go to section 4. So we have a conflict here, use one of the names Joerg suggested and cross reference it on the top of the ipsec.4 page: .Pp This manual pages describes the IPSEC. For the network device driver please see .Xr ifipsec 4 . christos
Re: RFC: ipsec(4) pseudo interface
On Mon, Dec 18, 2017 at 06:49:44PM +0900, Kengo NAKAHARA wrote: > (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 I'd call it either ifipsec(4) or ipsecif(4). Joerg
Re: RFC: ipsec(4) pseudo interface
In article <02c36311-2fcd-08cf-cc71-b472e7c01...@iij.ad.jp>, Kengo NAKAHARA 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(). christos
RFC: ipsec(4) pseudo interface
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? Thanks, -- // Internet Initiative Japan Inc. Device Engineering Section, IoT Platform Development Department, Network Division, Technology Unit Kengo NAKAHARA
