mopidy-websettings icon indicating copy to clipboard operation
mopidy-websettings copied to clipboard

Ability to set advanced WiFi options

Open DavisNT opened this issue 9 years ago • 23 comments

This fixes #10 on Websettings side. :exclamation: Additionally changes in startup script (https://github.com/woutervanwijk/Pi-MusicBox/pull/271) are required!

DavisNT avatar Apr 11 '15 14:04 DavisNT

Changing the startup script, and more importantly testing it with a variety of wifi cards (most of which are full of driver bugs) is the challenge here. There is no point merging this without the corresponding changes.

kingosticks avatar Apr 11 '15 15:04 kingosticks

Thanks! I was developing startup script changes and I have created a pull request for them now. I have also added a notification in Websettings about buggy drivers.

P.S. I am testing all of this now on my Raspberry Pi model B with TP-Link TL-WN725N Ver:2.1 Wifi adapter and updated Raspbian (and Python packages) as described in https://github.com/woutervanwijk/Pi-MusicBox/wiki/Updating-components-to-latest-versions

DavisNT avatar Apr 11 '15 15:04 DavisNT

Looks like there is another bug - wifi_scan_ssid has a default value of true in settingsspec.ini but if it is not mentioned in .ini file it is disabled by default in Websettings.

DavisNT avatar Apr 11 '15 16:04 DavisNT

I have done some tests.

Preparations

pip install -U https://github.com/DavisNT/mopidy-websettings/archive/develop.zip
curl https://raw.githubusercontent.com/DavisNT/Pi-MusicBox/develop/filechanges/opt/musicbox/startup.sh > /opt/musicbox/startup.sh
chmod 4755 /opt/musicbox/startup.sh

Test results

Test case 1: new settings are not present in .ini file Files viewed in SSH session:

root@Player:~# grep wifi_ /boot/config/settings.ini
wifi_network = Demo network
wifi_password = Demo Key 12345
root@Player:~# cat /etc/wpa.conf
    ctrl_interface=DIR=/var/run/wpa_supplicant GROUP=netdev
    update_config=1
    network={
        key_mgmt=WPA-PSK
        ssid="Demo network"
        psk="Demo Key 12345"
        scan_ssid=1
    }

Defaults are also shown in Websettings.

Test case 2: Websettings is opened and Update settings (reboot) is clicked without opening/changing anything. Files viewed in SSH session:

root@Player:~# grep wifi_ /boot/config/settings.ini
wifi_network = Demo network
wifi_password = Demo Key 12345
wifi_wpa2_aes = false
wifi_scan_ssid = true
root@Player:~# cat /etc/wpa.conf
    ctrl_interface=DIR=/var/run/wpa_supplicant GROUP=netdev
    update_config=1
    network={
        key_mgmt=WPA-PSK
        ssid="Demo network"
        psk="Demo Key 12345"
        scan_ssid=1
    }

Defaults are also shown in Websettings.

Test case 3: both settings changed in Websettings Files viewed in SSH session:

root@Player:~# grep wifi_ /boot/config/settings.ini
wifi_network = Demo network
wifi_password = Demo Key 12345
wifi_wpa2_aes = true
wifi_scan_ssid = false
root@Player:~# cat /etc/wpa.conf
    ctrl_interface=DIR=/var/run/wpa_supplicant GROUP=netdev
    update_config=1
    network={
        key_mgmt=WPA-PSK
        ssid="Demo network"
        psk="Demo Key 12345"
        scan_ssid=0
        proto=RSN
        pairwise=CCMP
        group=CCMP
    }

Changed settings are also displayed in Websettings.

DavisNT avatar Apr 13 '15 05:04 DavisNT

If none of the new settings are changed in Websettings, these pull-requests only add key_mgmt=WPA-PSK parameter inside file /etc/wpa.conf.

@kingosticks Can you do the testing with various adapters? Another alternative would be adding key_mgmt=WPA-PSK only if Use only WPA2-AES is enabled. Then there would be no changes to /etc/wpa.conf when both new settings have default values.

DavisNT avatar Apr 13 '15 20:04 DavisNT

Is there any way I can facilitate review of this change and get it merged?

DavisNT avatar Apr 23 '15 15:04 DavisNT

Hi sorry I forgot to reply to this one. I don't have any spare wireless sticks to do any useful tests I am afraid.

As a general rule, advanced/specific options which most people don't care about/need, are generally left out of musicbox.The few that do want them can then go on to customise their install as needed. I haven't really had look at this yet to see where this stuff falls.

kingosticks avatar Apr 23 '15 16:04 kingosticks

Thanks for response! Will adding key_mgmt=WPA-PSK only if Use only WPA2-AES is enabled facilitate the review/merge? In that case there will be no changes to /etc/wpa.conf in default configuration (if default configuration is not changed).

DavisNT avatar Apr 23 '15 16:04 DavisNT

That certainly sounds like a good idea. Lets try and keep the simple case simple.

kingosticks avatar Apr 23 '15 16:04 kingosticks

I have made this change (it is referenced in pull request https://github.com/woutervanwijk/Pi-MusicBox/pull/271).

Repeated test results

Test case 1: new settings are not present in .ini file Files viewed in SSH session:

root@Player:~# grep wifi_ /boot/config/settings.ini
wifi_network = Demo network
wifi_password = Demo Key 12345
root@Player:~# cat /etc/wpa.conf
    ctrl_interface=DIR=/var/run/wpa_supplicant GROUP=netdev
    update_config=1
    network={
        ssid="Demo network"
        psk="Demo Key 12345"
        scan_ssid=1
    }

Defaults are also shown in Websettings.

Test case 2: Websettings is opened and Update settings (reboot) is clicked without opening/changing anything. Files viewed in SSH session:

root@Player:~# grep wifi_ /boot/config/settings.ini
wifi_network = Demo network
wifi_password = Demo Key 12345
wifi_wpa2_aes = false
wifi_scan_ssid = true
root@Player:~# cat /etc/wpa.conf
    ctrl_interface=DIR=/var/run/wpa_supplicant GROUP=netdev
    update_config=1
    network={
        ssid="Demo network"
        psk="Demo Key 12345"
        scan_ssid=1
    }

Defaults are also shown in Websettings.

Test case 3: both settings changed in Websettings Files viewed in SSH session:

root@Player:~# grep wifi_ /boot/config/settings.ini
wifi_network = Demo network
wifi_password = Demo Key 12345
wifi_wpa2_aes = true
wifi_scan_ssid = false
root@Player:~# cat /etc/wpa.conf
    ctrl_interface=DIR=/var/run/wpa_supplicant GROUP=netdev
    update_config=1
    network={
        ssid="Demo network"
        psk="Demo Key 12345"
        scan_ssid=0
        key_mgmt=WPA-PSK
        proto=RSN
        pairwise=CCMP
        group=CCMP
    }

Changed settings are also displayed in Websettings.

DavisNT avatar Apr 23 '15 19:04 DavisNT

Currently there are no changes to wpa.conf if new settings are not changed. Can you please do review and merge this? Or are there any other issues/impediments associated with this change?

DavisNT avatar May 01 '15 21:05 DavisNT

I will get to this, sorry. On 1 May 2015 22:13, "Dāvis Mošenkovs" [email protected] wrote:

Currently there are no changes to wpa.conf if new settings are not changed. Can you please do review and merge this? Or are there any other issues/impediments associated with this change?

— Reply to this email directly or view it on GitHub https://github.com/woutervanwijk/mopidy-websettings/pull/12#issuecomment-98240043 .

kingosticks avatar May 01 '15 22:05 kingosticks

Hey, finally had a good look at this. The code technically looks fine and I appreciate all your efforts here but I've got a couple of questions.

  • More options = more confusion and having scan_ssid configurable doesn't gain you anything (except speed, but there are much better optimisations available in that department). So I am afraid i don't see the point there.
  • Having spent some time looking over wpa_supplicant it looks like all the options you are setting here are already covered by the defaults. What does explicitly restricting them to these options give you? Is it just to force AES encryption and presumably fail if that's not supported (by either end), is that useful behaviour?
  • What is the point in changing key_mgmt from the default?

I'd imagine most people just leave the wifi_network undefined and provide their own fully customised copy of /etc/wpa.conf. Are these settings really useful enough to other people that they belong in the user interface?

kingosticks avatar May 05 '15 18:05 kingosticks

Hi! Thanks for looking into this and sorry for my late response! Here are my comments:

  1. Disabling scan_ssid will (AFAIK only in cases when booted player is moved into range of WiFi network) improve scanning speed and mitigate a theoretical "security risk" (ability for the intruder to determine to what WiFi network the player tries to connect when the player is not within the range of the network). I mainly added this because wpa_supplicant docs states "this will add latency to scanning, so enable this only when needed" and (before you mentioned that it is a workaround for buggy drivers) scan_ssid=1 looked like a default providing no real benefits, but promoting hiding SSID (which is a false-sense-of-security measure as SSID is sent in 802.11 management frames).
  2. Setting proto=RSN, pairwise=CCMP and group=CCMP should help preventing some really complex attacks (e.g. RSN to WPA downgrade + TKIP explotation) on WiFi networks allowing WPA and legacy ciphers.
  3. key_mgmt=WPA-PSK should do nothing unless there is some kind of unknown bug in wpa_supplicant (or the defaults will gt changed). This was the reason I added it into default configuration in the beginning.

Mostly this patch is for "security paranoids" and I know that there are bigger security issues in Pi MusicBox (e.g. globally writable boot partition that contains files modified by Mopidy process). I agree that these options will not be usable for most users and keeping settings as simple as possible is a good point for not including these changes in Pi MusicBox.

However I think that possibility to customize Pi MusicBox configuration could be improved. May be we could introduce an option (that would be hidden in interface) in settings.ini that would contain a list of files that startup.sh will not overwrite? AFAIK this would solve many other customization issues as well. Currently I didn't found any cleaner ways to customize wpa_supplicant configuration than changing /etc/network/interfaces and creating modified wpa.conf with another filename.

DavisNT avatar May 10 '15 21:05 DavisNT

I think that is a very good idea. As it is, providing you don't set the wifi_network option, musicbox won't overwrite any wpa_supplicant config that you setup yourself. But that fact could be made more obvious, and it could extend to other things (as you say).

Regarding scan_ssid, its just a case of having something that works for everyone (albeit unnecessarily slowly in some cases) versus something more complicated and potentially confusing for users, many of which are new to this. The security risk here is really quite paranoid! But anyway, if we sort out a clear way for people to easily provide their own custom settings this isn't an issue. On 10 May 2015 22:21, "Dāvis Mošenkovs" [email protected] wrote:

Hi! Thanks for looking into this and sorry for my late response! Here are my comments:

  1. Disabling scan_ssid will (AFAIK only in cases when booted player is moved into range of WiFi network) improve scanning speed and mitigate a theoretical "security risk" (ability for the intruder to determine to what WiFi network the player tries to connect when the player is not within the range of the network). I mainly added this because wpa_supplicant docs states "this will add latency to scanning, so enable this only when needed" and (before you mentioned that it is a workaround for buggy drivers) scan_ssid=1 looked like a default providing no real benefits, but promoting hiding SSID (which is a false-sense-of-security measure as SSID is sent in 802.11 management frames).
  2. Setting proto=RSN, pairwise=CCMP and group=CCMP should help preventing some really complex attacks (e.g. RSN to WPA downgrade + TKIP explotation) on WiFi networks allowing WPA and legacy ciphers.
  3. key_mgmt=WPA-PSK should do nothing unless there is some kind of unknown bug in wpa_supplicant (or the defaults will gt changed). This was the reason I added it into default configuration in the beginning.

Mostly this patch is for "security paranoids" and I know that there are bigger security issues in Pi MusicBox (e.g. globally writable boot partition that contains files modified by Mopidy process). I agree that these options will not be usable for most users and keeping settings as simple as possible is a good point for not including these changes in Pi MusicBox.

However I think that possibility to customize Pi MusicBox configuration could be improved. May be we could introduce an option (that would be hidden in interface) in settings.ini that would contain a list of files that startup.sh will not overwrite? AFAIK this would solve many other customization issues as well. Currently I didn't found any cleaner ways to customize wpa_supplicant configuration than changing /etc/network/interfaces and creating modified wpa.conf with another filename.

— Reply to this email directly or view it on GitHub https://github.com/woutervanwijk/mopidy-websettings/pull/12#issuecomment-100703556 .

kingosticks avatar May 10 '15 21:05 kingosticks

We could allow people to just drop their custom asound.conf or wpa_supplicant.conf or samba whatever configs in the /boot/config directory. The startup script can then copy the files to the correct place if they exist, and otherwise run through the current script as normal. On 10 May 2015 22:57, "Nick Steel" [email protected] wrote:

I think that is a very good idea. As it is, providing you don't set the wifi_network option, musicbox won't overwrite any wpa_supplicant config that you setup yourself. But that fact could be made more obvious, and it could extend to other things (as you say).

Regarding scan_ssid, its just a case of having something that works for everyone (albeit unnecessarily slowly in some cases) versus something more complicated and potentially confusing for users, many of which are new to this. The security risk here is really quite paranoid! But anyway, if we sort out a clear way for people to easily provide their own custom settings this isn't an issue. On 10 May 2015 22:21, "Dāvis Mošenkovs" [email protected] wrote:

Hi! Thanks for looking into this and sorry for my late response! Here are my comments:

  1. Disabling scan_ssid will (AFAIK only in cases when booted player is moved into range of WiFi network) improve scanning speed and mitigate a theoretical "security risk" (ability for the intruder to determine to what WiFi network the player tries to connect when the player is not within the range of the network). I mainly added this because wpa_supplicant docs states "this will add latency to scanning, so enable this only when needed" and (before you mentioned that it is a workaround for buggy drivers) scan_ssid=1 looked like a default providing no real benefits, but promoting hiding SSID (which is a false-sense-of-security measure as SSID is sent in 802.11 management frames).
  2. Setting proto=RSN, pairwise=CCMP and group=CCMP should help preventing some really complex attacks (e.g. RSN to WPA downgrade + TKIP explotation) on WiFi networks allowing WPA and legacy ciphers.
  3. key_mgmt=WPA-PSK should do nothing unless there is some kind of unknown bug in wpa_supplicant (or the defaults will gt changed). This was the reason I added it into default configuration in the beginning.

Mostly this patch is for "security paranoids" and I know that there are bigger security issues in Pi MusicBox (e.g. globally writable boot partition that contains files modified by Mopidy process). I agree that these options will not be usable for most users and keeping settings as simple as possible is a good point for not including these changes in Pi MusicBox.

However I think that possibility to customize Pi MusicBox configuration could be improved. May be we could introduce an option (that would be hidden in interface) in settings.ini that would contain a list of files that startup.sh will not overwrite? AFAIK this would solve many other customization issues as well. Currently I didn't found any cleaner ways to customize wpa_supplicant configuration than changing /etc/network/interfaces and creating modified wpa.conf with another filename.

— Reply to this email directly or view it on GitHub https://github.com/woutervanwijk/mopidy-websettings/pull/12#issuecomment-100703556 .

kingosticks avatar May 11 '15 08:05 kingosticks

Ability to place config files inside FAT32 partition sounds like a great idea! :+1: Also I think proper way of customizing Pi MusicBox should be documented and link to documentation could be put in Websettings page (at least if musicbox = true).

A related off-topic: The only thing I am wondering now is how to protect /boot from being world-writable (and preferably also from being world-readable). Are /boot/config/settings.ini and /boot/config/streamuris.js only files that are used by Mopidy?

DavisNT avatar May 11 '15 19:05 DavisNT

Yes. Just those two currently I think.

Ability to place config files inside FAT32 partition sounds like a great idea! [image: :+1:]

A related off-topic: The only thing I am wondering now is how to protect /boot from being world-writable (and preferably also from being world-readable). Are /boot/config/settings.ini and /boot/config/streamuris.js only files that are used by Mopidy?

— Reply to this email directly or view it on GitHub https://github.com/woutervanwijk/mopidy-websettings/pull/12#issuecomment-101018644 .

kingosticks avatar May 11 '15 19:05 kingosticks

I have two ideas about restricting access to /boot:

  1. Some kind of bind (or most likely bindfs) mount that could control access.
  2. Mounting /boot with umask=027 and having a temporary folder /opt/musicbox/config writable by Mopidy. A startup script (e.g. existing startup.sh or a separate script) would copy files from /boot/config to /opt/musicbox/config and a shutdown script would copy files back from /opt/musicbox/config to /boot/config.

What do you think about these options? I actually like second idea much better, because it is much more simple.

DavisNT avatar May 11 '15 20:05 DavisNT

With the hope of preventing what exactly? On 11 May 2015 21:36, "Dāvis Mošenkovs" [email protected] wrote:

I have two ideas about this:

  1. Some kind of bind (or most likely bindfs) mount that could control access.
  2. Mounting /boot with umask=027 and having a temporary folder /opt/musicbox/config writable by Mopidy. A startup script (e.g. existing startup.sh or a separate script) would copy files from /boot/config to /opt/musicbox/config and a shutdown script would copy files back from /opt/musicbox/config to /boot/config.

What do you think about these options? I actually like second idea much better, because it is much more simple.

— Reply to this email directly or view it on GitHub https://github.com/woutervanwijk/mopidy-websettings/pull/12#issuecomment-101040383 .

kingosticks avatar May 11 '15 21:05 kingosticks

With the idea of preventing globally writable /boot. It is very good that Mopidy already is not running as root, but currently any application (running under any user account) can gain unlimited access to entire OS (by installing so called insecure kernel or compromising boot configuration and waiting for the user to reboot). I assume that Raspbian originally had /boot writable only by root (or mounted read-only).

DavisNT avatar May 12 '15 05:05 DavisNT

I'm not sure what it originally had but you'd hope so.

Would an additional very small partition specifically for the two musicbox config files be another alternative?

There is the possibility that we'll want the musicbox process to be able to write to some of the other files in /boot. Specifically to handle the new device tree overlay stuff when switching soundcards. On 12 May 2015 06:52, "Dāvis Mošenkovs" [email protected] wrote:

With the idea of preventing globally writable /boot. It is very good that Mopidy already is not running as root, but currently any application (running under any user account) can gain unlimited access to entire OS (by installing so called insecure kernel or compromising boot configuration and waiting for the user to reboot). I assume that Raspbian originally had /boot writable only by root (or mounted read-only).

— Reply to this email directly or view it on GitHub https://github.com/woutervanwijk/mopidy-websettings/pull/12#issuecomment-101140519 .

kingosticks avatar May 12 '15 07:05 kingosticks

Hi! Sorry for my late response!

I think additional partition is not very good idea! I suggest staying as close to Raspbian as possible. Actually I think it would be a great idea to create apt package of Pi MusicBox that could be used to update Pi MusicBox specific customizations (or even installed on vanilla Raspbian to turn it into Pi MusicBox).

For making parts of /boot writable I would suggest one of these options:

  1. Copying files back and forth by scripts.
  2. Using BindFS to do the re-mounting with less restrictive permissions.

DavisNT avatar May 17 '15 21:05 DavisNT