WiFi dialog: add scripts for enabling/disabling WiFi connection

[jotaen4tinypilot]

Related #131, parts (a) and (b).

This PR adds two scripts for enabling and disabling the WiFi network interface on the device, so that you can also access TinyPilot through a wireless network connection.

I already discussed the basic approach last week with Charles (see comment and discussion below), because of his familiarity with networking/WiFi. Since then, I did more testing in various scenarios, and also already opened two subsequent draft PRs for the backend and frontend. They are WIP still, but they may give a rough impression of how the WiFi feature will look in its entirety. Overall, it seems to be looking good, so I thought it might make sense to already start the review process.

A few notes on the implementation:

• We write into the wpa_supplicants.conf file directly to configure the WiFi connection, and we use the marker-section mechanism to manage the TinyPilot-specific part of the configuration. Depending on whether the WiFi network is password-protected or not, we either use the wpa_passphrase command, or we generate the config section manually.
• I’m not sure it’s worth to validate the --country input value in the enable-wifi script: I’d plan to do validation in the backend later on, and the network service seems to behave graceful against invalid country codes. For the --psk flag, we have to do a basic length validation, because otherwise the wpa_passphrase command would fail.
• For testing on device, you can use the ifconfig command to check whether the wlan network interface is up and active. (I.e., whether ifconfig outputs a wlan block, and whether that has a inet/inet6 address assigned to it.)

---

Original draft PR comment (2024-06-13)

This PR is a draft for now, because I wanted to evaluate the technical approach of the underlying WiFi-related logic first, before investing time into the implementation of the application logic (i.e., the Python part and the UI).

I saw that you @cghague have been involved into our WiFi FAQ document, so I was hoping that you are maybe familiar with the technical domain. Could you look over the privileged scripts of this PR and give me some initial, high-level feedback about whether the proposed approach seems sensible to you? At this point, I’d mainly like to avoid going into a wrong direction, or to miss any potential caveats or other issues I may have forgotten to consider.

A few notes on the implementation:

• For the purposes of Configure WiFi options from the UI - Menu #131, we need to fully automate the logic for enabling and disabling WiFi, so that we can invoke both procedures from the Python backend programmatically. The enable-wifi script obviously would have to parse CLI arguments to parametrize the SSID, PSK, and country code. The disable-wifi script wouldn’t take any arguments.
• I’ve successfully tested the scripts on my device, and so far they appear to work fine. By issuing rfkill unblock wifi and wpa_cli -i wlan0 reconfigure, it seems to be possible to effectuate the changes and activate WiFi without having to reboot the device. This would make things easier for us in the UI, but it would also be more convenient and snappier for the end-user.
• My thinking was that it doesn’t make sense to use the raspi-config tool in the scripts, because that seems to be too limited. For example, we don’t want to just keep adding new WiFi configs to the wpa_supplicant.conf file, but we basically want to maintain one “TinyPilot-owned” custom WiFi config, which we then also can delete or modify in a targeted way. For this, it seemed more sensible to me to write into the wpa_supplicant.conf file directly, using the marker-section technique that we already employ elsewhere. The downside is that this requires more low-levelled logic on our side.

If you’d generally agree with the proposed approach and underlying reasoning, I’d proceed to wrap up this PR and to build the surrounding application logic.

[cghague]

Thanks, @jotaen4tinypilot! I've read through this change and the linked issues. The general approach seems reasonable, with just a few potential concerns I've outlined below.

Bookworm Compatibility

Using wpa_supplicant.conf likely won't work with Bookworm as it uses NetworkManager instead, and that directly controls an isolated instance of the wpa_supplicant daemon. However, some of our other features also won't be compatible with Bookworm, so this may be something that we've already implicitly accepted.

Multiple Network Support

Having a single TinyPilot-managed connection makes things simple for us, but it is very limiting and somewhat unusual. It's plausible users availing themselves of the Wi-Fi feature would expect to be able to move their TinyPilot devices around and have them work with multiple remembered networks.

Network Security

This implementation will only work with WPA networks using pre-shared keys. It will not work with open networks, WEP-protected networks, or networks with enterprise-grade security.

WEP is broken and best avoided, so omitting support for it is arguably for the best. Enterprise-grade security feels like something we could add in a future iteration. However, we should mention these limitations in the interface and script documentation.

We should consider supporting open networks despite their obvious security risks. Most requests for Wi-Fi support that we've received have come from users trying to connect to Wi-Fi in places like hotels, which often use open networks. Omitting support for open networks would rule out this use case.

Applying Settings

Using rfkill to toggle Wi-Fi should be fine, but I remember some complications related to using the wpa_cli -i wlan0 reconfigure command. I can't check the specifics right now, as I'm away from my devices, but I believe the issue is that dhcpcd hooks into wpa_supplicant directly, and that can cause unexpected behavior when using the wpa_cli command.

I can see that this approach worked during your tests, but it's something to be aware of and might warrant further investigation. That said, the Bullseye version of raspi-config uses the same command, so it's most likely okay!

raspi-config

We use raspi-config in many of our tutorials, but I don't think we call it programmatically anywhere, so that could be an interesting discussion. In this case, using it would abstract away the lower-level logic and make the process compatible with both Bullseye and Bookworm, but it would also remove some of the control we might want. Given our goals, I suggest avoiding relying on a third-party script.

[jotaen4tinypilot]

@cghague thanks for your thorough reply, that’s very helpful!

Bookworm Compatibility

We can’t trivially use NetworkManager on Bullseye, though, correct? I’d otherwise think that the footprint of us using wpa_supplicant.conf directly (as proposed here) doesn’t seem overly extensive to me. But we’d still have to migrate once we want to start supporting Bookworm.

Multiple Network Support

I actually haven’t considered this aspect yet, and I think this is a fair point. I think it’s also a bit of a product-related question whether we want to support configuring multiple WiFi connections right from the start, or whether it would be okay for us to push this down the road and only offer the ability to configure a single WiFi connection in the beginning. @shalver-tp thoughts?

From a technical perspective, if we’d push multi-WiFi down the road, I think we shouldn’t lose much of the initial work (as in: we probably can keep many parts of the single-WiFi implementation, so we wouldn’t have to throw away and redo everything). I’d estimate that most complexity would be in the frontend, because we’d have to add UI so that users can review and manage a list of WiFi connections, as opposed to a single one. We probably need an additional dialog “screen” (state) for that, similar to how we do it for managing multiple users in the “Security” dialog.

Network Security

Ah, good point – I think it’s probably easiest then if we make the password optional.

[jotaen4tinypilot]

Multiple Network Support

I actually haven’t considered this aspect yet, and I think this is a fair point. I think it’s also a bit of a product-related question whether we want to support configuring multiple WiFi connections right from the start, or whether it would be okay for us to push this down the road and only offer the ability to configure a single WiFi connection in the beginning. @shalver-tp thoughts?

@shalver-tp just a gentle nudge about the above topic (just in case you e.g. happened to miss this): I thought it would be best to consider and decide on this question before continuing to invest more work into the current (single-network) approach, especially before starting to work on the UI bit.

[scott-tp]

@jotaen4tinypilot Sorry, I missed this one when I was out last week; I apologize.

I think it would be nice to be able to "save" multiple wi-fi networks, but I think this is still a functional enhancement if we limit to one network at a time. In the end, this decision to only support single wifi puts a bit more burden on the user to change/update the wi-fi credentials when they change environments, but still brings enhanced network functionality.

Omit multiple wi-fi networks for now.

The other discussion on network security, et al all looks good to me. Thanks!

[jotaen4tinypilot]

Awesome, thanks for the feedback. I agree that storing multiple connections would be useful in certain scenarios, but I think we already add value if we ship single-WiFi in the first iteration.

I’ve created a separate issue for tracking the multi-WiFi idea.

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

⏳ @jdeanwallace please review this Pull Request

[jdeanwallace]

Automated comment from CodeApprove ➜

⏳ Approval Pending (5 unresolved comments)
Approval will be granted automatically when all comments are resolved

Nice, these are some good looking scripts. Works well on device too.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 10
Usage: ${0##*/} [--help] [--country COUNTRY] [--ssid SSID] [--psk PSK]

Seeing as --ssid and --country are required fields, can we remove the square brackets from around thier usage template?

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 16
                      it must be 8-63 characters in length.

While testing I noticed that this script will still exit successfully even if the wifi connection failed to connect to the network. I'm sure this is intentional, but perhaps we can add a note about how this script doesn't verify connection status?

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 65
if [[ -z "${WIFI_COUNTRY}" ]]; then

(optional) Can we validate that the country code has a length of 2 characters?

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 126
} | sudo tee --append "${CONFIG_FILE}" > /dev/null

Is sudo necessary seeing as this script is already privileged?

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 126
} | sudo tee --append "${CONFIG_FILE}" > /dev/null

Seeing as we're piping stdout to /dev/null, could we avoid using tee and append the output directly to the config file? Something like

{
  ...
} >> "${CONFIG_FILE}"

---

In: dev-scripts/mock-scripts/enable-wifi:

> Line 1
#!/bin/bash

In our other mock scripts we have a comment that references script on device that is being mocked. Can we do the same here?

Same for the other mock script.

---

👀 @jotaen4tinypilot it's your turn please take a look

[jotaen4tinypilot]

Automated comment from CodeApprove ➜

---

In: Discussion
Thanks for the review! Just fyi, I’ll hold off on merging until the subsequent PRs have progressed.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:
Oh right, thanks!

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:
Resolved

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 16
                      it must be 8-63 characters in length.

Yeah, that’s right – we cannot check really check the network status, as e.g. the user may enter the credentials in advance, or the WiFi is temporarily down or so.

I’ve added a note to the help text to clarify that the script only stores and activates the config.

---

In: debian-pkg/opt/tinypilot-privileged/scripts/enable-wifi:

> Line 71
if [[ -z "${WIFI_COUNTRY}" ]]; then

Resolved

---

In: dev-scripts/mock-scripts/enable-wifi:

> Line 1
#!/bin/bash

Resolved

[jdeanwallace]

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

Review approved on CodeApprove