Hopefully, fix most case-sensitivity issues

- DNs are always used in canonical form: lowercase, no spaces. This is
  how they are internally handled and stored in paths and fields such as
  member and memberof
- Attribute names now can have any combination of lower/uppercase and
  stuff should work
- When modifying an attribute with a name that hase a different
  lower/upper combination than the previously stored value, keep the
  previous attribute name
- Trim spaces from values and do not store empty values

TODO.md

@@ -1,6 +1,2 @@
-- Switch to `go mod` for building Bottin
-
 - Implement missing search filters (in applyFilter)
 - Add an initial prefix to the consul key value
-
-- Potential bugs with different combinations of lower/uppercase names

bottin.hcl.example

@@ -12,7 +12,7 @@ job "directory" {
     task "server" {
       driver = "docker"
       config {
-        image = "lxpz/bottin_amd64:13"
+        image = "lxpz/bottin_amd64:14"
         readonly_rootfs = true
         port_map {
           ldap_port = 389

main.go

@@ -10,6 +10,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/signal"
+	"strings"
 	"syscall"
 
 	ldap "bottin/ldapserver"
@@ -19,15 +20,18 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-const ATTR_USERPASSWORD = "userpassword"
-const ATTR_MEMBER = "member"
+// System managed attributes (cannot be changed by user, see checkRestrictedAttr)
 const ATTR_MEMBEROF = "memberof"
 const ATTR_ENTRYUUID = "entryuuid"
 const ATTR_CREATORSNAME = "creatorsname"
 const ATTR_CREATETIMESTAMP = "createtimestamp"
 const ATTR_MODIFIERSNAME = "modifiersname"
 const ATTR_MODIFYTIMESTAMP = "modifytimestamp"
+
+// Attributes that we are interested in at various points
 const ATTR_OBJECTCLASS = "objectclass"
+const ATTR_MEMBER = "member"
+const ATTR_USERPASSWORD = "userpassword"
 
 type ConfigFile struct {
 	Suffix        string   `json:"suffix"`
@@ -260,20 +264,27 @@ func (server *Server) newUserState() ldap.UserState {
 }
 
 func (server *Server) init() error {
-	path, err := dnToConsul(server.config.Suffix)
+	// Check that suffix is in canonical format in config file
+	suffix_canonical, err := server.checkDN(server.config.Suffix, false)
 	if err != nil {
 		return err
 	}
+	if suffix_canonical != server.config.Suffix {
+		return fmt.Errorf("Please write suffix in canonical format: %s", suffix_canonical)
+	}
 
-	pair, _, err := server.kv.Get(path+"/attribute="+ATTR_OBJECTCLASS, nil)
+	// Check that root object exists.
+	// If it does, we're done. Otherwise, we have some initialization to do.
+	exists, err := server.objectExists(server.config.Suffix)
 	if err != nil {
 		return err
 	}
-
-	if pair != nil {
+	if exists {
 		return nil
 	}
 
+	// We have to initialize the server.
+	// Create a root object and an admin object.
 	base_attributes := Entry{
 		ATTR_OBJECTCLASS:        []string{"top", "dcObject", "organization"},
 		"structuralobjectclass": []string{"organization"},
@@ -333,16 +344,27 @@ func (server *Server) addElements(dn string, attrs Entry) error {
 		return err
 	}
 
-	for k, v := range attrs {
+	for k, valuesNC := range attrs {
 		path := prefix + "/attribute=" + k
-		if len(v) == 0 {
+
+		// Trim spaces and remove empty values
+		values := []string{}
+		for _, v := range valuesNC {
+			vv := strings.TrimSpace(v)
+			if len(vv) > 0 {
+				values = append(values, vv)
+			}
+		}
+
 		// If we have zero values, delete associated k/v pair
+		// Otherwise, write new values
+		if len(values) == 0 {
 			_, err := server.kv.Delete(path, nil)
 			if err != nil {
 				return err
 			}
 		} else {
-			json, err := json.Marshal(v)
+			json, err := json.Marshal(values)
 			if err != nil {
 				return err
 			}
@@ -362,16 +384,23 @@ func (server *Server) getAttribute(dn string, attr string) ([]string, error) {
 		return nil, err
 	}
 
-	pair, _, err := server.kv.Get(path+"/attribute="+attr, nil)
+	pairs, _, err := server.kv.List(path+"/attribute=", nil)
 	if err != nil {
 		return nil, err
 	}
 
-	if pair == nil {
-		return []string{}, nil
+	values := []string{}
+	for _, pair := range pairs {
+		if strings.EqualFold(pair.Key, path+"/attribute="+attr) {
+			newVals, err := parseValue(pair.Value)
+			if err != nil {
+				return nil, err
+			}
+			values = append(values, newVals...)
+		}
 	}
 
-	return parseValue(pair.Value)
+	return values, nil
 }
 
 func (server *Server) objectExists(dn string) (bool, error) {
@@ -388,7 +417,7 @@ func (server *Server) objectExists(dn string) (bool, error) {
 }
 
 func (server *Server) checkDN(dn string, allow_extend bool) (string, error) {
-	// 1. Canonicalize: remove spaces between things
+	// 1. Canonicalize: remove spaces between things and put all in lower case
 	dn, err := canonicalDN(dn)
 	if err != nil {
 		return "", err

util.go

@@ -98,27 +98,31 @@ func parseDN(dn string) ([]DNComponent, error) {
 			return nil, fmt.Errorf("Wrong DN component: %s (expected type=value)", rdn)
 		}
 		ret = append(ret, DNComponent{
-			Type:  strings.TrimSpace(splits[0]),
-			Value: strings.TrimSpace(splits[1]),
+			Type:  strings.ToLower(strings.TrimSpace(splits[0])),
+			Value: strings.ToLower(strings.TrimSpace(splits[1])),
 		})
 	}
 	return ret, nil
 }
 
+func unparseDN(path []DNComponent) string {
+	ret := ""
+	for _, c := range path {
+		if ret != "" {
+			ret = ret + ","
+		}
+		ret = ret + c.Type + "=" + c.Value
+	}
+	return ret
+}
+
 func canonicalDN(dn string) (string, error) {
 	path, err := parseDN(dn)
 	if err != nil {
 		return "", err
 	}
 
-	ret := ""
-	for _, c := range path {
-		if ret != "" {
-			ret = ret + ","
-		}
-		ret = ret + c.Type + "=" + c.Value
-	}
-	return ret, nil
+	return unparseDN(path), nil
 }
 
 func checkRestrictedAttr(attr string) error {
@@ -162,3 +166,12 @@ func valueMatch(attr, val1, val2 string) bool {
 		return strings.EqualFold(val1, val2)
 	}
 }
+
+func listContains(list []string, key string) bool {
+	for _, v := range list {
+		if key == v {
+			return true
+		}
+	}
+	return false
+}

write.go

@@ -58,7 +58,16 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in
 		return ldap.LDAPResultEntryAlreadyExists, nil
 	}
 
-	// TODO: check that parent object exists
+	// Check that parent object exists
+	parentDn := unparseDN(dnSplit[1:])
+	parentExists, err := server.objectExists(parentDn)
+	if err != nil {
+		return ldap.LDAPResultOperationsError, err
+	}
+	if !parentExists {
+		return ldap.LDAPResultNoSuchObject, fmt.Errorf(
+			"Parent object %s does not exist", parentDn)
+	}
 
 	// If adding a group, track of who the members will be so that their memberOf field can be updated later
 	members := []string{}
@@ -77,8 +86,9 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in
 		if err != nil {
 			return ldap.LDAPResultObjectClassViolation, err
 		}
-		// If they are writing a member key, we have to check they are adding valid members
 		if strings.EqualFold(key, ATTR_MEMBER) {
+			// If they are writing a member list, we have to check they are adding valid members
+			// Also, rewrite member list to use canonical DN syntax (no spaces, all lowercase)
 			for _, member := range vals_str {
 				member_canonical, err := server.checkDN(member, false)
 				if err != nil {
@@ -93,19 +103,31 @@ func (server *Server) handleAddInternal(state *State, r *message.AddRequest) (in
 						"Cannot add %s to members, it does not exist!",
 						member_canonical)
 				}
+				members = append(members, member_canonical)
 			}
-			members = append(members, vals_str...)
-		}
+			entry[key] = members
+		} else {
 			if prev, ok := entry[key]; ok {
 				entry[key] = append(prev, vals_str...)
 			} else {
 				entry[key] = vals_str
 			}
 		}
+	}
 
-	if _, ok := entry[ATTR_OBJECTCLASS]; !ok {
+	// Ensure object has at least one objectclass value
+	hasObjectClass := false
+	for k := range entry {
+		if strings.EqualFold(k, ATTR_OBJECTCLASS) {
+			hasObjectClass = true
+			break
+		}
+	}
+	if !hasObjectClass {
 		entry[ATTR_OBJECTCLASS] = []string{"top"}
 	}
+
+	// Write system attributes
 	entry[ATTR_CREATORSNAME] = []string{state.login.user}
 	entry[ATTR_CREATETIMESTAMP] = []string{genTimestamp()}
 	entry[ATTR_ENTRYUUID] = []string{genUuid()}
@@ -306,11 +328,25 @@ func (server *Server) handleModifyInternal(state *State, r *message.ModifyReques
 	addMembers, delMembers := []string{}, []string{}
 
 	// Produce new entry values to be saved
-	newEntry := Entry{}
+	entry := Entry{}
+
 	for _, change := range r.Changes() {
 		attr := string(change.Modification().Type_())
-		values := change.Modification().Vals()
+		changeValues := []string{}
+		for _, v := range change.Modification().Vals() {
+			changeValues = append(changeValues, string(v))
+		}
 
+		// If we already had an attribute with this name before,
+		// make sure we are using the same lowercase/uppercase
+		for prevAttr := range prevEntry {
+			if strings.EqualFold(attr, prevAttr) {
+				attr = prevAttr
+				break
+			}
+		}
+
+		// Check that this attribute is not system-managed thus restricted
 		err = checkRestrictedAttr(attr)
 		if err != nil {
 			return ldap.LDAPResultObjectClassViolation, err
@@ -326,112 +362,100 @@ func (server *Server) handleModifyInternal(state *State, r *message.ModifyReques
 			return ldap.LDAPResultInsufficientAccessRights, nil
 		}
 
-		if change.Operation() == ldap.ModifyRequestChangeOperationAdd {
-			newEntry[attr] = prevEntry[attr]
-			for _, val := range values {
-				present := false
-				for _, prevVal := range newEntry[attr] {
-					if prevVal == string(val) {
-						present = true
-						break
-					}
-				}
-				if !present {
-					newEntry[attr] = append(newEntry[attr], string(val))
+		// If we are changing ATTR_MEMBER, rewrite all values to canonical form
 		if strings.EqualFold(attr, ATTR_MEMBER) {
-						addMembers = append(addMembers, string(val))
+			for i := range changeValues {
+				canonical_val, err := server.checkDN(changeValues[i], false)
+				if err != nil {
+					return ldap.LDAPResultInvalidDNSyntax, err
+				}
+				changeValues[i] = canonical_val
+			}
+		}
+
+		// If we don't yet have a new value for this attr,
+		// but one existed before, initialize entry[attr] to the old value
+		// so that later on what we do is simply modify entry[attr] in place
+		// (this allows to handle sequences of several changes on the same attr)
+		if _, ok := entry[attr]; !ok {
+			if _, ok := prevEntry[attr]; ok {
+				entry[attr] = prevEntry[attr]
+			}
+		}
+
+		// Apply effective modification on entry[attr]
+		if change.Operation() == ldap.ModifyRequestChangeOperationAdd {
+			for _, val := range changeValues {
+				if !listContains(entry[attr], val) {
+					entry[attr] = append(entry[attr], val)
+					if strings.EqualFold(attr, ATTR_MEMBER) {
+						addMembers = append(addMembers, val)
 					}
 				}
 			}
 		} else if change.Operation() == ldap.ModifyRequestChangeOperationDelete {
-			if len(values) == 0 {
+			if len(changeValues) == 0 {
 				// Delete everything
-				newEntry[attr] = []string{}
 				if strings.EqualFold(attr, ATTR_MEMBER) {
-					delMembers = append(delMembers, prevEntry[attr]...)
+					delMembers = append(delMembers, entry[attr]...)
 				}
+				entry[attr] = []string{}
 			} else {
 				// Delete only those specified
-				newEntry[attr] = []string{}
-				for _, prevVal := range prevEntry[attr] {
-					keep := true
-					for _, delVal := range values {
-						if string(delVal) == prevVal {
-							keep = false
-							break
-						}
-					}
-					if keep {
-						newEntry[attr] = append(newEntry[attr], prevVal)
+				newList := []string{}
+				for _, prevVal := range entry[attr] {
+					if !listContains(changeValues, prevVal) {
+						newList = append(newList, prevVal)
 					} else {
 						if strings.EqualFold(attr, ATTR_MEMBER) {
 							delMembers = append(delMembers, prevVal)
 						}
 					}
 				}
+				entry[attr] = newList
 			}
 		} else if change.Operation() == ldap.ModifyRequestChangeOperationReplace {
-			newEntry[attr] = []string{}
-			for _, newVal := range values {
-				newEntry[attr] = append(newEntry[attr], string(newVal))
-			}
 			if strings.EqualFold(attr, ATTR_MEMBER) {
-				for _, newMem := range newEntry[attr] {
-					mustAdd := true
-					for _, prevMem := range prevEntry[attr] {
-						if prevMem == newMem {
-							mustAdd = false
-							break
-						}
-					}
-					if mustAdd {
+				for _, newMem := range changeValues {
+					if !listContains(entry[attr], newMem) {
 						addMembers = append(addMembers, newMem)
 					}
 				}
-				for _, prevMem := range prevEntry[attr] {
-					mustDel := true
-					for _, newMem := range newEntry[attr] {
-						if newMem == prevMem {
-							mustDel = false
-							break
-						}
-					}
-					if mustDel {
+				for _, prevMem := range entry[attr] {
+					if !listContains(changeValues, prevMem) {
 						delMembers = append(delMembers, prevMem)
 					}
 				}
 			}
+			entry[attr] = changeValues
 		}
 	}
 
 	// Check that added members actually exist
 	for i := range addMembers {
-		addMem, err := server.checkDN(addMembers[i], false)
-		if err != nil {
-			return ldap.LDAPResultInvalidDNSyntax, err
-		}
-		exists, err := server.objectExists(addMem)
+		exists, err := server.objectExists(addMembers[i])
 		if err != nil {
 			return ldap.LDAPResultOperationsError, err
 		}
 		if !exists {
 			return ldap.LDAPResultNoSuchObject, fmt.Errorf(
-				"Cannot add member %s, it does not exist", addMem)
+				"Cannot add member %s, it does not exist", addMembers[i])
 		}
-		addMembers[i] = addMem
 	}
 
-	if v, ok := newEntry[ATTR_OBJECTCLASS]; ok && len(v) == 0 {
+	for k, v := range entry {
+		if strings.EqualFold(k, ATTR_OBJECTCLASS) && len(v) == 0 {
 			return ldap.LDAPResultInsufficientAccessRights, fmt.Errorf(
 				"Cannot remove all objectclass values")
 		}
+	}
 
 	// Now, the modification has been processed and accepted and we want to commit it
-	newEntry[ATTR_MODIFIERSNAME] = []string{state.login.user}
-	newEntry[ATTR_MODIFYTIMESTAMP] = []string{genTimestamp()}
+	entry[ATTR_MODIFIERSNAME] = []string{state.login.user}
+	entry[ATTR_MODIFYTIMESTAMP] = []string{genTimestamp()}
 
 	// Save the edited values
-	err = server.addElements(dn, newEntry)
+	err = server.addElements(dn, entry)
 	if err != nil {
 		return ldap.LDAPResultOperationsError, err
 	}