Road to 1.0 — comprehensive audit of spider-gazelle/crystal-ldap (correctness, concurrency, RFC 4511 completeness, API/SemVer, tests)

AUDIT.md

Road to 1.0 — Comprehensive audit of crystal-ldap

Audited against master. The core architecture is genuinely good — BER encoding cleanly delegated to bindata, request/response correlation by messageID over Promise, a dedicated read fiber decoupled from write, a real FilterParser, and a composable filter algebra (&/|/~). The bones are solid. This document maps what stands between today and a 1.0 tag, organized by severity tier with a suggested phased plan. Everything is cited file:line.

A short umbrella issue links here; per-tier child issues can track the work.

ℹ️ Produced with Claude Opus 4.8 [1M] (multi-pass static review); the headline RFC/wire-format findings were verified by hand against the source.

---

Tier 0 — Blockers

Crystal only typechecks methods on instantiation, and protocol bugs surface only against a real server — so these pass a bare crystal build but detonate when the path is exercised.

#	file:line	Bug
1	request.cr:9	@msg_id = -1 ⇒ next_message_id returns 0 for the first request. RFC 4511 §4.1.1.1: messageID 0 is reserved for unsolicited notifications and MUST NOT be used. bind_spec.cr:6-9 even encodes 0x02 0x01 0x00. Start at 1.
2	client.cr:90 vs :69	Cross-fiber Hash mutation without a lock: the process! fiber does @results[id] << response (default-block creates the array) while the caller fiber does @results.delete(id). Hash is not fiber-safe — corruption under multi-threading (-Dpreview_mt). @requests is mutexed; @results is not.

Correction (was item 3 — "binary attributes corrupted"): reclassified to Tier 4, not a Tier-0 blocker. On closer inspection there is no data loss: Crystal's String.new(bytes) copies bytes verbatim (no validation) and String#to_slice returns them unchanged, so binary attribute values survive in the String. It's a typing/ergonomics issue (the API advertises String for values that may be binary), addressed by a typed Bytes/Entry API — see Tier 4.

---

Tier 1 — Protocol completeness (RFC 4511)

The Tag enum (ldap.cr:15-52) declares the whole operation set, but only a slice is wired:

• Operations MISSING — only Bind, Search, StartTLS have a builder + client method. No Modify (§4.6), Add (§4.7), Delete (§4.8), ModifyDN (§4.9), Compare (§4.10), Abandon (§4.11), Unbind (§4.3), or generic Extended (§4.12). This is the bulk of a 1.0.
• Paged results STUBBED — PAGED_RESULTS OID (request.cr:47) and the paged_searches_supported param (request.cr:101) exist but are never referenced. Searches beyond the server size-limit are silently truncated with no pagination loop.
• Referrals dropped — Tag::SearchResultReferral is a # TODO:: (client.cr:88); referrals are silently discarded.
• Filters incomplete — extensible match := is commented out (filter.cr:14 Type::Extensible; filter_parser.cr:46); approx match ~= (filter tag [8]) absent.
• Unbind never sent — close (client.cr:39) tears down the socket without an UnbindRequest.
• Simple bind only — no SASL; VERSION = 3 (ldap.cr:13) is unused, bind hardcodes set_integer(3) (request.cr:39).

Solid and done well: simple bind + StartTLS upgrade, search with scope/deref/attrs/size/time, server-side sort controls, and the string-filter parser.

---

Tier 2 — Correctness & concurrency

• Code.from_value raises a bare ArgumentError (response.cr:92, not an LDAP::Error) for any result code outside the partial enum (missing e.g. 9, 15, proprietary >80). A valid-but-unlisted code crashes the bind/search. Use from_value? + an Other fallback.
• No operation timeout — write(...).get (client.cr:54,69) blocks forever if the server never answers. 1.0 needs a deadline.
• SizeLimitExceeded / TimeLimitExceeded treated as success (response.cr:48 SEARCH_SUCCESS, client.cr:75) — partial results returned with no signal to the caller.
• Unbounded read_bytes(ASN1::BER) (client.cr:104) — a malicious server advertising a huge BER length triggers a massive allocation (memory DoS). No length cap.
• close unsynchronized (client.cr:39) vs the mutexed write.
• parse_bind_response indexes sequence[3] for serverSaslCreds (response.cr:83), but an optional referral can occupy that slot.

---

Tier 3 — Performance / memory / CPU

• No streaming API — the whole result set accumulates in @results then .map (client.cr:70-77); a large search spikes memory. A search(...) { |entry| ... } yielding variant would avoid buffering.
• @results[id] leak — if the terminal SearchResult never arrives (error / abandon), the accumulated SearchReturnedData are never purged (client.cr:90); the delete only runs on the success path.
• Unbounded BER allocation (same vector as Tier 2 DoS), in the hot read loop.

---

Tier 4 — Public API / SemVer surface

• Client#search(*args, **opts) (client.cr:68) — untyped splat delegation: no discoverable signature, zero compile-time checking. Should mirror Request#search.
• Dead public params — return_referrals and paged_searches_supported (request.cr:97,101) are frozen into the public surface yet never used.
• Internals leaking — Client#write(id, sequence : BER) (client.cr:43) and Response#payload : Array(BER) (response.cr:58) expose raw bindata types.
• Return type Array(Hash(String, Array(String))) — DN indistinguishable from attributes, no typed Entry, and no typed binary view: octet-string values that are binary (objectGUID, objectSid, userCertificate, jpegPhoto) are returned as String. The bytes are preserved (recoverable via String#to_slice), so this is not data loss — but the type is misleading and anything UTF-8-aware downstream (JSON, logging, case-folding) misbehaves. A typed Bytes/Entry API would fix it. (Reclassified here from Tier 0.)
• AuthError raised for search failures (client.cr:75) — wrong type; and exceptions carry no result_code accessor (string-only), so callers can only string-match.
• Misleading filter names — join = AND, intersect = OR (filter.cr:115-129).
• shard.yml — version 0.9.1, crystal floor >= 0.36.1 (ancient); add documentation/repository.

---

Tier 5 — Tests, CI, packaging

• Two specs only (bind_spec, filter_parser_spec). Untested: SearchRequest encoding, search-data parsing, sort controls, all Filter combinators (&/|/~, substring, escape/unescape), error paths, the process! read loop, TLS upgrade, the error model.
• filter_parser_spec.cr:11-21 ("variety of filters") has no assertions — it only checks that parsing doesn't raise.
• No integration test against a real / mocked LDAP server.
• CI is good (recently added) — ameba + crystal tool format + crystal spec across 1.0.0/latest/nightly. None of the Tier-0 bugs would be caught by the current suite.

---

Suggested path to 1.0

1. Stop the bleeding — fix the 2 Tier-0 blockers (messageID base, @results lock), each with a failing spec first. (Small, mechanical, high-trust — PR-able now.)
2. Harden correctness — from_value? fallback, operation timeouts, surface size/time-limit-exceeded, bound the BER read.
3. Finish or fence the protocol — either implement the missing operations (Modify/Add/Delete/ModifyDN/Compare/Abandon/Unbind) + paged-results + referrals, or scope 1.0 to Bind+Search cleanly and document the rest as 1.x.
4. API freeze — type Client#search, drop the dead params, stop leaking BER, introduce a typed Entry with Bytes support, unify the error model with a result_code-bearing exception — before the tag, since these are breaking.
5. Tests — cover the filter algebra, search encode/decode round-trips, error paths, and a real integration spec.

---

Audit methodology: multi-pass static review of the master source tree across correctness, concurrency, performance, RFC 4511 protocol completeness, public-API/SemVer surface, and test/CI coverage. Findings are cited by file and line; the headline RFC/wire-format bugs were verified by direct reading.

Produced with Claude Opus 4.8 [1M], with the headline findings hand-verified against the source.