Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

Hi Andrej,

On Thu, May 13, 2021 at 11:52:32AM +0200, Andrej Shadura wrote:
> On 12/05/2021 19:27, Ariel D'Alessandro wrote:
> > Implement WPA3-SAE authentication on connman side.
> > 
> > Based on Tizen project:
> > 
> >   https://review.tizen.org/git/?p=platform/upstream/connman.git
> > 
> > Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> 
> Based on my understanding how this should work, this looks reasonable to me.

Same here, this looks pretty straightforward. Thanks for picking this
up.

The patch should be spitted into one which adds the SAE to the Security
property of the Service API (please also update the documentation). The
second patch adds the plugins/wifi.c implementation.

I am not really into the WiFi topic anymore, so I am adding the iwd
mailing list. Maybe we get some input for the new SAE Security
property, e.g. how this could work with iwd.

Thanks,
Daniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 139 bytes --]

Hi Ariel,

I just realize this patch was not send to the connman (iwd) mailing
list. Could you post it there as well?

Thanks,
Daniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Ariel D'Alessandro]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

Hi Daniel,

On 5/13/21 7:56 AM, Daniel Wagner wrote:
> Hi Ariel,
> 
> I just realize this patch was not send to the connman (iwd) mailing
> list. Could you post it there as well?

You mean iwd(a)lists.01.org?
I sent it to connman(a)lists.01.org , but I'm not sure it got there
because my subscription request is still pending.


Thanks,
Ariel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

Hi Daniel,

>>> Implement WPA3-SAE authentication on connman side.
>>> 
>>> Based on Tizen project:
>>> 
>>>  https://review.tizen.org/git/?p=platform/upstream/connman.git
>>> 
>>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>> 
>> Based on my understanding how this should work, this looks reasonable to me.
> 
> Same here, this looks pretty straightforward. Thanks for picking this
> up.
> 
> The patch should be spitted into one which adds the SAE to the Security
> property of the Service API (please also update the documentation). The
> second patch adds the plugins/wifi.c implementation.
> 
> I am not really into the WiFi topic anymore, so I am adding the iwd
> mailing list. Maybe we get some input for the new SAE Security
> property, e.g. how this could work with iwd.

don’t change the Service API. Even WPA3 Personal is just PSK and so there is no need to add another security property entry. Actually if you are using iwd, then the detail between WPA2 and WPA3 is never exposed to ConnMan.

Frankly, if you want good WiFi support, ditch wpa_supplicant and run iwd.

Regards

Marcel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Hi Marcel,

On Thu, May 13, 2021 at 09:01:05PM +0200, Marcel Holtmann wrote:
> > I am not really into the WiFi topic anymore, so I am adding the iwd
> > mailing list. Maybe we get some input for the new SAE Security
> > property, e.g. how this could work with iwd.
>
> don’t change the Service API. Even WPA3 Personal is just PSK and so there is
> no need to add another security property entry.

I somehow expected PSK also covers WPA3. This means any code for
enabling WPA3 for wpa_supplicant should go solely into plugins/wifi.c.
Thanks for clarifying this point.

> Actually if you are using iwd, then the detail between WPA2 and WPA3
> is never exposed to ConnMan.

Nice! I didn't know that iwd already supports WPA3.

Thanks,
Daniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Ariel D'Alessandro]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

Hi Daniel, Marcel,

On 5/14/21 3:47 AM, Daniel Wagner wrote:
> Hi Marcel,
> 
> On Thu, May 13, 2021 at 09:01:05PM +0200, Marcel Holtmann wrote:
>>> I am not really into the WiFi topic anymore, so I am adding the iwd
>>> mailing list. Maybe we get some input for the new SAE Security
>>> property, e.g. how this could work with iwd.
>>
>> don’t change the Service API. Even WPA3 Personal is just PSK and so there is
>> no need to add another security property entry.
> 
> I somehow expected PSK also covers WPA3. This means any code for
> enabling WPA3 for wpa_supplicant should go solely into plugins/wifi.c.
> Thanks for clarifying this point.

Thanks for discussing the implementations details.

So AFAIU from the above, the patchset is implementing wpa_supplicant
WPA3-SAE support in the right place: wifi plugin. Any change required? I
understand that documentation should be updated probably, anything else?

> 
>> Actually if you are using iwd, then the detail between WPA2 and WPA3
>> is never exposed to ConnMan.
> 
> Nice! I didn't know that iwd already supports WPA3.

I'm planning to extend the support to iwd in a follow-up patchset. I'd
like to get this approved/merged first.

Thanks,
Ariel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

Hi Ariel,

>>>> I am not really into the WiFi topic anymore, so I am adding the iwd
>>>> mailing list. Maybe we get some input for the new SAE Security
>>>> property, e.g. how this could work with iwd.
>>> 
>>> don’t change the Service API. Even WPA3 Personal is just PSK and so there is
>>> no need to add another security property entry.
>> 
>> I somehow expected PSK also covers WPA3. This means any code for
>> enabling WPA3 for wpa_supplicant should go solely into plugins/wifi.c.
>> Thanks for clarifying this point.
> 
> Thanks for discussing the implementations details.
> 
> So AFAIU from the above, the patchset is implementing wpa_supplicant
> WPA3-SAE support in the right place: wifi plugin. Any change required? I
> understand that documentation should be updated probably, anything else?

if you need to update the Service API documentation, then you are doing something wrong.

>>> Actually if you are using iwd, then the detail between WPA2 and WPA3
>>> is never exposed to ConnMan.
>> 
>> Nice! I didn't know that iwd already supports WPA3.
> 
> I'm planning to extend the support to iwd in a follow-up patchset. I'd
> like to get this approved/merged first.

No idea what would be needed for iwd support. It already does support WPA3. With iwd, no changes to ConnMan are needed for WPA3 support.

Regards

Marcel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

Hi,

On Wed, May 19, 2021 at 08:58:01AM +0200, Marcel Holtmann wrote:
> > So AFAIU from the above, the patchset is implementing wpa_supplicant
> > WPA3-SAE support in the right place: wifi plugin. Any change required? I
> > understand that documentation should be updated probably, anything else?
> 
> if you need to update the Service API documentation, then you are
> doing something wrong.

The only thing you need to do is to extend the wifi.c plugin to set to
SAE if it's available when PSK is selected via the D-Bus API. As Marcel
pointed out this should not be exposed to the user.

Thanks,
DAniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Ariel D'Alessandro]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

Hi Daniel,

On 5/19/21 4:32 AM, Daniel Wagner wrote:
> Hi,
> 
> On Wed, May 19, 2021 at 08:58:01AM +0200, Marcel Holtmann wrote:
>>> So AFAIU from the above, the patchset is implementing wpa_supplicant
>>> WPA3-SAE support in the right place: wifi plugin. Any change required? I
>>> understand that documentation should be updated probably, anything else?
>>
>> if you need to update the Service API documentation, then you are
>> doing something wrong.
> 
> The only thing you need to do is to extend the wifi.c plugin to set to
> SAE if it's available when PSK is selected via the D-Bus API. As Marcel
> pointed out this should not be exposed to the user.

Sorry, I'm not sure how to address that. I might be missing something
here, so quick questions:

* Which D-Bus API call would set the service security to PSK even if
it's registered as SAE capable?
* Why is this a requirement and why should that go in plugin/wifi.c?

Thanks a lot for your help,
Ariel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

Hi Ariel,

On Tue, May 25, 2021 at 06:53:44PM -0300, Ariel D'Alessandro wrote:
> On 5/19/21 4:32 AM, Daniel Wagner wrote:
> > The only thing you need to do is to extend the wifi.c plugin to set to
> > SAE if it's available when PSK is selected via the D-Bus API. As Marcel
> > pointed out this should not be exposed to the user.
> 
> Sorry, I'm not sure how to address that. I might be missing something
> here, so quick questions:
> 
> * Which D-Bus API call would set the service security to PSK even if
> it's registered as SAE capable?

This can be done via the Service API property Security.

> * Why is this a requirement and why should that go in plugin/wifi.c?

There is no need to expose implementation details to the user API. SAE
belongs to the family of PSK, hence if the user selects PSK via the
Service API, the implementation should choose the right protocol. Since
wpa_supplicant does not do this on it's own the plugin has to do this.

If you compare this to the iwd setup, the plugins/iwd.c does not even
know that SAE is used, it doesn't care. iwd does the right thing.

Basically this means, if SAE is available just use it, there is no need
to bother the user.
x
Hope this helps.

Thanks,
Daniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Ariel D'Alessandro]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

Hi Daniel,

On 5/26/21 4:05 AM, Daniel Wagner wrote:
> Hi Ariel,
> 
> On Tue, May 25, 2021 at 06:53:44PM -0300, Ariel D'Alessandro wrote:
>> On 5/19/21 4:32 AM, Daniel Wagner wrote:
[snip]
> There is no need to expose implementation details to the user API. SAE
> belongs to the family of PSK, hence if the user selects PSK via the
> Service API, the implementation should choose the right protocol. Since
> wpa_supplicant does not do this on it's own the plugin has to do this.
> 
> If you compare this to the iwd setup, the plugins/iwd.c does not even
> know that SAE is used, it doesn't care. iwd does the right thing.
> 
> Basically this means, if SAE is available just use it, there is no need
> to bother the user.

Got it, thanks for the explanation.

So, correct me if I'm wrong:

* Changes to src/service.c should be dropped. No need to add
CONNMAN_SERVICE_SECURITY_SAE as we'll be treating WPA3-SAE as
CONNMAN_SERVICE_SECURITY_PSK.

* A WPA3-SAE service can be named as *_psk. No need to name it as *_sae.

Looking at Tizen's implementation:

* In order to check if SAE is available, key_mgmt capabilities should be
stored in GSupplicantSSID as done in [0]. This way d-bus field key_mgmt
can be set accordingly, similar to what's done in [1].

Note that Tizen is also storing this inside struct connman_network [2].
Let me know if this is allowed or should be addressed in a different way.

Thanks a lot,
Ariel

[0]
https://review.tizen.org/git/?p=platform/upstream/connman.git;a=blob;f=gsupplicant/gsupplicant.h;h=05af5de128544b1c1e36e7cfce5ae024c217d6ee;hb=HEAD#l228

[1]
https://review.tizen.org/git/?p=platform/upstream/connman.git;a=blob;f=gsupplicant/supplicant.c;h=c727c074373d67ddc238634bd4399be204e2aed4;hb=HEAD#l7199

[2]
https://review.tizen.org/git/?p=platform/upstream/connman.git;a=blob;f=src/network.c;h=870749f0182911a4e7221e0fd531fd654398bf7d;hb=HEAD#l122

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

Hi Ariel,

On Wed, May 26, 2021 at 06:21:37PM -0300, Ariel D'Alessandro wrote:
> So, correct me if I'm wrong:
> 
> * Changes to src/service.c should be dropped. No need to add
> CONNMAN_SERVICE_SECURITY_SAE as we'll be treating WPA3-SAE as
> CONNMAN_SERVICE_SECURITY_PSK.

Yes

> * A WPA3-SAE service can be named as *_psk. No need to name it as
> *_sae.

Yes

> Looking at Tizen's implementation:
> 
> * In order to check if SAE is available, key_mgmt capabilities should be
> stored in GSupplicantSSID as done in [0]. This way d-bus field key_mgmt
> can be set accordingly, similar to what's done in [1].

Yes, that looks about right.

> Note that Tizen is also storing this inside struct connman_network [2].
> Let me know if this is allowed or should be addressed in a different way.

I think key_mgmt could go into 'struct wifi_data'. No need to store this
information a layer higher up.

Thanks,
Daniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Ariel D'Alessandro]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

Daniel,

On 5/27/21 3:56 AM, Daniel Wagner wrote:
> Hi Ariel,
> 
> On Wed, May 26, 2021 at 06:21:37PM -0300, Ariel D'Alessandro wrote:
[...]>> Looking at Tizen's implementation:
>>
>> * In order to check if SAE is available, key_mgmt capabilities should be
>> stored in GSupplicantSSID as done in [0]. This way d-bus field key_mgmt
>> can be set accordingly, similar to what's done in [1].
> 
> Yes, that looks about right.
> 
>> Note that Tizen is also storing this inside struct connman_network [2].
>> Let me know if this is allowed or should be addressed in a different way.
> 
> I think key_mgmt could go into 'struct wifi_data'. No need to store this
> information a layer higher up.

The issue with 'struct wifi_data' is that it's attached to the device
instead of each network connection, see [0]. So, we can't store the
'key_mgmt' property there as it's a shared object.

For example, iwd plugin has two different private data structs:
iwd_device [1] and iwd_network [2].

We could have something similar in plugin/wifi.c. I'll send another RFC
patchset v2, so we can review it in-place.

Thanks,
Ariel

[0]
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/plugins/wifi.c#n795
[1]
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/plugins/iwd.c#n75
[2]
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/plugins/iwd.c#n87

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Daniel Wagner]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

Hi,

On Thu, May 27, 2021 at 05:28:25PM -0300, Ariel D'Alessandro wrote:
> > I think key_mgmt could go into 'struct wifi_data'. No need to store this
> > information a layer higher up.
>
> The issue with 'struct wifi_data' is that it's attached to the device
> instead of each network connection, see [0]. So, we can't store the
> 'key_mgmt' property there as it's a shared object.
> 
> For example, iwd plugin has two different private data structs:
> iwd_device [1] and iwd_network [2].
> 
> We could have something similar in plugin/wifi.c. I'll send another RFC
> patchset v2, so we can review it in-place.

Yes, attach the key_mgmt to the network object via

  connman_network_{set|get}_data()

Thanks,
Daniel

Re: [RFC connman 1/1] WIP: Add WPA3-SAE support

[Ariel D'Alessandro]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

Hi Daniel,

On 5/28/21 2:20 AM, Daniel Wagner wrote:
> Hi,
> 
> On Thu, May 27, 2021 at 05:28:25PM -0300, Ariel D'Alessandro wrote:
>>> I think key_mgmt could go into 'struct wifi_data'. No need to store this
>>> information a layer higher up.
>>
>> The issue with 'struct wifi_data' is that it's attached to the device
>> instead of each network connection, see [0]. So, we can't store the
>> 'key_mgmt' property there as it's a shared object.
>>
>> For example, iwd plugin has two different private data structs:
>> iwd_device [1] and iwd_network [2].
>>
>> We could have something similar in plugin/wifi.c. I'll send another RFC
>> patchset v2, so we can review it in-place.
> 
> Yes, attach the key_mgmt to the network object via
> 
>   connman_network_{set|get}_data()

Indeed, that's what I submitted yesterday in RFC patchset v2.
See
https://lists.01.org/hyperkitty/list/iwd(a)lists.01.org/thread/CQRZSD3TXLILEDRGPNOGW4DIFPOAGLMK/