config: Check for duplicated entries in all scopes (#3872)

Validate Minio config by checking if there is double json key
in any scope level. The returned error contains the json path
to the duplicated key.

cmd/config-v14.go

@@ -18,11 +18,13 @@ package cmd
 
 import (
 	"errors"
+	"io/ioutil"
 	"os"
 	"strings"
 	"sync"
 
 	"github.com/minio/minio/pkg/quick"
+	"github.com/tidwall/gjson"
 )
 
 // Read Write mutex for safe access to ServerConfig.
@@ -158,6 +160,116 @@ func loadConfig(envParams envParams) error {
 	return nil
 }
 
+// doCheckDupJSONKeys recursively detects duplicate json keys
+func doCheckDupJSONKeys(key, value gjson.Result) error {
+	// Key occurrences map of the current scope to count
+	// if there is any duplicated json key.
+	keysOcc := make(map[string]int)
+
+	// Holds the found error
+	var checkErr error
+
+	// Iterate over keys in the current json scope
+	value.ForEach(func(k, v gjson.Result) bool {
+		// If current key is not null, check if its
+		// value contains some duplicated keys.
+		if k.Type != gjson.Null {
+			keysOcc[k.String()]++
+			checkErr = doCheckDupJSONKeys(k, v)
+		}
+		return checkErr == nil
+	})
+
+	// Check found err
+	if checkErr != nil {
+		return errors.New(key.String() + " => " + checkErr.Error())
+	}
+
+	// Check for duplicated keys
+	for k, v := range keysOcc {
+		if v > 1 {
+			return errors.New(key.String() + " => `" + k + "` entry is duplicated")
+		}
+	}
+
+	return nil
+}
+
+// Check recursively if a key is duplicated in the same json scope
+// e.g.:
+//  `{ "key" : { "key" ..` is accepted
+//  `{ "key" : { "subkey" : "val1", "subkey": "val2" ..` throws subkey duplicated error
+func checkDupJSONKeys(json string) error {
+	// Parse config with gjson library
+	config := gjson.Parse(json)
+
+	// Create a fake rootKey since root json doesn't seem to have representation
+	// in gjson library.
+	rootKey := gjson.Result{Type: gjson.String, Str: minioConfigFile}
+
+	// Check if loaded json contains any duplicated keys
+	return doCheckDupJSONKeys(rootKey, config)
+}
+
+// validateConfig checks for
+func validateConfig() error {
+
+	// Get file config path
+	configFile := getConfigFile()
+
+	srvCfg := &serverConfigV14{}
+
+	// Load config file
+	qc, err := quick.New(srvCfg)
+	if err != nil {
+		return err
+	}
+	if err = qc.Load(configFile); err != nil {
+		return err
+	}
+
+	// Check if config version is valid
+	if srvCfg.GetVersion() != v14 {
+		return errors.New("bad config version, expected: " + v14)
+	}
+
+	// Load config file json and check for duplication json keys
+	jsonBytes, err := ioutil.ReadFile(configFile)
+	if err != nil {
+		return err
+	}
+	if err := checkDupJSONKeys(string(jsonBytes)); err != nil {
+		return err
+	}
+
+	// Validate region field
+	if srvCfg.GetRegion() == "" {
+		return errors.New("region config is empty")
+	}
+
+	// Validate browser field
+	if b := strings.ToLower(srvCfg.GetBrowser()); b != "on" && b != "off" {
+		return errors.New("invalid browser config")
+	}
+
+	// Validate credential field
+	if err := srvCfg.Credential.Validate(); err != nil {
+		return err
+	}
+
+	// Validate logger field
+	if err := srvCfg.Logger.Validate(); err != nil {
+		return err
+	}
+
+	// Validate notify field
+	if err := srvCfg.Notify.Validate(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // serverConfig server config.
 var serverConfig *serverConfigV14
 

cmd/config-v14_test.go

@@ -17,9 +17,13 @@
 package cmd
 
 import (
+	"io/ioutil"
 	"os"
+	"path/filepath"
 	"reflect"
 	"testing"
+
+	"github.com/tidwall/gjson"
 )
 
 func TestServerConfig(t *testing.T) {
@@ -167,3 +171,117 @@ func TestServerConfigWithEnvs(t *testing.T) {
 		t.Errorf("Expecting access key to be `minio123` found %s", cred.SecretKey)
 	}
 }
+
+func TestCheckDupJSONKeys(t *testing.T) {
+	testCases := []struct {
+		json       string
+		shouldPass bool
+	}{
+		{`{}`, true},
+		{`{"version" : "13"}`, true},
+		{`{"version" : "13", "version": "14"}`, false},
+		{`{"version" : "13", "credential": {"accessKey": "12345"}}`, true},
+		{`{"version" : "13", "credential": {"accessKey": "12345", "accessKey":"12345"}}`, false},
+		{`{"version" : "13", "notify": {"amqp": {"1"}, "webhook":{"3"}}}`, true},
+		{`{"version" : "13", "notify": {"amqp": {"1"}, "amqp":{"3"}}}`, false},
+		{`{"version" : "13", "notify": {"amqp": {"1":{}, "2":{}}}}`, true},
+		{`{"version" : "13", "notify": {"amqp": {"1":{}, "1":{}}}}`, false},
+	}
+
+	for i, testCase := range testCases {
+		err := doCheckDupJSONKeys(gjson.Result{}, gjson.Parse(testCase.json))
+		if testCase.shouldPass && err != nil {
+			t.Errorf("Test %d, should pass but it failed with err = %v", i+1, err)
+		}
+		if !testCase.shouldPass && err == nil {
+			t.Errorf("Test %d, should fail but it succeed.", i+1)
+		}
+	}
+
+}
+
+func TestValidateConfig(t *testing.T) {
+	rootPath, err := newTestConfig(globalMinioDefaultRegion)
+	if err != nil {
+		t.Fatalf("Init Test config failed")
+	}
+	// remove the root directory after the test ends.
+	defer removeAll(rootPath)
+
+	configPath := filepath.Join(rootPath, minioConfigFile)
+
+	v := v14
+
+	testCases := []struct {
+		configData string
+		shouldPass bool
+	}{
+		// Test 1 - wrong json
+		{`{`, false},
+
+		// Test 2 - empty json
+		{`{}`, false},
+
+		// Test 3 - wrong config version
+		{`{"version": "10"}`, false},
+
+		// Test 4 - wrong browser parameter
+		{`{"version": "` + v + `", "browser": "foo"}`, false},
+
+		// Test 5 - missing credential
+		{`{"version": "` + v + `", "browser": "on"}`, false},
+
+		// Test 6 - missing secret key
+		{`{"version": "` + v + `", "browser": "on", "credential" : {"accessKey":"minio", "secretKey":""}}`, false},
+
+		// Test 7 - missing region
+		{`{"version": "` + v + `", "browser": "on", "credential" : {"accessKey":"minio", "secretKey":"minio123"}}`, false},
+
+		// Test 8 - success
+		{`{"version": "` + v + `", "browser": "on", "region":"us-east-1", "credential" : {"accessKey":"minio", "secretKey":"minio123"}}`, true},
+
+		// Test 9 - duplicated json keys
+		{`{"version": "` + v + `", "browser": "on", "browser": "on", "region":"us-east-1", "credential" : {"accessKey":"minio", "secretKey":"minio123"}}`, false},
+
+		// Test 10 - Wrong Console logger level
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "logger": { "console": { "enable": true, "level": "foo" } }}`, false},
+
+		// Test 11 - Wrong File logger level
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "logger": { "file": { "enable": true, "level": "foo" } }}`, false},
+
+		// Test 12 - Test AMQP
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "amqp": { "1": { "enable": true, "url": "", "exchange": "", "routingKey": "", "exchangeType": "", "mandatory": false, "immediate": false, "durable": false, "internal": false, "noWait": false, "autoDeleted": false }}}}`, false},
+
+		// Test 13 - Test NATS
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "nats": { "1": { "enable": true, "address": "", "subject": "", "username": "", "password": "", "token": "", "secure": false, "pingInterval": 0, "streaming": { "enable": false, "clusterID": "", "clientID": "", "async": false, "maxPubAcksInflight": 0 } } }}}`, false},
+
+		// Test 14 - Test ElasticSearch
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "url": "", "index": "" } }}}`, false},
+
+		// Test 15 - Test Redis
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "address": "", "password": "", "key": "" } }}}`, false},
+
+		// Test 16 - Test PostgreSQL
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "postgresql": { "1": { "enable": true, "connectionString": "", "table": "", "host": "", "port": "", "user": "", "password": "", "database": "" }}}}`, false},
+
+		// Test 17 - Test Kafka
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "kafka": { "1": { "enable": true, "brokers": null, "topic": "" } }}}`, false},
+
+		// Test 18 - Test Webhook
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "webhook": { "1": { "enable": true, "endpoint": "" } }}}`, false},
+	}
+
+	for i, testCase := range testCases {
+		if err := ioutil.WriteFile(configPath, []byte(testCase.configData), 0700); err != nil {
+			t.Error(err)
+		}
+		err := validateConfig()
+		if testCase.shouldPass && err != nil {
+			t.Errorf("Test %d, should pass but it failed with err = %v", i+1, err)
+		}
+		if !testCase.shouldPass && err == nil {
+			t.Errorf("Test %d, should fail but it succeed.", i+1)
+		}
+	}
+
+}

cmd/credential.go

@@ -19,6 +19,7 @@ package cmd
 import (
 	"crypto/rand"
 	"encoding/base64"
+	"errors"
 	"os"
 
 	"github.com/minio/mc/pkg/console"
@@ -75,6 +76,16 @@ type credential struct {
 	secretKeyHash []byte
 }
 
+func (c *credential) Validate() error {
+	if !isAccessKeyValid(c.AccessKey) {
+		return errors.New("Invalid access key")
+	}
+	if !isSecretKeyValid(c.SecretKey) {
+		return errors.New("Invalid secret key")
+	}
+	return nil
+}
+
 // Generate a bcrypt hashed key for input secret key.
 func mustGetHashedSecretKey(secretKey string) []byte {
 	hashedSecretKey, err := bcrypt.GenerateFromPassword([]byte(secretKey), bcrypt.DefaultCost)

cmd/logger-console-hook.go

@@ -16,7 +16,12 @@
 
 package cmd
 
-import "github.com/Sirupsen/logrus"
+import (
+	"fmt"
+	"strings"
+
+	"github.com/Sirupsen/logrus"
+)
 
 // consoleLogger - default logger if not other logging is enabled.
 type consoleLogger struct {
@@ -24,6 +29,14 @@ type consoleLogger struct {
 	Level  string `json:"level"`
 }
 
+func (c *consoleLogger) Validate() error {
+	level := strings.ToLower(c.Level)
+	if level != "error" && level != "fatal" && level != "" {
+		return fmt.Errorf("`%s` level value not recognized", c.Level)
+	}
+	return nil
+}
+
 // enable console logger.
 func enableConsoleLogger() {
 	clogger := serverConfig.Logger.GetConsole()

cmd/logger-file-hook.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"strings"
 
 	"github.com/Sirupsen/logrus"
 )
@@ -30,6 +31,14 @@ type fileLogger struct {
 	Level    string `json:"level"`
 }
 
+func (f *fileLogger) Validate() error {
+	level := strings.ToLower(f.Level)
+	if level != "error" && level != "fatal" && level != "" {
+		return fmt.Errorf("`%s` level value not recognized", f.Level)
+	}
+	return nil
+}
+
 type localFile struct {
 	*os.File
 }

cmd/logger.go

@@ -47,6 +47,20 @@ type logger struct {
 
 /// Logger related.
 
+// Validate logger contents
+func (l *logger) Validate() error {
+	if l == nil {
+		return nil
+	}
+	if err := l.Console.Validate(); err != nil {
+		return fmt.Errorf("`Console` field: %s", err.Error())
+	}
+	if err := l.File.Validate(); err != nil {
+		return fmt.Errorf("`File` field: %s", err.Error())
+	}
+	return nil
+}
+
 // SetFile set new file logger.
 func (l *logger) SetFile(flogger fileLogger) {
 	l.Lock()

cmd/notifier-config.go

@@ -16,7 +16,10 @@
 
 package cmd
 
-import "sync"
+import (
+	"fmt"
+	"sync"
+)
 
 // Notifier represents collection of supported notification queues.
 type notifier struct {
@@ -41,6 +44,15 @@ func (a amqpConfigs) Clone() amqpConfigs {
 	return a2
 }
 
+func (a amqpConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("AMQP [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
 type natsConfigs map[string]natsNotify
 
 func (a natsConfigs) Clone() natsConfigs {
@@ -51,6 +63,15 @@ func (a natsConfigs) Clone() natsConfigs {
 	return a2
 }
 
+func (a natsConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("NATS [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
 type elasticSearchConfigs map[string]elasticSearchNotify
 
 func (a elasticSearchConfigs) Clone() elasticSearchConfigs {
@@ -61,6 +82,15 @@ func (a elasticSearchConfigs) Clone() elasticSearchConfigs {
 	return a2
 }
 
+func (a elasticSearchConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("ElasticSearch [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
 type redisConfigs map[string]redisNotify
 
 func (a redisConfigs) Clone() redisConfigs {
@@ -71,6 +101,15 @@ func (a redisConfigs) Clone() redisConfigs {
 	return a2
 }
 
+func (a redisConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("Redis [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
 type postgreSQLConfigs map[string]postgreSQLNotify
 
 func (a postgreSQLConfigs) Clone() postgreSQLConfigs {
@@ -81,6 +120,15 @@ func (a postgreSQLConfigs) Clone() postgreSQLConfigs {
 	return a2
 }
 
+func (a postgreSQLConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("PostgreSQL [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
 type kafkaConfigs map[string]kafkaNotify
 
 func (a kafkaConfigs) Clone() kafkaConfigs {
@@ -91,6 +139,15 @@ func (a kafkaConfigs) Clone() kafkaConfigs {
 	return a2
 }
 
+func (a kafkaConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("Kafka [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
 type webhookConfigs map[string]webhookNotify
 
 func (a webhookConfigs) Clone() webhookConfigs {
@@ -101,6 +158,43 @@ func (a webhookConfigs) Clone() webhookConfigs {
 	return a2
 }
 
+func (a webhookConfigs) Validate() error {
+	for k, v := range a {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("Webhook [%s] configuration invalid: %s", k, err.Error())
+		}
+	}
+	return nil
+}
+
+func (n *notifier) Validate() error {
+	if n == nil {
+		return nil
+	}
+	if err := n.AMQP.Validate(); err != nil {
+		return err
+	}
+	if err := n.NATS.Validate(); err != nil {
+		return err
+	}
+	if err := n.ElasticSearch.Validate(); err != nil {
+		return err
+	}
+	if err := n.Redis.Validate(); err != nil {
+		return err
+	}
+	if err := n.PostgreSQL.Validate(); err != nil {
+		return err
+	}
+	if err := n.Kafka.Validate(); err != nil {
+		return err
+	}
+	if err := n.Webhook.Validate(); err != nil {
+		return err
+	}
+	return nil
+}
+
 func (n *notifier) SetAMQPByID(accountID string, amqpn amqpNotify) {
 	n.Lock()
 	defer n.Unlock()

cmd/notify-amqp.go

@@ -40,6 +40,16 @@ type amqpNotify struct {
 	AutoDeleted  bool   `json:"autoDeleted"`
 }
 
+func (a *amqpNotify) Validate() error {
+	if !a.Enable {
+		return nil
+	}
+	if _, err := checkNetURL(a.URL); err != nil {
+		return err
+	}
+	return nil
+}
+
 type amqpConn struct {
 	params amqpNotify
 	*amqp.Connection

cmd/notify-elasticsearch.go

@@ -33,6 +33,16 @@ type elasticSearchNotify struct {
 	Index  string `json:"index"`
 }
 
+func (e *elasticSearchNotify) Validate() error {
+	if !e.Enable {
+		return nil
+	}
+	if _, err := checkNetURL(e.URL); err != nil {
+		return err
+	}
+	return nil
+}
+
 type elasticClient struct {
 	*elastic.Client
 	params elasticSearchNotify

cmd/notify-kafka.go

@@ -17,6 +17,7 @@
 package cmd
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 
@@ -39,6 +40,16 @@ type kafkaNotify struct {
 	Topic string `json:"topic"`
 }
 
+func (k *kafkaNotify) Validate() error {
+	if !k.Enable {
+		return nil
+	}
+	if len(k.Brokers) == 0 {
+		return errors.New("No broker specified")
+	}
+	return nil
+}
+
 // kafkaConn contains the active connection to the Kafka cluster and
 // the topic to send event notifications to.
 type kafkaConn struct {

cmd/notify-nats.go

@@ -48,6 +48,16 @@ type natsNotify struct {
 	Streaming    natsNotifyStreaming `json:"streaming"`
 }
 
+func (n *natsNotify) Validate() error {
+	if !n.Enable {
+		return nil
+	}
+	if _, err := checkNetURL(n.Address); err != nil {
+		return err
+	}
+	return nil
+}
+
 // natsIOConn abstracts connection to any type of NATS server
 type natsIOConn struct {
 	params   natsNotify

cmd/notify-postgresql.go

@@ -84,6 +84,16 @@ type postgreSQLNotify struct {
 	Database string `json:"database"`
 }
 
+func (p *postgreSQLNotify) Validate() error {
+	if !p.Enable {
+		return nil
+	}
+	if _, err := checkNetURL(p.Host); err != nil {
+		return err
+	}
+	return nil
+}
+
 type pgConn struct {
 	connStr       string
 	table         string

cmd/notify-redis.go

@@ -32,6 +32,16 @@ type redisNotify struct {
 	Key      string `json:"key"`
 }
 
+func (r *redisNotify) Validate() error {
+	if !r.Enable {
+		return nil
+	}
+	if _, err := checkNetURL(r.Addr); err != nil {
+		return err
+	}
+	return nil
+}
+
 type redisConn struct {
 	*redis.Pool
 	params redisNotify

cmd/notify-webhook.go

@@ -32,6 +32,16 @@ type webhookNotify struct {
 	Endpoint string `json:"endpoint"`
 }
 
+func (w *webhookNotify) Validate() error {
+	if !w.Enable {
+		return nil
+	}
+	if _, err := checkNetURL(w.Endpoint); err != nil {
+		return err
+	}
+	return nil
+}
+
 type httpConn struct {
 	*http.Client
 	Endpoint string

cmd/server-main.go

@@ -127,7 +127,7 @@ func initConfig() {
 	// Config file does not exist, we create it fresh and return upon success.
 	if !isConfigFileExists() {
 		if err := newConfig(envs); err != nil {
-			console.Fatalf("Unable to initialize minio config for the first time. Err: %s.\n", err)
+			console.Fatalf("Unable to initialize minio config for the first time. Error: %s.\n", err)
 		}
 		console.Println("Created minio configuration file successfully at " + getConfigDir())
 		return
@@ -136,9 +136,14 @@ func initConfig() {
 	// Migrate any old version of config / state files to newer format.
 	migrate()
 
+	// Validate config file
+	if err := validateConfig(); err != nil {
+		console.Fatalf("Cannot validate configuration file. Error: %s\n", err)
+	}
+
 	// Once we have migrated all the old config, now load them.
 	if err := loadConfig(envs); err != nil {
-		console.Fatalf("Unable to initialize minio config. Err: %s.\n", err)
+		console.Fatalf("Unable to initialize minio config. Error: %s.\n", err)
 	}
 }
 

cmd/utils.go

@@ -285,3 +285,16 @@ func isFile(path string) bool {
 
 	return false
 }
+
+// checkNetURL - checks if passed address correspond
+// to a network address (and not file system path)
+func checkNetURL(address string) (*url.URL, error) {
+	u, err := url.Parse(address)
+	if err != nil {
+		return nil, fmt.Errorf("`%s` invalid: %s", address, err.Error())
+	}
+	if u.Host == "" {
+		return nil, fmt.Errorf("`%s` invalid network URL", address)
+	}
+	return u, nil
+}