Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiFi: Support for hidden networks and misc fixes. #874

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

iabdalkader
Copy link
Contributor

@iabdalkader iabdalkader commented Apr 22, 2024

Note that for hidden networks, the BSSID is Not available, since it was not found in a scan. However, it can still be retrieved with:

static_cast<WhdSTAInterface *>(wifi_if)->get_bssid(bssid);

I'm not sure why we're not using that, maybe for portability ?

Also not that uint8_t* BSSID(uint8_t* bssid); returns a reversed MAC (due to a legacy bug in Nina) however uint8_t* BSSID(uint8_t networkItem, uint8_t* bssid); does Not return a reversed MAC. I didn't change that.

Copy link
Collaborator

@sebromero sebromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, thanks Ibrahim. I suggest for the sake of portability und user friendliness to not expose non-arduino types to the user. Can we define our own enum for the security type?

@iabdalkader
Copy link
Contributor Author

iabdalkader commented Apr 22, 2024

I suggest for the sake of portability und user friendliness to not expose non-arduino types to the user. Can we define our own enum for the security type?

We seem to have our own enum wl_enc_type and I've reused it, that's why I added enum2sec. The enum returned/expected is ENC_TYPE_xxx.

@sebromero
Copy link
Collaborator

I suggest for the sake of portability und user friendliness to not expose non-arduino types to the user. Can we define our own enum for the security type?

We seem to have our own enum wl_enc_type and I've reused it, that's why I added enum2sec. The enum returned/expected is ENC_TYPE_xxx.

Hmm, okay. The enum doesn't look very Arduino-ish, but then that's a different problem 😅

@iabdalkader
Copy link
Contributor Author

Hmm, okay. The enum doesn't look very Arduino-ish, but then that's a different problem 😅

Yes I noticed. They seem to be documented/used here, see printEncryptionType

https://www.arduino.cc/reference/en/libraries/wifinina/wifi.scannetworks/

@JAndrassy
Copy link
Contributor

JAndrassy commented Apr 23, 2024

For WEP different connect parameters are required and the API should be like this https://www.arduino.cc/reference/en/libraries/wifi/wifi.begin/

I did this a few weeks ago, but then I didn't do a PR because:

  • WiFi.ancryptionType() returned AUTO if encryption type was specified at connect and not scanned
  • If I remember it right, AUTO encryption type at connect worked, so the encryption type parameter in begin was maybe not necessary
  • maybe the results I saw where caused by the fact that I don't have Portenta or Giga. To test Mbed WiFi I use my ESPHost_EMAC library with a Nano Connect, which may behave differently at low level.

@iabdalkader
Copy link
Contributor Author

iabdalkader commented Apr 23, 2024

For WEP different connect parameters are required and the API should be like this

I implemented the missing WiFi.begin(ssid, keyIndex, key); that accepts a key index. However, I don't see any connect function that accepts a key index, not in the mbed WiFiInterface class or in the underlying WHD STA interface, so the key index is not used. This lets you connect to WEP networks, the other option is we don't support WEP, I'm fine either way, but I don't see a reason why we must use the key index.

If I remember it right, AUTO encryption type at connect worked, so the encryption type parameter in begin was maybe not necessary

There's no AUTO encryption type, it's just an enum we've added, maybe it maps to something valid for ESP but it does not map to anything valid for WHD. Those are the valid security types in mbedos: https://os.mbed.com/docs/mbed-os/v6.16/mbed-os-api-doxy/group__netsocket.html#ga0bfcdf6a9abae30715b5e7f43ae4a0c5

And those are the actual security types they map to in the WHD driver (use on the Giga, Nicla, Portenta): https://github.com/ARMmbed/mbed-os/blob/7ae592dabe59e9f26a4ce56190af61fb8b46d10d/connectivity/drivers/emac/COMPONENT_WHD/interface/WhdSTAInterface.cpp#L141

I tried unknown, the comments say you shouldn't but I did anyway and it does not work. So the encryption type must be specified when connecting. By default WPA/WPA2 will be used, if the network was network was not found in a scan. This should work for most cases, but it still gives the user the option to override if it does not.

@JAndrassy
Copy link
Contributor

@iabdalkader
Copy link
Contributor Author

here is how WEP is handled in WHD: https://github.com/ARMmbed/mbed-os/blob/7ae592dabe59e9f26a4ce56190af61fb8b46d10d/connectivity/drivers/emac/COMPONENT_WHD/interface/WhdSTAInterface.cpp#L329

It seems to expect a very specific key format, I'll take a look later. Although I'm not sure if WEP is worth the trouble, it's as secure as having no security at all.

@iabdalkader
Copy link
Contributor Author

@JAndrassy I've formatted the WEP key in the same way that the driver expects, see: https://github.com/ARMmbed/mbed-os/blob/7ae592dabe59e9f26a4ce56190af61fb8b46d10d/connectivity/drivers/wifi/COMPONENT_WHD/wifi-host-driver/src/whd_wifi_api.c#L1115
However I don't have anything to test this with, there's absolutely nothing that I have that still creates a WEP AP. Also if this same code is meant to be used with other network interfaces, I'm not sure how this can be made portable.

@facchinm
Copy link
Member

I'm all in for removing WEP, if someone is still using it thinking there's any security he'd better leave the network open.

@iabdalkader
Copy link
Contributor Author

Okay I'll implement the OPEN begin() and remove WEP, and maybe leave the implementation here for reference.

- Add support for connecting to hidden networks (issue arduino#855).
- Implement `begin()` for OPEN security.
- Deprecate WEP security (issue arduino#819).
- Add sanity checks to all functions accepting network index from the user.

Signed-off-by: iabdalkader <[email protected]>
@iabdalkader
Copy link
Contributor Author

Updated, begin(ssid) is now implemented and WEP is deprecated.

For reference, this is the WEP key function. Since all 4 keys are required, the key is repeated.

int arduino::WiFiClass::begin(const char* ssid, uint8_t key_idx, const char* key) {
  // The low-level driver expects all 4 keys to be passed in a buffer with the following format:
  // <idx> <len> <key>, <idx> <len> <key>, etc..
  uint8_t buf[(2 + 32) * 4] = { 0 };
  size_t keylen = min(32, strlen(key));
  size_t buflen = (keylen + 2) * 4;

  // Repeat the key.
  for (int i=0; i<buflen; i += (keylen + 2)) {
      buf[i+0] = i / (keylen + 2);
      buf[i+1] = keylen;
      memcpy(&buf[i+2], key, keylen);
  }

  return begin(ssid, (const char *) buf, ENC_TYPE_WEP);
}

@iabdalkader iabdalkader merged commit 1338021 into arduino:main Apr 26, 2024
11 checks passed
@iabdalkader iabdalkader deleted the wifi_updates branch April 26, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connectivity to hidden WiFi networks .begin() for WEP is in Wifi.h but is not implemented in Wifi.cpp.
4 participants