From d8e3de0cae4662de1252534de6ead8a0af5ba1b6 Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Thu, 5 Dec 2019 04:47:42 -0800
Subject: [PATCH] Ensure comment is always a valid key (#8604)

Also fix LDAP leaky connection
---
 cmd/config/config.go   | 54 +++++++++++++++++++++++-------------------
 cmd/sts-handlers.go    |  3 +++
 pkg/madmin/parse-kv.go | 38 ++++++++++++++++++++---------
 3 files changed, 59 insertions(+), 36 deletions(-)

diff --git a/cmd/config/config.go b/cmd/config/config.go
index 19b558c3f..60d10e142 100644
--- a/cmd/config/config.go
+++ b/cmd/config/config.go
@@ -399,6 +399,12 @@ func LookupRegion(kv KVS) (string, error) {
 func CheckValidKeys(subSys string, kv KVS, validKVS KVS) error {
 	nkv := KVS{}
 	for _, kv := range kv {
+		// Comment is a valid key, its also fully optional
+		// ignore it since it is a valid key for all
+		// sub-systems.
+		if kv.Key == Comment {
+			continue
+		}
 		if _, ok := validKVS.Lookup(kv.Key); !ok {
 			nkv = append(nkv, kv)
 		}
@@ -557,23 +563,19 @@ func (c Config) SetKVS(s string, defaultKVS map[string]KVS) error {
 			continue
 		}
 		if len(kv) == 1 && prevK != "" {
-			kvs = append(kvs, KV{
-				Key: prevK,
-				Value: strings.Join([]string{
-					kvs.Get(prevK),
-					madmin.SanitizeValue(kv[0]),
-				}, KvSpaceSeparator),
-			})
+			value := strings.Join([]string{
+				kvs.Get(prevK),
+				madmin.SanitizeValue(kv[0]),
+			}, KvSpaceSeparator)
+			kvs.Set(prevK, value)
 			continue
 		}
-		if len(kv) == 1 {
-			return Errorf(SafeModeKind, "key '%s', cannot have empty value", kv[0])
+		if len(kv) == 2 {
+			prevK = kv[0]
+			kvs.Set(prevK, madmin.SanitizeValue(kv[1]))
+			continue
 		}
-		prevK = kv[0]
-		kvs = append(kvs, KV{
-			Key:   kv[0],
-			Value: madmin.SanitizeValue(kv[1]),
-		})
+		return Errorf(SafeModeKind, "key '%s', cannot have empty value", kv[0])
 	}
 
 	tgt := Default
@@ -587,25 +589,27 @@ func (c Config) SetKVS(s string, defaultKVS map[string]KVS) error {
 	_, defaultOk := defaultKVS[subSys].Lookup(Enable)
 	if !ok && defaultOk {
 		// implicit state "on" if not specified.
-		kvs = append(kvs, KV{
-			Key:   Enable,
-			Value: EnableOn,
-		})
+		kvs.Set(Enable, EnableOn)
 	}
 
-	currKVS := c[subSys][tgt]
+	currKVS, ok := c[subSys][tgt]
+	if !ok {
+		currKVS = defaultKVS[subSys]
+	}
 
 	for _, kv := range kvs {
+		if kv.Key == Comment {
+			// Skip comment and add it later.
+			continue
+		}
 		currKVS.Set(kv.Key, kv.Value)
 	}
 
-	for _, defaultKV := range defaultKVS[subSys] {
-		_, ok := c[subSys][tgt].Lookup(defaultKV.Key)
-		if !ok {
-			currKVS.Set(defaultKV.Key, defaultKV.Value)
-		}
+	v, ok := kvs.Lookup(Comment)
+	if ok {
+		currKVS.Set(Comment, v)
 	}
 
-	copy(c[subSys][tgt], currKVS)
+	c[subSys][tgt] = currKVS
 	return nil
 }
diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go
index 593a9e6c1..79208beed 100644
--- a/cmd/sts-handlers.go
+++ b/cmd/sts-handlers.go
@@ -484,6 +484,9 @@ func (sts *stsAPIHandlers) AssumeRoleWithLDAPIdentity(w http.ResponseWriter, r *
 		return
 	}
 
+	// Close ldap connection to avoid leaks.
+	defer ldapConn.Close()
+
 	usernameSubs, _ := xldap.NewSubstituter("username", ldapUsername)
 	// We ignore error below as we already validated the username
 	// format string at startup.
diff --git a/pkg/madmin/parse-kv.go b/pkg/madmin/parse-kv.go
index 1a9bc67ce..e75226e8a 100644
--- a/pkg/madmin/parse-kv.go
+++ b/pkg/madmin/parse-kv.go
@@ -40,6 +40,23 @@ func (kvs KVS) Empty() bool {
 	return len(kvs) == 0
 }
 
+// Set sets a value, if not sets a default value.
+func (kvs *KVS) Set(key, value string) {
+	for i, kv := range *kvs {
+		if kv.Key == key {
+			(*kvs)[i] = KV{
+				Key:   key,
+				Value: value,
+			}
+			return
+		}
+	}
+	*kvs = append(*kvs, KV{
+		Key:   key,
+		Value: value,
+	})
+}
+
 // Get - returns the value of a key, if not found returns empty.
 func (kvs KVS) Get(key string) string {
 	v, ok := kvs.Lookup(key)
@@ -174,20 +191,19 @@ func (t *Targets) AddTarget(s string) error {
 			continue
 		}
 		if len(kv) == 1 && prevK != "" {
-			kvs = append(kvs, KV{
-				Key:   prevK,
-				Value: strings.Join([]string{kvs.Get(prevK), SanitizeValue(kv[0])}, KvSpaceSeparator),
-			})
+			value := strings.Join([]string{
+				kvs.Get(prevK),
+				SanitizeValue(kv[0]),
+			}, KvSpaceSeparator)
+			kvs.Set(prevK, value)
 			continue
 		}
-		if len(kv) == 1 {
-			return fmt.Errorf("value for key '%s' cannot be empty", kv[0])
+		if len(kv) == 2 {
+			prevK = kv[0]
+			kvs.Set(prevK, SanitizeValue(kv[1]))
+			continue
 		}
-		prevK = kv[0]
-		kvs = append(kvs, KV{
-			Key:   kv[0],
-			Value: SanitizeValue(kv[1]),
-		})
+		return fmt.Errorf("value for key '%s' cannot be empty", kv[0])
 	}
 
 	for i := range *t {