Better validation of all config file fields (#6090)

Add Validate() to serverConfig to call it at server
startup and in Admin SetConfig handler to minimize
errors scenario after server restart.

cmd/admin-handlers.go

@@ -712,7 +712,7 @@ func (a adminAPIHandlers) SetConfigHandler(w http.ResponseWriter, r *http.Reques
 	err = json.Unmarshal(configBytes, &config)
 	if err != nil {
 		logger.LogIf(ctx, err)
-		writeErrorResponseJSON(w, toAPIErrorCode(err), r.URL)
+		writeCustomErrorResponseJSON(w, ErrAdminConfigBadJSON, err.Error(), r.URL)
 		return
 	}
 
@@ -727,6 +727,11 @@ func (a adminAPIHandlers) SetConfigHandler(w http.ResponseWriter, r *http.Reques
 		}
 	}
 
+	if err := config.Validate(); err != nil {
+		writeCustomErrorResponseJSON(w, ErrAdminConfigBadJSON, err.Error(), r.URL)
+		return
+	}
+
 	// Write config received from request onto a temporary file on
 	// all nodes.
 	tmpFileName := fmt.Sprintf(minioConfigTmpFormat, mustGetUUID())

cmd/admin-handlers_test.go

@@ -38,22 +38,24 @@ import (
 
 var (
 	configJSON = []byte(`{
-	"version": "13",
+	"version": "26",
 	"credential": {
 		"accessKey": "minio",
 		"secretKey": "minio123"
 	},
-	"region": "us-west-1",
-	"logger": {
-		"console": {
-			"enable": true,
-			"level": "fatal"
-		},
-		"file": {
-			"enable": false,
-			"fileName": "",
-			"level": ""
-		}
+	"region": "",
+	"browser": "on",
+	"worm": "off",
+	"domain": "",
+	"storageclass": {
+		"standard": "",
+		"rrs": ""
+	},
+	"cache": {
+		"drives": [],
+		"expiry": 90,
+		"maxuse": 80,
+		"exclude": []
 	},
 	"notify": {
 		"amqp": {
@@ -63,6 +65,7 @@ var (
 				"exchange": "",
 				"routingKey": "",
 				"exchangeType": "",
+				"deliveryMode": 0,
 				"mandatory": false,
 				"immediate": false,
 				"durable": false,
@@ -71,6 +74,47 @@ var (
 				"autoDeleted": false
 			}
 		},
+		"elasticsearch": {
+			"1": {
+				"enable": false,
+				"format": "",
+				"url": "",
+				"index": ""
+			}
+		},
+		"kafka": {
+			"1": {
+				"enable": false,
+				"brokers": null,
+				"topic": ""
+			}
+		},
+		"mqtt": {
+			"1": {
+				"enable": false,
+				"broker": "",
+				"topic": "",
+				"qos": 0,
+				"clientId": "",
+				"username": "",
+				"password": "",
+				"reconnectInterval": 0,
+				"keepAliveInterval": 0
+			}
+		},
+		"mysql": {
+			"1": {
+				"enable": false,
+				"format": "",
+				"dsnString": "",
+				"table": "",
+				"host": "",
+				"port": "",
+				"user": "",
+				"password": "",
+				"database": ""
+			}
+		},
 		"nats": {
 			"1": {
 				"enable": false,
@@ -90,24 +134,10 @@ var (
 				}
 			}
 		},
-		"elasticsearch": {
-			"1": {
-				"enable": false,
-				"url": "",
-				"index": ""
-			}
-		},
-		"redis": {
-			"1": {
-				"enable": false,
-				"address": "",
-				"password": "",
-				"key": ""
-			}
-		},
 		"postgresql": {
 			"1": {
 				"enable": false,
+				"format": "",
 				"connectionString": "",
 				"table": "",
 				"host": "",
@@ -117,11 +147,13 @@ var (
 				"database": ""
 			}
 		},
-		"kafka": {
+		"redis": {
 			"1": {
 				"enable": false,
-				"brokers": null,
-				"topic": ""
+				"format": "",
+				"address": "",
+				"password": "",
+				"key": ""
 			}
 		},
 		"webhook": {

cmd/config-current.go

@@ -23,6 +23,7 @@ import (
 	"reflect"
 	"sync"
 
+	"github.com/miekg/dns"
 	"github.com/minio/minio/cmd/logger"
 
 	"github.com/minio/minio/pkg/auth"
@@ -129,6 +130,84 @@ func (s *serverConfig) GetCacheConfig() CacheConfig {
 	return s.Cache
 }
 
+func (s *serverConfig) Validate() error {
+	if s.Version != serverConfigVersion {
+		return fmt.Errorf("configuration version mismatch. Expected: ‘%s’, Got: ‘%s’", serverConfigVersion, s.Version)
+	}
+
+	// Validate credential fields only when
+	// they are not set via the environment
+	// Error out if global is env credential is not set and config has invalid credential
+	if !globalIsEnvCreds && !s.Credential.IsValid() {
+		return errors.New("invalid credential in config file")
+	}
+
+	// Region: nothing to validate
+	// Browser, Worm, Cache and StorageClass values are already validated during json unmarshal
+
+	if s.Domain != "" {
+		if _, ok := dns.IsDomainName(s.Domain); !ok {
+			return errors.New("invalid domain name")
+		}
+	}
+
+	for _, v := range s.Notify.AMQP {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("amqp: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.Elasticsearch {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("elasticsearch: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.Kafka {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("kafka: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.MQTT {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("mqtt: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.MySQL {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("mysql: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.NATS {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("nats: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.PostgreSQL {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("postgreSQL: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.Redis {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("redis: %s", err.Error())
+		}
+	}
+
+	for _, v := range s.Notify.Webhook {
+		if err := v.Validate(); err != nil {
+			return fmt.Errorf("webhook: %s", err.Error())
+		}
+	}
+
+	return nil
+}
+
 // Save config file to corresponding backend
 func Save(configFile string, data interface{}) error {
 	return quick.SaveConfig(data, configFile, globalEtcdClient)
@@ -318,15 +397,8 @@ func getValidConfig() (*serverConfig, error) {
 		return nil, err
 	}
 
-	if srvCfg.Version != serverConfigVersion {
-		return nil, fmt.Errorf("configuration version mismatch. Expected: ‘%s’, Got: ‘%s’", serverConfigVersion, srvCfg.Version)
-	}
-
-	// Validate credential fields only when
-	// they are not set via the environment
-	// Error out if global is env credential is not set and config has invalid credential
-	if !globalIsEnvCreds && !srvCfg.Credential.IsValid() {
-		return nil, errors.New("invalid credential in config file " + getConfigFile())
+	if err = srvCfg.Validate(); err != nil {
+		return nil, err
 	}
 
 	return srvCfg, nil

cmd/config-current_test.go

@@ -174,55 +174,55 @@ func TestValidateConfig(t *testing.T) {
 		{`{"version": "` + v + `", "browser": "on", "browser": "on", "region":"us-east-1", "credential" : {"accessKey":"minio", "secretKey":"minio123"}}`, false},
 
 		// Test 11 - 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 }}}}`, true},
+		{`{"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 12 - 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 } } }}}`, true},
+		{`{"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 13 - Test ElasticSearch
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "url": "", "index": "" } }}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "url": "", "index": "" } }}}`, false},
 
 		// Test 14 - Test Redis
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "address": "", "password": "", "key": "" } }}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "address": "", "password": "", "key": "" } }}}`, false},
 
 		// Test 15 - 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": "" }}}}`, true},
+		{`{"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 16 - Test Kafka
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "kafka": { "1": { "enable": true, "brokers": null, "topic": "" } }}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "kafka": { "1": { "enable": true, "brokers": null, "topic": "" } }}}`, false},
 
 		// Test 17 - Test Webhook
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "webhook": { "1": { "enable": true, "endpoint": "" } }}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "webhook": { "1": { "enable": true, "endpoint": "" } }}}`, false},
 
 		// Test 18 - Test MySQL
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mysql": { "1": { "enable": true, "dsnString": "",  "table": "", "host": "", "port": "", "user": "", "password": "", "database": "" }}}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mysql": { "1": { "enable": true, "dsnString": "",  "table": "", "host": "", "port": "", "user": "", "password": "", "database": "" }}}}`, false},
 
 		// Test 19 - Test Format for MySQL
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mysql": { "1": { "enable": true, "dsnString": "",  "format": "invalid", "table": "xxx", "host": "10.0.0.1", "port": "3306", "user": "abc", "password": "pqr", "database": "test1" }}}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mysql": { "1": { "enable": true, "dsnString": "",  "format": "invalid", "table": "xxx", "host": "10.0.0.1", "port": "3306", "user": "abc", "password": "pqr", "database": "test1" }}}}`, false},
 
 		// Test 20 - Test valid Format for MySQL
 		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mysql": { "1": { "enable": true, "dsnString": "",  "format": "namespace", "table": "xxx", "host": "10.0.0.1", "port": "3306", "user": "abc", "password": "pqr", "database": "test1" }}}}`, true},
 
 		// Test 21 - Test Format for PostgreSQL
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "postgresql": { "1": { "enable": true, "connectionString": "", "format": "invalid", "table": "xxx", "host": "myhost", "port": "5432", "user": "abc", "password": "pqr", "database": "test1" }}}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "postgresql": { "1": { "enable": true, "connectionString": "", "format": "invalid", "table": "xxx", "host": "myhost", "port": "5432", "user": "abc", "password": "pqr", "database": "test1" }}}}`, false},
 
 		// Test 22 - Test valid Format for PostgreSQL
 		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "postgresql": { "1": { "enable": true, "connectionString": "", "format": "namespace", "table": "xxx", "host": "myhost", "port": "5432", "user": "abc", "password": "pqr", "database": "test1" }}}}`, true},
 
 		// Test 23 - Test Format for ElasticSearch
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "format": "invalid", "url": "example.com", "index": "myindex" } }}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "format": "invalid", "url": "example.com", "index": "myindex" } }}}`, false},
 
 		// Test 24 - Test valid Format for ElasticSearch
 		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "format": "namespace", "url": "example.com", "index": "myindex" } }}}`, true},
 
 		// Test 25 - Test Format for Redis
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "invalid", "address": "example.com:80", "password": "xxx", "key": "key1" } }}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "invalid", "address": "example.com:80", "password": "xxx", "key": "key1" } }}}`, false},
 
 		// Test 26 - Test valid Format for Redis
 		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "namespace", "address": "example.com:80", "password": "xxx", "key": "key1" } }}}`, true},
 
 		// Test 27 - Test MQTT
-		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mqtt": { "1": { "enable": true, "broker": "",  "topic": "", "qos": 0, "clientId": "", "username": "", "password": ""}}}}`, true},
+		{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "mqtt": { "1": { "enable": true, "broker": "",  "topic": "", "qos": 0, "clientId": "", "username": "", "password": ""}}}}`, false},
 	}
 
 	for i, testCase := range testCases {

cmd/disk-cache-config.go

@@ -18,6 +18,7 @@ package cmd
 
 import (
 	"encoding/json"
+	"errors"
 	"path/filepath"
 	"strings"
 
@@ -44,6 +45,15 @@ func (cfg *CacheConfig) UnmarshalJSON(data []byte) (err error) {
 	if err = json.Unmarshal(data, _cfg); err != nil {
 		return err
 	}
+
+	if _cfg.Expiry < 0 {
+		return errors.New("config expiry value should not be negative")
+	}
+
+	if _cfg.MaxUse < 0 {
+		return errors.New("config max use value should not be null or negative")
+	}
+
 	if _, err = parseCacheDrives(_cfg.Drives); err != nil {
 		return err
 	}