From 44d25a62f3b52abb8f8c9570b801feb1f41d5248 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 10:42:58 +0800 Subject: [PATCH 01/11] feat: decode responses in pure Lua, drop rasn/Rust Move the decode seam from rasn.decode_ldap to protocol.decode_message and delete the Rust crate, its build plumbing and the CI toolchain step. The rockspecs become builtin builds. t/decode.t's vectors are the ones merged in #34 and verified against rasn; they are unchanged here, so the suite staying green is the parity proof. --- .cargo/config.toml | 11 --- .github/workflows/ci.yml | 3 - .gitignore | 8 +- Cargo.toml | 17 ---- Makefile | 49 ++--------- lib/resty/ldap/client.lua | 42 ++++------ rockspec/lua-resty-ldap-local-0.rockspec | 23 ++---- rockspec/lua-resty-ldap-main-0.rockspec | 23 ++---- src/ldap_codec/decoder.rs | 101 ----------------------- src/ldap_codec/encoder.rs | 11 --- src/ldap_codec/mod.rs | 2 - src/lib.rs | 14 ---- t/decode.t | 8 +- t/lib/ldap_decode.lua | 5 -- t/patch/unknown_op.patch | 2 +- utils/check-rust.sh | 7 -- 16 files changed, 46 insertions(+), 280 deletions(-) delete mode 100644 .cargo/config.toml delete mode 100644 Cargo.toml delete mode 100644 src/ldap_codec/decoder.rs delete mode 100644 src/ldap_codec/encoder.rs delete mode 100644 src/ldap_codec/mod.rs delete mode 100644 src/lib.rs delete mode 100644 t/lib/ldap_decode.lua delete mode 100755 utils/check-rust.sh diff --git a/.cargo/config.toml b/.cargo/config.toml deleted file mode 100644 index d47f983..0000000 --- a/.cargo/config.toml +++ /dev/null @@ -1,11 +0,0 @@ -[target.x86_64-apple-darwin] -rustflags = [ - "-C", "link-arg=-undefined", - "-C", "link-arg=dynamic_lookup", -] - -[target.aarch64-apple-darwin] -rustflags = [ - "-C", "link-arg=-undefined", - "-C", "link-arg=dynamic_lookup", -] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9d94261..e70e800 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,9 +31,6 @@ jobs: # Test::Nginx sudo cpanm --notest Test::Nginx - # rust - curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y - # luarocks sudo apt -y install lua5.1 luarocks luarocks make rockspec/lua-resty-ldap-local-0.rockspec --local --tree ./deps # install deps to local diff --git a/.gitignore b/.gitignore index 7da5785..18483c8 100644 --- a/.gitignore +++ b/.gitignore @@ -1,9 +1,5 @@ # dev t/servroot -deps/ - -# Added by cargo - -/target -/Cargo.lock +# luarocks --tree ./deps build output (see .github/workflows/ci.yml) +deps/ diff --git a/Cargo.toml b/Cargo.toml deleted file mode 100644 index 4ec4258..0000000 --- a/Cargo.toml +++ /dev/null @@ -1,17 +0,0 @@ -[package] -name = "lua-resty-ldap" -version = "0.2.2" -edition = "2021" - -[lib] -name = "rasn" -crate-type = ["cdylib"] - -[dependencies] -mlua = { version = "0.8.6", features = ["module", "send", "luajit"] } -rasn = "0.6.1" -rasn-ldap = "0.6.0" -bytes = "1.4.0" - -[profile.release] -strip = true diff --git a/Makefile b/Makefile index fbc42b4..2711e95 100644 --- a/Makefile +++ b/Makefile @@ -1,52 +1,17 @@ -UNAME ?= $(shell uname) -INSTALL ?= install -ECHO ?= echo -C_SO_NAME := librasn.so - -ifeq ($(UNAME),Darwin) - C_SO_NAME := librasn.dylib -endif - -all: - @echo --- Build - @echo CFLAGS: $(CFLAGS) - @echo LIBFLAG: $(LIBFLAG) - @echo LUA_LIBDIR: $(LUA_LIBDIR) - @echo LUA_BINDIR: $(LUA_BINDIR) - @echo LUA_INCDIR: $(LUA_INCDIR) - - @echo --- Check Rust toolchain - @utils/check-rust.sh - - @echo --- Build Rust cdylib - cargo build --release - -### install: Install the library to runtime -.PHONY: install -install: - @echo --- Install - @echo INST_PREFIX: $(INST_PREFIX) - @echo INST_BINDIR: $(INST_BINDIR) - @echo INST_LIBDIR: $(INST_LIBDIR) - @echo INST_LUADIR: $(INST_LUADIR) - @echo INST_CONFDIR: $(INST_CONFDIR) - $(INSTALL) -d $(INST_LUADIR)/resty/ldap/ - $(INSTALL) lib/resty/ldap/*.lua $(INST_LUADIR)/resty/ldap/ - $(INSTALL) target/release/${C_SO_NAME} $(INST_LIBDIR)/rasn.so - -### dev: Create a development ENV +### dev: Install runtime dependencies locally .PHONY: dev dev: luarocks install rockspec/lua-resty-ldap-main-0.rockspec --only-deps --local +### test: Run the test suite +.PHONY: test +test: + git apply t/patch/unknown_op.patch + prove -r t/; ret=$$?; git apply t/patch/unknown_op.patch -R; exit $$ret + ### help: Show Makefile rules .PHONY: help help: @echo Makefile rules: @echo @grep -E '^### [-A-Za-z0-9_]+:' Makefile | sed 's/###/ /' - -test: - git apply t/patch/unknown_op.patch - prove -r t/ - git apply t/patch/unknown_op.patch -R diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index 442dbfc..4fa122d 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -1,18 +1,12 @@ local bunpack = require("lua_pack").unpack local protocol = require("resty.ldap.protocol") -local to_hex = require("resty.string").to_hex -local ok, rasn = pcall(require, "rasn") - -if not ok then - error("failed to load rasn library: " .. rasn) -end local tostring = tostring local fmt = string.format local tcp = ngx.socket.tcp local table_insert = table.insert local string_char = string.char -local rasn_decode = rasn.decode_ldap +local decode_ldap = protocol.decode_message local _M = {} @@ -65,16 +59,16 @@ local function _start_tls(sock) end local packet = packet_header .. packet - local ok, res, err = pcall(rasn_decode, packet) - if not ok or err then - return nil, fmt( - "failed to decode ldap message: %s, message: %s", - not ok and res or err, -- error returned in second value by pcall - to_hex(packet) - ) + local res, err = decode_ldap(packet) + if not res then + sock:close() + -- the body can carry DNs and attribute values; keep it out of the error + return fmt("failed to decode ldap message: %s (%d bytes)", + err or "unknown", #packet) end if res.protocol_op ~= protocol.APP_NO.ExtendedResponse then + sock:close() return fmt("received incorrect op in packet: %d, expected %d", res.protocol_op, protocol.APP_NO.ExtendedResponse) end @@ -82,6 +76,7 @@ local function _start_tls(sock) if res.result_code ~= 0 then local error_msg = protocol.ERROR_MSG[res.result_code] + sock:close() return fmt("error: %s, details: %s", error_msg or ("Unknown error occurred (code: " .. res.result_code .. ")"), res.diagnostic_msg or "") @@ -143,7 +138,7 @@ local function _init_socket(self) self.socket = sock end -local function _send_recieve(cli, request, multi_resp_hint) +local function _send_receive(cli, request, multi_resp_hint) -- initialize socket local err = _init_socket(cli) if err then @@ -192,13 +187,12 @@ local function _send_recieve(cli, request, multi_resp_hint) end local packet = packet_header .. packet - local ok, res, err = pcall(rasn_decode, packet) - if not ok or err then - return nil, fmt( - "failed to decode ldap message: %s, message: %s", - not ok and res or err, -- error returned in second value by pcall - to_hex(packet) - ) + local res, err = decode_ldap(packet) + if not res then + socket:close() + -- the body can carry DNs and attribute values; keep it out of the error + return nil, fmt("failed to decode ldap message: %s (%d bytes)", + err or "unknown", #packet) end table_insert(result, res) @@ -257,7 +251,7 @@ end function _M.simple_bind(self, dn, password) - local res, err = _send_recieve(self, protocol.simple_bind_request(dn, password)) + local res, err = _send_receive(self, protocol.simple_bind_request(dn, password)) if not res then return false, err end @@ -296,7 +290,7 @@ function _M.search(self, base_dn, scope, deref_aliases, size_limit, time_limit, return false, err end - local res, err = _send_recieve(self, search_req, true) -- mark as potential multi-response operation + local res, err = _send_receive(self, search_req, true) -- mark as potential multi-response operation if not res then return false, err end diff --git a/rockspec/lua-resty-ldap-local-0.rockspec b/rockspec/lua-resty-ldap-local-0.rockspec index 1523bfa..7c4a7e1 100644 --- a/rockspec/lua-resty-ldap-local-0.rockspec +++ b/rockspec/lua-resty-ldap-local-0.rockspec @@ -17,20 +17,11 @@ dependencies = { } build = { - type = "make", - build_variables = { - CFLAGS="$(CFLAGS)", - LIBFLAG="$(LIBFLAG)", - LUA_LIBDIR="$(LUA_LIBDIR)", - LUA_BINDIR="$(LUA_BINDIR)", - LUA_INCDIR="$(LUA_INCDIR)", - LUA="$(LUA)", - }, - install_variables = { - INST_PREFIX="$(PREFIX)", - INST_BINDIR="$(BINDIR)", - INST_LIBDIR="$(LIBDIR)", - INST_LUADIR="$(LUADIR)", - INST_CONFDIR="$(CONFDIR)", - }, + type = "builtin", + modules = { + ["resty.ldap.asn1"] = "lib/resty/ldap/asn1.lua", + ["resty.ldap.filter"] = "lib/resty/ldap/filter.lua", + ["resty.ldap.protocol"] = "lib/resty/ldap/protocol.lua", + ["resty.ldap.client"] = "lib/resty/ldap/client.lua", + } } diff --git a/rockspec/lua-resty-ldap-main-0.rockspec b/rockspec/lua-resty-ldap-main-0.rockspec index bf078ac..b30db85 100644 --- a/rockspec/lua-resty-ldap-main-0.rockspec +++ b/rockspec/lua-resty-ldap-main-0.rockspec @@ -18,20 +18,11 @@ dependencies = { } build = { - type = "make", - build_variables = { - CFLAGS="$(CFLAGS)", - LIBFLAG="$(LIBFLAG)", - LUA_LIBDIR="$(LUA_LIBDIR)", - LUA_BINDIR="$(LUA_BINDIR)", - LUA_INCDIR="$(LUA_INCDIR)", - LUA="$(LUA)", - }, - install_variables = { - INST_PREFIX="$(PREFIX)", - INST_BINDIR="$(BINDIR)", - INST_LIBDIR="$(LIBDIR)", - INST_LUADIR="$(LUADIR)", - INST_CONFDIR="$(CONFDIR)", - }, + type = "builtin", + modules = { + ["resty.ldap.asn1"] = "lib/resty/ldap/asn1.lua", + ["resty.ldap.filter"] = "lib/resty/ldap/filter.lua", + ["resty.ldap.protocol"] = "lib/resty/ldap/protocol.lua", + ["resty.ldap.client"] = "lib/resty/ldap/client.lua", + } } diff --git a/src/ldap_codec/decoder.rs b/src/ldap_codec/decoder.rs deleted file mode 100644 index ebd1f5e..0000000 --- a/src/ldap_codec/decoder.rs +++ /dev/null @@ -1,101 +0,0 @@ -use mlua::prelude::{Lua, LuaResult, LuaValue}; -use bytes::Bytes; -use rasn::der; -use rasn_ldap::{LdapMessage, ProtocolOp}; - -fn bytes_to_string(b: Bytes) -> Result { - return String::from_utf8(b.to_vec()); -} - -pub fn decode<'lua>( - lua: &'lua Lua, - v: LuaValue<'lua>, -) -> LuaResult<(LuaValue<'lua>, LuaValue<'lua>)> { - let der = match v { - LuaValue::String(v) => v, - _ => { - return Ok(( - LuaValue::Nil, - LuaValue::String(lua.create_string("wrong format on input data")?), - )) - } - }; - - let lm = match der::decode::(der.as_bytes()) { - Ok(lm) => lm, - Err(err) => { - let err_str = format!("{}", err.to_string()); - - return Ok(( - LuaValue::Nil, - LuaValue::String(lua.create_string(err_str.as_bytes())?), - )); - } - }; - - let result = lua.create_table()?; - match lm.protocol_op { - ProtocolOp::BindResponse(resp) => { - result.set("protocol_op", 1)?; - result.set("result_code", resp.result_code as i64)?; - result.set("matched_dn", bytes_to_string(resp.matched_dn).unwrap())?; - result.set( - "diagnostic_msg", - bytes_to_string(resp.diagnostic_message).unwrap(), - )?; - return Ok((LuaValue::Table(result), LuaValue::Nil)); - } - ProtocolOp::SearchResEntry(entry) => { - result.set("protocol_op", 4)?; - result.set("entry_dn", bytes_to_string(entry.object_name).unwrap())?; - - let attributes = lua.create_table()?; - for attribute in entry.attributes.into_iter() { - let attribute_vals = lua.create_table()?; - for val in attribute.vals.into_iter() { - attribute_vals.push(bytes_to_string(val).unwrap())? - } - attributes.set(bytes_to_string(attribute.r#type).unwrap(), attribute_vals)?; - } - - result.set("attributes", attributes)?; - return Ok((LuaValue::Table(result), LuaValue::Nil)); - } - ProtocolOp::SearchResDone(done) => { - let resp = done.0; - result.set("protocol_op", 5)?; - result.set("result_code", resp.result_code as i64)?; - result.set("matched_dn", bytes_to_string(resp.matched_dn).unwrap())?; - result.set( - "diagnostic_msg", - bytes_to_string(resp.diagnostic_message).unwrap(), - )?; - return Ok((LuaValue::Table(result), LuaValue::Nil)); - } - ProtocolOp::ModifyResponse(resp0) => { - let resp = resp0.0; - result.set("protocol_op", 7)?; - result.set("result_code", resp.result_code as i64)?; - result.set("matched_dn", bytes_to_string(resp.matched_dn).unwrap())?; - result.set( - "diagnostic_msg", - bytes_to_string(resp.diagnostic_message).unwrap(), - )?; - return Ok((LuaValue::Table(result), LuaValue::Nil)); - } - ProtocolOp::SearchResRef(_) => { - return Ok(( - LuaValue::Nil, - LuaValue::String( - lua.create_string("decoder not yet implement: search result reference")?, - ), - )) - } - _ => { - return Ok(( - LuaValue::Nil, - LuaValue::String(lua.create_string("decoder not yet implement")?), - )) - } - } -} diff --git a/src/ldap_codec/encoder.rs b/src/ldap_codec/encoder.rs deleted file mode 100644 index 463785e..0000000 --- a/src/ldap_codec/encoder.rs +++ /dev/null @@ -1,11 +0,0 @@ -use mlua::prelude::{Lua, LuaResult, LuaValue}; - -pub fn encode<'lua>( - lua: &'lua Lua, - _: LuaValue<'lua>, -) -> LuaResult<(LuaValue<'lua>, LuaValue<'lua>)> { - Ok(( - LuaValue::Nil, - LuaValue::String(lua.create_string("not yet implement")?), - )) -} diff --git a/src/ldap_codec/mod.rs b/src/ldap_codec/mod.rs deleted file mode 100644 index 7b77088..0000000 --- a/src/ldap_codec/mod.rs +++ /dev/null @@ -1,2 +0,0 @@ -pub mod encoder; -pub mod decoder; diff --git a/src/lib.rs b/src/lib.rs deleted file mode 100644 index 32e52d6..0000000 --- a/src/lib.rs +++ /dev/null @@ -1,14 +0,0 @@ -use ldap_codec::{decoder::decode, encoder::encode}; -use mlua::prelude::{Lua, LuaResult, LuaTable}; - -mod ldap_codec; - -#[mlua::lua_module] -fn rasn(lua: &Lua) -> LuaResult { - let exports = lua.create_table()?; - - exports.set("encode_ldap", lua.create_function(encode)?)?; - exports.set("decode_ldap", lua.create_function(decode)?)?; - - Ok(exports) -} diff --git a/t/decode.t b/t/decode.t index 2a13578..7b4cbb1 100644 --- a/t/decode.t +++ b/t/decode.t @@ -20,8 +20,8 @@ __DATA__ --- config location /t { content_by_lua_block { - local decode = require("ldap_decode") local protocol = require("resty.ldap.protocol") + local decode = protocol.decode_message local ldap_hex = require("ldap_hex") local res = assert(decode(ldap_hex("30 0c 02 01 01 61 07 0a 01 00 04 00 04 00"))) assert(res.protocol_op == protocol.APP_NO.BindResponse, "op " .. tostring(res.protocol_op)) @@ -43,8 +43,8 @@ ok --- config location /t { content_by_lua_block { - local decode = require("ldap_decode") local protocol = require("resty.ldap.protocol") + local decode = protocol.decode_message local ldap_hex = require("ldap_hex") local res = assert(decode(ldap_hex("30 0c 02 01 02 65 07 0a 01 20 04 00 04 00"))) assert(res.protocol_op == protocol.APP_NO.SearchResultDone, "op") @@ -64,8 +64,8 @@ ok --- config location /t { content_by_lua_block { - local decode = require("ldap_decode") local protocol = require("resty.ldap.protocol") + local decode = protocol.decode_message local ldap_hex = require("ldap_hex") -- entry_dn "x"; attribute s = { "\1\0\2" } (3 bytes, contains NUL) local res = assert(decode(ldap_hex("30 16 02 01 03 64 11 04 01 78 30 0c 30 0a 04 01 73 31 05 04 03 01 00 02"))) @@ -90,7 +90,7 @@ ok --- config location /t { content_by_lua_block { - local decode = require("ldap_decode") + local decode = require("resty.ldap.protocol").decode_message local ldap_hex = require("ldap_hex") local res = assert(decode(ldap_hex( "30 11 02 01 03 64 0c 04 01 78 30 07 30 05 04 01 73 31 00"))) diff --git a/t/lib/ldap_decode.lua b/t/lib/ldap_decode.lua deleted file mode 100644 index 8e08426..0000000 --- a/t/lib/ldap_decode.lua +++ /dev/null @@ -1,5 +0,0 @@ -local ok, rasn = pcall(require, "rasn") -if ok and type(rasn) == "table" and rasn.decode_ldap then - return rasn.decode_ldap -end -return require("resty.ldap.protocol").decode_message diff --git a/t/patch/unknown_op.patch b/t/patch/unknown_op.patch index 52bb839..2ab3395 100644 --- a/t/patch/unknown_op.patch +++ b/t/patch/unknown_op.patch @@ -8,7 +8,7 @@ index 6b8eb18..45cef3f 100644 +function _M.unknown(self, hex_data, multi_resp_hint) + local raw_data = hex_data:gsub("%x%x", function(digits) return string.char(tonumber(digits, 16)) end) -+ local res, err = _send_recieve(self, raw_data, multi_resp_hint or false) ++ local res, err = _send_receive(self, raw_data, multi_resp_hint or false) + if not res then + return false, err + end diff --git a/utils/check-rust.sh b/utils/check-rust.sh deleted file mode 100755 index 7d47072..0000000 --- a/utils/check-rust.sh +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/sh - -command -v cargo >/dev/null 2>&1 && { echo "The cargo is installed"; } || { - echo "The cargo is not installed" - echo "This rock contains the Rust code: make sure you have a Rust development environment installed and try again" - exit 1 -} From e928c1becff7a3bfe249735294f0e4db4a2f6fcd Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 10:43:33 +0800 Subject: [PATCH 02/11] fix: pin AttributeValue to OCTET STRING and bound messageID Both are decoder-side follow-ups from the #35 review that only become reachable now that decode_message is the live seam. asn1.decode accepts any universal type inside the vals SET, so a server could put a number or a table into attributes[type], which the output shape says is always an array of strings; a nested SEQUENCE/SET made callers doing string work on the value raise. rasn dropped such an attribute entirely, so this is also a behaviour change at the cutover. messageID was accepted well outside RFC 4511's 0..maxInt while the request side already wraps at maxInt. --- lib/resty/ldap/protocol.lua | 33 +++++++++++++++++--- t/asn1_integer.t | 38 +++++++++++++++++++++++ t/decode_hostile.t | 62 +++++++++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 4 deletions(-) diff --git a/lib/resty/ldap/protocol.lua b/lib/resty/ldap/protocol.lua index 4481559..58c87db 100644 --- a/lib/resty/ldap/protocol.lua +++ b/lib/resty/ldap/protocol.lua @@ -243,6 +243,12 @@ local function is_sequence(obj) and obj.cons end +local function is_set(obj) + return obj.class == asn1.CLASS.UNIVERSAL + and obj.tag == asn1.TAG.SET + and obj.cons +end + -- protocolOp tags a server may send; anything else must not reach parse_ldap_result local LDAP_RESULT_OPS = { [_M.APP_NO.BindResponse] = true, @@ -308,13 +314,29 @@ local function parse_search_entry(packet, op, res) "AttributeDescription") if terr then return nil, terr end -- vals is a SET OF: enforce it so the stored value is always an array - local vend, vals, verr = decode_typed(packet, vpos, pastop, - asn1.TAG.SET, "attribute vals") - if verr then return nil, verr end + local vset, serr = asn1_get_object(packet, vpos, pastop) + if not vset then return nil, serr end + if not is_set(vset) then + return nil, "attribute vals is not a universal constructed SET" + end -- PartialAttribute has exactly two components; nothing may follow vals - if vend ~= pastop then + if vset.offset + vset.len ~= pastop then return nil, "trailing bytes in PartialAttribute" end + -- AttributeValue ::= OCTET STRING (s4.1.5); asn1.decode would take any + -- universal type here and break the string-array contract + local vals = {} + local n = 0 + local vp = vset.offset + local vstop = vset.offset + vset.len + while vp < vstop do + local val, verr + vp, val, verr = decode_typed(packet, vp, vstop, + asn1.TAG.OCTET_STRING, "AttributeValue") + if verr then return nil, verr end + n = n + 1 + vals[n] = val + end attributes[atype] = vals -- ALWAYS an array (empty for typesOnly) apos = pastop end @@ -365,6 +387,9 @@ function _M.decode_message(packet) local pos, message_id, merr = decode_typed(packet, env.offset, envstop, asn1.TAG.INTEGER, "messageID") if merr then return nil, merr end + if message_id < 0 or message_id > LDAP_MAX_INT then + return nil, "messageID out of range" + end local op, oerr = asn1_get_object(packet, pos, envstop) -- protocolOp if not op then return nil, oerr end diff --git a/t/asn1_integer.t b/t/asn1_integer.t index 3bd7208..d3b17a5 100644 --- a/t/asn1_integer.t +++ b/t/asn1_integer.t @@ -283,3 +283,41 @@ GET /t ok --- no_error_log [error] + +=== TEST 7: messageID outside 0..maxInt is rejected +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local protocol = require("resty.ldap.protocol") + local ldap_hex = require("ldap_hex") + + -- representable, so TEST 6's overflow guard never saw these + local bad = { + {"30 0c 02 01 ff 65 07 0a 01 00 04 00 04 00", "messageID -1"}, + {"30 10 02 05 00 80 00 00 00 65 07 0a 01 00 04 00 04 00", "messageID maxInt+1"}, + {"30 10 02 05 ff 7f ff ff ff 65 07 0a 01 00 04 00 04 00", "messageID -maxInt-2"}, + } + for _, c in ipairs(bad) do + local res, err = protocol.decode_message(ldap_hex(c[1])) + assert(res == nil and err ~= nil, + c[2] .. " must be rejected; got " .. tostring(res and res.message_id)) + end + + -- both ends of the legal range still decode + local zero = assert(protocol.decode_message(ldap_hex( + "30 0c 02 01 00 65 07 0a 01 00 04 00 04 00"))) + assert(zero.message_id == 0, "messageID 0") + local max = assert(protocol.decode_message(ldap_hex( + "30 0f 02 04 7f ff ff ff 65 07 0a 01 00 04 00 04 00"))) + assert(max.message_id == 2147483647, "messageID maxInt") + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] diff --git a/t/decode_hostile.t b/t/decode_hostile.t index d2c9e00..2f2c4d1 100644 --- a/t/decode_hostile.t +++ b/t/decode_hostile.t @@ -371,3 +371,65 @@ GET /t ok --- no_error_log [error] + +=== TEST 11: every AttributeValue must be an OCTET STRING +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local protocol = require("resty.ldap.protocol") + local ldap_hex = require("ldap_hex") + + -- rasn dropped the whole attribute here; the pure-Lua decoder used to + -- store the value, leaking a number or a table into a string array + local cases = { + { "INTEGER value", + "30 14 02 01 03 64 0f 04 01 78 30 0a 30 08 04 01 73 31 03 02 01 05" }, + { "ENUMERATED value", + "30 14 02 01 03 64 0f 04 01 78 30 0a 30 08 04 01 73 31 03 0a 01 07" }, + { "SEQUENCE value", + "30 13 02 01 03 64 0e 04 01 78 30 09 30 07 04 01 73 31 02 30 00" }, + { "nested SET value", + "30 16 02 01 03 64 11 04 01 78 30 0c 30 0a 04 01 73 31 05 31 03 04 01 41" }, + { "one good value then an INTEGER", + "30 17 02 01 03 64 12 04 01 78 30 0d 30 0b 04 01 73 31 06 04 01 41 02 01 05" }, + } + + local leaks = {} + for _, c in ipairs(cases) do + local pok, res, err = pcall(protocol.decode_message, ldap_hex(c[2])) + if not pok then + table.insert(leaks, c[1] .. " RAISED: " .. tostring(res)) + elseif res ~= nil or err == nil then + table.insert(leaks, string.format("%s accepted (vals[1] is a %s)", + c[1], type(res.attributes.s and res.attributes.s[1]))) + end + end + if #leaks > 0 then + ngx.say(#leaks .. " non-OCTET-STRING values accepted:") + ngx.say(table.concat(leaks, "\n")) + return + end + + -- the tightened check must not cost BER leniency + local lenient = assert(protocol.decode_message(ldap_hex( + "30 17 02 01 03 64 12 04 01 78 30 0d 30 0b 04 01 73 31 06 04 81 03 41 42 43"))) + assert(lenient.attributes.s[1] == "ABC", "non-minimal length still decodes") + + -- and well-formed entries are untouched + local ok = assert(protocol.decode_message(ldap_hex( + "30 14 02 01 03 64 0f 04 01 78 30 0a 30 08 04 01 73 31 03 04 01 41"))) + assert(ok.attributes.s[1] == "A", "single value") + local empty = assert(protocol.decode_message(ldap_hex( + "30 11 02 01 03 64 0c 04 01 78 30 07 30 05 04 01 73 31 00"))) + assert(#empty.attributes.s == 0, "typesOnly empty SET stays an empty array") + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] From 590b79ecffa1a536af1166c51e06bf89a5310286 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 10:44:20 +0800 Subject: [PATCH 03/11] feat: session API, filter escaping and resty.ldap compat layer Adds connect/set_keepalive/close so callers can pin a connection across operations, and closes rather than pools any socket that carried a Bind so its identity cannot leak into a reused connection. Connection pools are also partitioned by TLS verification policy, since sslhandshake() is a no-op on a reused socket. filter.escape() applies RFC 4515 escaping; resty.ldap.ldap_authenticate returns the pre-#26 entry point, escaping the bind DN per RFC 4514. simple_bind now validates its request before opening a socket, the last of the #35 review follow-ups: passing simple_bind_request's (nil, err) straight through expanded the error into multi_resp_hint and connected before failing to send. It also distinguishes a transport failure (nil) from rejected credentials (false) so ldap_authenticate cannot report an unreachable server as a bad password. CI loads t/fixtures/ad.ldif, whose non-UTF-8 attribute value is decoded by t/search_ad.t. --- .github/workflows/ci.yml | 2 + README.md | 56 +++- lib/resty/ldap.lua | 106 +++++++ lib/resty/ldap/client.lua | 170 ++++++++++-- lib/resty/ldap/filter.lua | 27 ++ rockspec/lua-resty-ldap-local-0.rockspec | 1 + rockspec/lua-resty-ldap-main-0.rockspec | 1 + t/client.t | 338 +++++++++++++++++++++++ t/compat_ldap.t | 230 +++++++++++++++ t/filter.t | 70 +++++ t/fixtures/ad.ldif | 9 + t/search.t | 17 +- t/search_ad.t | 78 ++++++ t/session.t | 195 +++++++++++++ 14 files changed, 1268 insertions(+), 32 deletions(-) create mode 100644 lib/resty/ldap.lua create mode 100644 t/compat_ldap.t create mode 100644 t/fixtures/ad.ldif create mode 100644 t/search_ad.t create mode 100644 t/session.t diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e70e800..19fa574 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,5 +53,7 @@ jobs: docker logs openldap || true exit 1 fi + docker cp t/fixtures/ad.ldif openldap:/tmp/ad.ldif + docker exec openldap ldapadd -x -H ldap://127.0.0.1:1389 -D "cn=admin,dc=example,dc=org" -w adminpassword -f /tmp/ad.ldif export PATH=$OPENRESTY_PREFIX/nginx/sbin:$OPENRESTY_PREFIX/luajit/bin:$PATH make test diff --git a/README.md b/README.md index 357cfe4..6145e9d 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ To load this module: `bind_dn` and `password` can be `nil` values, that means the client is instructed to do anonymous bind. -`res` is a boolean type value that will be true when authentication is successful, when it is false, `err` will contain errors. +`res` is `true` when authentication succeeds and `false` when the server rejects the bind (or the request cannot be encoded); on a transport or decoding failure `res` is `nil`. In both failure cases `err` carries the error. #### search @@ -84,3 +84,57 @@ To load this module: `filter` is an LDAP filter expression in string. Default is `(objectClass=*)`. `attributes` is an array table that contains one to more query fields that you need to have the LDAP server return. Default is `["objectClass"]`. + +#### connect + +**syntax:** *ok, err = client:connect()* + +Establishes and pins the underlying connection, so that subsequent operations on this client (for example a `simple_bind` followed by a `search`) run on that same connection instead of each drawing one from the connection pool. Release the connection with `set_keepalive` or `close` when done. + +An unrecoverable socket error also releases the pin, so the next operation draws a fresh connection instead of reusing the dead one. + +#### set_keepalive + +**syntax:** *ok, err = client:set_keepalive()* + +Releases a pinned connection back into the connection pool. The client remains usable; subsequent operations draw pooled connections as before. + +#### close + +**syntax:** *ok, err = client:close()* + +Closes a pinned connection without returning it to the pool. + +### resty.ldap + +Compatibility entrypoint kept for APISIX's `ldap-auth` plugin, preserving the v0.1.0 `ldap_authenticate` contract as a thin wrapper over `resty.ldap.client`. New code should use `resty.ldap.client` instead. + +```lua + local ldap = require "resty.ldap" +``` + +#### ldap_authenticate + +**syntax:** *ok, err, user_dn = ldap.ldap_authenticate(username, password, conf)* + +Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the server rejects the credentials, and `nil` on a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is always closed after the bind attempt — a socket that carried a Bind is never returned to the pool. + +`username` is escaped per RFC 4514 when the bind DN is built, so DN metacharacters (`, + " \ < > ; =`, a leading space or `#`, a trailing space, or a NUL) are treated as literal characters of the RDN value instead of injecting extra RDN components. Any username string is accepted, as in v0.1.0. + +`user_dn` is the canonical (escaped) DN the bind was performed with; callers that key identity on the DN (e.g. the APISIX `ldap-auth` consumer lookup) should use it instead of rebuilding the DN from the raw username. It is returned whenever a bind was attempted, and is `nil` on connect/STARTTLS/handshake failures. + +`conf` is a table of below items (note the key names differ from `resty.ldap.client`'s `client_config`; they follow the v0.1.0 / `ldap-auth` contract): + +| key | type | default value | Description | +| ----------- | ----------- | ----------- | ----------- | +| `ldap_host` | string | localhost | LDAP server host. | +| `ldap_port` | number | 389 | LDAP server port. | +| `timeout` | number | 10000 | Socket timeout in milliseconds. | +| `keepalive` | number | 60000 | Accepted for v0.1.0 compatibility, but unused: sockets that carried a bind are always closed, never pooled. | +| `start_tls` | boolean | false | Issue the StartTLS extended operation before binding. | +| `ldaps` | boolean | false | Connect using LDAP over TLS. | +| `tls_verify` | boolean | false | Verify the server certificate during the TLS handshake. This is the key APISIX's `ldap-auth` passes. | +| `verify_ldap_host` | boolean | false | Legacy alias for `tls_verify` (pre-0.2). Honoured only when `tls_verify` is not set. | +| `base_dn` | string | ou=users,dc=example,dc=org | Base DN the username is appended to. | +| `attribute` | string | cn | RDN attribute the username is bound as. | + diff --git a/lib/resty/ldap.lua b/lib/resty/ldap.lua new file mode 100644 index 0000000..0583026 --- /dev/null +++ b/lib/resty/ldap.lua @@ -0,0 +1,106 @@ +local log = ngx.log +local ERR = ngx.ERR +local client = require "resty.ldap.client" + + +local tostring = tostring + +local default_conf = { + timeout = 10000, + start_tls = false, + ldap_host = "localhost", + ldap_port = 389, + ldaps = false, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + keepalive = 60000, +} + +local function set_conf_default_values(conf) + for k, v in pairs(default_conf) do + if conf[k] == nil then + conf[k] = v + end + end +end + + +local function resolve_tls_verify(conf) + if conf.tls_verify ~= nil then + return conf.tls_verify + end + if conf.verify_ldap_host ~= nil then + return conf.verify_ldap_host + end + return false +end + + +-- RFC 4514 s2.4 escaping for an RDN value; the bind DN below is built from a +-- client-supplied username. Not filter.escape(), which is RFC 4515 and leaves +-- ',' and '+' alone. +local function escape_dn_value(value) + local lead = value:sub(1, 1) + -- a lone leading space is covered by the lead rule alone + local trail = #value > 1 and value:sub(-1) or "" + + local out = value:gsub('([\\",+<>;=])', "\\%1"):gsub("%z", "\\00") + + if trail == " " then + out = out:sub(1, -2) .. "\\ " + end + if lead == " " or lead == "#" then + out = "\\" .. out + end + + return out +end + + +local _M = {} + + +function _M.ldap_authenticate(given_username, given_password, conf) + set_conf_default_values(conf) + + -- same coercion contract as protocol.simple_bind_request + if type(given_username) == "number" then + given_username = tostring(given_username) + end + if type(given_username) ~= "string" then + return false, "bind username must be a string" + end + + local cli = client:new(conf.ldap_host, conf.ldap_port, { + socket_timeout = conf.timeout, + keepalive_timeout = conf.keepalive, + start_tls = conf.start_tls, + ldaps = conf.ldaps, + ssl_verify = resolve_tls_verify(conf), + }) + + local ok, err = cli:connect() + if not ok then + log(ERR, "failed to connect to ", conf.ldap_host, ":", + tostring(conf.ldap_port), ": ", err) + return nil, err + end + + local who = conf.attribute .. "=" .. escape_dn_value(given_username) .. + "," .. conf.base_dn + local is_authenticated, bind_err = cli:simple_bind(who, given_password) + + -- the socket carried a Bind attempt: always close it, never pool it + cli:close() + + if is_authenticated == nil then + -- transport failure mid-bind; the client already dropped the socket + return nil, bind_err + end + + -- who: the canonical escaped bind DN, for callers that key identity on it + return is_authenticated, bind_err, who +end + + +return _M diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index 4fa122d..99ab603 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -5,26 +5,40 @@ local tostring = tostring local fmt = string.format local tcp = ngx.socket.tcp local table_insert = table.insert -local string_char = string.char local decode_ldap = protocol.decode_message +-- Upper bound on a single LDAP message body (bytes). A well-formed length such +-- as 84 7f ff ff ff is legal BER but would force a multi-GiB allocation in the +-- worker. 16 MiB comfortably exceeds any real entry. +local MAX_LDAP_MESSAGE_SIZE = 16 * 1024 * 1024 + local _M = {} local mt = { __index = _M } +-- returns body length + header bytes, or nil + error (length is validated) local function calculate_payload_length(encStr, pos, socket) local elen pos, elen = bunpack(encStr, "C", pos) + -- 0x80 (indefinite) and 0xff (reserved) are illegal, not short-form lengths + if elen == 0x80 or elen == 0xff then + return nil, nil, "invalid BER length: indefinite or reserved form" + end + if elen > 128 then elen = elen - 128 local elenCalc = 0 local elenNext - for i = 1, elen do + for _ = 1, elen do elenCalc = elenCalc * 256 - encStr = encStr .. socket:receive(1) + local byte, err = socket:receive(1) + if not byte then + return nil, nil, fmt("receive length header failed: %s", err) + end + encStr = encStr .. byte pos, elenNext = bunpack(encStr, "C", pos) elenCalc = elenCalc + elenNext end @@ -32,7 +46,11 @@ local function calculate_payload_length(encStr, pos, socket) elen = elenCalc end - return pos, elen, encStr + if elen > MAX_LDAP_MESSAGE_SIZE then + return nil, nil, fmt("ldap message too large: %d bytes", elen) + end + + return elen, encStr end local function _start_tls(sock) @@ -50,7 +68,11 @@ local function _start_tls(sock) end return fmt("receive response header failed: %s", err) end - local _, packet_len, packet_header = calculate_payload_length(len, 2, sock) + local packet_len, packet_header, lerr = calculate_payload_length(len, 2, sock) + if not packet_len then + sock:close() + return lerr + end local packet, err = sock:receive(packet_len) if not packet then @@ -91,16 +113,28 @@ local function _init_socket(self) sock:settimeout(socket_config.socket_timeout) - -- keep TLS connections in a separate pool to avoid reusing non-secure - -- connections and vice-versa, because STARTTLS use the same port + -- Partition the pool by transport mode and verification policy: + -- sslhandshake() returns immediately on a reused connection, so a pooled + -- ssl_verify=false connection must never serve one that demands verification. + local pool_suffix = "" + if socket_config.start_tls then + pool_suffix = ":starttls" + elseif socket_config.ldaps then + pool_suffix = ":ldaps" + end + if pool_suffix ~= "" then + pool_suffix = pool_suffix .. (socket_config.ssl_verify and ":verify" or ":noverify") + end + local opts = { - pool = host .. ":" .. port .. (socket_config.start_tls and ":starttls" or ""), + pool = host .. ":" .. port .. pool_suffix, pool_size = socket_config.keepalive_pool_size, } - -- override the value when the user specifies connection pool name + -- override the value when the user specifies connection pool name, + -- still partitioned by policy if socket_config.keepalive_pool_name and socket_config.keepalive_pool_name ~= "" then - opts.pool = socket_config.keepalive_pool_name + opts.pool = socket_config.keepalive_pool_name .. pool_suffix end local ok, err = sock:connect(host, port, opts) @@ -128,6 +162,7 @@ local function _init_socket(self) end if socket_config.start_tls or socket_config.ldaps then + local _ _, err = sock:sslhandshake(true, host, socket_config.ssl_verify) if err ~= nil then return fmt("do TLS handshake on %s:%s failed: %s", @@ -138,11 +173,26 @@ local function _init_socket(self) self.socket = sock end +-- drop the socket (and its pin) after an unrecoverable error +local function _reset_socket(cli) + local sock = cli.socket + cli.socket = nil + cli.pinned = nil + cli.bound = nil + if sock then + sock:close() + end +end + local function _send_receive(cli, request, multi_resp_hint) - -- initialize socket - local err = _init_socket(cli) - if err then - return nil, fmt("initialize socket failed: %s", err) + -- In a pinned session the socket is already checked out; otherwise check out + -- one for this single operation. + if not cli.pinned then + local err = _init_socket(cli) + if err then + cli.bound = nil + return nil, fmt("initialize socket failed: %s", err) + end end local socket = cli.socket @@ -150,13 +200,10 @@ local function _send_receive(cli, request, multi_resp_hint) -- send req local bytes, err = cli.socket:send(request) if not bytes then + _reset_socket(cli) return nil, fmt("send request failed: %s", err) end - -- Each response in a multi-response body has ASCII NULL(0x00) as its ending, - -- so here the reader is created using receiveuntil. - local reader = socket:receiveuntil(string_char(0x00)) - local result = {} -- When the client sends a search request, the server will return several -- different entries in a string-like concatenation, sto we must use a @@ -166,15 +213,20 @@ local function _send_receive(cli, request, multi_resp_hint) -- Takes the packet header of a single request body, which has a length -- of two bytes, where the second byte is the length of this response -- body packet. - local len, err = reader(2) + local len, err = socket:receive(2) if not len then if err == "timeout" then - socket:close() + _reset_socket(cli) return nil, fmt("receive response failed: %s", err) end break -- read done, data has been taken to the end end - local _, packet_len, packet_header = calculate_payload_length(len, 2, socket) + + local packet_len, packet_header, lerr = calculate_payload_length(len, 2, socket) + if not packet_len then + _reset_socket(cli) + return nil, lerr + end -- Get the data of the specified length local packet, err = socket:receive(packet_len) @@ -182,15 +234,15 @@ local function _send_receive(cli, request, multi_resp_hint) -- When the packet header is read but the packet body cannot be read, -- this error is considered unacceptable and therefore an error is -- returned directly instead of processing the received data. - socket:close() - return nil, err + _reset_socket(cli) + return nil, fmt("receive response failed: %s", err) end local packet = packet_header .. packet local res, err = decode_ldap(packet) if not res then - socket:close() -- the body can carry DNs and attribute values; keep it out of the error + _reset_socket(cli) return nil, fmt("failed to decode ldap message: %s (%d bytes)", err or "unknown", #packet) end @@ -213,8 +265,17 @@ local function _send_receive(cli, request, multi_resp_hint) end end - -- put back into the connection pool - socket:setkeepalive(cli.socket_config.keepalive_timeout) + -- single-shot ops return the socket to the pool; a pinned session is + -- released explicitly by the caller. A bound socket carries the Bind + -- identity while the pool key does not, so close it instead of pooling. + if not cli.pinned then + if cli.bound then + cli.bound = nil + socket:close() + else + socket:setkeepalive(cli.socket_config.keepalive_timeout) + end + end return multi_resp_hint and result or result[1] end @@ -250,10 +311,63 @@ function _M.new(_, host, port, client_config) end +function _M.connect(self) + if self.pinned then + return true + end + local err = _init_socket(self) + if err then + return nil, err + end + self.pinned = true + return true +end + + +function _M.set_keepalive(self) + if not self.pinned then + return true + end + self.pinned = nil + local sock = self.socket + self.socket = nil + if not sock then + return true + end + if self.bound then + self.bound = nil + return sock:close() + end + return sock:setkeepalive(self.socket_config.keepalive_timeout) +end + + +function _M.close(self) + self.pinned = nil + self.bound = nil + local sock = self.socket + self.socket = nil + if not sock then + return true + end + return sock:close() +end + + function _M.simple_bind(self, dn, password) - local res, err = _send_receive(self, protocol.simple_bind_request(dn, password)) + -- as the last argument a (nil, err) return would expand into + -- _send_receive's multi_resp_hint and send a nil request + local req, berr = protocol.simple_bind_request(dn, password) + if not req then + return false, berr + end + + self.bound = true + local res, err = _send_receive(self, req) if not res then - return false, err + -- transport/decode failure: nil, so callers can tell an unreachable + -- or broken server (nil) from rejected credentials (false) + return nil, err end if res.protocol_op ~= protocol.APP_NO.BindResponse then diff --git a/lib/resty/ldap/filter.lua b/lib/resty/ldap/filter.lua index 524deb1..7bb67f9 100644 --- a/lib/resty/ldap/filter.lua +++ b/lib/resty/ldap/filter.lua @@ -190,4 +190,31 @@ function _M.compile(str) end +local ESCAPE_MAP = { + ["\0"] = "\\00", + ["*"] = "\\2a", + ["("] = "\\28", + [")"] = "\\29", + ["\\"] = "\\5c", + ["="] = "\\3d", + ["<"] = "\\3c", + [">"] = "\\3e", +} + +-- RFC 4515 s3: escape the octets that are special in an assertion value so that +-- untrusted input cannot change the filter's structure. The compiler un-escapes +-- \HH back to raw bytes, so escape() is its exact inverse for these octets. +-- The five RFC-mandated specials are `* ( ) \ NUL`; `= < >` are not RFC-special +-- but the grammar above is stricter and rejects them raw inside a value, so we +-- escape them too (escaping any octet is RFC-legal and round-trips). Note: valid +-- multi-byte UTF-8 passes through the grammar unescaped; a lone invalid-UTF-8 +-- byte (0x80-0xFF) is not escaped here and fails the filter closed. +function _M.escape(value) + if type(value) ~= "string" then + return nil, "value must be a string" + end + return (value:gsub("[%z%*%(%)\\=<>]", ESCAPE_MAP)) +end + + return _M diff --git a/rockspec/lua-resty-ldap-local-0.rockspec b/rockspec/lua-resty-ldap-local-0.rockspec index 7c4a7e1..6d1fa3d 100644 --- a/rockspec/lua-resty-ldap-local-0.rockspec +++ b/rockspec/lua-resty-ldap-local-0.rockspec @@ -19,6 +19,7 @@ dependencies = { build = { type = "builtin", modules = { + ["resty.ldap"] = "lib/resty/ldap.lua", ["resty.ldap.asn1"] = "lib/resty/ldap/asn1.lua", ["resty.ldap.filter"] = "lib/resty/ldap/filter.lua", ["resty.ldap.protocol"] = "lib/resty/ldap/protocol.lua", diff --git a/rockspec/lua-resty-ldap-main-0.rockspec b/rockspec/lua-resty-ldap-main-0.rockspec index b30db85..908df4b 100644 --- a/rockspec/lua-resty-ldap-main-0.rockspec +++ b/rockspec/lua-resty-ldap-main-0.rockspec @@ -20,6 +20,7 @@ dependencies = { build = { type = "builtin", modules = { + ["resty.ldap"] = "lib/resty/ldap.lua", ["resty.ldap.asn1"] = "lib/resty/ldap/asn1.lua", ["resty.ldap.filter"] = "lib/resty/ldap/filter.lua", ["resty.ldap.protocol"] = "lib/resty/ldap/protocol.lua", diff --git a/t/client.t b/t/client.t index 51ab0cd..2063b8a 100644 --- a/t/client.t +++ b/t/client.t @@ -152,3 +152,341 @@ GET /t --- no_error_log [error] --- error_code: 200 + + +=== TEST 7: resty.ldap.ldap_authenticate binds a valid user +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok, "authenticate failed: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 8: a pooled unverified TLS connection is never reused with verification on +--- http_config eval: $::HttpConfig +--- config + location /t { + lua_ssl_trusted_certificate ../../certs/mycacert.crt; + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + -- pool a connection established without certificate verification + local a = ldap_client:new("localhost", 1636, { ldaps = true, ssl_verify = false }) + assert(a:connect()) + assert(a.socket:getreusedtimes() == 0, "first connection must be fresh") + assert(a:set_keepalive()) + + -- ssl_verify=true must not reuse it (a fresh connection is required) + local b = ldap_client:new("localhost", 1636, { ldaps = true, ssl_verify = true }) + assert(b:connect()) + assert(b.socket:getreusedtimes() == 0, + "an unverified pooled connection must not serve ssl_verify=true") + assert(b:set_keepalive()) + + -- control: the same policy does reuse its own pooled connection + local c = ldap_client:new("localhost", 1636, { ldaps = true, ssl_verify = false }) + assert(c:connect()) + assert(c.socket:getreusedtimes() > 0, "same-policy reuse should hit the pool") + assert(c:set_keepalive()) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 9: starttls pools are partitioned by verification policy too +--- http_config eval: $::HttpConfig +--- config + location /t { + lua_ssl_trusted_certificate ../../certs/mycacert.crt; + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + local a = ldap_client:new("localhost", 1389, { start_tls = true, ssl_verify = false }) + assert(a:connect()) + assert(a.socket:getreusedtimes() == 0, "first connection must be fresh") + assert(a:set_keepalive()) + + local b = ldap_client:new("localhost", 1389, { start_tls = true, ssl_verify = true }) + assert(b:connect()) + assert(b.socket:getreusedtimes() == 0, + "an unverified pooled STARTTLS connection must not serve ssl_verify=true") + assert(b:set_keepalive()) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 10: client returns a controlled error when a response header contains a 0x00 byte +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + local closed, step = false, 0 + local sock = { + send = function(_, p) return #p end, + receive = function() + step = step + 1 + if step == 1 then + return "\48\00" -- header: SEQUENCE tag, then a 0x00 length octet + end + return "" -- the declared zero-length body + end, + close = function() closed = true return true end, + } + + local client = ldap_client:new("127.0.0.1", 1389) + client.socket = sock + client.pinned = true + + local res, err = client:simple_bind( + "cn=user01,ou=users,dc=example,dc=org", "password1") + assert(res == nil, "a 0x00 header byte must be a transport failure (nil), got " .. + tostring(res)) + assert(err:find("failed to decode ldap message", 1, true), + "unexpected err: " .. tostring(err)) + assert(closed, "the unusable socket must be closed") + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 11: a single-shot bind never returns its socket to the shared pool +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + -- control: an unbound single-shot search does re-enter the pool + local a = ldap_client:new("127.0.0.1", 1389) + assert(a:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + local b = ldap_client:new("127.0.0.1", 1389) + assert(b:connect()) + assert(b.socket:getreusedtimes() > 0, "unbound search should hit the pool") + assert(b:set_keepalive()) + + -- a single-shot admin bind checks out the pooled socket, binds on + -- it, and must close it on release instead of pooling it again + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + + -- a new anonymous client must get a fresh connection; its search + -- must not inherit the admin identity + local d = ldap_client:new("127.0.0.1", 1389) + assert(d:connect()) + assert(d.socket:getreusedtimes() == 0, + "admin-bound socket leaked into the pool") + local res, serr = d:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(res, "anonymous search: " .. tostring(serr)) + assert(#res == 1, "one entry") + assert(d:set_keepalive()) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 12: ldap_authenticate never pools a socket that carried a bind +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local conf = { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + } + + -- a failed bind closes its socket instead of pooling it; the pool + -- is observable as the default host:port pool + local ok = ldap.ldap_authenticate("user01", "wrong-password", conf) + assert(ok == false, "a wrong password must be rejected (false), got " .. tostring(ok)) + local probe = ngx.socket.tcp() + assert(probe:connect("127.0.0.1", 1389)) + assert(probe:getreusedtimes() == 0, + "failed-bind socket leaked into the pool") + probe:close() + + -- a successful bind leaves the socket holding the user's identity, + -- so it too must be closed, never pooled + local ok2, err2 = ldap.ldap_authenticate("user01", "password1", conf) + assert(ok2, "authenticate failed: " .. tostring(err2)) + local probe2 = ngx.socket.tcp() + assert(probe2:connect("127.0.0.1", 1389)) + assert(probe2:getreusedtimes() == 0, + "user-bound socket leaked into the pool") + probe2:close() + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 13: client returns a controlled error when the response header is unreadable +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + local closed = false + local sock = { + send = function(_, p) return #p end, + receive = function() return nil, "timeout" end, + close = function() closed = true return true end, + } + + local client = ldap_client:new("127.0.0.1", 1389) + client.socket = sock + client.pinned = true + + local res, err = client:simple_bind( + "cn=user01,ou=users,dc=example,dc=org", "password1") + assert(res == nil, "a receive timeout must be a transport failure (nil), got " .. + tostring(res)) + assert(err:find("receive response failed", 1, true), + "unexpected err: " .. tostring(err)) + assert(closed, "the unusable socket must be closed") + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 14: client returns a controlled error when the response body is truncated +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + local closed, step = false, 0 + local sock = { + send = function(_, p) return #p end, + receive = function() + step = step + 1 + if step == 1 then + return "\48\05" -- header: SEQUENCE tag, body length 5 + end + return nil, "closed" -- body read fails + end, + close = function() closed = true return true end, + } + + local client = ldap_client:new("127.0.0.1", 1389) + client.socket = sock + client.pinned = true + + local res, err = client:simple_bind( + "cn=user01,ou=users,dc=example,dc=org", "password1") + assert(res == nil, "a truncated body must be a transport failure (nil), got " .. + tostring(res)) + assert(err:find("receive response failed", 1, true), + "unexpected err: " .. tostring(err)) + assert(closed, "the unusable socket must be closed") + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + +=== TEST 15: an invalid bind dn/password is reported without opening a socket +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + -- port 1 has no listener: a socket error here instead of the + -- validation message means simple_bind connected before checking + local client = ldap_client:new("127.0.0.1", 1) + + local res, err = client:simple_bind({}, "password1") + assert(res == false, "table dn must fail") + assert(err == "bind dn must be a string", "got: " .. tostring(err)) + + local res2, err2 = client:simple_bind("cn=user01", true) + assert(res2 == false, "boolean password must fail") + assert(err2 == "bind password must be a string", "got: " .. tostring(err2)) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] diff --git a/t/compat_ldap.t b/t/compat_ldap.t new file mode 100644 index 0000000..0d0830f --- /dev/null +++ b/t/compat_ldap.t @@ -0,0 +1,230 @@ +use Test::Nginx::Socket::Lua; + +log_level('info'); +no_shuffle(); +no_long_string(); +repeat_each(1); +plan 'no_plan'; + +our $HttpConfig = <<'_EOC_'; + lua_package_path 'lib/?.lua;t/lib/?.lua;/usr/share/lua/5.1/?.lua;;'; + lua_package_cpath 'deps/lib/lua/5.1/?.so;;'; + resolver 127.0.0.53; +_EOC_ + +run_tests(); + +__DATA__ + +=== TEST 1: tls_verify=true enforces certificate verification +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + -- no lua_ssl_trusted_certificate, so verifying the test CA must fail + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "localhost", + ldap_port = 1636, + ldaps = true, + tls_verify = true, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok == nil, "a rejected handshake must be a transport failure (nil), got " .. + tostring(ok)) + assert(err:find("TLS handshake", 1, true), "unexpected err: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- error_log +certificate verify error + + + +=== TEST 2: tls_verify=false skips verification and binds +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "localhost", + ldap_port = 1636, + ldaps = true, + tls_verify = false, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok, "handshake without verification should bind: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 3: verify_ldap_host=true is honoured as a legacy alias for tls_verify +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "localhost", + ldap_port = 1636, + ldaps = true, + verify_ldap_host = true, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok == nil, "legacy verify_ldap_host must enforce verification: " .. + "expected a transport failure (nil), got " .. tostring(ok)) + assert(err:find("TLS handshake", 1, true), "unexpected err: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- error_log +certificate verify error + + + +=== TEST 4: a DN-metacharacter username is escaped into the bind DN +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + -- "al,ice" must reach the server as cn=al\,ice and fail as a clean + -- credential error; sent raw, the server would refuse the DN itself + local ok, err, user_dn = ldap.ldap_authenticate("al,ice", "password1", { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok == false, "an unknown escaped username must be rejected (false), got " .. + tostring(ok)) + assert(err:find("credential", 1, true), + "expected a credential error: " .. tostring(err)) + assert(user_dn == "cn=al\\,ice,ou=users,dc=example,dc=org", + "expected the escaped bind DN, got: " .. tostring(user_dn)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 5: ldap_authenticate binds a valid user with the raw DN +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local ok, err, user_dn = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok, "authenticate failed: " .. tostring(err)) + assert(user_dn == "cn=user01,ou=users,dc=example,dc=org", + "expected the canonical bind DN, got: " .. tostring(user_dn)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 6: a wrong password is a clean auth failure +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local ok, err = ldap.ldap_authenticate("user01", "wrong-password", { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok == false, "a wrong password must be rejected (false), got " .. tostring(ok)) + assert(err:find("credential", 1, true), "expected a credential error: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 7: an earlier unverified call never lets a tls_verify=true call skip verification +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + + -- authenticate with verification disabled first + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "localhost", + ldap_port = 1636, + ldaps = true, + tls_verify = false, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok, "unverified handshake should bind: " .. tostring(err)) + + -- whatever state the first call left behind (it must not pool its + -- bound socket), a verifying call needs a fresh handshake, which + -- fails: no trusted certificate is configured + local ok2, err2 = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "localhost", + ldap_port = 1636, + ldaps = true, + tls_verify = true, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok2 == nil, "the unverified call must not satisfy a verifying one: " .. + "expected a transport failure (nil), got " .. tostring(ok2)) + assert(err2:find("TLS handshake", 1, true), "unexpected err: " .. tostring(err2)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- error_log +certificate verify error diff --git a/t/filter.t b/t/filter.t index 7737fde..5626762 100644 --- a/t/filter.t +++ b/t/filter.t @@ -449,3 +449,73 @@ GET /t --- no_error_log [error] --- error_code: 200 + +=== TEST 5: escape escapes the five special octets +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local filter = require("resty.ldap.filter") + assert(filter.escape("a*b") == "a\\2ab", "star") + assert(filter.escape("a(b") == "a\\28b", "lparen") + assert(filter.escape("a)b") == "a\\29b", "rparen") + assert(filter.escape("a\\b") == "a\\5cb", "backslash") + assert(filter.escape("a\0b") == "a\\00b", "nul") + assert(filter.escape("plain") == "plain", "plain unchanged") + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + +=== TEST 6: escaped user input cannot widen the filter (injection) +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local filter = require("resty.ldap.filter") + local raw = "*)(uid=*" + local esc = filter.escape(raw) + local ast = assert(filter.compile("(cn=" .. esc .. ")")) + assert(ast.item_type == "simple", "must be a simple item, got " .. tostring(ast.item_type)) + assert(ast.filter_type == "equal", "must be equality") + -- the compiler un-escapes back to the exact raw bytes + assert(ast.attribute_value == raw, "value round-trips to raw: " .. tostring(ast.attribute_value)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + +=== TEST 7: escape covers < and > which the grammar rejects raw +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local filter = require("resty.ldap.filter") + assert(filter.escape("ab") == "a\\3eb", "gt") + -- round-trip: a value with < and > compiles to a simple equality + -- whose attribute_value is the exact raw input + local raw = "ac" + local ast = assert(filter.compile("(cn=" .. filter.escape(raw) .. ")"), + "value with must compile after escaping") + assert(ast.item_type == "simple" and ast.filter_type == "equal", "simple equality") + assert(ast.attribute_value == raw, "round-trips to raw: " .. tostring(ast.attribute_value)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] diff --git a/t/fixtures/ad.ldif b/t/fixtures/ad.ldif new file mode 100644 index 0000000..17d11b7 --- /dev/null +++ b/t/fixtures/ad.ldif @@ -0,0 +1,9 @@ +dn: cn=Jane Doe,ou=users,dc=example,dc=org +objectClass: inetOrgPerson +objectClass: organizationalPerson +objectClass: person +cn: Jane Doe +sn: Doe +uid: jdoe +userPassword: janesecret +description:: AQAC diff --git a/t/search.t b/t/search.t index 4800933..07740a8 100644 --- a/t/search.t +++ b/t/search.t @@ -371,7 +371,8 @@ GET /t ngx.exit(401) end - assert(#res == 1, "result length is not equal to 1") + -- 2 entries: the base entry plus cn=Jane Doe from t/fixtures/ad.ldif + assert(#res == 2, "result length is not equal to 2") assert(res[1].entry_dn == "dc=example,dc=org", "result entry_dn is not equal to dc=example,dc=org") } } @@ -533,8 +534,8 @@ GET /t ngx.exit(401) end - -- whole subtree: 6 entries bootstrapped by the image - assert(#res == 6, "result length is not equal to 6") + -- 6 bootstrapped by the image plus cn=Jane Doe from t/fixtures/ad.ldif + assert(#res == 7, "result length is not equal to 7") } } --- request @@ -588,6 +589,9 @@ GET /t local client = ldap_client:new("127.0.0.1", 1389) + -- pin the session so the modify runs on the bound connection + assert(client:connect()) + -- auth local res, err = client:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") if not res then @@ -609,6 +613,8 @@ GET /t assert(res.protocol_op == 7, "protocol_op is not equal to 7, " .. res.protocol_op) assert(res.result_code == 0, "result_code is not equal to 0, " .. res.result_code) + + client:close() } } --- request @@ -662,6 +668,9 @@ GET /t local client = ldap_client:new("127.0.0.1", 1389) + -- pin the session so the modify runs on the bound connection + assert(client:connect()) + -- auth local res, err = client:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") if not res then @@ -683,6 +692,8 @@ GET /t assert(res.protocol_op == 7, "protocol_op is not equal to 7, " .. res.protocol_op) assert(res.result_code == 0, "result_code is not equal to 0, " .. res.result_code) + + client:close() } } --- request diff --git a/t/search_ad.t b/t/search_ad.t new file mode 100644 index 0000000..14bc0b9 --- /dev/null +++ b/t/search_ad.t @@ -0,0 +1,78 @@ +use Test::Nginx::Socket::Lua; + +log_level('info'); +no_shuffle(); +no_long_string(); +repeat_each(1); +plan 'no_plan'; + +our $HttpConfig = <<'_EOC_'; + lua_package_path 'lib/?.lua;/usr/share/lua/5.1/?.lua;;'; + lua_package_cpath 'deps/lib/lua/5.1/?.so;;'; + resolver 127.0.0.53; +_EOC_ + +run_tests(); + +__DATA__ + +=== TEST 1: search by login attribute (uid) returns an entry whose RDN is cn +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + local filter = require("resty.ldap.filter") + + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:connect()) + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + + local login = "jdoe" + local flt = "(uid=" .. filter.escape(login) .. ")" + local res, err = c:search("ou=users,dc=example,dc=org", + protocol.SEARCH_SCOPE_WHOLE_SUBTREE, nil, nil, nil, nil, flt, {"uid", "description"}) + assert(res, "search: " .. tostring(err)) + assert(#res == 1, "exactly one entry, got " .. #res) + assert(res[1].entry_dn == "cn=Jane Doe,ou=users,dc=example,dc=org", + "DN is the RDN-based DN: " .. tostring(res[1].entry_dn)) + assert(res[1].attributes.uid[1] == "jdoe", "uid value") + assert(c:set_keepalive()) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + +=== TEST 2: binary attribute value with embedded NUL survives end-to-end +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:connect()) + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + local res = assert(c:search("ou=users,dc=example,dc=org", + protocol.SEARCH_SCOPE_WHOLE_SUBTREE, nil, nil, nil, nil, + "(uid=jdoe)", {"description"})) + assert(#res == 1, "one entry") + local d = res[1].attributes.description[1] + assert(#d == 3, "description length is 3 (not truncated at NUL): got " .. #d) + assert(d == "\1\0\2", "description bytes preserved") + c:close() + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] diff --git a/t/session.t b/t/session.t new file mode 100644 index 0000000..e081adb --- /dev/null +++ b/t/session.t @@ -0,0 +1,195 @@ +use Test::Nginx::Socket::Lua; + +log_level('info'); +no_shuffle(); +no_long_string(); +repeat_each(1); +plan 'no_plan'; + +our $HttpConfig = <<'_EOC_'; + lua_package_path 'lib/?.lua;/usr/share/lua/5.1/?.lua;;'; + lua_package_cpath 'deps/lib/lua/5.1/?.so;;'; + resolver 127.0.0.53; +_EOC_ + +run_tests(); + +__DATA__ + +=== TEST 1: bind and search share one pinned connection +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + local c = ldap_client:new("127.0.0.1", 1389) + + -- prime the keepalive pool with a single-shot op, so the pinned + -- checkout below is observable via getreusedtimes() (a brand-new + -- connection always reports 0, pooled ones report >= 1) + assert(c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + + assert(c:connect()) + local pinned_sock = c.socket + + local ok, err = c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") + assert(ok, "bind: " .. tostring(err)) + + local res, serr = c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(res, "search: " .. tostring(serr)) + assert(#res == 1 and res[1].entry_dn == "dc=example,dc=org", "base entry") + + -- the socket must have been reused (bind + search on the same conn) + assert(rawequal(c.socket, pinned_sock), "socket changed mid-session") + assert(c.socket:getreusedtimes() >= 1, "connection was not pinned/reused") + + assert(c:set_keepalive()) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + +=== TEST 2: single-shot (no session) still works unchanged +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + local c = ldap_client:new("127.0.0.1", 1389) + local res = assert(c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + assert(#res == 1, "one entry") + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + +=== TEST 3: close() unpins the session and the next op reconnects +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:connect()) + assert(c.pinned, "connect pins the session") + local pinned_sock = c.socket + + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + assert(rawequal(c.socket, pinned_sock), "socket changed mid-session") + + assert(c:close()) + assert(c.pinned == nil, "close unpins") + assert(c.socket == nil, "close drops the socket") + + -- a later op checks out its own connection, still unpinned + local res, err = c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(res, "search after close: " .. tostring(err)) + assert(#res == 1, "one entry") + assert(c.pinned == nil, "single-shot op stays unpinned") + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + +=== TEST 4: a hard socket error unpins the session instead of stranding it +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:connect()) + + -- kill the pinned connection underneath the client + c.socket:close() + + local ok, err = c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") + assert(not ok, "bind on a dead socket must fail") + assert(err ~= nil, "bind on a dead socket reports an error") + + -- the failure must release the pin, not strand the client on it + assert(c.pinned == nil, "hard error left the session pinned") + assert(c.socket == nil, "hard error left a dead socket attached") + + -- and the client is usable again without an explicit close() + local res, serr = c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(res, "client unusable after a hard error: " .. tostring(serr)) + assert(#res == 1, "one entry") + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- error_log +attempt to send data on a closed socket + +=== TEST 5: releasing a bound session closes the socket instead of pooling it +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + -- prime the pool so the pinned checkout below is observably reused + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + + assert(c:connect()) + assert(c.socket:getreusedtimes() >= 1, "pinned checkout should hit the pool") + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + assert(c:set_keepalive()) -- bound: must close, not pool + + -- a new anonymous client must get a fresh connection; its search + -- must not inherit the admin identity + local d = ldap_client:new("127.0.0.1", 1389) + assert(d:connect()) + assert(d.socket:getreusedtimes() == 0, + "admin-bound socket leaked into the pool") + local res, serr = d:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(res, "anonymous search: " .. tostring(serr)) + assert(#res == 1 and res[1].entry_dn == "dc=example,dc=org", "base entry") + assert(d:set_keepalive()) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] From 55bd373f565b28800320cdfa40cce723f98c730e Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 11:23:25 +0800 Subject: [PATCH 04/11] fix: report dropped connections and make test cleanup interrupt-safe Review follow-ups on this PR. _send_receive treated any non-timeout failure on the first header read as a clean end of stream, so a pooled socket the server had already dropped ("closed", not "timeout") returned an empty result and simple_bind answered nil with no error. ldap_authenticate then reported an unreachable server with no diagnostic. Break only once a message has been decoded. The test target reversed unknown_op.patch on failure but not on Ctrl-C. An EXIT trap alone is not enough: sh skips it when the shell is killed by an untrapped signal, so trap the signals too and let them exit normally. Also guard the decode_hostile.t nil-with-no-error arm, which indexed a nil result while reporting that the decoder had violated its contract. --- Makefile | 4 +++- lib/resty/ldap/client.lua | 6 ++++-- t/client.t | 39 +++++++++++++++++++++++++++++++++++++++ t/decode_hostile.t | 7 +++++-- 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 2711e95..afd4633 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,9 @@ dev: .PHONY: test test: git apply t/patch/unknown_op.patch - prove -r t/; ret=$$?; git apply t/patch/unknown_op.patch -R; exit $$ret + trap 'git apply t/patch/unknown_op.patch -R' EXIT; \ + trap 'exit 130' INT TERM HUP; \ + prove -r t/ ### help: Show Makefile rules .PHONY: help diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index 99ab603..d61325d 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -215,9 +215,11 @@ local function _send_receive(cli, request, multi_resp_hint) -- body packet. local len, err = socket:receive(2) if not len then - if err == "timeout" then + -- with nothing decoded yet this is a failed exchange, not a clean + -- end: a pooled socket the server already dropped reads "closed" + if err == "timeout" or #result == 0 then _reset_socket(cli) - return nil, fmt("receive response failed: %s", err) + return nil, fmt("receive response failed: %s", err or "unknown") end break -- read done, data has been taken to the end end diff --git a/t/client.t b/t/client.t index 2063b8a..0412abb 100644 --- a/t/client.t +++ b/t/client.t @@ -490,3 +490,42 @@ GET /t ok --- no_error_log [error] + + +=== TEST 16: a dropped connection before any response is a reported failure +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + -- a pooled socket the server has already dropped reads "closed", + -- not "timeout", on the very first header read + local closed = false + local sock = { + send = function(_, p) return #p end, + receive = function() return nil, "closed" end, + close = function() closed = true return true end, + } + + local client = ldap_client:new("127.0.0.1", 1389) + client.socket = sock + client.pinned = true + + local res, err = client:simple_bind( + "cn=user01,ou=users,dc=example,dc=org", "password1") + assert(res == nil, "a dropped connection must be a transport failure (nil)") + assert(err ~= nil, "it must carry a diagnostic, got nil") + assert(err:find("receive response failed", 1, true), + "unexpected err: " .. tostring(err)) + assert(closed, "the unusable socket must be closed") + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] diff --git a/t/decode_hostile.t b/t/decode_hostile.t index 2f2c4d1..bcb0292 100644 --- a/t/decode_hostile.t +++ b/t/decode_hostile.t @@ -400,9 +400,12 @@ ok local pok, res, err = pcall(protocol.decode_message, ldap_hex(c[2])) if not pok then table.insert(leaks, c[1] .. " RAISED: " .. tostring(res)) - elseif res ~= nil or err == nil then + elseif res ~= nil then table.insert(leaks, string.format("%s accepted (vals[1] is a %s)", - c[1], type(res.attributes.s and res.attributes.s[1]))) + c[1], type(res.attributes and res.attributes.s + and res.attributes.s[1]))) + elseif err == nil then + table.insert(leaks, c[1] .. " returned nil with no error") end end if #leaks > 0 then From 11817e04b21e639d2097401288aeb84b52710706 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 12:08:37 +0800 Subject: [PATCH 05/11] fix: fail a search that ends before SearchResultDone _send_receive treated a read failure as a clean end of stream once at least one message had been decoded, so a connection dropped after a few SearchResultEntry messages left search() returning those partial entries as a success and discarding the final result code. A read failure is never a clean end: a search ends at SearchResultDone and a single-response operation at its one message, and both already break out of the loop before reading again. Drop the break so EOF, RST and timeouts alike reset the socket and report an error. SearchResultDone then becomes the only way a multi-response read can succeed. Also drop the comment claiming a read timeout does not end the loop, which has been untrue since before this branch. --- lib/resty/ldap/client.lua | 14 ++++-------- t/client.t | 48 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index d61325d..ff66237 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -215,13 +215,11 @@ local function _send_receive(cli, request, multi_resp_hint) -- body packet. local len, err = socket:receive(2) if not len then - -- with nothing decoded yet this is a failed exchange, not a clean - -- end: a pooled socket the server already dropped reads "closed" - if err == "timeout" or #result == 0 then - _reset_socket(cli) - return nil, fmt("receive response failed: %s", err or "unknown") - end - break -- read done, data has been taken to the end + -- a search ends at SearchResultDone and a single-response op at its + -- one message; a timeout or close before either truncates the + -- exchange, so fail rather than return what arrived so far + _reset_socket(cli) + return nil, fmt("receive response failed: %s", err or "unknown") end local packet_len, packet_header, lerr = calculate_payload_length(len, 2, socket) @@ -259,8 +257,6 @@ local function _send_receive(cli, request, multi_resp_hint) -- just a straight stack of LDAP messages. Therefore the parser implementor -- does not know exactly how many bytes of data should be fetched, and has -- to read in greedy mode. - -- The socket read timeout will be used as a fallback when an exception is - -- encountered and this does not end the loop. if not multi_resp_hint or (res and res.protocol_op == protocol.APP_NO.SearchResultDone) then break diff --git a/t/client.t b/t/client.t index 0412abb..fc501b8 100644 --- a/t/client.t +++ b/t/client.t @@ -7,7 +7,7 @@ repeat_each(1); plan 'no_plan'; our $HttpConfig = <<'_EOC_'; - lua_package_path 'lib/?.lua;/usr/share/lua/5.1/?.lua;;'; + lua_package_path 'lib/?.lua;t/lib/?.lua;/usr/share/lua/5.1/?.lua;;'; lua_package_cpath 'deps/lib/lua/5.1/?.so;;'; resolver 127.0.0.53; _EOC_ @@ -529,3 +529,49 @@ GET /t ok --- no_error_log [error] + + +=== TEST 17: a search cut short before SearchResultDone must fail, not return partial results +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local ldap_hex = require("ldap_hex") + + -- one complete SearchResultEntry, then the peer goes away without + -- ever sending SearchResultDone + local entry = ldap_hex("30 16 02 01 03 64 11 04 01 78" .. + " 30 0c 30 0a 04 01 73 31 05 04 03 01 00 02") + local step, closed = 0, false + local sock = { + send = function(_, p) return #p end, + receive = function(_, n) + step = step + 1 + if step == 1 then return entry:sub(1, 2) end + if step == 2 then return entry:sub(3) end + return nil, "closed" + end, + close = function() closed = true return true end, + } + + local client = ldap_client:new("127.0.0.1", 1389) + client.socket = sock + client.pinned = true + + local res, err = client:search("dc=example,dc=org") + assert(res == false, + "a truncated search must fail; got " .. type(res) .. + " with " .. tostring(type(res) == "table" and #res or "n/a") .. " entries") + assert(err ~= nil, "it must carry a diagnostic, got nil") + assert(closed, "the unusable socket must be closed") + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] From f17d87178ea9eaba58c29d3c17818b5809fcd3f0 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 12:19:55 +0800 Subject: [PATCH 06/11] test: assert STARTTLS pools reuse within the same verification policy TEST 9 only asserted that a ssl_verify=true client refuses an unverified pooled connection, so an over-partitioned pool key would have disabled STARTTLS keepalive entirely without failing the test. Add the same-policy reuse control TEST 8 already carries for ldaps. --- t/client.t | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/client.t b/t/client.t index fc501b8..e397aa5 100644 --- a/t/client.t +++ b/t/client.t @@ -237,6 +237,12 @@ ok "an unverified pooled STARTTLS connection must not serve ssl_verify=true") assert(b:set_keepalive()) + -- control: the same policy does reuse its own pooled connection + local c = ldap_client:new("localhost", 1389, { start_tls = true, ssl_verify = false }) + assert(c:connect()) + assert(c.socket:getreusedtimes() > 0, "same-policy reuse should hit the pool") + assert(c:set_keepalive()) + ngx.say("ok") } } From 7bf0df3c9e9edb88a648d12094911d1f1312b88e Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 14:27:09 +0800 Subject: [PATCH 07/11] fix: require the target directory and verify TLS by default Two placeholder defaults in the compat entrypoint could silently point a bind at a directory the caller never configured. `ldap_host = "localhost"` and `base_dn = "ou=users,dc=example,dc=org"` are both gone; the keys are now checked before any socket I/O and return `nil, " is required"`. Getting either wrong can now only fail loudly. The remaining defaults are kept: a wrong `ldap_port` or `attribute` fails loudly on its own, and the rest are mode flags or timeouts. TLS verification now defaults to on, in `client_config.ssl_verify` as well as the compat `tls_verify`. `opts.ssl_verify or false` could not tell an omitted key from an explicit `false`, so the primary API was fail-open too. `verify_ldap_host` is no longer read. It was v0.1.0's key, inherited from kong; APISIX's ldap-auth has sent `tls_verify` since its first commit, and v0.1.0 reads neither that nor anything else by that name -- so `tls_verify` has been a silent no-op against the pinned 0.1.0. Honouring both keys is unnecessary once verification is the default: a caller who asked for verification still gets it, and one who deliberately disabled it via the old key now fails loudly instead of being downgraded in silence. APISIX is unaffected by the defaults: `ldap_uri` and `base_dn` are required by its plugin schema, and `tls_verify` is always populated from the schema default before it reaches the library. What does change for it is that `tls_verify: true` starts taking effect. BREAKING CHANGE: `resty.ldap.client` callers using `ldaps` or `start_tls` without an explicit `ssl_verify` now verify the server certificate, and need `lua_ssl_trusted_certificate` set or `ssl_verify = false` passed. --- README.md | 13 ++++---- lib/resty/ldap.lua | 23 ++++++------- lib/resty/ldap/client.lua | 7 +++- t/client.t | 27 ++++++++++++++-- t/compat_ldap.t | 68 ++++++++++++++++++++++++++++++++++----- 5 files changed, 106 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 6145e9d..94e4b68 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ local client = ldap_client:new("127.0.0.1", 1389, { keepalive_timeout = 60000, start_tls = false, ldaps = false, - ssl_verify = false + ssl_verify = true }) local err = client:simple_bind("cn=user01,ou=users,dc=example,dc=org", "password1") ``` @@ -49,7 +49,7 @@ To load this module: | `keepalive_timeout` | number | 60000 | An optional value in milliseconds that defines how long an idle connection to LDAP server will live before being closed. | | `start_tls` | boolean | false | Set it to `true` to issue StartTLS (Transport Layer Security) extended operation over ldap connection. If the start_tls setting is enabled, ensure the ldaps setting is disabled. | | `ldaps` | boolean | false | Set to `true` to connect using the LDAPS protocol (LDAP over TLS). When ldaps is configured, you must use port 636. If the ldap setting is enabled, ensure the start_tls setting is disabled. | -| `ssl_verify` | boolean | false | Set to true to authenticate LDAP server. The server certificate will be verified according to the CA certificates specified by the `lua_ssl_trusted_certificate` directive. | +| `ssl_verify` | boolean | true | Verify the LDAP server's certificate against the CA certificates specified by the `lua_ssl_trusted_certificate` directive. Set to `false` to skip verification. | | `keepalive_pool_name` | string | host:ip | Set and override the default connection pool name for scenarios where the same connection parameters are used but with a different authentication method. The default value is the same as the OpenResty rule, and the value is the `host:port` of the LDAP server. | | `keepalive_pool_size` | number | 2 | Set the size of a certain connection pool. According to OpenResty's rule, it can only be set when the pool is created and cannot be changed dynamically. | @@ -117,7 +117,7 @@ Compatibility entrypoint kept for APISIX's `ldap-auth` plugin, preserving the v0 **syntax:** *ok, err, user_dn = ldap.ldap_authenticate(username, password, conf)* -Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the server rejects the credentials, and `nil` on a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is always closed after the bind attempt — a socket that carried a Bind is never returned to the pool. +Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the server rejects the credentials, and `nil` on an invalid `conf` or a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is always closed after the bind attempt — a socket that carried a Bind is never returned to the pool. `username` is escaped per RFC 4514 when the bind DN is built, so DN metacharacters (`, + " \ < > ; =`, a leading space or `#`, a trailing space, or a NUL) are treated as literal characters of the RDN value instead of injecting extra RDN components. Any username string is accepted, as in v0.1.0. @@ -127,14 +127,13 @@ Binds against the directory as `=,` with the given | key | type | default value | Description | | ----------- | ----------- | ----------- | ----------- | -| `ldap_host` | string | localhost | LDAP server host. | +| `ldap_host` | string | *required* | LDAP server host. | | `ldap_port` | number | 389 | LDAP server port. | | `timeout` | number | 10000 | Socket timeout in milliseconds. | | `keepalive` | number | 60000 | Accepted for v0.1.0 compatibility, but unused: sockets that carried a bind are always closed, never pooled. | | `start_tls` | boolean | false | Issue the StartTLS extended operation before binding. | | `ldaps` | boolean | false | Connect using LDAP over TLS. | -| `tls_verify` | boolean | false | Verify the server certificate during the TLS handshake. This is the key APISIX's `ldap-auth` passes. | -| `verify_ldap_host` | boolean | false | Legacy alias for `tls_verify` (pre-0.2). Honoured only when `tls_verify` is not set. | -| `base_dn` | string | ou=users,dc=example,dc=org | Base DN the username is appended to. | +| `tls_verify` | boolean | true | Verify the server certificate during the TLS handshake. This is the key APISIX's `ldap-auth` passes. | +| `base_dn` | string | *required* | Base DN the username is appended to. | | `attribute` | string | cn | RDN attribute the username is bound as. | diff --git a/lib/resty/ldap.lua b/lib/resty/ldap.lua index 0583026..8f910cf 100644 --- a/lib/resty/ldap.lua +++ b/lib/resty/ldap.lua @@ -8,10 +8,9 @@ local tostring = tostring local default_conf = { timeout = 10000, start_tls = false, - ldap_host = "localhost", ldap_port = 389, ldaps = false, - base_dn = "ou=users,dc=example,dc=org", + tls_verify = true, attribute = "cn", keepalive = 60000, } @@ -25,17 +24,6 @@ local function set_conf_default_values(conf) end -local function resolve_tls_verify(conf) - if conf.tls_verify ~= nil then - return conf.tls_verify - end - if conf.verify_ldap_host ~= nil then - return conf.verify_ldap_host - end - return false -end - - -- RFC 4514 s2.4 escaping for an RDN value; the bind DN below is built from a -- client-supplied username. Not filter.escape(), which is RFC 4515 and leaves -- ',' and '+' alone. @@ -63,6 +51,13 @@ local _M = {} function _M.ldap_authenticate(given_username, given_password, conf) set_conf_default_values(conf) + if conf.ldap_host == nil then + return nil, "ldap_host is required" + end + if conf.base_dn == nil then + return nil, "base_dn is required" + end + -- same coercion contract as protocol.simple_bind_request if type(given_username) == "number" then given_username = tostring(given_username) @@ -76,7 +71,7 @@ function _M.ldap_authenticate(given_username, given_password, conf) keepalive_timeout = conf.keepalive, start_tls = conf.start_tls, ldaps = conf.ldaps, - ssl_verify = resolve_tls_verify(conf), + ssl_verify = conf.tls_verify, }) local ok, err = cli:connect() diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index ff66237..f84e05d 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -285,12 +285,17 @@ function _M.new(_, host, port, client_config) end local opts = client_config or {} + local ssl_verify = opts.ssl_verify + if ssl_verify == nil then + ssl_verify = true + end + local socket_config = { socket_timeout = opts.socket_timeout or 10000, keepalive_timeout = opts.keepalive_timeout or (60 * 1000), -- 10 min start_tls = opts.start_tls or false, ldaps = opts.ldaps or false, - ssl_verify = opts.ssl_verify or false, + ssl_verify = ssl_verify, -- Specify the connection pool name directly to ensure that connections -- with the same connection parameters but using different authentication diff --git a/t/client.t b/t/client.t index e397aa5..3c53270 100644 --- a/t/client.t +++ b/t/client.t @@ -92,7 +92,7 @@ simple bind failed, error: The supplied credential is invalid content_by_lua_block { local ldap_client = require("resty.ldap.client") - local client = ldap_client:new("127.0.0.1", 1636, { ldaps = true }) + local client = ldap_client:new("127.0.0.1", 1636, { ldaps = true, ssl_verify = false }) local res, err = client:simple_bind("cn=user01,ou=users,dc=example,dc=org", "password1") if not res then ngx.log(ngx.ERR, err) @@ -115,7 +115,7 @@ GET /t content_by_lua_block { local ldap_client = require("resty.ldap.client") - local client = ldap_client:new("127.0.0.1", 1389, { start_tls = true }) + local client = ldap_client:new("127.0.0.1", 1389, { start_tls = true, ssl_verify = false }) local res, err = client:simple_bind("cn=user01,ou=users,dc=example,dc=org", "password1") if not res then ngx.log(ngx.ERR, err) @@ -581,3 +581,26 @@ GET /t ok --- no_error_log [error] + + +=== TEST 18: an omitted ssl_verify verifies the server certificate +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + + -- no ssl_verify key, and no lua_ssl_trusted_certificate to satisfy it + local client = ldap_client:new("127.0.0.1", 1636, { ldaps = true }) + local res, err = client:simple_bind("cn=user01,ou=users,dc=example,dc=org", "password1") + assert(res == nil, "an omitted ssl_verify must still verify, got " .. tostring(res)) + assert(err:find("TLS handshake", 1, true), "unexpected err: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- error_log +certificate verify error diff --git a/t/compat_ldap.t b/t/compat_ldap.t index 0d0830f..ead7b3c 100644 --- a/t/compat_ldap.t +++ b/t/compat_ldap.t @@ -73,21 +73,21 @@ ok -=== TEST 3: verify_ldap_host=true is honoured as a legacy alias for tls_verify +=== TEST 3: verification is on when tls_verify is unset --- http_config eval: $::HttpConfig --- config location /t { content_by_lua_block { local ldap = require("resty.ldap") + -- no tls_verify key at all, and no lua_ssl_trusted_certificate local ok, err = ldap.ldap_authenticate("user01", "password1", { - ldap_host = "localhost", - ldap_port = 1636, - ldaps = true, - verify_ldap_host = true, - base_dn = "ou=users,dc=example,dc=org", - attribute = "cn", + ldap_host = "127.0.0.1", + ldap_port = 1636, + ldaps = true, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", }) - assert(ok == nil, "legacy verify_ldap_host must enforce verification: " .. + assert(ok == nil, "an unset tls_verify must still verify: " .. "expected a transport failure (nil), got " .. tostring(ok)) assert(err:find("TLS handshake", 1, true), "unexpected err: " .. tostring(err)) ngx.say("ok") @@ -228,3 +228,55 @@ GET /t ok --- error_log certificate verify error + + + +=== TEST 8: a missing ldap_host is a config error, never a bind against localhost +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok == nil, "a missing ldap_host must not authenticate, got " .. tostring(ok)) + assert(err == "ldap_host is required", "unexpected err: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + + +=== TEST 9: a missing base_dn is a config error, never a bind under a placeholder DN +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + -- the directory really does hold ou=users,dc=example,dc=org, so a + -- placeholder default would authenticate under a DN never configured + local ok, err = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "127.0.0.1", + ldap_port = 1389, + attribute = "cn", + }) + assert(ok == nil, "a missing base_dn must not authenticate, got " .. tostring(ok)) + assert(err == "base_dn is required", "unexpected err: " .. tostring(err)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] From e75f19a55ea5239824a7f5d0bae9aa08df75a9f6 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Mon, 27 Jul 2026 16:38:38 +0800 Subject: [PATCH 08/11] perf: reuse bind connections by partitioning the pool key A socket that carried a Bind was closed rather than pooled, so every authentication paid for a fresh connection. On `ldaps` or `start_tls` that is a TCP handshake plus a TLS handshake -- and for STARTTLS an extra ExtendedRequest round trip -- where reusing the connection costs one Bind, a single round trip. v0.1.0 pooled these sockets; closing them was a regression, and the reason for it was narrower than the remedy. The reason was real: the pool key is `host:port[:tls][:verify]` and cannot name a Bind identity, so a socket left authenticated as one user could be drawn by `search()`, which sends no Bind and expects to be anonymous. That search would then run under the previous user's authorization. Partition the key instead. Sockets that carried a Bind pool under a `:bind` suffix, drawn only by a caller whose next message is a BindRequest; everything else draws from the anonymous pool, which now never holds a socket that was bound. Reuse within `:bind` is safe because RFC 4513 s4 moves the session to anonymous on receipt of a Bind and only to the new identity if it succeeds -- so the previous identity is destroyed before the new credentials are checked, and a rejected Bind cannot inherit it. The Bind is therefore always sent, never skipped on a pool hit; skipping it on a DN-keyed pool would turn a pooled socket into a cached credential. `_send_receive` no longer returns the socket to the pool. setkeepalive is irreversible, so the response has to be validated first: a bind answered with something other than a BindResponse may have the real BindResponse still queued behind it, and that socket must be dropped rather than pooled. The previous close-on-bind behaviour was masking that case. Callers now release, and the return value of setkeepalive is logged -- "connection in dubious state" reports unread bytes left on a connection. `bound` is now set only on a pinned session. `connect()` has to pick a pool before the caller has said what it will do, so it draws from the anonymous pool and a Bind on that session leaves a socket that cannot be returned there; it is closed on release, as before. A single-shot bind needs no such flag: it came from `:bind` and belongs back there. --- README.md | 4 +- lib/resty/ldap.lua | 20 ++++------ lib/resty/ldap/client.lua | 77 ++++++++++++++++++++++++++++----------- t/client.t | 73 ++++++++++++++++++++++--------------- t/compat_ldap.t | 61 ++++++++++++++++++++++++++++--- 5 files changed, 163 insertions(+), 72 deletions(-) diff --git a/README.md b/README.md index 94e4b68..cc496f6 100644 --- a/README.md +++ b/README.md @@ -99,6 +99,8 @@ An unrecoverable socket error also releases the pin, so the next operation draws Releases a pinned connection back into the connection pool. The client remains usable; subsequent operations draw pooled connections as before. +A session that issued a `simple_bind` closes its connection instead of pooling it. + #### close **syntax:** *ok, err = client:close()* @@ -130,7 +132,7 @@ Binds against the directory as `=,` with the given | `ldap_host` | string | *required* | LDAP server host. | | `ldap_port` | number | 389 | LDAP server port. | | `timeout` | number | 10000 | Socket timeout in milliseconds. | -| `keepalive` | number | 60000 | Accepted for v0.1.0 compatibility, but unused: sockets that carried a bind are always closed, never pooled. | +| `keepalive` | number | 60000 | How long an idle connection lives in the pool, in milliseconds. | | `start_tls` | boolean | false | Issue the StartTLS extended operation before binding. | | `ldaps` | boolean | false | Connect using LDAP over TLS. | | `tls_verify` | boolean | true | Verify the server certificate during the TLS handshake. This is the key APISIX's `ldap-auth` passes. | diff --git a/lib/resty/ldap.lua b/lib/resty/ldap.lua index 8f910cf..f12b70d 100644 --- a/lib/resty/ldap.lua +++ b/lib/resty/ldap.lua @@ -51,10 +51,10 @@ local _M = {} function _M.ldap_authenticate(given_username, given_password, conf) set_conf_default_values(conf) - if conf.ldap_host == nil then + if conf.ldap_host == nil or conf.ldap_host == "" then return nil, "ldap_host is required" end - if conf.base_dn == nil then + if conf.base_dn == nil or conf.base_dn == "" then return nil, "base_dn is required" end @@ -74,22 +74,16 @@ function _M.ldap_authenticate(given_username, given_password, conf) ssl_verify = conf.tls_verify, }) - local ok, err = cli:connect() - if not ok then - log(ERR, "failed to connect to ", conf.ldap_host, ":", - tostring(conf.ldap_port), ": ", err) - return nil, err - end - local who = conf.attribute .. "=" .. escape_dn_value(given_username) .. "," .. conf.base_dn + -- not pinned: simple_bind validates before opening a socket, and releases it + -- into the bind pool for the next authenticate to reuse local is_authenticated, bind_err = cli:simple_bind(who, given_password) - -- the socket carried a Bind attempt: always close it, never pool it - cli:close() - if is_authenticated == nil then - -- transport failure mid-bind; the client already dropped the socket + -- connect, TLS or transport failure; the client already dropped the socket + log(ERR, "failed to bind to ", conf.ldap_host, ":", + tostring(conf.ldap_port), ": ", bind_err) return nil, bind_err end diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index f84e05d..fa2e7d1 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -3,6 +3,8 @@ local protocol = require("resty.ldap.protocol") local tostring = tostring local fmt = string.format +local log = ngx.log +local ERR = ngx.ERR local tcp = ngx.socket.tcp local table_insert = table.insert local decode_ldap = protocol.decode_message @@ -53,19 +55,20 @@ local function calculate_payload_length(encStr, pos, socket) return elen, encStr end +-- every failure path closes sock: _init_socket has not adopted it yet, so +-- nothing else can, and the connection would linger until garbage collection local function _start_tls(sock) -- send STARTTLS request local bytes, err = sock:send(protocol.start_tls_request()) if not bytes then + sock:close() return fmt("send request failed: %s", err) end -- receive STARTTLS response local len, err = sock:receive(2) if not len then - if err == "timeout" then - sock:close() - end + sock:close() return fmt("receive response header failed: %s", err) end local packet_len, packet_header, lerr = calculate_payload_length(len, 2, sock) @@ -105,7 +108,7 @@ local function _start_tls(sock) end end -local function _init_socket(self) +local function _init_socket(self, will_bind) local host = self.host local port = self.port local socket_config = self.socket_config @@ -126,6 +129,13 @@ local function _init_socket(self) pool_suffix = pool_suffix .. (socket_config.ssl_verify and ":verify" or ":noverify") end + -- Sockets that carried a Bind pool separately, drawn only by a caller that + -- binds first: RFC 4513 s4 resets the session to anonymous on receipt of that + -- Bind, so no identity carries over. Everything else stays anonymous. + if will_bind then + pool_suffix = pool_suffix .. ":bind" + end + local opts = { pool = host .. ":" .. port .. pool_suffix, pool_size = socket_config.keepalive_pool_size, @@ -147,12 +157,13 @@ local function _init_socket(self) local count, err = sock:getreusedtimes() if not count then -- connection was closed, just return instead + sock:close() return fmt("get %s:%s connection re-used time failed: %s", host, tostring(port), err) end if count == 0 then - -- STARTTLS + -- STARTTLS (_start_tls closes the socket on every failure) local err = _start_tls(sock) if err then return fmt("launch STARTTLS connection on %s:%s failed: %s", @@ -165,6 +176,7 @@ local function _init_socket(self) local _ _, err = sock:sslhandshake(true, host, socket_config.ssl_verify) if err ~= nil then + sock:close() return fmt("do TLS handshake on %s:%s failed: %s", host, tostring(port), err) end @@ -184,13 +196,32 @@ local function _reset_socket(cli) end end -local function _send_receive(cli, request, multi_resp_hint) +-- return a single-shot socket to its pool; a pinned one is released by its owner +local function _release_socket(cli) + if cli.pinned then + return + end + + local sock = cli.socket + cli.socket = nil + if not sock then + return + end + + local ok, err = sock:setkeepalive(cli.socket_config.keepalive_timeout) + if not ok then + -- ngx_lua already dropped the socket; log it because "connection in + -- dubious state" means a response went unread + log(ERR, "failed to return the connection to the pool: ", err) + end +end + +local function _send_receive(cli, request, multi_resp_hint, will_bind) -- In a pinned session the socket is already checked out; otherwise check out -- one for this single operation. if not cli.pinned then - local err = _init_socket(cli) + local err = _init_socket(cli, will_bind) if err then - cli.bound = nil return nil, fmt("initialize socket failed: %s", err) end end @@ -263,18 +294,8 @@ local function _send_receive(cli, request, multi_resp_hint) end end - -- single-shot ops return the socket to the pool; a pinned session is - -- released explicitly by the caller. A bound socket carries the Bind - -- identity while the pool key does not, so close it instead of pooling. - if not cli.pinned then - if cli.bound then - cli.bound = nil - socket:close() - else - socket:setkeepalive(cli.socket_config.keepalive_timeout) - end - end - + -- the caller releases: setkeepalive is irreversible, so the response has to + -- be validated first return multi_resp_hint and result or result[1] end @@ -365,8 +386,13 @@ function _M.simple_bind(self, dn, password) return false, berr end - self.bound = true - local res, err = _send_receive(self, req) + -- only pinned sockets need this: they came from the anonymous pool, so a Bind + -- leaves them unpoolable. A single-shot one came from the bind pool already. + if self.pinned then + self.bound = true + end + + local res, err = _send_receive(self, req, nil, true) if not res then -- transport/decode failure: nil, so callers can tell an unreachable -- or broken server (nil) from rejected credentials (false) @@ -374,10 +400,14 @@ function _M.simple_bind(self, dn, password) end if res.protocol_op ~= protocol.APP_NO.BindResponse then + -- the BindResponse may still be queued behind this one: not reusable + _reset_socket(self) return false, fmt("Received incorrect Op in packet: %d, expected %d", res.protocol_op, protocol.APP_NO.BindResponse) end + _release_socket(self) + if res.result_code ~= 0 then local error_msg = protocol.ERROR_MSG[res.result_code] @@ -412,6 +442,9 @@ function _M.search(self, base_dn, scope, deref_aliases, size_limit, time_limit, return false, err end + -- the exchange ended at SearchResultDone, so the socket is clean even on failure + _release_socket(self) + for index, item in ipairs(res) do if item.protocol_op == protocol.APP_NO.SearchResultDone then if item.result_code ~= 0 then diff --git a/t/client.t b/t/client.t index 3c53270..4e1ed90 100644 --- a/t/client.t +++ b/t/client.t @@ -298,7 +298,7 @@ ok -=== TEST 11: a single-shot bind never returns its socket to the shared pool +=== TEST 11: a single-shot bind pools apart from the anonymous pool --- http_config eval: $::HttpConfig --- config location /t { @@ -306,31 +306,35 @@ ok local ldap_client = require("resty.ldap.client") local protocol = require("resty.ldap.protocol") - -- control: an unbound single-shot search does re-enter the pool + -- prime the anonymous pool: an unbound single-shot search re-enters it local a = ldap_client:new("127.0.0.1", 1389) assert(a:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + + -- a single-shot admin bind draws from the bind pool, which is still + -- empty, so it opens its own connection and leaves that one alone local b = ldap_client:new("127.0.0.1", 1389) - assert(b:connect()) - assert(b.socket:getreusedtimes() > 0, "unbound search should hit the pool") - assert(b:set_keepalive()) + assert(b:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) - -- a single-shot admin bind checks out the pooled socket, binds on - -- it, and must close it on release instead of pooling it again + -- exactly one checkout behind the anonymous socket. Had the bind + -- borrowed it this would be 2 (pooled back) or 0 (closed), and + -- either way the search below could inherit the admin identity. local c = ldap_client:new("127.0.0.1", 1389) - assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) - - -- a new anonymous client must get a fresh connection; its search - -- must not inherit the admin identity - local d = ldap_client:new("127.0.0.1", 1389) - assert(d:connect()) - assert(d.socket:getreusedtimes() == 0, - "admin-bound socket leaked into the pool") - local res, serr = d:search("dc=example,dc=org", + assert(c:connect()) + assert(c.socket:getreusedtimes() == 1, + "a bind must not draw from the anonymous pool, got " .. + c.socket:getreusedtimes()) + local res, serr = c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") assert(res, "anonymous search: " .. tostring(serr)) assert(#res == 1, "one entry") - assert(d:set_keepalive()) + assert(c:set_keepalive()) + + -- the bind pool did keep its socket, so the next bind reuses it + local probe = ngx.socket.tcp() + assert(probe:connect("127.0.0.1", 1389, { pool = "127.0.0.1:1389:bind" })) + assert(probe:getreusedtimes() > 0, "the bound socket was not pooled") + probe:close() ngx.say("ok") } @@ -344,7 +348,7 @@ ok -=== TEST 12: ldap_authenticate never pools a socket that carried a bind +=== TEST 12: ldap_authenticate reuses the bind pool and never the anonymous one --- http_config eval: $::HttpConfig --- config location /t { @@ -357,24 +361,33 @@ ok attribute = "cn", } - -- a failed bind closes its socket instead of pooling it; the pool - -- is observable as the default host:port pool - local ok = ldap.ldap_authenticate("user01", "wrong-password", conf) - assert(ok == false, "a wrong password must be rejected (false), got " .. tostring(ok)) + -- the first call opens a connection, the second reuses it: one + -- socket, two binds on it + local ok, err = ldap.ldap_authenticate("user01", "password1", conf) + assert(ok, "authenticate failed: " .. tostring(err)) + local ok2, err2 = ldap.ldap_authenticate("user01", "password1", conf) + assert(ok2, "authenticate failed: " .. tostring(err2)) + local probe = ngx.socket.tcp() - assert(probe:connect("127.0.0.1", 1389)) - assert(probe:getreusedtimes() == 0, - "failed-bind socket leaked into the pool") + assert(probe:connect("127.0.0.1", 1389, { pool = "127.0.0.1:1389:bind" })) + assert(probe:getreusedtimes() == 2, + "the second authenticate should have reused the pooled socket, got " .. + probe:getreusedtimes()) probe:close() - -- a successful bind leaves the socket holding the user's identity, - -- so it too must be closed, never pooled - local ok2, err2 = ldap.ldap_authenticate("user01", "password1", conf) - assert(ok2, "authenticate failed: " .. tostring(err2)) + -- a rejected bind leaves the session anonymous (RFC 4513 s4), so its + -- socket is poolable too, and the next call still rebinds on it + local ok3 = ldap.ldap_authenticate("user01", "wrong-password", conf) + assert(ok3 == false, "a wrong password must be rejected (false), got " .. tostring(ok3)) + local ok4, err4 = ldap.ldap_authenticate("user01", "password1", conf) + assert(ok4, "a reused socket must still authenticate: " .. tostring(err4)) + + -- and none of those sockets ever entered the anonymous pool, where an + -- unbound search would have picked up a user's identity local probe2 = ngx.socket.tcp() assert(probe2:connect("127.0.0.1", 1389)) assert(probe2:getreusedtimes() == 0, - "user-bound socket leaked into the pool") + "a bound socket leaked into the anonymous pool") probe2:close() ngx.say("ok") diff --git a/t/compat_ldap.t b/t/compat_ldap.t index ead7b3c..4821ea8 100644 --- a/t/compat_ldap.t +++ b/t/compat_ldap.t @@ -24,7 +24,7 @@ __DATA__ local ldap = require("resty.ldap") -- no lua_ssl_trusted_certificate, so verifying the test CA must fail local ok, err = ldap.ldap_authenticate("user01", "password1", { - ldap_host = "localhost", + ldap_host = "127.0.0.1", ldap_port = 1636, ldaps = true, tls_verify = true, @@ -53,7 +53,7 @@ certificate verify error content_by_lua_block { local ldap = require("resty.ldap") local ok, err = ldap.ldap_authenticate("user01", "password1", { - ldap_host = "localhost", + ldap_host = "127.0.0.1", ldap_port = 1636, ldaps = true, tls_verify = false, @@ -196,7 +196,7 @@ ok -- authenticate with verification disabled first local ok, err = ldap.ldap_authenticate("user01", "password1", { - ldap_host = "localhost", + ldap_host = "127.0.0.1", ldap_port = 1636, ldaps = true, tls_verify = false, @@ -205,11 +205,11 @@ ok }) assert(ok, "unverified handshake should bind: " .. tostring(err)) - -- whatever state the first call left behind (it must not pool its - -- bound socket), a verifying call needs a fresh handshake, which + -- the first call pools its socket under :noverify:bind, so the + -- verifying call cannot draw it and needs a fresh handshake, which -- fails: no trusted certificate is configured local ok2, err2 = ldap.ldap_authenticate("user01", "password1", { - ldap_host = "localhost", + ldap_host = "127.0.0.1", ldap_port = 1636, ldaps = true, tls_verify = true, @@ -244,6 +244,16 @@ certificate verify error }) assert(ok == nil, "a missing ldap_host must not authenticate, got " .. tostring(ok)) assert(err == "ldap_host is required", "unexpected err: " .. tostring(err)) + + -- an empty string is the same misconfiguration, set rather than omitted + local ok2, err2 = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok2 == nil, "an empty ldap_host must not authenticate, got " .. tostring(ok2)) + assert(err2 == "ldap_host is required", "unexpected err: " .. tostring(err2)) ngx.say("ok") } } @@ -271,6 +281,45 @@ ok }) assert(ok == nil, "a missing base_dn must not authenticate, got " .. tostring(ok)) assert(err == "base_dn is required", "unexpected err: " .. tostring(err)) + + -- an empty base_dn would bind under "cn=user01," and be reported as + -- a credential failure rather than the misconfiguration it is + local ok2, err2 = ldap.ldap_authenticate("user01", "password1", { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "", + attribute = "cn", + }) + assert(ok2 == nil, "an empty base_dn must not authenticate, got " .. tostring(ok2)) + assert(err2 == "base_dn is required", "unexpected err: " .. tostring(err2)) + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] + + +=== TEST 10: an invalid password is reported without opening a socket +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + -- port 1 has no listener: a socket error here instead of the + -- validation message means the bind connected before checking + local ok, err = ldap.ldap_authenticate("user01", true, { + ldap_host = "127.0.0.1", + ldap_port = 1, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + }) + assert(ok == false, "a non-string password must be rejected (false), got " .. + tostring(ok)) + assert(err == "bind password must be a string", "unexpected err: " .. tostring(err)) ngx.say("ok") } } From d5cdb2bd79da4cbe7786c9f4a9ba5a31aa98554c Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Wed, 29 Jul 2026 10:03:04 +0800 Subject: [PATCH 09/11] refactor: hold the connection open until the caller releases it Drop connect()/pinned and the bound flag: the held socket is the session state. Operations open the connection lazily and never release it silently; the caller ends the session with set_keepalive() or close(). set_keepalive() still closes a connection that carried a bind after being drawn from the anonymous pool, so the bound identity cannot leak to an unauthenticated caller. --- README.md | 22 +++---- lib/resty/ldap.lua | 8 ++- lib/resty/ldap/client.lua | 109 +++++++++++----------------------- t/client.t | 44 ++++++++------ t/search.t | 13 ++--- t/search_ad.t | 2 - t/session.t | 119 +++++++++++++++++++++++++------------- 7 files changed, 163 insertions(+), 154 deletions(-) diff --git a/README.md b/README.md index cc496f6..6e37538 100644 --- a/README.md +++ b/README.md @@ -67,9 +67,9 @@ To load this module: #### search -**syntax:** *res, err = client:search(base_dn?, scope?, deref_aliases?, size_limit?, time_limit?, types_only?, filter?, attributes?)* +**syntax:** *res, err = client:search(base_dn, scope?, deref_aliases?, size_limit?, time_limit?, types_only?, filter?, attributes?)* -`base_dn` is the base DN you need to search. Default is `dc=example,dc=org`. +`base_dn` is the base DN you need to search. It is required; a `nil` value is rejected before any request is sent. `scope` is a flag field in the search protocol that specifies how the LDAP server performs the search, such as baseDN only, all subtrees, etc. You can import those values from protocol.lua, `SEARCH_SCOPE_BASE_OBJECT`, `SEARCH_SCOPE_SINGLE_LEVEL` and `SEARCH_SCOPE_WHOLE_SUBTREE`. Default is `SEARCH_SCOPE_WHOLE_SUBTREE`. @@ -85,27 +85,27 @@ To load this module: `attributes` is an array table that contains one to more query fields that you need to have the LDAP server return. Default is `["objectClass"]`. -#### connect +#### Connection lifecycle -**syntax:** *ok, err = client:connect()* +The client opens its connection on the first operation and holds it, so a `simple_bind` followed by a `search` runs on that one connection. Nothing is released implicitly: the caller ends the session with `set_keepalive` (return it to the pool) or `close` (drop it), once it is done issuing operations. -Establishes and pins the underlying connection, so that subsequent operations on this client (for example a `simple_bind` followed by a `search`) run on that same connection instead of each drawing one from the connection pool. Release the connection with `set_keepalive` or `close` when done. +> **Note:** this changed in v0.3.0. Operations used to return the connection to the pool on their own, so a caller that never released it still got connection reuse. Such a caller now holds the connection until it is garbage collected at the end of the request, and gets no reuse — add a `set_keepalive` or `close` call when you are done with the client. -An unrecoverable socket error also releases the pin, so the next operation draws a fresh connection instead of reusing the dead one. +An unrecoverable socket error drops the connection, so the next operation opens a new one instead of reusing the dead one. + +Connection pools are partitioned by transport mode (plain, STARTTLS, LDAPS) and TLS verification policy. A connection opened by a `simple_bind` is additionally drawn from — and returned to — a pool reserved for callers that bind, so releasing it never hands its identity to an unauthenticated caller; whoever draws it next binds first, which resets the LDAP session (RFC 4513 section 4). A client that runs unauthenticated operations and then a `simple_bind` keeps everything on its one connection, which now carries that identity; the client tracks this, and `set_keepalive` closes such a connection instead of returning it to the anonymous pool. #### set_keepalive **syntax:** *ok, err = client:set_keepalive()* -Releases a pinned connection back into the connection pool. The client remains usable; subsequent operations draw pooled connections as before. - -A session that issued a `simple_bind` closes its connection instead of pooling it. +Returns the connection to the connection pool and detaches it from the client, closing it instead when pooling would leak a bound identity (see [Connection lifecycle](#connection-lifecycle)). The client remains usable; the next operation opens or draws another connection. Returns `true` when there is no connection to release. #### close **syntax:** *ok, err = client:close()* -Closes a pinned connection without returning it to the pool. +Closes the connection without returning it to the pool, and detaches it from the client. ### resty.ldap @@ -119,7 +119,7 @@ Compatibility entrypoint kept for APISIX's `ldap-auth` plugin, preserving the v0 **syntax:** *ok, err, user_dn = ldap.ldap_authenticate(username, password, conf)* -Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the server rejects the credentials, and `nil` on an invalid `conf` or a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is always closed after the bind attempt — a socket that carried a Bind is never returned to the pool. +Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the server rejects the credentials, and `nil` on an invalid `conf` or a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is released after the bind attempt, into the pool reserved for connections that carry a Bind, so the next call reuses it. `username` is escaped per RFC 4514 when the bind DN is built, so DN metacharacters (`, + " \ < > ; =`, a leading space or `#`, a trailing space, or a NUL) are treated as literal characters of the RDN value instead of injecting extra RDN components. Any username string is accepted, as in v0.1.0. diff --git a/lib/resty/ldap.lua b/lib/resty/ldap.lua index f12b70d..f21169c 100644 --- a/lib/resty/ldap.lua +++ b/lib/resty/ldap.lua @@ -76,10 +76,14 @@ function _M.ldap_authenticate(given_username, given_password, conf) local who = conf.attribute .. "=" .. escape_dn_value(given_username) .. "," .. conf.base_dn - -- not pinned: simple_bind validates before opening a socket, and releases it - -- into the bind pool for the next authenticate to reuse local is_authenticated, bind_err = cli:simple_bind(who, given_password) + -- release the connection into the bind pool for the next call to reuse + local released, release_err = cli:set_keepalive() + if not released then + log(ERR, "failed to return the connection to the pool: ", release_err) + end + if is_authenticated == nil then -- connect, TLS or transport failure; the client already dropped the socket log(ERR, "failed to bind to ", conf.ldap_host, ":", diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index fa2e7d1..21644cb 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -3,8 +3,6 @@ local protocol = require("resty.ldap.protocol") local tostring = tostring local fmt = string.format -local log = ngx.log -local ERR = ngx.ERR local tcp = ngx.socket.tcp local table_insert = table.insert local decode_ldap = protocol.decode_message @@ -14,13 +12,18 @@ local decode_ldap = protocol.decode_message -- worker. 16 MiB comfortably exceeds any real entry. local MAX_LDAP_MESSAGE_SIZE = 16 * 1024 * 1024 +-- fixed BER TLV prefix: identifier octet + initial length octet +local BER_HEADER_LEN = 2 + local _M = {} local mt = { __index = _M } --- returns body length + header bytes, or nil + error (length is validated) -local function calculate_payload_length(encStr, pos, socket) +-- takes the BER_HEADER_LEN-byte header (reading further length octets from the +-- socket as needed); returns body length + header bytes, or nil + error +local function calculate_payload_length(encStr, socket) local elen + local pos = BER_HEADER_LEN -- the initial length octet is the header's last byte pos, elen = bunpack(encStr, "C", pos) @@ -66,12 +69,12 @@ local function _start_tls(sock) end -- receive STARTTLS response - local len, err = sock:receive(2) + local len, err = sock:receive(BER_HEADER_LEN) if not len then sock:close() return fmt("receive response header failed: %s", err) end - local packet_len, packet_header, lerr = calculate_payload_length(len, 2, sock) + local packet_len, packet_header, lerr = calculate_payload_length(len, sock) if not packet_len then sock:close() return lerr @@ -129,9 +132,8 @@ local function _init_socket(self, will_bind) pool_suffix = pool_suffix .. (socket_config.ssl_verify and ":verify" or ":noverify") end - -- Sockets that carried a Bind pool separately, drawn only by a caller that - -- binds first: RFC 4513 s4 resets the session to anonymous on receipt of that - -- Bind, so no identity carries over. Everything else stays anonymous. + -- a connection opened by a Bind pools separately: every drawer binds first, + -- and that Bind resets the session (RFC 4513 s4), so no identity carries over if will_bind then pool_suffix = pool_suffix .. ":bind" end @@ -183,43 +185,23 @@ local function _init_socket(self, will_bind) end self.socket = sock + self.from_bind_pool = will_bind end --- drop the socket (and its pin) after an unrecoverable error +-- drop the socket after an unrecoverable error local function _reset_socket(cli) local sock = cli.socket cli.socket = nil - cli.pinned = nil - cli.bound = nil + cli.from_bind_pool = nil + cli.unpoolable = nil if sock then sock:close() end end --- return a single-shot socket to its pool; a pinned one is released by its owner -local function _release_socket(cli) - if cli.pinned then - return - end - - local sock = cli.socket - cli.socket = nil - if not sock then - return - end - - local ok, err = sock:setkeepalive(cli.socket_config.keepalive_timeout) - if not ok then - -- ngx_lua already dropped the socket; log it because "connection in - -- dubious state" means a response went unread - log(ERR, "failed to return the connection to the pool: ", err) - end -end - local function _send_receive(cli, request, multi_resp_hint, will_bind) - -- In a pinned session the socket is already checked out; otherwise check out - -- one for this single operation. - if not cli.pinned then + -- opened on first use, held until the caller releases it + if not cli.socket then local err = _init_socket(cli, will_bind) if err then return nil, fmt("initialize socket failed: %s", err) @@ -237,14 +219,11 @@ local function _send_receive(cli, request, multi_resp_hint, will_bind) local result = {} -- When the client sends a search request, the server will return several - -- different entries in a string-like concatenation, sto we must use a + -- different entries in a string-like concatenation, so we must use a -- loop to complete the bulk extraction of the data. -- This does not affect the response of a single "response body". while true do - -- Takes the packet header of a single request body, which has a length - -- of two bytes, where the second byte is the length of this response - -- body packet. - local len, err = socket:receive(2) + local len, err = socket:receive(BER_HEADER_LEN) if not len then -- a search ends at SearchResultDone and a single-response op at its -- one message; a timeout or close before either truncates the @@ -253,7 +232,7 @@ local function _send_receive(cli, request, multi_resp_hint, will_bind) return nil, fmt("receive response failed: %s", err or "unknown") end - local packet_len, packet_header, lerr = calculate_payload_length(len, 2, socket) + local packet_len, packet_header, lerr = calculate_payload_length(len, socket) if not packet_len then _reset_socket(cli) return nil, lerr @@ -294,8 +273,6 @@ local function _send_receive(cli, request, multi_resp_hint, will_bind) end end - -- the caller releases: setkeepalive is irreversible, so the response has to - -- be validated first return multi_resp_hint and result or result[1] end @@ -335,31 +312,16 @@ function _M.new(_, host, port, client_config) end -function _M.connect(self) - if self.pinned then - return true - end - local err = _init_socket(self) - if err then - return nil, err - end - self.pinned = true - return true -end - - function _M.set_keepalive(self) - if not self.pinned then - return true - end - self.pinned = nil local sock = self.socket + local unpoolable = self.unpoolable self.socket = nil + self.from_bind_pool = nil + self.unpoolable = nil if not sock then return true end - if self.bound then - self.bound = nil + if unpoolable then return sock:close() end return sock:setkeepalive(self.socket_config.keepalive_timeout) @@ -367,10 +329,10 @@ end function _M.close(self) - self.pinned = nil - self.bound = nil local sock = self.socket self.socket = nil + self.from_bind_pool = nil + self.unpoolable = nil if not sock then return true end @@ -386,10 +348,10 @@ function _M.simple_bind(self, dn, password) return false, berr end - -- only pinned sockets need this: they came from the anonymous pool, so a Bind - -- leaves them unpoolable. A single-shot one came from the bind pool already. - if self.pinned then - self.bound = true + -- a bind on a connection drawn from the anonymous pool makes it unpoolable: + -- set_keepalive would hand its identity to the next anonymous drawer + if self.socket and not self.from_bind_pool then + self.unpoolable = true end local res, err = _send_receive(self, req, nil, true) @@ -406,8 +368,6 @@ function _M.simple_bind(self, dn, password) res.protocol_op, protocol.APP_NO.BindResponse) end - _release_socket(self) - if res.result_code ~= 0 then local error_msg = protocol.ERROR_MSG[res.result_code] @@ -423,8 +383,12 @@ end function _M.search(self, base_dn, scope, deref_aliases, size_limit, time_limit, types_only, filter, attributes) + if not base_dn then + return false, "base_dn cannot be nil" + end + local search_req, err = protocol.search_request( - base_dn or 'dc=example,dc=org', + base_dn, scope or protocol.SEARCH_SCOPE_WHOLE_SUBTREE, deref_aliases or protocol.SEARCH_DEREF_ALIASES_ALWAYS, size_limit or 0, -- size limit @@ -442,9 +406,6 @@ function _M.search(self, base_dn, scope, deref_aliases, size_limit, time_limit, return false, err end - -- the exchange ended at SearchResultDone, so the socket is clean even on failure - _release_socket(self) - for index, item in ipairs(res) do if item.protocol_op == protocol.APP_NO.SearchResultDone then if item.result_code ~= 0 then diff --git a/t/client.t b/t/client.t index 4e1ed90..3080f21 100644 --- a/t/client.t +++ b/t/client.t @@ -186,23 +186,29 @@ ok lua_ssl_trusted_certificate ../../certs/mycacert.crt; content_by_lua_block { local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + local function anon_search(c) + return c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + end -- pool a connection established without certificate verification local a = ldap_client:new("localhost", 1636, { ldaps = true, ssl_verify = false }) - assert(a:connect()) + assert(anon_search(a)) assert(a.socket:getreusedtimes() == 0, "first connection must be fresh") assert(a:set_keepalive()) -- ssl_verify=true must not reuse it (a fresh connection is required) local b = ldap_client:new("localhost", 1636, { ldaps = true, ssl_verify = true }) - assert(b:connect()) + assert(anon_search(b)) assert(b.socket:getreusedtimes() == 0, "an unverified pooled connection must not serve ssl_verify=true") assert(b:set_keepalive()) -- control: the same policy does reuse its own pooled connection local c = ldap_client:new("localhost", 1636, { ldaps = true, ssl_verify = false }) - assert(c:connect()) + assert(anon_search(c)) assert(c.socket:getreusedtimes() > 0, "same-policy reuse should hit the pool") assert(c:set_keepalive()) @@ -225,21 +231,27 @@ ok lua_ssl_trusted_certificate ../../certs/mycacert.crt; content_by_lua_block { local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + local function anon_search(c) + return c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + end local a = ldap_client:new("localhost", 1389, { start_tls = true, ssl_verify = false }) - assert(a:connect()) + assert(anon_search(a)) assert(a.socket:getreusedtimes() == 0, "first connection must be fresh") assert(a:set_keepalive()) local b = ldap_client:new("localhost", 1389, { start_tls = true, ssl_verify = true }) - assert(b:connect()) + assert(anon_search(b)) assert(b.socket:getreusedtimes() == 0, "an unverified pooled STARTTLS connection must not serve ssl_verify=true") assert(b:set_keepalive()) -- control: the same policy does reuse its own pooled connection local c = ldap_client:new("localhost", 1389, { start_tls = true, ssl_verify = false }) - assert(c:connect()) + assert(anon_search(c)) assert(c.socket:getreusedtimes() > 0, "same-policy reuse should hit the pool") assert(c:set_keepalive()) @@ -277,7 +289,6 @@ ok local client = ldap_client:new("127.0.0.1", 1389) client.socket = sock - client.pinned = true local res, err = client:simple_bind( "cn=user01,ou=users,dc=example,dc=org", "password1") @@ -306,27 +317,28 @@ ok local ldap_client = require("resty.ldap.client") local protocol = require("resty.ldap.protocol") - -- prime the anonymous pool: an unbound single-shot search re-enters it + -- prime the anonymous pool: an unbound search re-enters it on release local a = ldap_client:new("127.0.0.1", 1389) assert(a:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + assert(a:set_keepalive()) - -- a single-shot admin bind draws from the bind pool, which is still - -- empty, so it opens its own connection and leaves that one alone + -- an admin bind draws from the bind pool, which is still empty, so it + -- opens its own connection and leaves the anonymous one alone local b = ldap_client:new("127.0.0.1", 1389) assert(b:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + assert(b:set_keepalive()) -- exactly one checkout behind the anonymous socket. Had the bind -- borrowed it this would be 2 (pooled back) or 0 (closed), and -- either way the search below could inherit the admin identity. local c = ldap_client:new("127.0.0.1", 1389) - assert(c:connect()) - assert(c.socket:getreusedtimes() == 1, - "a bind must not draw from the anonymous pool, got " .. - c.socket:getreusedtimes()) local res, serr = c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") assert(res, "anonymous search: " .. tostring(serr)) + assert(c.socket:getreusedtimes() == 1, + "a bind must not draw from the anonymous pool, got " .. + c.socket:getreusedtimes()) assert(#res == 1, "one entry") assert(c:set_keepalive()) @@ -418,7 +430,6 @@ ok local client = ldap_client:new("127.0.0.1", 1389) client.socket = sock - client.pinned = true local res, err = client:simple_bind( "cn=user01,ou=users,dc=example,dc=org", "password1") @@ -461,7 +472,6 @@ ok local client = ldap_client:new("127.0.0.1", 1389) client.socket = sock - client.pinned = true local res, err = client:simple_bind( "cn=user01,ou=users,dc=example,dc=org", "password1") @@ -529,7 +539,6 @@ ok local client = ldap_client:new("127.0.0.1", 1389) client.socket = sock - client.pinned = true local res, err = client:simple_bind( "cn=user01,ou=users,dc=example,dc=org", "password1") @@ -576,7 +585,6 @@ ok local client = ldap_client:new("127.0.0.1", 1389) client.socket = sock - client.pinned = true local res, err = client:search("dc=example,dc=org") assert(res == false, diff --git a/t/search.t b/t/search.t index 07740a8..70c8ec8 100644 --- a/t/search.t +++ b/t/search.t @@ -24,7 +24,12 @@ __DATA__ local ldap_client = require("resty.ldap.client") local client = ldap_client:new("127.0.0.1", 1389) - local res, err = client:search() + + local bres, berr = client:search() + assert(bres == false, "nil base_dn must fail") + assert(berr == "base_dn cannot be nil", "got: " .. tostring(berr)) + + local res, err = client:search("dc=example,dc=org") if not res then ngx.log(ngx.ERR, err) ngx.exit(401) @@ -589,9 +594,6 @@ GET /t local client = ldap_client:new("127.0.0.1", 1389) - -- pin the session so the modify runs on the bound connection - assert(client:connect()) - -- auth local res, err = client:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") if not res then @@ -668,9 +670,6 @@ GET /t local client = ldap_client:new("127.0.0.1", 1389) - -- pin the session so the modify runs on the bound connection - assert(client:connect()) - -- auth local res, err = client:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") if not res then diff --git a/t/search_ad.t b/t/search_ad.t index 14bc0b9..b185da8 100644 --- a/t/search_ad.t +++ b/t/search_ad.t @@ -26,7 +26,6 @@ __DATA__ local filter = require("resty.ldap.filter") local c = ldap_client:new("127.0.0.1", 1389) - assert(c:connect()) assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) local login = "jdoe" @@ -57,7 +56,6 @@ ok local ldap_client = require("resty.ldap.client") local protocol = require("resty.ldap.protocol") local c = ldap_client:new("127.0.0.1", 1389) - assert(c:connect()) assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) local res = assert(c:search("ou=users,dc=example,dc=org", protocol.SEARCH_SCOPE_WHOLE_SUBTREE, nil, nil, nil, nil, diff --git a/t/session.t b/t/session.t index e081adb..ca25a11 100644 --- a/t/session.t +++ b/t/session.t @@ -16,7 +16,7 @@ run_tests(); __DATA__ -=== TEST 1: bind and search share one pinned connection +=== TEST 1: bind and search share one connection until it is released --- http_config eval: $::HttpConfig --- config location /t { @@ -26,28 +26,26 @@ __DATA__ local c = ldap_client:new("127.0.0.1", 1389) - -- prime the keepalive pool with a single-shot op, so the pinned - -- checkout below is observable via getreusedtimes() (a brand-new - -- connection always reports 0, pooled ones report >= 1) - assert(c:search("dc=example,dc=org", - protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) - - assert(c:connect()) - local pinned_sock = c.socket + -- prime the bind pool so reuse below is observable via getreusedtimes() + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + assert(c:set_keepalive()) local ok, err = c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") assert(ok, "bind: " .. tostring(err)) + local session_sock = c.socket + assert(session_sock:getreusedtimes() >= 1, + "a released bind connection should have been pooled and reused") local res, serr = c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") assert(res, "search: " .. tostring(serr)) assert(#res == 1 and res[1].entry_dn == "dc=example,dc=org", "base entry") - -- the socket must have been reused (bind + search on the same conn) - assert(rawequal(c.socket, pinned_sock), "socket changed mid-session") - assert(c.socket:getreusedtimes() >= 1, "connection was not pinned/reused") + -- the search must have run on the bind's connection, not its own + assert(rawequal(c.socket, session_sock), "socket changed mid-session") assert(c:set_keepalive()) + assert(c.socket == nil, "set_keepalive releases the connection") ngx.say("ok") } } @@ -58,7 +56,7 @@ ok --- no_error_log [error] -=== TEST 2: single-shot (no session) still works unchanged +=== TEST 2: a lone operation needs no explicit connect --- http_config eval: $::HttpConfig --- config location /t { @@ -69,6 +67,8 @@ ok local res = assert(c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) assert(#res == 1, "one entry") + assert(c.socket ~= nil, "the connection is held until released") + assert(c:set_keepalive()) ngx.say("ok") } } @@ -79,7 +79,7 @@ ok --- no_error_log [error] -=== TEST 3: close() unpins the session and the next op reconnects +=== TEST 3: close() drops the connection and the next op opens another --- http_config eval: $::HttpConfig --- config location /t { @@ -88,23 +88,20 @@ ok local protocol = require("resty.ldap.protocol") local c = ldap_client:new("127.0.0.1", 1389) - assert(c:connect()) - assert(c.pinned, "connect pins the session") - local pinned_sock = c.socket - assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) - assert(rawequal(c.socket, pinned_sock), "socket changed mid-session") + local session_sock = c.socket + assert(session_sock ~= nil, "the bind opened a connection") assert(c:close()) - assert(c.pinned == nil, "close unpins") assert(c.socket == nil, "close drops the socket") - -- a later op checks out its own connection, still unpinned + -- a later op opens its own connection local res, err = c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") assert(res, "search after close: " .. tostring(err)) assert(#res == 1, "one entry") - assert(c.pinned == nil, "single-shot op stays unpinned") + assert(not rawequal(c.socket, session_sock), "closed socket was reused") + assert(c:set_keepalive()) ngx.say("ok") } @@ -116,7 +113,7 @@ ok --- no_error_log [error] -=== TEST 4: a hard socket error unpins the session instead of stranding it +=== TEST 4: a hard socket error detaches the connection instead of stranding it --- http_config eval: $::HttpConfig --- config location /t { @@ -125,17 +122,17 @@ ok local protocol = require("resty.ldap.protocol") local c = ldap_client:new("127.0.0.1", 1389) - assert(c:connect()) + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) - -- kill the pinned connection underneath the client + -- kill the held connection underneath the client c.socket:close() - local ok, err = c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword") - assert(not ok, "bind on a dead socket must fail") - assert(err ~= nil, "bind on a dead socket reports an error") + local ok, err = c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(not ok, "a search on a dead socket must fail") + assert(err ~= nil, "a search on a dead socket reports an error") - -- the failure must release the pin, not strand the client on it - assert(c.pinned == nil, "hard error left the session pinned") + -- the failure must detach the dead socket, not strand the client on it assert(c.socket == nil, "hard error left a dead socket attached") -- and the client is usable again without an explicit close() @@ -143,6 +140,7 @@ ok protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") assert(res, "client unusable after a hard error: " .. tostring(serr)) assert(#res == 1, "one entry") + assert(c:set_keepalive()) ngx.say("ok") } @@ -154,7 +152,7 @@ ok --- error_log attempt to send data on a closed socket -=== TEST 5: releasing a bound session closes the socket instead of pooling it +=== TEST 5: a bind on an already-open connection stays on it; close ends the mix --- http_config eval: $::HttpConfig --- config location /t { @@ -162,25 +160,26 @@ attempt to send data on a closed socket local ldap_client = require("resty.ldap.client") local protocol = require("resty.ldap.protocol") - -- prime the pool so the pinned checkout below is observably reused + -- an unbound search opens the connection (anonymous pool) local c = ldap_client:new("127.0.0.1", 1389) assert(c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + local session_sock = c.socket - assert(c:connect()) - assert(c.socket:getreusedtimes() >= 1, "pinned checkout should hit the pool") + -- a later bind runs on that same connection assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) - assert(c:set_keepalive()) -- bound: must close, not pool + assert(rawequal(c.socket, session_sock), "the bind switched connections") - -- a new anonymous client must get a fresh connection; its search - -- must not inherit the admin identity + -- close: pooling would hand the admin identity to the next unbound caller + assert(c:close()) + + -- so the next anonymous search cannot draw the bound connection local d = ldap_client:new("127.0.0.1", 1389) - assert(d:connect()) - assert(d.socket:getreusedtimes() == 0, - "admin-bound socket leaked into the pool") local res, serr = d:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") assert(res, "anonymous search: " .. tostring(serr)) + assert(d.socket:getreusedtimes() == 0, + "the bound connection leaked into the anonymous pool") assert(#res == 1 and res[1].entry_dn == "dc=example,dc=org", "base entry") assert(d:set_keepalive()) @@ -193,3 +192,43 @@ GET /t ok --- no_error_log [error] + +=== TEST 6: set_keepalive after a bind on an anonymous-pool connection closes it +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap_client = require("resty.ldap.client") + local protocol = require("resty.ldap.protocol") + + -- an unbound search opens the connection (anonymous pool) + local c = ldap_client:new("127.0.0.1", 1389) + assert(c:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) + + -- a later bind runs on that same connection + assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + + -- the client must close here instead of pooling: this connection came + -- from the anonymous pool, and returning it would hand the admin + -- identity to the next unbound caller + assert(c:set_keepalive()) + assert(c.socket == nil, "set_keepalive releases the connection") + + local d = ldap_client:new("127.0.0.1", 1389) + local res, serr = d:search("dc=example,dc=org", + protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") + assert(res, "anonymous search: " .. tostring(serr)) + assert(d.socket:getreusedtimes() == 0, + "the bound connection leaked into the anonymous pool") + assert(d:set_keepalive()) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] From 1962f2d391a36eadb8e024e8e9b6145803f26312 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Wed, 29 Jul 2026 13:45:40 +0800 Subject: [PATCH 10/11] refactor: pool connections by address and TLS policy only Per review, bind state is no longer part of the pool key: the :bind suffix and the unpoolable release guard are gone, set_keepalive returns connections to the pool as-is, and every session establishes its own bind state on the wire (RFC 4513 s4). Transport and protocol failures still close the connection instead of pooling it. ldap_authenticate now refuses an empty or missing password before any request is sent: a non-empty DN with an empty password forms an unauthenticated bind (RFC 4513 s5.1.2) that some servers accept as an anonymous success. --- README.md | 6 +-- lib/resty/ldap.lua | 11 +++++- lib/resty/ldap/client.lua | 31 ++------------- t/client.t | 80 ++++++++++++++++++++++++++------------- t/compat_ldap.t | 2 +- t/session.t | 65 ++++++------------------------- 6 files changed, 84 insertions(+), 111 deletions(-) diff --git a/README.md b/README.md index 6e37538..d1aa427 100644 --- a/README.md +++ b/README.md @@ -93,13 +93,13 @@ The client opens its connection on the first operation and holds it, so a `simpl An unrecoverable socket error drops the connection, so the next operation opens a new one instead of reusing the dead one. -Connection pools are partitioned by transport mode (plain, STARTTLS, LDAPS) and TLS verification policy. A connection opened by a `simple_bind` is additionally drawn from — and returned to — a pool reserved for callers that bind, so releasing it never hands its identity to an unauthenticated caller; whoever draws it next binds first, which resets the LDAP session (RFC 4513 section 4). A client that runs unauthenticated operations and then a `simple_bind` keeps everything on its one connection, which now carries that identity; the client tracks this, and `set_keepalive` closes such a connection instead of returning it to the anonymous pool. +Connection pools are partitioned by transport mode (plain, STARTTLS, LDAPS) and TLS verification policy; bind state is not part of the pool key. A released connection keeps whatever bind state its session left, so a drawn connection's identity is unknown until the session establishes its own: issue a `simple_bind` first whenever identity matters — each bind resets the LDAP session (RFC 4513 section 4). A connection that hits a transport or protocol error is closed, never pooled. #### set_keepalive **syntax:** *ok, err = client:set_keepalive()* -Returns the connection to the connection pool and detaches it from the client, closing it instead when pooling would leak a bound identity (see [Connection lifecycle](#connection-lifecycle)). The client remains usable; the next operation opens or draws another connection. Returns `true` when there is no connection to release. +Returns the connection to the connection pool and detaches it from the client. The client remains usable; the next operation opens or draws another connection. Returns `true` when there is no connection to release. #### close @@ -119,7 +119,7 @@ Compatibility entrypoint kept for APISIX's `ldap-auth` plugin, preserving the v0 **syntax:** *ok, err, user_dn = ldap.ldap_authenticate(username, password, conf)* -Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the server rejects the credentials, and `nil` on an invalid `conf` or a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is released after the bind attempt, into the pool reserved for connections that carry a Bind, so the next call reuses it. +Binds against the directory as `=,` with the given password. `ok` is `true` when authentication succeeds, `false` when the credentials are rejected — an empty or missing password is refused before any request is sent, as it would form an unauthenticated bind (RFC 4513 section 5.1.2) — and `nil` on an invalid `conf` or a connect/TLS/transport failure; in both failure cases `err` carries the error. The connection is released into the pool after the bind attempt — whatever its outcome — and the next call reuses it and rebinds. `username` is escaped per RFC 4514 when the bind DN is built, so DN metacharacters (`, + " \ < > ; =`, a leading space or `#`, a trailing space, or a NUL) are treated as literal characters of the RDN value instead of injecting extra RDN components. Any username string is accepted, as in v0.1.0. diff --git a/lib/resty/ldap.lua b/lib/resty/ldap.lua index f21169c..ca4a546 100644 --- a/lib/resty/ldap.lua +++ b/lib/resty/ldap.lua @@ -66,6 +66,14 @@ function _M.ldap_authenticate(given_username, given_password, conf) return false, "bind username must be a string" end + -- RFC 4513 s5.1.2: a non-empty DN with an empty password is an + -- unauthenticated bind, which some servers accept as an anonymous + -- success -- an authentication bypass, so it must never reach the wire. + -- Other invalid types are rejected by protocol.simple_bind_request. + if given_password == nil or given_password == "" then + return false, "bind password is missing or empty" + end + local cli = client:new(conf.ldap_host, conf.ldap_port, { socket_timeout = conf.timeout, keepalive_timeout = conf.keepalive, @@ -78,7 +86,8 @@ function _M.ldap_authenticate(given_username, given_password, conf) "," .. conf.base_dn local is_authenticated, bind_err = cli:simple_bind(who, given_password) - -- release the connection into the bind pool for the next call to reuse + -- pooled even when the bind was rejected: a failed bind leaves the + -- session anonymous (RFC 4513 s4), and the next call rebinds anyway local released, release_err = cli:set_keepalive() if not released then log(ERR, "failed to return the connection to the pool: ", release_err) diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index 21644cb..98afd44 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -111,7 +111,7 @@ local function _start_tls(sock) end end -local function _init_socket(self, will_bind) +local function _init_socket(self) local host = self.host local port = self.port local socket_config = self.socket_config @@ -132,12 +132,6 @@ local function _init_socket(self, will_bind) pool_suffix = pool_suffix .. (socket_config.ssl_verify and ":verify" or ":noverify") end - -- a connection opened by a Bind pools separately: every drawer binds first, - -- and that Bind resets the session (RFC 4513 s4), so no identity carries over - if will_bind then - pool_suffix = pool_suffix .. ":bind" - end - local opts = { pool = host .. ":" .. port .. pool_suffix, pool_size = socket_config.keepalive_pool_size, @@ -185,24 +179,21 @@ local function _init_socket(self, will_bind) end self.socket = sock - self.from_bind_pool = will_bind end -- drop the socket after an unrecoverable error local function _reset_socket(cli) local sock = cli.socket cli.socket = nil - cli.from_bind_pool = nil - cli.unpoolable = nil if sock then sock:close() end end -local function _send_receive(cli, request, multi_resp_hint, will_bind) +local function _send_receive(cli, request, multi_resp_hint) -- opened on first use, held until the caller releases it if not cli.socket then - local err = _init_socket(cli, will_bind) + local err = _init_socket(cli) if err then return nil, fmt("initialize socket failed: %s", err) end @@ -314,16 +305,10 @@ end function _M.set_keepalive(self) local sock = self.socket - local unpoolable = self.unpoolable self.socket = nil - self.from_bind_pool = nil - self.unpoolable = nil if not sock then return true end - if unpoolable then - return sock:close() - end return sock:setkeepalive(self.socket_config.keepalive_timeout) end @@ -331,8 +316,6 @@ end function _M.close(self) local sock = self.socket self.socket = nil - self.from_bind_pool = nil - self.unpoolable = nil if not sock then return true end @@ -348,13 +331,7 @@ function _M.simple_bind(self, dn, password) return false, berr end - -- a bind on a connection drawn from the anonymous pool makes it unpoolable: - -- set_keepalive would hand its identity to the next anonymous drawer - if self.socket and not self.from_bind_pool then - self.unpoolable = true - end - - local res, err = _send_receive(self, req, nil, true) + local res, err = _send_receive(self, req) if not res then -- transport/decode failure: nil, so callers can tell an unreachable -- or broken server (nil) from rejected credentials (false) diff --git a/t/client.t b/t/client.t index 3080f21..adeccdc 100644 --- a/t/client.t +++ b/t/client.t @@ -309,7 +309,7 @@ ok -=== TEST 11: a single-shot bind pools apart from the anonymous pool +=== TEST 11: bind and unbound sessions share one pool --- http_config eval: $::HttpConfig --- config location /t { @@ -317,37 +317,29 @@ ok local ldap_client = require("resty.ldap.client") local protocol = require("resty.ldap.protocol") - -- prime the anonymous pool: an unbound search re-enters it on release + -- an unbound search opens a connection and pools it on release local a = ldap_client:new("127.0.0.1", 1389) assert(a:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) assert(a:set_keepalive()) - -- an admin bind draws from the bind pool, which is still empty, so it - -- opens its own connection and leaves the anonymous one alone + -- a bind draws that same pooled connection: the pool is keyed by + -- host, port and TLS policy only, not by bind state local b = ldap_client:new("127.0.0.1", 1389) assert(b:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) + assert(b.socket:getreusedtimes() == 1, + "the bind should draw the pooled connection, got " .. + b.socket:getreusedtimes()) assert(b:set_keepalive()) - -- exactly one checkout behind the anonymous socket. Had the bind - -- borrowed it this would be 2 (pooled back) or 0 (closed), and - -- either way the search below could inherit the admin identity. + -- and the bound connection re-enters the same pool for the next session local c = ldap_client:new("127.0.0.1", 1389) - local res, serr = c:search("dc=example,dc=org", - protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") - assert(res, "anonymous search: " .. tostring(serr)) - assert(c.socket:getreusedtimes() == 1, - "a bind must not draw from the anonymous pool, got " .. + assert(c:simple_bind("cn=user01,ou=users,dc=example,dc=org", "password1")) + assert(c.socket:getreusedtimes() == 2, + "the bound connection should have been pooled again, got " .. c.socket:getreusedtimes()) - assert(#res == 1, "one entry") assert(c:set_keepalive()) - -- the bind pool did keep its socket, so the next bind reuses it - local probe = ngx.socket.tcp() - assert(probe:connect("127.0.0.1", 1389, { pool = "127.0.0.1:1389:bind" })) - assert(probe:getreusedtimes() > 0, "the bound socket was not pooled") - probe:close() - ngx.say("ok") } } @@ -360,7 +352,7 @@ ok -=== TEST 12: ldap_authenticate reuses the bind pool and never the anonymous one +=== TEST 12: ldap_authenticate reuses one pooled connection across calls --- http_config eval: $::HttpConfig --- config location /t { @@ -381,25 +373,26 @@ ok assert(ok2, "authenticate failed: " .. tostring(err2)) local probe = ngx.socket.tcp() - assert(probe:connect("127.0.0.1", 1389, { pool = "127.0.0.1:1389:bind" })) + assert(probe:connect("127.0.0.1", 1389)) assert(probe:getreusedtimes() == 2, "the second authenticate should have reused the pooled socket, got " .. probe:getreusedtimes()) probe:close() -- a rejected bind leaves the session anonymous (RFC 4513 s4), so its - -- socket is poolable too, and the next call still rebinds on it + -- socket is pooled like any other, and the next call rebinds on it local ok3 = ldap.ldap_authenticate("user01", "wrong-password", conf) assert(ok3 == false, "a wrong password must be rejected (false), got " .. tostring(ok3)) local ok4, err4 = ldap.ldap_authenticate("user01", "password1", conf) assert(ok4, "a reused socket must still authenticate: " .. tostring(err4)) - -- and none of those sockets ever entered the anonymous pool, where an - -- unbound search would have picked up a user's identity + -- the rejected bind's socket was pooled and rebound: one socket, + -- two checkouts since the probe closed the previous one local probe2 = ngx.socket.tcp() assert(probe2:connect("127.0.0.1", 1389)) - assert(probe2:getreusedtimes() == 0, - "a bound socket leaked into the anonymous pool") + assert(probe2:getreusedtimes() == 2, + "the rejected bind's socket should have been pooled, got " .. + probe2:getreusedtimes()) probe2:close() ngx.say("ok") @@ -625,3 +618,38 @@ GET /t ok --- error_log certificate verify error + + +=== TEST 19: ldap_authenticate rejects an empty or missing password before any bind +--- http_config eval: $::HttpConfig +--- config + location /t { + content_by_lua_block { + local ldap = require("resty.ldap") + local conf = { + ldap_host = "127.0.0.1", + ldap_port = 1389, + base_dn = "ou=users,dc=example,dc=org", + attribute = "cn", + } + + -- RFC 4513 s5.1.2: an empty password with a non-empty DN is an + -- unauthenticated bind, which some servers accept as an anonymous + -- success; the client must refuse to send it + local ok, err = ldap.ldap_authenticate("user01", "", conf) + assert(ok == false, "an empty password must be rejected (false), got " .. tostring(ok)) + assert(err:find("missing or empty", 1, true), "unexpected err: " .. tostring(err)) + + local ok2, err2 = ldap.ldap_authenticate("user01", nil, conf) + assert(ok2 == false, "a nil password must be rejected (false), got " .. tostring(ok2)) + assert(err2:find("missing or empty", 1, true), "unexpected err: " .. tostring(err2)) + + ngx.say("ok") + } + } +--- request +GET /t +--- response_body +ok +--- no_error_log +[error] diff --git a/t/compat_ldap.t b/t/compat_ldap.t index 4821ea8..e21701c 100644 --- a/t/compat_ldap.t +++ b/t/compat_ldap.t @@ -205,7 +205,7 @@ ok }) assert(ok, "unverified handshake should bind: " .. tostring(err)) - -- the first call pools its socket under :noverify:bind, so the + -- the first call pools its socket under :noverify, so the -- verifying call cannot draw it and needs a fresh handshake, which -- fails: no trusted certificate is configured local ok2, err2 = ldap.ldap_authenticate("user01", "password1", { diff --git a/t/session.t b/t/session.t index ca25a11..ee29221 100644 --- a/t/session.t +++ b/t/session.t @@ -26,7 +26,7 @@ __DATA__ local c = ldap_client:new("127.0.0.1", 1389) - -- prime the bind pool so reuse below is observable via getreusedtimes() + -- prime the pool so reuse below is observable via getreusedtimes() assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) assert(c:set_keepalive()) @@ -152,7 +152,7 @@ ok --- error_log attempt to send data on a closed socket -=== TEST 5: a bind on an already-open connection stays on it; close ends the mix +=== TEST 5: a bound connection is pooled as-is and the next session rebinds on it --- http_config eval: $::HttpConfig --- config location /t { @@ -160,67 +160,26 @@ attempt to send data on a closed socket local ldap_client = require("resty.ldap.client") local protocol = require("resty.ldap.protocol") - -- an unbound search opens the connection (anonymous pool) + -- an unbound search opens the connection, a bind runs on it, and the + -- release pools it unchanged: bind state is not part of the pool key local c = ldap_client:new("127.0.0.1", 1389) assert(c:search("dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) local session_sock = c.socket - - -- a later bind runs on that same connection assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) assert(rawequal(c.socket, session_sock), "the bind switched connections") - - -- close: pooling would hand the admin identity to the next unbound caller - assert(c:close()) - - -- so the next anonymous search cannot draw the bound connection - local d = ldap_client:new("127.0.0.1", 1389) - local res, serr = d:search("dc=example,dc=org", - protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") - assert(res, "anonymous search: " .. tostring(serr)) - assert(d.socket:getreusedtimes() == 0, - "the bound connection leaked into the anonymous pool") - assert(#res == 1 and res[1].entry_dn == "dc=example,dc=org", "base entry") - assert(d:set_keepalive()) - - ngx.say("ok") - } - } ---- request -GET /t ---- response_body -ok ---- no_error_log -[error] - -=== TEST 6: set_keepalive after a bind on an anonymous-pool connection closes it ---- http_config eval: $::HttpConfig ---- config - location /t { - content_by_lua_block { - local ldap_client = require("resty.ldap.client") - local protocol = require("resty.ldap.protocol") - - -- an unbound search opens the connection (anonymous pool) - local c = ldap_client:new("127.0.0.1", 1389) - assert(c:search("dc=example,dc=org", - protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)")) - - -- a later bind runs on that same connection - assert(c:simple_bind("cn=admin,dc=example,dc=org", "adminpassword")) - - -- the client must close here instead of pooling: this connection came - -- from the anonymous pool, and returning it would hand the admin - -- identity to the next unbound caller assert(c:set_keepalive()) - assert(c.socket == nil, "set_keepalive releases the connection") + -- the next session draws that same connection and rebinds to reach its + -- own desired state: each bind resets the session (RFC 4513 s4) local d = ldap_client:new("127.0.0.1", 1389) - local res, serr = d:search("dc=example,dc=org", + assert(d:simple_bind("cn=user01,ou=users,dc=example,dc=org", "password1")) + assert(d.socket:getreusedtimes() == 1, + "the bound connection should have been pooled and reused, got " .. + d.socket:getreusedtimes()) + local res, serr = d:search("ou=users,dc=example,dc=org", protocol.SEARCH_SCOPE_BASE_OBJECT, nil, nil, nil, nil, "(objectClass=*)") - assert(res, "anonymous search: " .. tostring(serr)) - assert(d.socket:getreusedtimes() == 0, - "the bound connection leaked into the anonymous pool") + assert(res, "search on the rebound connection: " .. tostring(serr)) assert(d:set_keepalive()) ngx.say("ok") From f5d91e964ee14179ead43a44387ce7a21a2c6655 Mon Sep 17 00:00:00 2001 From: janiussyafiq Date: Wed, 29 Jul 2026 15:46:40 +0800 Subject: [PATCH 11/11] refactor: delegate _reset_socket to close() --- lib/resty/ldap/client.lua | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/resty/ldap/client.lua b/lib/resty/ldap/client.lua index 98afd44..9dcdc61 100644 --- a/lib/resty/ldap/client.lua +++ b/lib/resty/ldap/client.lua @@ -183,11 +183,7 @@ end -- drop the socket after an unrecoverable error local function _reset_socket(cli) - local sock = cli.socket - cli.socket = nil - if sock then - sock:close() - end + cli:close() end local function _send_receive(cli, request, multi_resp_hint)