AGENTS.md: Update for hardened search, bind, and panic handling

AGENTS.md

@@ -8,47 +8,53 @@ NetAuth RPC calls via the `netauth/protocol` protobuf types. It is not a full
 LDAP server and never stores data; it only reshapes and serves live NetAuth
 data. Single static binary (`ldapd`), intended to run bound to localhost.
 
-There are **no tests anywhere** in this repo (no `*_test.go` files). Do not
-assume `go test ./...` validates anything.
-
 ## Commands
 
 ```sh
-go build ./...            # build
+go build ./...
 go vet ./...
-go run .                  # run locally (reads config like the real binary, see below)
+go test ./...             # or: go test -race ./internal/ldap/
+go run .                  # needs a config file or NETAUTH_LDAP_DOMAIN
 go mod vendor             # repo vendors deps; vendor/ and dist/ are gitignored
 
 # Production-style build (matches Dockerfile)
 CGO_ENABLED=0 GOOS=linux go build -a -ldflags '-extldflags "-static"' -o ldapd .
 ```
 
-Release is via [goreleaser](.goreleaser.yml) triggered by `v*` tags in
+CI ([.github/workflows/test.yml](.github/workflows/test.yml)) runs build, vet
-[.github/workflows/release.yml](.github/workflows/release.yml); the before-hooks
+and `go test -race` on pushes to master and on PRs. Release is via
+[goreleaser](.goreleaser.yml) triggered by `v*` tags in
+[.github/workflows/release.yml](.github/workflows/release.yml); its before-hooks
 run `go mod vendor` and `scripts/prepare.sh` (regenerates `NOTICE` from vendored
-license files via `scripts/vendor-licenses`, which needs the vendor tree present).
+license files via `scripts/vendor-licenses`, which needs the vendor tree
-Note `dist/config.yaml` is a generated/stale goreleaser artifact, not the config
+present). Note `dist/config.yaml` is a stale generated artifact, not the config
-actually used.
+actually used — the real one is `.goreleaser.yml` at the repo root.
 
 ## Layout
 
 ```
 main.go                    # wiring only: config, logging, client, serve
 internal/ldap/             # the LDAP server (package ldap)
-  ldap.go                  # New(), routing, SetDomain(), Serve/ServeTLS
+  ldap.go                  # New(), route (re)building, Serve/ServeTLS, SetDomain
+  handler.go               # panic guard wrapper + per-op failure responders
+  conn.go                  # per-connection bind state (bound/markBound/markUnbound)
   type.go                  # server struct + naClient interface (the seam for fakes)
   option.go                # functional options: WithLogger, WithNetAuth, WithAnonBind
   bind.go                  # bind handler (auth against NetAuth)
   search.go                # RootDSE, entity & group search handlers
+  attributes.go            # RFC 4511 attribute selection matching
   bleve_mapper.go          # LDAP filter -> Bleve query string translation
   util.go                  # DN parsing (entityIDFromDN)
-internal/buildinfo/        # Version var ("dev"), advertised in RootDSE
+  *_test.go                # tests; see "Testing" below
+internal/buildinfo/        # Version var, advertised in RootDSE; set by goreleaser
 ```
 
 Architecture: `main` builds a `viper` config + `netauth` client, constructs the
 server with functional options, and calls `Serve`/`ServeTLS`. The underlying
 LDAP engine is a `RouteMux` (net/http-style routing on LDAP ops) with handlers
-keyed by base DN and scope.
+keyed by base DN and scope. Every handler is wrapped by `server.guard` so a
+panic cannot take the process down (the vendored server dispatches requests in
+goroutines with no recovery of its own).
 
 ## Non-obvious gotchas
 
@@ -56,56 +62,77 @@ keyed by base DN and scope.
   `github.com/ps78674/ldapserver` + `github.com/ps78674/goldap`, not the more
   common `nwaples/ldapserver` / `go-ldap` packages. Import as
   `ldap "github.com/ps78674/ldapserver"`. Check the vendored API before
-  assuming upstream signatures.
+  assuming upstream signatures. The fork has real limits: TLS floor is
-- **Routes depend on the domain**: `New()` only registers NotFound/Abandon/Bind
+  SSL 3.0, `Server.Handle` calls `log.Fatalln` on double registration, and
-  and the RootDSE route. The `ou=entities` / `ou=groups` search routes are
+  there is no panic recovery in the request path.
-  registered as a side effect of `SetDomain()`, which **must be called** after
+- **The server struct is its own Handler**: `New()` registers `*server` once
-  `New()` or searches won't route at all. Calling it twice re-registers routes.
+  (the fork forbids re-registration), and `ServeLDAP` delegates to whatever
-- **Base DN construction**: `SetDomain` takes a plain domain (`example.com`),
+  `s.routes` mux is current. `SetDomain` rebuilds the mux in place, which is
-  splits on `.`, prepends `dc=netauth`, yielding `dc=netauth,dc=example,dc=com`.
+  why it is safe to call repeatedly. Never call `x.Handle()` again.
-  Tree layout under it is fixed: `uid=<id>,ou=entities,<base>` and
+- **SetDomain is required and validated**: returns an error for an empty
-  `cn=<name>,ou=groups,<base>`.
+  domain or empty labels; searches do not route until it succeeds. Naming
-- **Supported LDAP filters are extremely limited** (`bleve_mapper.go`): only
+  context is always `dc=netauth,dc=<label>,...`. Tree layout under it is fixed:
-  equality match, OR, and attribute-present filters; only the attributes `uid`
+  `uid=<id>,ou=entities,<base>` and `cn=<name>,ou=groups,<base>`.
-  (→ `ID:`) and `cn` (→ `Name:`); values are passed through as raw Bleve query
+- **Bind state is per connection**: searches (except RootDSE) are refused with
-  strings to NetAuth's search. Everything else returns
+  `insufficientAccessRights` until a successful bind on the same connection
-  `unwillingToPerform`/"Filter type not supported".
+  (`conn.go`). A failed bind clears the state per RFC 4511. Anonymous binds
-- **Anonymous bind fall-through** (`bind.go`): when `allowAnon` is enabled and
+  succeed only when `WithAnonBind(true)` and also mark the connection bound.
-  the bind DN is empty, a success response is written but the handler **does
+- **Filter support** (`bleve_mapper.go`): equality, present, AND, OR, NOT
-  not return** — execution continues into `entityIDFromDN("")`, which errors
+  (nested only) and simple substrings (`js*`, `*smith`, `a*th`) are supported;
-  and writes a second response. This appears to be a bug; be careful not to
+  only `uid` (→ `ID`) and `cn` (→ `Name`). Everything else errors rather than
-  "fix" it blindly if behavior parity matters, and definitely add a response
+  returning wrong results. Rules worth knowing before touching this file:
-  write guard if you touch it.
+  - Plain values are quoted; wildcard values must match `^[\w.\-*]+$` to
-- **Only simple bind** — no SASL; anything else is rejected outright.
+    prevent Bleve query injection.
-- **DN parsing known defect**: `entityIDFromDN` splits on `,` without handling
+  - A top-level (or all-negative) filter errors instead of silently returning
-  escaped commas (documented as acceptable since NetAuth IDs can't contain
+    zero results, because Bleve cannot anchor a query made only of `-` clauses.
-  commas). Error strings there have typos ("is underneath", "must be rooted");
+  - AND branches become `+term`; negated branches stay `-term` un-prefixed.
-  leave them alone unless asked.
+- **DN parsing**: `entityIDFromDN` splits on `,` without handling escaped
-- **Search fan-out**: each entity result triggers an extra `EntityGroups` RPC
+  commas (accepted defect: NetAuth IDs cannot contain commas). It validates
-  for `memberOf`, and each group result triggers `GroupMembers` for `member`.
+  the DN has enough components and that the naming context matches exactly.
-  A broad search is N+1 RPCs against NetAuth. Group expansion is done
+- **Search fan-out**: entity results include `memberOf` and group results
-  server-side by NetAuth (flattened, fully expanded groups).
+  include `member`, each costing one extra NetAuth RPC per entry. These are
+  only fetched when the client's attribute selection asks for them
+  (`attributes.go`); an empty selection or `*` means all.
 - **Logging is dual-library**: app logging uses `hashicorp/go-hclog`; the
   vendored ldapserver uses `logrus`, and `main.go` hijacks the global logrus
   logger to funnel protocol traces into hclog at forced Trace level via
   `NETAUTH_LOGLEVEL`. Use `s.l` (hclog, keyed with `"key", value` pairs) inside
   the server package.
 - **NetAuth dependency seam**: server code talks to NetAuth only through the
-  `naClient` interface in `type.go:12`. If you add a NetAuth call, add it to
+  `naClient` interface in `type.go`. If you add a NetAuth call, add it to that
-  that interface, not directly to the concrete client.
+  interface, not directly to the concrete client.
-- **Version injection mismatch**: goreleaser ldflags set `main.version` etc.,
+
-  but the RootDSE reports `internal/buildinfo.Version`, which stays `"dev"` in
+## Testing
-  built binaries.
+
+Tests live in `internal/ldap/*_test.go`. Two constraints shape them:
+
+- **goldap types are unconstructible from outside**: the message/filter types
+  have unexported fields and no exported constructors. Tests hand-encode BER
+  and run it through the real decoder; see the `ber*` helpers in
+  `testutil_test.go` (`berBindRequest`, `berSearchRequestWithAttrs`,
+  `berEqualityFilter`, `berSubstringFilter`, ...). Follow that pattern rather
+  than trying to build request structs directly.
+- **Result codes have no getters**: the result types expose setters only, so
+  `resultCode()` in `result_test.go` re-encodes a response and parses the code
+  back out of the DER.
+- **Fakes**: implement `naClient` (`fakeClient` in `bind_test.go` records
+  calls and returns canned protobuf data). Per-connection state is faked by
+  setting `s.conn` to return a `fakeConn` (`boundTestServer` in
+  `search_test.go`). Search tests must bind before searching.
+- Handlers take the `ldap.ResponseWriter` interface, so assertions use
+  `recordingWriter` (handler_test.go), which captures the exact response
+  sequence.
 
 ## Configuration
 
 viper-driven; config file `config.{yaml,...}` searched in `/etc/netauth/`,
-`$HOME/.netauth/`, `.` — **a missing config file is fatal** (exit 5), even with
+`$HOME/.netauth/`, `.`. A missing config file is tolerated (env-only
-all values provided by env. Env vars use the `NETAUTH_` prefix with `_`
+deployments); a corrupt one is fatal (exit 5). Env vars use the `NETAUTH_`
-replacing `.` (e.g. `NETAUTH_LDAP_BIND`, `NETAUTH_LDAP_DOMAIN`,
+prefix with `_` replacing `.` (e.g. `NETAUTH_LDAP_BIND`,
-`NETAUTH_LOGLEVEL`). Known keys (defaults in `main.go:16-22`):
+`NETAUTH_LDAP_DOMAIN`, `NETAUTH_LOGLEVEL`). Known keys (defaults in
-`ldap.bind` (localhost:389), `ldap.tls` (false), `ldap.key`, `ldap.cert`,
+`main.go`): `ldap.bind` (localhost:389), `ldap.tls` (false), `ldap.key`,
-`ldap.allow_anon` (false), `ldap.domain` (no default — required).
+`ldap.cert`, `ldap.allow_anon` (false), `ldap.domain` (no default — required;
+startup exits with 3 if invalid).
 
 ## Style notes
 
@@ -114,3 +141,5 @@ replacing `.` (e.g. `NETAUTH_LDAP_BIND`, `NETAUTH_LDAP_DOMAIN`,
 - Handlers are methods on `*server` named `handle<X>`; response pattern is
   construct response → `w.Write(res)` on every exit path.
 - Loop style favors `for i := range x` throughout.
+- Errors returned to LDAP clients go in diagnostic messages; keep them
+  non-technical, they are shown verbatim by LDAP browsers.