Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Ability to set advanced WiFi options #12

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

DavisNT
Copy link
Contributor

@DavisNT DavisNT commented Apr 11, 2015

This fixes #10 on Websettings side. ❗ Additionally changes in startup script (pimusicbox/pimusicbox#271) are required!

@kingosticks
Copy link
Member

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.

@DavisNT
Copy link
Contributor Author

DavisNT commented Apr 11, 2015

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
Copy link
Contributor Author

DavisNT commented Apr 11, 2015

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.

In settingsspec.ini there is a definition:
wifi_scan_ssid = "boolean(default=true)"

Hovever if this value is not present in .ini file the setting appears as
disabled in web.
@DavisNT
Copy link
Contributor Author

DavisNT commented Apr 13, 2015

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
Copy link
Contributor Author

DavisNT commented Apr 13, 2015

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
Copy link
Contributor Author

DavisNT commented Apr 23, 2015

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

@kingosticks
Copy link
Member

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.

@DavisNT
Copy link
Contributor Author

DavisNT commented Apr 23, 2015

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).

@kingosticks
Copy link
Member

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

@DavisNT
Copy link
Contributor Author

DavisNT commented Apr 23, 2015

I have made this change (it is referenced in pull request pimusicbox/pimusicbox#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
Copy link
Contributor Author

DavisNT commented May 1, 2015

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?

@kingosticks
Copy link
Member

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
#12 (comment)
.

@kingosticks
Copy link
Member

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?

@DavisNT
Copy link
Contributor Author

DavisNT commented May 10, 2015

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.

@kingosticks
Copy link
Member

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
#12 (comment)
.

@kingosticks
Copy link
Member

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
#12 (comment)
.

@DavisNT
Copy link
Contributor Author

DavisNT commented May 11, 2015

Ability to place config files inside FAT32 partition sounds like a great idea! 👍
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?

@kingosticks
Copy link
Member

Yes. Just those two currently I think.

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

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
#12 (comment)
.

@DavisNT
Copy link
Contributor Author

DavisNT commented May 11, 2015

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.

@kingosticks
Copy link
Member

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
#12 (comment)
.

@DavisNT
Copy link
Contributor Author

DavisNT commented May 12, 2015

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).

@kingosticks
Copy link
Member

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
#12 (comment)
.

@DavisNT
Copy link
Contributor Author

DavisNT commented May 17, 2015

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to set advanced WiFi options
2 participants