Merge ~teknoraver/snappy-hwe-snaps/+git/wifi-ap:master into ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master

[Matteo Croce]

[Konrad Zapałowicz (kzapalowicz)]

Code looks ok, but IMHO needs more documentation

review: Needs Fixing (code)

[Simon Fels (morphis)]

See comments in line.

review: Needs Fixing

[Simon Fels (morphis)]

Also missing changes to snapcraft.yaml to build the service as part of the snap.

review: Needs Fixing

[Simon Fels (morphis)]

See comments in line

review: Needs Fixing

[Simon Fels (morphis)]

LGTM

review: Approve

[Tony Espy (awe)]

Based on our earlier discussion, +1 from me.

review: Approve

[Matteo Croce (teknoraver)]

I'm thinking that the opposite of the shell escape should be done when reading the configuration file with something like:

func unescapeTextByShell(input string) string {
 input = strings.Trim(input, `"'`)
 if strings.ContainsAny(input, "\\") {
  re := regexp.MustCompile("\\\\([\\\\$\\`\\\"])")
  input = re.ReplaceAllString(input, "$1")
 }
 return input
}

I'm thinking if we should rather strip the characters $"\` instead of messing with such regular expressions.

[Simon Fels (morphis)]

@matteo: if we test this there should not be any issue with using such a regular expression.

[Matteo Croce (teknoraver)]

> @matteo: if we test this there should not be any issue with using such a
> regular expression.

The test actually are configuration get, and JSON -> config file.
I'll add a test which does JSON -> config -> JSON to test escape and unescape

[Simon Fels (morphis)]

Did you already added the tests you described in your last comment? Otherwise I would like to merge this now.

[Matteo Croce (teknoraver)]

Not yet, sorry

[Simon Fels (morphis)]

@Matteo: Then lets do this as follow up MP. I will merge this one now.

diff --git a/service/main.go b/service/main.go
new file mode 100644
index 0000000..17134ed
--- /dev/null
+++ b/service/main.go
@@ -0,0 +1,217 @@
+//
+// Copyright (C) 2016 Canonical Ltd
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License version 3 as
+// published by the Free Software Foundation.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package main
+
+import (
+	"bufio"
+	"encoding/json"
+	"fmt"
+	"io/ioutil"
+	"log"
+	"net/http"
+	"os"
+	"regexp"
+	"strconv"
+	"strings"
+
+	"github.com/gorilla/mux"
+)
+
+/* JSON message format, as described here:
+{
+	"result": {
+		"key" : "val"
+	},
+	"status": "OK",
+	"status-code": 200,
+	"type": "sync"
+}
+*/
+
+type Response struct {
+	Result     map[string]string `json:"result"`
+	Status     string            `json:"status"`
+	StatusCode int               `json:"status-code"`
+	Type       string            `json:"type"`
+}
+
+func makeErrorResponse(code int, message, kind string) Response {
+	return Response{
+		Type:       "error",
+		Status:     http.StatusText(code),
+		StatusCode: code,
+		Result: map[string]string{
+			"message": message,
+			"kind":    kind,
+		},
+	}
+}
+
+func makeResponse(status int, result map[string]string) Response {
+	return Response{
+		Type:       "sync",
+		Status:     http.StatusText(status),
+		StatusCode: status,
+		Result:     result,
+	}
+}
+
+func sendHttpResponse(writer http.ResponseWriter, response Response) {
+	writer.WriteHeader(response.StatusCode)
+	data, _ := json.Marshal(response)
+	fmt.Fprintln(writer, string(data))
+}
+
+func getConfigOnPath(path string) string {
+	return path + "/config"
+}
+
+// Array of paths where the config file can be found.
+// The first one is readonly, the others are writable
+// they are readed in order and the configuration is merged
+var cfgpaths []string = []string{getConfigOnPath(os.Getenv("SNAP")),
+	getConfigOnPath(os.Getenv("SNAP_DATA")), getConfigOnPath(os.Getenv("SNAP_USER_DATA"))}
+
+const (
+	PORT                  = 5005
+	CONFIGURATION_API_URI = "/v1/configuration"
+)
+
+func main() {
+	r := mux.NewRouter().StrictSlash(true)
+
+	r.HandleFunc(CONFIGURATION_API_URI, getConfiguration).Methods(http.MethodGet)
+	r.HandleFunc(CONFIGURATION_API_URI, changeConfiguration).Methods(http.MethodPost)
+
+	log.Fatal(http.ListenAndServe("127.0.0.1:"+strconv.Itoa(PORT), r))
+}
+
+// Convert eg. WIFI_OPERATION_MODE to wifi.operation-mode
+func convertKeyToRepresentationFormat(key string) string {
+	new_key := strings.ToLower(key)
+	new_key = strings.Replace(new_key, "_", ".", 1)
+	return strings.Replace(new_key, "_", "-", -1)
+}
+
+func convertKeyToStorageFormat(key string) string {
+	// Convert eg. wifi.operation-mode to WIFI_OPERATION_MODE
+	key = strings.ToUpper(key)
+	key = strings.Replace(key, ".", "_", -1)
+	return strings.Replace(key, "-", "_", -1)
+}
+
+func readConfigurationFile(filePath string, config map[string]string) (err error) {
+	file, err := os.Open(filePath)
+	if err != nil {
+		return nil
+	}
+
+	defer file.Close()
+
+	for scanner := bufio.NewScanner(file); scanner.Scan(); {
+		// Ignore all empty or commented lines
+		if line := scanner.Text(); len(line) != 0 && line[0] != '#' {
+			// Line must be in the KEY=VALUE format
+			if parts := strings.Split(line, "="); len(parts) == 2 {
+				value := unescapeTextByShell(parts[1])
+				config[convertKeyToRepresentationFormat(parts[0])] = value
+			}
+		}
+	}
+
+	return nil
+}
+
+func readConfiguration(paths []string, config map[string]string) (err error) {
+	for _, location := range paths {
+		if readConfigurationFile(location, config) != nil {
+			return fmt.Errorf("Failed to read configuration file '%s'", location)
+		}
+	}
+
+	return nil
+}
+
+func getConfiguration(writer http.ResponseWriter, request *http.Request) {
+	config := make(map[string]string)
+	if readConfiguration(cfgpaths, config) == nil {
+		sendHttpResponse(writer, makeResponse(http.StatusOK, config))
+	} else {
+		errResponse := makeErrorResponse(http.StatusInternalServerError, "Failed to read configuration data", "internal-error")
+		sendHttpResponse(writer, errResponse)
+	}
+}
+
+// Escape shell special characters, avoid injection
+// eg. SSID set to "My AP$(nc -lp 2323 -e /bin/sh)"
+// to get a root shell
+func escapeTextForShell(input string) string {
+	if strings.ContainsAny(input, "\\\"'`$\n\t #") {
+		input = strings.Replace(input, `\`, `\\`, -1)
+		input = strings.Replace(input, `"`, `\"`, -1)
+		input = strings.Replace(input, "`", "\\`", -1)
+		input = strings.Replace(input, `$`, `\$`, -1)
+
+		input = `"` + input + `"`
+	}
+	return input
+}
+
+// Do the reverse of escapeTextForShell() here
+// strip any \ followed by \$`"
+func unescapeTextByShell(input string) string {
+	input = strings.Trim(input, `"'`)
+	if strings.ContainsAny(input, "\\") {
+		re := regexp.MustCompile("\\\\([\\\\$\\`\\\"])")
+		input = re.ReplaceAllString(input, "$1")
+	}
+	return input
+}
+
+func changeConfiguration(writer http.ResponseWriter, request *http.Request) {
+	// Write in SNAP_DATA
+	confWrite := getConfigOnPath(os.Getenv("SNAP_DATA"))
+
+	file, err := os.Create(confWrite)
+	if err != nil {
+		errResponse := makeErrorResponse(http.StatusInternalServerError, "Can't write configuration file", "internal-error")
+		sendHttpResponse(writer, errResponse)
+		return
+	}
+	defer file.Close()
+
+	body, err := ioutil.ReadAll(request.Body)
+	if err != nil {
+		errResponse := makeErrorResponse(http.StatusInternalServerError, "Error reading the request body", "internal-error")
+		sendHttpResponse(writer, errResponse)
+		return
+	}
+
+	var items map[string]string
+	if json.Unmarshal(body, &items) != nil {
+		errResponse := makeErrorResponse(http.StatusInternalServerError, "Malformed request", "internal-error")
+		sendHttpResponse(writer, errResponse)
+		return
+	}
+
+	for key, value := range items {
+		key = convertKeyToStorageFormat(key)
+		value = escapeTextForShell(value)
+		file.WriteString(fmt.Sprintf("%s=%s\n", key, value))
+	}
+
+	sendHttpResponse(writer, makeResponse(http.StatusOK, nil))
+}
diff --git a/service/main_test.go b/service/main_test.go
new file mode 100644
index 0000000..d15430b
--- /dev/null
+++ b/service/main_test.go
@@ -0,0 +1,209 @@
+//
+// Copyright (C) 2016 Canonical Ltd
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License version 3 as
+// published by the Free Software Foundation.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"gopkg.in/check.v1"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"strings"
+	"testing"
+)
+
+// gopkg.in/check.v1 stuff
+func Test(t *testing.T) { check.TestingT(t) }
+
+type S struct{}
+
+var _ = check.Suite(&S{})
+
+// Test the config file path append routine
+func (s *S) TestPath(c *check.C) {
+	c.Assert(getConfigOnPath("/test"), check.Equals, "/test/config")
+}
+
+// List of tokens to be translated
+var cfgKeys = [...][2]string{
+	{"DISABLED", "disabled"},
+	{"WIFI_SSID", "wifi.ssid"},
+	{"WIFI_INTERFACE", "wifi.interface"},
+	{"WIFI_INTERFACE_MODE", "wifi.interface-mode"},
+	{"DHCP_RANGE_START", "dhcp.range-start"},
+	{"MYTOKEN", "mytoken"},
+	{"CFG_TOKEN", "cfg.token"},
+	{"MY_TOKEN$", "my.token$"},
+}
+
+// Test token conversion from internal format
+func (s *S) TestConvertKeyToRepresentationFormat(c *check.C) {
+	for _, st := range cfgKeys {
+		c.Assert(convertKeyToRepresentationFormat(st[0]), check.Equals, st[1])
+	}
+}
+
+// Test token conversion to internal format
+func (s *S) TestConvertKeyToStorageFormat(c *check.C) {
+	for _, st := range cfgKeys {
+		c.Assert(convertKeyToStorageFormat(st[1]), check.Equals, st[0])
+	}
+}
+
+// List of malicious tokens which needs to be escaped
+func (s *S) TestEscapeShell(c *check.C) {
+	cmds := [...][2]string{
+		{"my_ap", "my_ap"},
+		{`my ap`, `"my ap"`},
+		{`my "ap"`, `"my \"ap\""`},
+		{`$(ps ax)`, `"\$(ps ax)"`},
+		{"`ls /`", "\"\\`ls /\\`\""},
+		{`c:\dir`, `"c:\\dir"`},
+	}
+	for _, st := range cmds {
+		c.Assert(escapeTextForShell(st[0]), check.Equals, st[1])
+	}
+}
+
+func (s *S) TestGetConfiguration(c *check.C) {
+	// Check it we get a valid JSON as configuration
+	req, err := http.NewRequest(http.MethodGet, CONFIGURATION_API_URI, nil)
+	c.Assert(err, check.IsNil)
+
+	rec := httptest.NewRecorder()
+
+	getConfiguration(rec, req)
+
+	body, err := ioutil.ReadAll(rec.Result().Body)
+	c.Assert(err, check.IsNil)
+
+	// Parse the returned JSON
+	var resp Response
+	err = json.Unmarshal(body, &resp)
+	c.Assert(err, check.IsNil)
+
+	// Check for 200 status code
+	c.Assert(resp.Status, check.Equals, http.StatusText(http.StatusOK))
+	c.Assert(resp.StatusCode, check.Equals, http.StatusOK)
+	c.Assert(resp.Type, check.Equals, "sync")
+}
+
+func (s *S) TestChangeConfiguration(c *check.C) {
+	// Test a non writable path:
+	os.Setenv("SNAP_DATA", "/nodir")
+
+	req, err := http.NewRequest(http.MethodPost, CONFIGURATION_API_URI, nil)
+	c.Assert(err, check.IsNil)
+
+	rec := httptest.NewRecorder()
+
+	changeConfiguration(rec, req)
+
+	c.Assert(rec.Code, check.Equals, http.StatusInternalServerError)
+
+	body, err := ioutil.ReadAll(rec.Result().Body)
+	c.Assert(err, check.IsNil)
+
+	// Parse the returned JSON
+	var resp Response
+	err = json.Unmarshal(body, &resp)
+	c.Assert(err, check.IsNil)
+
+	// Check for 500 status code and other error fields
+	c.Assert(resp.Status, check.Equals, http.StatusText(http.StatusInternalServerError))
+	c.Assert(resp.StatusCode, check.Equals, http.StatusInternalServerError)
+	c.Assert(resp.Type, check.Equals, "error")
+	c.Assert(resp.Result["kind"], check.Equals, "internal-error")
+	c.Assert(resp.Result["message"], check.Equals, "Can't write configuration file")
+
+	// Test an invalid JSON
+	os.Setenv("SNAP_DATA", "/tmp")
+	req, err = http.NewRequest(http.MethodPost, CONFIGURATION_API_URI, strings.NewReader("not a JSON content"))
+	c.Assert(err, check.IsNil)
+
+	rec = httptest.NewRecorder()
+
+	changeConfiguration(rec, req)
+
+	c.Assert(rec.Code, check.Equals, http.StatusInternalServerError)
+
+	body, err = ioutil.ReadAll(rec.Result().Body)
+	c.Assert(err, check.IsNil)
+
+	// Parse the returned JSON
+	resp = Response{}
+	err = json.Unmarshal(body, &resp)
+	c.Assert(err, check.IsNil)
+
+	// Check for 500 status code and other error fields
+	c.Assert(resp.Status, check.Equals, http.StatusText(http.StatusInternalServerError))
+	c.Assert(resp.StatusCode, check.Equals, http.StatusInternalServerError)
+	c.Assert(resp.Type, check.Equals, "error")
+	c.Assert(resp.Result["kind"], check.Equals, "internal-error")
+	c.Assert(resp.Result["message"], check.Equals, "Malformed request")
+
+	// Test a succesful configuration set
+
+	// Values to be used in the config
+	values := map[string]string{
+		"wifi.security":            "wpa2",
+		"wifi.ssid":                "UbuntuAP",
+		"wifi.security-passphrase": "12345678",
+	}
+
+	// Convert the map into JSON
+	args, err := json.Marshal(values)
+	c.Assert(err, check.IsNil)
+
+	req, err = http.NewRequest(http.MethodPost, CONFIGURATION_API_URI, bytes.NewReader(args))
+	c.Assert(err, check.IsNil)
+
+	rec = httptest.NewRecorder()
+
+	// Do the request
+	changeConfiguration(rec, req)
+
+	c.Assert(rec.Code, check.Equals, http.StatusOK)
+
+	// Read the result JSON
+	body, err = ioutil.ReadAll(rec.Result().Body)
+	c.Assert(err, check.IsNil)
+
+	// Parse the returned JSON
+	resp = Response{}
+	err = json.Unmarshal(body, &resp)
+	c.Assert(err, check.IsNil)
+
+	// Check for 200 status code and other succesful fields
+	c.Assert(resp.Status, check.Equals, http.StatusText(http.StatusOK))
+	c.Assert(resp.StatusCode, check.Equals, http.StatusOK)
+	c.Assert(resp.Type, check.Equals, "sync")
+
+	// Read the generated config and check that values were set
+	config, err := ioutil.ReadFile(getConfigOnPath(os.Getenv("SNAP_DATA")))
+	c.Assert(err, check.IsNil)
+
+	for key, value := range values {
+		c.Assert(strings.Contains(string(config),
+			convertKeyToStorageFormat(key)+"="+value+"\n"),
+			check.Equals, true)
+	}
+
+	// don't leave garbage in /tmp
+	os.Remove(getConfigOnPath(os.Getenv("SNAP_DATA")))
+}
diff --git a/snapcraft.yaml b/snapcraft.yaml
index c4f1c0d..bba52b0 100644
--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -20,6 +20,9 @@ apps:
       - network-manager
   config:
     command: bin/config.sh
+  management-service:
+    command: bin/service
+    daemon: simple
 
 parts:
   scripts:
@@ -44,6 +47,13 @@ parts:
         - bin/iwconfig
     snap:
       - $binaries
+
+  management-service:
+    plugin: go
+    source: ./service
+    snap:
+      - bin
+
   dnsmasq:
     plugin: make
     source: https://git.launchpad.net/~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap
@@ -99,7 +109,7 @@ parts:
         - rtl8188/hostapd
     snap:
       - $binaries
-  nmlci:
+  nmcli:
     plugin: nil
     stage-packages:
       - network-manager