Restrict the set of allowed characters in plugin IDs

We will want to put plugin id in a control id, which is sent to an app
and then to GUI. When we get a control request from GUI, we will want
to extract the plugin ID from the control name. To do it unambiguously
we need some separator made of chars that are not allowed in a plugin
name. This is to avoid the situation when there are two plugins:
"Plugin" and "PluginFoo". "Plugin" exposes a control named
"FooControl" and "PluginFoo" exposes a control named "Control". Faking
the control names which will be sent to the app would result in two
"PluginFooControl".

One possible option for plugin ID and control name separator would be
"/", but that won't work, since the request sent from GUI to the app
to <probe>/<node>/<control> would actually be
<probe>/<node>/<plugin>/<control> and as such wouldn't match the URL
template in RegisterControlRoutes().

probe/plugins/registry.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"net/url"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strings"
 	"sync"
@@ -27,6 +28,7 @@ var (
 	transport                 = makeUnixRoundTripper
 	maxResponseBytes    int64 = 50 * 1024 * 1024
 	errResponseTooLarge       = fmt.Errorf("response must be shorter than 50MB")
+	validPluginName           = regexp.MustCompile("^[A-Za-z0-9]+([-][A-Za-z0-9]+)*$")
 )
 
 const (
@@ -103,7 +105,12 @@ func (r *Registry) scan() error {
 			continue
 		}
 		client := &http.Client{Transport: tr, Timeout: pluginTimeout}
-		plugins[path] = NewPlugin(r.context, path, client, r.apiVersion, r.handshakeMetadata)
+		plugin, err := NewPlugin(r.context, path, client, r.apiVersion, r.handshakeMetadata)
+		if err != nil {
+			log.Warningf("plugins: error loading plugin %s: %v", path, err)
+			continue
+		}
+		plugins[path] = plugin
 		log.Infof("plugins: added plugin %s", path)
 	}
 	// remove plugins which weren't found
@@ -216,16 +223,19 @@ type Plugin struct {
 
 // NewPlugin loads and initializes a new plugin. If client is nil,
 // http.DefaultClient will be used.
-func NewPlugin(ctx context.Context, socket string, client *http.Client, expectedAPIVersion string, handshakeMetadata map[string]string) *Plugin {
+func NewPlugin(ctx context.Context, socket string, client *http.Client, expectedAPIVersion string, handshakeMetadata map[string]string) (*Plugin, error) {
+	id := strings.TrimSuffix(filepath.Base(socket), filepath.Ext(socket))
+	if !validPluginName.MatchString(id) {
+		return nil, fmt.Errorf("invalid plugin id %q", id)
+	}
+
 	params := url.Values{}
 	for k, v := range handshakeMetadata {
 		params.Add(k, v)
 	}
 
-	id := strings.TrimSuffix(filepath.Base(socket), filepath.Ext(socket))
-
 	ctx, cancel := context.WithCancel(ctx)
-	return &Plugin{
+	plugin := &Plugin{
 		PluginSpec:         xfer.PluginSpec{ID: id, Label: id},
 		context:            ctx,
 		socket:             socket,
@@ -234,6 +244,7 @@ func NewPlugin(ctx context.Context, socket string, client *http.Client, expected
 		client:             client,
 		cancel:             cancel,
 	}
+	return plugin, nil
 }
 
 // Report gets the latest report from the plugin

probe/plugins/registry_internal_test.go

@@ -518,3 +518,55 @@ func TestRegistryRejectsPluginResponsesWhichAreTooLarge(t *testing.T) {
 		{ID: "foo", Label: "foo", Status: `error: response must be shorter than 50MB`},
 	})
 }
+
+func TestRegistryChecksForValidPluginIDs(t *testing.T) {
+	setup(
+		t,
+		mockPlugin{
+			t:       t,
+			Name:    "testPlugin",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"testPlugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+		mockPlugin{
+			t:       t,
+			Name:    "P-L-U-G-I-N",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"P-L-U-G-I-N","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+		mockPlugin{
+			t:       t,
+			Name:    "another-testPlugin",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"another-testPlugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+		mockPlugin{
+			t:       t,
+			Name:    "testPlugin!",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"testPlugin!","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+		mockPlugin{
+			t:       t,
+			Name:    "test~plugin",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"test~plugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+		mockPlugin{
+			t:       t,
+			Name:    "testPlugin-",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"testPlugin-","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+		mockPlugin{
+			t:       t,
+			Name:    "-testPlugin",
+			Handler: stringHandler(http.StatusOK, `{"Plugins":[{"id":"-testPlugin","label":"testPlugin","interfaces":["reporter"],"api_version":"1"}]}`),
+		}.file(),
+	)
+	defer restore(t)
+
+	root := "/plugins"
+	r, err := NewRegistry(root, "1", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+
+	r.Report()
+	checkLoadedPluginIDs(t, r.ForEach, []string{"P-L-U-G-I-N", "another-testPlugin", "testPlugin"})
+}