diff --git a/README.md b/README.md index 4ff695d..5a7f8d4 100644 --- a/README.md +++ b/README.md @@ -154,21 +154,26 @@ curl -k -X PUT https://localhost:8080/api/v1/users \ #### Search Groups -A `query` (minimum 2 characters) or an explicit `filter` is **required**. Listing every -group is not supported: it walks the entire directory and is prohibitively expensive on -real domains. `query` matches `cn` and `sAMAccountName` as a substring, so partial input -returns partial matches. Results are capped by `AD_MAX_SEARCH_RESULTS`. +A `query` of at least 2 characters is **required**. Listing every group is not supported: +it walks the entire directory and is prohibitively expensive on real domains. `query` +matches `cn` and `sAMAccountName` as a substring, so partial input returns partial +matches. Results are capped by `AD_MAX_SEARCH_RESULTS`. + +Raw LDAP filters are **not** accepted on this endpoint. `query` is escaped and wrapped in +`AD_GROUP_FILTER` server-side, so a caller can only narrow the search, never widen it. +Use `/api/v1/search` if you need raw-filter access. ```bash curl -k "https://localhost:8080/api/v1/groups?query=admins" \ -H "X-Session-ID: your-session-id" -# With optional baseDN +# With optional baseDN — must be at or below the configured search base curl -k "https://localhost:8080/api/v1/groups?query=admins&baseDN=ou=Groups,dc=example,dc=com" \ -H "X-Session-ID: your-session-id" ``` -Omitting both `query` and `filter` returns `400`. +Returns `400` when `query` is missing or shorter than 2 characters, when a `filter` +parameter is supplied, or when `baseDN` falls outside the configured search base. #### Resolve Groups by DN @@ -200,6 +205,10 @@ curl -k -X POST https://localhost:8080/api/v1/groups/remove-member \ ``` #### Search (with custom Base DN) + +This endpoint accepts a raw LDAP `filter` for advanced use. A supplied `baseDN` must be +at or below the configured search base; anything outside it returns `400`. + ```bash # GET request with query parameters curl -k "https://localhost:8080/api/v1/search?baseDN=ou=Users,dc=example,dc=com&filter=(objectClass=user)&attributes=cn,mail,title&sizeLimit=100" \ diff --git a/handlers/handler.go b/handlers/handler.go index 3b70634..c6c3172 100644 --- a/handlers/handler.go +++ b/handlers/handler.go @@ -24,7 +24,9 @@ const ( errMsgSessionNotFound = "Session not found" errMsgInvalidRequestBody = "Invalid request body" errMsgSearchNotPermitted = "Search is not permitted for this user" - errMsgQueryRequired = "A query or filter is required; listing all groups is not supported" + errMsgQueryRequired = "A query is required; listing all groups is not supported" + errMsgFilterNotAccepted = "Raw LDAP filters are not accepted; use the query parameter" + errMsgBaseDNOutOfScope = "baseDN must be within the configured search base" logKeyUserDN = "userDN" // minGroupQueryLen mirrors the UI's debounce threshold. Shorter queries match @@ -273,24 +275,39 @@ func (h *Handler) GetAllGroups(w http.ResponseWriter, r *http.Request) { return } - // Get optional baseDN from query params + // An optional baseDN may narrow the search, but only within the configured base. baseDN := r.URL.Query().Get("baseDN") if baseDN == "" { baseDN = h.config.AD.GetSearchBaseDN() + } else if !h.isWithinSearchBase(baseDN) { + writeJSON(w, http.StatusBadRequest, models.GroupsResponse{ + Success: false, + Error: errMsgBaseDNOutOfScope, + }) + return + } + + // A query is mandatory: an unfiltered listing walks every group under the base DN, + // which is prohibitively expensive on real directories. Raw LDAP filters are not + // accepted here — they would bypass the length floor and the configured group + // filter, letting a caller re-create the unbounded listing this endpoint refuses. + if raw := r.URL.Query().Get("filter"); raw != "" { + writeJSON(w, http.StatusBadRequest, models.GroupsResponse{ + Success: false, + Error: errMsgFilterNotAccepted, + }) + return } - // A query (or an explicit filter) is mandatory: an unfiltered listing walks every - // group under the base DN, which is prohibitively expensive on real directories. query := strings.TrimSpace(r.URL.Query().Get("query")) - explicitFilter := r.URL.Query().Get("filter") - if query == "" && explicitFilter == "" { + if query == "" { writeJSON(w, http.StatusBadRequest, models.GroupsResponse{ Success: false, Error: errMsgQueryRequired, }) return } - if query != "" && len([]rune(query)) < minGroupQueryLen { + if len([]rune(query)) < minGroupQueryLen { writeJSON(w, http.StatusBadRequest, models.GroupsResponse{ Success: false, Error: fmt.Sprintf("Query must be at least %d characters", minGroupQueryLen), @@ -298,15 +315,13 @@ func (h *Handler) GetAllGroups(w http.ResponseWriter, r *http.Request) { return } - // query builds a safe escaped filter; an explicit filter is used verbatim. - filter := explicitFilter - if query != "" { - escaped := ldap.EscapeFilter(query) - filter = fmt.Sprintf( - "(&%s(|(cn=*%s*)(sAMAccountName=*%s*)))", - h.config.AD.GroupFilter, escaped, escaped, - ) - } + // Escaped and wrapped in the configured group filter, so a caller can only ever + // narrow the search, never widen it. + escaped := ldap.EscapeFilter(query) + filter := fmt.Sprintf( + "(&%s(|(cn=*%s*)(sAMAccountName=*%s*)))", + h.config.AD.GroupFilter, escaped, escaped, + ) // Search for groups searchReq := ldap.NewSearchRequest( @@ -739,9 +754,16 @@ func (h *Handler) Search(w http.ResponseWriter, r *http.Request) { sizeLimit = req.SizeLimit } - // Use defaults if not provided + // Use defaults if not provided. A supplied baseDN may only narrow the search: + // the raw filter below is deliberately open, so scope stays anchored here. if baseDN == "" { baseDN = h.config.AD.GetSearchBaseDN() + } else if !h.isWithinSearchBase(baseDN) { + writeJSON(w, http.StatusBadRequest, models.SearchResponse{ + Success: false, + Error: errMsgBaseDNOutOfScope, + }) + return } // query takes precedence over filter: build a safe, escaped filter server-side. if query != "" { @@ -844,6 +866,25 @@ func (h *Handler) isExcludedDN(dn string) bool { return false } +// isWithinSearchBase reports whether dn is the configured search base or sits beneath +// it. A caller-supplied base DN may only narrow the search: without this check it could +// point at an unrelated part of the tree and escape the scoping SearchBaseDN enforces. +// Comparison is done on lowercased DNs: ldap.DN compares attribute values +// case-sensitively, whereas AD treats them case-insensitively, so "DC=Example" and +// "dc=example" must both be accepted. Parsing still happens on the real string, so +// escaping and multi-valued RDNs are handled structurally rather than by string match. +func (h *Handler) isWithinSearchBase(dn string) bool { + base, err := ldap.ParseDN(strings.ToLower(h.config.AD.GetSearchBaseDN())) + if err != nil { + return false + } + candidate, err := ldap.ParseDN(strings.ToLower(dn)) + if err != nil { + return false + } + return base.Equal(candidate) || base.AncestorOf(candidate) +} + // isExcludedGroup checks whether a group CN or DN matches the excluded groups list. func (h *Handler) isExcludedGroup(cn, dn string) bool { for _, excluded := range h.config.AD.ExcludedGroups { diff --git a/handlers/handler_test.go b/handlers/handler_test.go index 64180b4..7d5a96d 100644 --- a/handlers/handler_test.go +++ b/handlers/handler_test.go @@ -16,8 +16,12 @@ import ( "adel/session" ) -// testUsername is the session username shared by handler tests. -const testUsername = "testuser" +// Fixture values shared by handler tests. +const ( + testUsername = "testuser" + testGroupFilter = "(objectClass=group)" + testBaseDN = "dc=example,dc=com" +) func TestIsUserEnabled(t *testing.T) { tests := []struct { @@ -372,7 +376,7 @@ func TestGetAllGroupsForbiddenOutsideAllowedGroups(t *testing.T) { func TestGetAllGroupsRejectsUnboundedListing(t *testing.T) { // An allowed user must still not be able to list the whole directory: the query // requirement is a cost control, independent of the search allow-list. - cfg := &config.Config{AD: config.ADConfig{GroupFilter: "(objectClass=group)"}} + cfg := &config.Config{AD: config.ADConfig{GroupFilter: testGroupFilter}} h := NewHandler(cfg, nil) tests := []struct { @@ -408,9 +412,100 @@ func TestGetAllGroupsRejectsUnboundedListing(t *testing.T) { } } +func TestGetAllGroupsRejectsRawFilter(t *testing.T) { + // A raw filter would bypass both the length floor and AD_GROUP_FILTER, letting a + // caller re-create the unbounded listing the query requirement exists to prevent. + cfg := &config.Config{AD: config.ADConfig{ + GroupFilter: testGroupFilter, + BaseDN: testBaseDN, + }} + h := NewHandler(cfg, nil) + + tests := []struct { + name string + url string + }{ + {"filter alone", "/api/v1/groups?filter=%28objectClass%3D%2A%29"}, + {"filter alongside a valid query", "/api/v1/groups?query=admins&filter=%28objectClass%3D%2A%29"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tt.url, nil) + sess := &session.Session{Username: testUsername} + ctx := context.WithValue(req.Context(), middleware.SessionContextKey, sess) + rr := httptest.NewRecorder() + + h.GetAllGroups(rr, req.WithContext(ctx)) + + if rr.Code != http.StatusBadRequest { + t.Errorf("status = %d, want %d", rr.Code, http.StatusBadRequest) + } + var response models.GroupsResponse + if err := json.NewDecoder(rr.Body).Decode(&response); err != nil { + t.Fatalf("failed to decode response: %v", err) + return + } + if response.Success || response.Error == "" { + t.Errorf("response = %+v, want unsuccessful response with an error", response) + } + }) + } +} + +func TestIsWithinSearchBase(t *testing.T) { + cfg := &config.Config{AD: config.ADConfig{BaseDN: testBaseDN}} + h := NewHandler(cfg, nil) + + tests := []struct { + name string + dn string + want bool + }{ + {"the base itself", "dc=example,dc=com", true}, + {"a child OU", "ou=Groups,dc=example,dc=com", true}, + {"a deeper descendant", "cn=Admins,ou=Groups,dc=example,dc=com", true}, + {"differing case and spacing", "OU=Groups, DC=Example, DC=Com", true}, + {"an unrelated tree", "dc=evil,dc=com", false}, + {"a parent of the base", "dc=com", false}, + {"a suffix-matching impostor", "dc=notexample,dc=com", false}, + {"malformed input", "not-a-dn", false}, + {"empty input", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := h.isWithinSearchBase(tt.dn); got != tt.want { + t.Errorf("isWithinSearchBase(%q) = %v, want %v", tt.dn, got, tt.want) + } + }) + } +} + +func TestGetAllGroupsRejectsBaseDNOutsideSearchBase(t *testing.T) { + // A baseDN outside the configured base would escape the scoping SearchBaseDN exists + // to enforce, so it is refused before the search is issued. + cfg := &config.Config{AD: config.ADConfig{ + GroupFilter: testGroupFilter, + BaseDN: testBaseDN, + }} + h := NewHandler(cfg, nil) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/groups?query=admins&baseDN=dc%3Devil%2Cdc%3Dcom", nil) + sess := &session.Session{Username: testUsername} + ctx := context.WithValue(req.Context(), middleware.SessionContextKey, sess) + rr := httptest.NewRecorder() + + h.GetAllGroups(rr, req.WithContext(ctx)) + + if rr.Code != http.StatusBadRequest { + t.Errorf("status = %d, want %d", rr.Code, http.StatusBadRequest) + } +} + func TestResolveGroupsEmptyInputSkipsSearch(t *testing.T) { // No valid DNs means no LDAP search, so a nil session connection must not panic. - cfg := &config.Config{AD: config.ADConfig{GroupFilter: "(objectClass=group)"}} + cfg := &config.Config{AD: config.ADConfig{GroupFilter: testGroupFilter}} h := NewHandler(cfg, nil) body := strings.NewReader(`{"dns":[""," ","not-a-valid-dn"]}`)