internal/ldap: Support AND, OR and substring search filters

internal/ldap/bleve_mapper.go

@@ -3,40 +3,154 @@ package ldap
 import (
 	"errors"
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/ps78674/goldap/message"
 )
 
+// bleveWildcardValueRe matches the subset of characters that may appear
+// in an unquoted Bleve wildcard term. NetAuth entity and group names
+// consist of word characters, so anything outside this set can only
+// come from a client trying to smuggle query syntax in.
+var bleveWildcardValueRe = regexp.MustCompile(`^[\w.\-*]+$`)
+
+// buildBleveQuery translates a decoded LDAP search filter into a Bleve
+// query string for NetAuth. Only attribute equality, presence and the
+// simple substring forms are understood; everything else is refused so
+// that a client gets an error rather than wrong results.
 func (s *server) buildBleveQuery(f message.Filter) (string, error) {
-	s.l.Trace("Building search expression", "type", fmt.Sprintf("%T", f), "filter", fmt.Sprintf("%#v", f))
+	return s.bleveQuery(f, false)
-	var err error
-	var etmp string
-	var expr []string
-	switch f := f.(type) {
-	case message.FilterEqualityMatch:
-		etmp, err = mapToBleveStringQuery(string(f.AttributeDesc()), "=", string(f.AssertionValue()))
-		expr = append(expr, etmp)
-	case message.FilterOr:
-		for _, subf := range f {
-			s, err := s.buildBleveQuery(subf)
-			if err != nil {
-				return "", err
-			}
-			expr = append(expr, s)
-		}
-	case message.FilterPresent:
-		etmp, err = mapToBleveStringQuery(string(f), "=", "*")
-		expr = append(expr, etmp)
-	default:
-		s.l.Warn("Unsupported search filter", "filter", fmt.Sprintf("%#v", f))
-		err = errors.New("unsupported search filter")
-	}
-	return strings.Join(expr, " "), err
 }
 
-func mapToBleveStringQuery(attr, op, val string) (string, error) {
+// bleveQuery builds the query for a filter. The nested flag records
-	var predicate, operator string
+// whether this filter sits inside a boolean group, which determines
+// whether a negation is meaningful.
+func (s *server) bleveQuery(f message.Filter, nested bool) (string, error) {
+	s.l.Trace("Building search expression", "type", fmt.Sprintf("%T", f), "filter", fmt.Sprintf("%#v", f))
+
+	switch f := f.(type) {
+	case message.FilterEqualityMatch:
+		return mapToBleveStringQuery(string(f.AttributeDesc()), string(f.AssertionValue()))
+	case message.FilterPresent:
+		return mapToBleveStringQuery(string(f), "*")
+	case message.FilterSubstrings:
+		return s.bleveSubstringQuery(f)
+	case message.FilterNot:
+		// Bleve prohibits a clause with "-", but an expression made
+		// up entirely of prohibitions matches nothing at all. A
+		// top-level negation therefore has to be refused rather than
+		// quietly returning an empty result set.
+		if !nested {
+			s.l.Warn("Ignoring a top-level negated filter")
+			return "", errors.New("cannot negate the entire search")
+		}
+		e, err := s.bleveQuery(f.Filter, true)
+		if err != nil {
+			return "", err
+		}
+		if strings.HasPrefix(e, "-") {
+			// Double negation would leave Bleve with only
+			// prohibitions again.
+			return "", errors.New("cannot negate a negation")
+		}
+		if strings.HasPrefix(e, "(") {
+			return "-" + e, nil
+		}
+		return "-(" + e + ")", nil
+	case message.FilterAnd:
+		return s.bleveBooleanQuery(f, true)
+	case message.FilterOr:
+		return s.bleveBooleanQuery(f, false)
+	default:
+		s.l.Warn("Unsupported search filter", "filter", fmt.Sprintf("%#v", f))
+		return "", errors.New("unsupported search filter")
+	}
+}
+
+// bleveBooleanQuery maps an AND or OR filter onto a Bleve boolean
+// expression. AND clauses are prefixed with + to make them required,
+// while space separated clauses are already optional (OR) in Bleve.
+// A group with more than one branch is parenthesized so it can be
+// nested without changing its meaning.
+func (s *server) bleveBooleanQuery(filters []message.Filter, and bool) (string, error) {
+	var expr []string
+	for _, subf := range filters {
+		e, err := s.bleveQuery(subf, true)
+		if err != nil {
+			return "", err
+		}
+		if and && !strings.HasPrefix(e, "-") {
+			// A prohibited clause must not also be marked required.
+			e = "+" + e
+		}
+		expr = append(expr, e)
+	}
+	if len(expr) == 0 {
+		return "", errors.New("empty boolean filter")
+	}
+	if !hasPositiveClause(expr) {
+		// A group made up only of prohibitions leaves Bleve with no
+		// clause to match on, which would silently return nothing.
+		return "", errors.New("filter contains no positive clause")
+	}
+	if len(expr) == 1 {
+		return expr[0], nil
+	}
+	return "(" + strings.Join(expr, " ") + ")", nil
+}
+
+// hasPositiveClause reports whether any of the compiled subexpressions
+// can match on its own.
+func hasPositiveClause(expr []string) bool {
+	for _, e := range expr {
+		if !strings.HasPrefix(e, "-") {
+			return true
+		}
+	}
+	return false
+}
+
+// bleveSubstringQuery translates the substring forms clients actually
+// send, such as "(uid=js*)" or "(uid=*smith)", into Bleve wildcard
+// terms. Mixed inner wildcards like "(uid=a*b*)" are refused since a
+// single Bleve wildcard cannot express them faithfully.
+func (s *server) bleveSubstringQuery(f message.FilterSubstrings) (string, error) {
+	var initial, final string
+	var any []string
+	for _, sub := range f.Substrings() {
+		switch v := sub.(type) {
+		case message.SubstringInitial:
+			initial = string(v)
+		case message.SubstringFinal:
+			final = string(v)
+		case message.SubstringAny:
+			any = append(any, string(v))
+		}
+	}
+
+	switch {
+	case len(any) == 0:
+		return mapToBleveStringQuery(string(f.Type_()), initial+"*"+final)
+	case len(any) == 1 && initial == "" && final == "":
+		// A single inner substring like "(uid=*smith)" is matched
+		// with wildcards on both ends.
+		return mapToBleveStringQuery(string(f.Type_()), "*"+any[0]+"*")
+	default:
+		s.l.Warn("Unsupported substring filter", "initial", initial, "final", final, "any", any)
+		return "", errors.New("unsupported substring filter")
+	}
+}
+
+// mapToBleveStringQuery builds a single Bleve term for the supported
+// attributes. A bare "*" keeps its wildcard meaning for
+// attribute-present filters; plain values are quoted so they are
+// matched as one term and cannot introduce extra query clauses. Values
+// containing a wildcard must stay unquoted for the wildcard to work,
+// so those are restricted to the character set NetAuth names may
+// contain.
+func mapToBleveStringQuery(attr, val string) (string, error) {
+	var predicate string
 
 	switch attr {
 	case "uid":
@@ -47,27 +161,16 @@ func mapToBleveStringQuery(attr, op, val string) (string, error) {
 		return "", errors.New("search attribute is unsupported")
 	}
 
-	switch op {
-	case "=":
-		operator = ":"
-	default:
-		return "", errors.New("search comparison is unsupported")
-	}
-
-	return predicate + operator + quoteBleveValue(val), nil
-}
-
-// quoteBleveValue makes a value safe to embed in a Bleve query string.
-//
-// Assertion values come straight from the network and Bleve parses its
-// queries, so an unquoted value such as "x Name:y" would add terms the
-// client never asked for. Quoting keeps the value as a single term, and
-// the wildcard used for attribute-present filters stays outside the
-// quotes so that it keeps its meaning.
-func quoteBleveValue(val string) string {
 	if val == "*" {
-		return val
+		return predicate + ":*", nil
 	}
+	if strings.Contains(val, "*") {
+		if !bleveWildcardValueRe.MatchString(val) {
+			return "", errors.New("search value contains unsupported characters")
+		}
+		return predicate + ":" + val, nil
+	}
+
 	escaped := strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(val)
-	return `"` + escaped + `"`
+	return predicate + `:"` + escaped + `"`, nil
 }

internal/ldap/bleve_mapper_test.go

@@ -35,7 +35,51 @@ func TestBuildBleveQuery(t *testing.T) {
 				berRawEquality("uid", "jsmith"),
 				berRawEquality("uid", "jdoe"),
 			),
-			want: `ID:"jsmith" ID:"jdoe"`,
+			want: `(ID:"jsmith" ID:"jdoe")`,
+		},
+		{
+			name: "and of two matches",
+			filter: berAndFilter(t,
+				berRawEquality("uid", "jsmith"),
+				berRawPresent("uid"),
+			),
+			want: `(+ID:"jsmith" +ID:*)`,
+		},
+		{
+			name:   "prefix substring",
+			filter: berSubstringFilter(t, "uid", "js", nil, ""),
+			want:   "ID:js*",
+		},
+		{
+			name:   "suffix substring",
+			filter: berSubstringFilter(t, "uid", "", nil, "smith"),
+			want:   "ID:*smith",
+		},
+		{
+			name:   "contains substring",
+			filter: berSubstringFilter(t, "uid", "", []string{"mitt"}, ""),
+			want:   "ID:*mitt*",
+		},
+		{
+			name:   "prefix and suffix",
+			filter: berSubstringFilter(t, "uid", "jo", nil, "th"),
+			want:   "ID:jo*th",
+		},
+		{
+			name: "and with a nested negation",
+			filter: berAndFilter(t,
+				berRawPresent("uid"),
+				berElement(0xa2, berRawEquality("uid", "guest")),
+			),
+			want: `(+ID:* -(ID:"guest"))`,
+		},
+		{
+			name: "nested boolean groups",
+			filter: berAndFilter(t,
+				berRawEquality("uid", "jsmith"),
+				berRawOr(t, berRawEquality("uid", "jdoe"), berRawPresent("uid")),
+			),
+			want: `(+ID:"jsmith" +(ID:"jdoe" ID:*))`,
 		},
 	}
 	for _, tc := range cases {
@@ -61,7 +105,7 @@ func TestBuildBleveQueryEscapesValues(t *testing.T) {
 		want  string
 	}{
 		{"x Name:y", `ID:"x Name:y"`},
-		{`x" OR ID:"*`, `ID:"x\" OR ID:\"*"`},
+		{`x" OR ID:"y`, `ID:"x\" OR ID:\"y"`},
 		{`back\slash`, `ID:"back\\slash"`},
 		{"x AND Name:admin", `ID:"x AND Name:admin"`},
 	}
@@ -74,12 +118,47 @@ func TestBuildBleveQueryEscapesValues(t *testing.T) {
 			t.Errorf("value %q: got %q, want %q", tc.value, got, tc.want)
 		}
 	}
+
+	// Wildcards must stay unquoted to keep their meaning, so values
+	// that mix a wildcard with query syntax are refused outright
+	// rather than passed through.
+	for _, value := range []string{`x" OR ID:*`, `x* OR Name:*`, "*\x00"} {
+		if got, err := s.buildBleveQuery(berEqualityFilter(t, "uid", value)); err == nil {
+			t.Errorf("value %q was accepted as %q", value, got)
+		}
+	}
 }
 
 func TestBuildBleveQueryUnsupported(t *testing.T) {
 	s := &server{l: testLogger()}
 
-	if _, err := s.buildBleveQuery(berPresentFilter(t, "mail")); err == nil {
+	cases := []struct {
-		t.Error("an unsupported attribute should not produce a query")
+		name   string
+		filter message.Filter
+	}{
+		{"unsupported attribute", berPresentFilter(t, "mail")},
+		{"unsupported attribute in equality", berEqualityFilter(t, "mail", "x@y.z")},
+		{
+			"top-level negation has nothing to exclude from",
+			berNotFilter(t, berRawEquality("uid", "jsmith")),
+		},
+		{
+			"and of only negations",
+			berAndFilter(t,
+				berElement(0xa2, berRawEquality("uid", "jsmith")),
+				berElement(0xa2, berRawEquality("cn", "admins")),
+			),
+		},
+		{
+			"substring with inner wildcards",
+			berSubstringFilter(t, "uid", "a", []string{"b", "c"}, "d"),
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got, err := s.buildBleveQuery(tc.filter); err == nil {
+				t.Errorf("filter was accepted as %q", got)
+			}
+		})
 	}
 }

internal/ldap/testutil_test.go

@@ -47,12 +47,12 @@ func berSearchRequestWithAttrs(baseDN string, attrs ...string) []byte {
 		selection = append(selection, berOctetString(a))
 	}
 	search := berElement(0x63, // [APPLICATION 3] constructed
-		berOctetString(baseDN),         // baseObject
+		berOctetString(baseDN),          // baseObject
-		berElement(0x0a, []byte{2}),    // scope: wholeSubtree
+		berElement(0x0a, []byte{2}),     // scope: wholeSubtree
-		berElement(0x0a, []byte{0}),    // derefAliases: never
+		berElement(0x0a, []byte{0}),     // derefAliases: never
-		berInteger(0),                  // sizeLimit
+		berInteger(0),                   // sizeLimit
-		berInteger(0),                  // timeLimit
+		berInteger(0),                   // timeLimit
-		berElement(0x01, []byte{0x00}), // typesOnly: false
+		berElement(0x01, []byte{0x00}),  // typesOnly: false
 		berElement(0x87, []byte("uid")), // filter: (uid=*)
 		berElement(0x30, selection...),  // attributes
 	)
@@ -86,12 +86,55 @@ func berOrFilter(t *testing.T, filters ...[]byte) message.Filter {
 	return berDecodeFilter(t, berElement(0xa1, filters...))
 }
 
+// berAndFilter encodes an AND [0] of already-encoded filter elements.
+func berAndFilter(t *testing.T, filters ...[]byte) message.Filter {
+	t.Helper()
+	return berDecodeFilter(t, berElement(0xa0, filters...))
+}
+
+// berNotFilter encodes a NOT [2] wrapping an already-encoded filter.
+func berNotFilter(t *testing.T, filter []byte) message.Filter {
+	t.Helper()
+	return berDecodeFilter(t, berElement(0xa2, filter))
+}
+
 // berRawEquality returns the encoded bytes of an equality match so it
 // can be composed into a nested filter.
 func berRawEquality(attr, value string) []byte {
 	return berElement(0xa3, berOctetString(attr), berOctetString(value))
 }
 
+// berRawPresent returns the encoded bytes of a present filter.
+func berRawPresent(attr string) []byte {
+	return berElement(0x87, []byte(attr))
+}
+
+// berRawOr returns the encoded bytes of an OR of already-encoded
+// filter elements.
+func berRawOr(t *testing.T, filters ...[]byte) []byte {
+	t.Helper()
+	return berElement(0xa1, filters...)
+}
+
+// berSubstringFilter encodes a substrings [4] filter with the given
+// initial, any and final components.
+func berSubstringFilter(t *testing.T, attr, initial string, any []string, final string) message.Filter {
+	t.Helper()
+	var parts [][]byte
+	if initial != "" {
+		parts = append(parts, berElement(0x80, []byte(initial)))
+	}
+	for _, a := range any {
+		parts = append(parts, berElement(0x81, []byte(a)))
+	}
+	if final != "" {
+		parts = append(parts, berElement(0x82, []byte(final)))
+	}
+	inner := berElement(0x30, parts...)
+	ava := berElement(0xa4, berOctetString(attr), inner)
+	return berDecodeFilter(t, ava)
+}
+
 // berDecodeFilter wraps a raw filter element in a minimal search
 // request, decodes it and returns the parsed filter.
 func berDecodeFilter(t *testing.T, filterElement []byte) message.Filter {