feat: pure-Lua LDAP search (drop Rust rasn) - #32
Conversation
Restore get_object/decode for the pure-Lua LDAP response path (RFC api7/rfcs#116, slice 1): NUL-safe octet-string slicing, header-length based structural walk, indefinite-length rejection, and a recursion depth guard. Adds t/asn1.t (12 assertions).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces the Rust LDAP codec with Lua ASN.1 and LDAP processing, adds authentication and pinned-session APIs, strengthens malformed-input handling, updates packaging and CI provisioning, and expands integration coverage for TLS, searches, decoding, filtering, and binary attributes. ChangesLDAP protocol and ASN.1 implementation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
decode_message replaces the Rust rasn.decode_ldap, reproducing its exact output shape (LDAPResult ops; SearchResultEntry with always-array attributes) so client.lua and the existing tests are unchanged. Adds real SearchResultReference (op 19) handling, ModifyResponse/SearchResultReference to APP_NO, and envelope/tag-class guards. Adds t/decode.t (18 assertions).
…cation Two issues found in review: - decode: a constructed OCTET STRING (0x24) was accepted and its inner TLV bytes returned as the value. RFC 4511 s5.1 mandates primitive-only OCTET STRINGs; a constructed one is a BER-smuggling vector against auth parsers. Reject it, mirroring the existing indefinite-length rejection. - encode: asn1.put_object read its header back with a strlen-based ffi_string, truncating any length octet containing 0x00 (content length 256 -> 82 01 00), corrupting requests at 256-byte boundaries. Read exactly the bytes written. This also makes a zero-length value encode correctly as 80 00, so protocol.simple_bind_request no longer needs its manual anonymous-bind length-byte workaround. Adds t/asn1.t TEST 6 (constructed reject) and TEST 7 (length round-trip).
The LPeg grammar rejects raw < > (and =) inside a value, so escape()'s round-trip failed for those bytes; a username containing them would fail the (sAMAccountName=<input>) search. Escaping any octet is RFC 4515-legal and round-trips, so add \3c/\3e. Valid multi-byte UTF-8 still passes raw; a lone invalid-UTF-8 byte stays fail-closed. Adds t/filter.t TEST 102.
janiussyafiq
changed the title
- runs-on ubuntu-20.04 was retired by GitHub Actions; use ubuntu-latest - bitnami/openldap:2.6 is gone from Docker Hub; pin bitnamilegacy/openldap:2.6.10-debian-12-r4 - replace removed apt-key with a signed-by keyring for the OpenResty repo - drop the docker install block (preinstalled on runners); install Test::Nginx via cpanm --notest instead of cpan - replace the fixed sleep with a slapd readiness probe so loading ad.ldif does not race container bootstrap - regenerate t/certs as a set (slapd cert expired 2023-07-27; new certs valid to 2036) so the ldaps ssl_verify test passes in CI
- drop internal-doc and old-implementation references from comments (plan section citation, hardcoded-+2 note, Rust decoder narration); reword the strlen regression-test notes to present tense - clarify the put_object header-buffer size comment - renumber the new filter.t/client.t tests to follow the existing sequence (100-102 -> 5-7, 100 -> 7)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/resty/ldap/client.lua (1)
174-197: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPinned session state (
cli.pinned/cli.socket) is left inconsistent after a hard socket error.Lines 224-228 document that a pinned session is only released via
set_keepalive()/close(). But thesocket:close()calls on the timeout (176-178) and body-read-failure (195-196) paths close the underlying socket directly without clearingcli.pinned/cli.socket. A pinned client that hits one of these errors will keeppinned = truepointing at a now-closed socket; every subsequent call on that client will fail again against the dead socket until the caller happens to callclose(), which isn't obviously required by the contract described at 224-228.🔧 Proposed fix
local len, err = reader(2) if not len then if err == "timeout" then socket:close() + cli.socket = nil + cli.pinned = nil return nil, fmt("receive response failed: %s", err) end break end @@ local packet, err = socket:receive(packet_len) if not packet then socket:close() + cli.socket = nil + cli.pinned = nil return nil, err endAlso applies to: 224-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resty/ldap/client.lua` around lines 174 - 197, Update the timeout and packet-body read failure paths in the response-reading method to release the pinned session through the client’s existing cleanup mechanism, rather than directly closing the socket. Ensure both paths clear cli.pinned and cli.socket while preserving their current error returns, and keep the documented set_keepalive()/close() lifecycle consistent.
🧹 Nitpick comments (6)
.github/workflows/ci.yml (1)
17-17: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrevent credential persistence in the checkout action.
To improve security hygiene, it is recommended to set
persist-credentials: falseunless the workflow explicitly needs to push changes back to the repository. This prevents the GitHub token from remaining in the local.git/configwithin the runner.🛡️ Proposed fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 17, Update the actions/checkout@v4 step to disable credential persistence by setting persist-credentials to false, without changing other workflow behavior.Source: Linters/SAST tools
Makefile (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure the test patch is reverted if
provefails.In
make, if a command fails, the execution stops immediately. Ifprove -r t/fails, thegit apply -Rcommand will never be executed, leaving the local working tree modified and potentially causing subsequent test runs to fail.Modify the target to ensure the patch is reverted regardless of the test outcome, while preserving the exit code.
♻️ Proposed fix
test: git apply t/patch/unknown_op.patch - prove -r t/ - git apply t/patch/unknown_op.patch -R + `@prove` -r t/; RET=$$?; \ + git apply t/patch/unknown_op.patch -R; \ + exit $$RET🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 8 - 11, Update the Makefile test target around the git apply, prove, and reverse-apply commands so cleanup runs even when prove fails. Ensure the reverse patch command always executes, then preserve and return prove’s original exit status.lib/resty/ldap/asn1.lua (2)
273-292: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnrecognized tags silently decode to
nilwith no error.When
decoder[obj.tag]isnil(no registered decoder for that tag),decode()still returnsobj.offset + obj.len, nilwith no error — a "successful" decode that yields no value. In a hardened parser explicitly designed to surface decode errors rather than fail silently (per TEST 5's intent), this branch could let a caller mistake a missing decoder for a legitimatenil/absent value.♻️ Suggested fix: fail closed for unmapped tags
local d = decoder[obj.tag] local value, derr if d then value, derr = d(der, offset, obj, depth) if derr then return nil, nil, derr end + else + return nil, nil, "no decoder for tag " .. tostring(obj.tag) end return obj.offset + obj.len, value🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resty/ldap/asn1.lua` around lines 273 - 292, Update decode so an unrecognized obj.tag with no entry in decoder fails closed by returning a descriptive decode error instead of returning a successful offset with nil. Preserve the existing decoder invocation and error propagation for registered tags.
232-252: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ASN1_INTEGER_get/ASN1_ENUMERATED_gethave an ambiguous-1-on-error return.OpenSSL's docs state ASN1_INTEGER_get() also returns the value of a but it returns 0 if a is NULL and -1 on error (which is ambiguous because -1 is a legitimate value for an ASN1_INTEGER), and this ambiguity is called out explicitly as a reason to avoid the function: The ambiguous return values of ASN1_INTEGER_get() and ASN1_ENUMERATED_get() mean these functions should be avoided if possible. Here,
tonumber(v)is returned directly with no way to distinguish a legitimate-1value from a decode/overflow error. For LDAPmessageID/resultCodethis is low-risk in practice (small, well-bounded values), but it's a real correctness gap for any INTEGER whose true value happens to be-1or that overflows a Clong.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resty/ldap/asn1.lua` around lines 232 - 252, Replace the ASN1_INTEGER_get and ASN1_ENUMERATED_get conversions in the decoder[TAG.INTEGER] and decoder[TAG.ENUMERATED] handlers with an unambiguous value-decoding approach that preserves legitimate -1 values and detects overflow or conversion errors. Return a decoding error instead of silently accepting an ambiguous or out-of-range result, while retaining the existing ASN.1 object cleanup.t/asn1.t (1)
132-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for parent-boundary overrun.
TEST 5 covers truncated children (buffer too short overall) and zero-length INTEGER/ENUMERATED, but doesn't cover a child TLV that fits within the full buffer yet overruns its immediate parent's declared
len(e.g.30 03 04 05 41 42 43 44 45: outer SEQUENCE declares 3 bytes, inner OCTET STRING claims 5). See the related finding ondecode_childreninlib/resty/ldap/asn1.lua.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@t/asn1.t` around lines 132 - 165, Extend TEST 5 in t/asn1.t with a regression case where a child TLV exceeds its immediate parent’s declared length while remaining within the overall buffer, such as an outer SEQUENCE length of 3 containing an OCTET STRING claiming 5 bytes. Assert that asn1.decode returns no value and a non-nil error, preserving the existing success response and no-error-log expectations.t/session.t (1)
19-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
client:close().Only
connect()/set_keepalive()are exercised. Sinceclose()is a new part of the pinned-session contract (and interacts with the samepinned/socketfields flagged inclient.lua), a small test assertingclose()unpins and prevents reuse would round out coverage for this new feature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@t/session.t` around lines 19 - 60, Add a focused test alongside “bind and search share one pinned connection” that connects and pins a client, calls client:close(), then verifies the pinned socket state is cleared and a subsequent operation does not reuse that closed session. Assert the expected close behavior and keep the test isolated from the existing set_keepalive flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/resty/ldap.lua`:
- Around line 85-86: Secure the bind DN construction in the authentication flow
by validating or RFC 4514-escaping the attacker-controlled given_username before
concatenating it in who. Use DN-specific escaping for metacharacters and
leading/trailing whitespace or reject invalid usernames, while preserving the
existing ldap.bind_request call and configured conf.attribute/conf.base_dn
components.
In `@lib/resty/ldap/asn1.lua`:
- Around line 254-267: Update decode_children to validate each child’s returned
position against the parent boundary stop immediately after decode returns; if
pos exceeds stop, return a decode error instead of accepting the value or
exiting the loop. Preserve normal child collection when pos remains within the
declared boundary, and use the module’s existing error-construction conventions.
In `@lib/resty/ldap/client.lua`:
- Around line 68-71: Remove the raw packet contents from decode-failure errors
in both _start_tls and _send_recieve. Update the decode_ldap failure handling to
return only a generic descriptive message plus the decoder error (or “unknown”),
without calling to_hex(packet), while preserving the existing failure return
behavior.
- Around line 4-15: Extend the MAX_LDAP_MESSAGE_SIZE protection to every receive
using calculate_payload_length: in lib/resty/ldap/client.lua lines 59-61 within
_start_tls, and lib/resty/ldap/ldap.lua lines 75-77 within bind_request and
132-134 within start_tls, reject oversized packet_len values before
socket:receive by closing the socket and returning an error; keep the existing
_send_recieve guard and shared 16 MiB limit unchanged.
In `@lib/resty/ldap/ldap.lua`:
- Around line 35-56: Update calculate_payload_length to reject the BER
indefinite-length marker 0x80 before interpreting long-form lengths, rather than
treating it as a 128-byte length. Apply the same validation in the corresponding
length-parsing logic in the LDAP client module, preserving existing handling for
valid definite lengths.
In `@lib/resty/ldap/protocol.lua`:
- Around line 205-229: Add bounds validation in parse_search_entry for every
decoded PartialAttribute: reject the attribute if pa.offset + pa.len exceeds
astop, and ensure the type and values decodes remain within the attribute’s
declared range before accepting them. Return the relevant parse error on
violations, while preserving normal attribute accumulation and iteration for
valid entries.
- Around line 231-244: Update parse_search_reference so each asn1_decode result
is validated against stop before accepting the decoded URI and adding it to
uris; return an error when the decoded element advances beyond the operation
boundary, matching the bounds handling used by
parse_search_entry/decode_children.
- Around line 191-203: Update parse_ldap_result to enforce the operation
boundary op.offset + op.len while decoding resultCode, matchedDN, and
diagnosticMessage. Validate each returned position before continuing, reject any
TLV that extends beyond the boundary, and ensure the final diagnosticMessage
position remains within the same limit.
---
Outside diff comments:
In `@lib/resty/ldap/client.lua`:
- Around line 174-197: Update the timeout and packet-body read failure paths in
the response-reading method to release the pinned session through the client’s
existing cleanup mechanism, rather than directly closing the socket. Ensure both
paths clear cli.pinned and cli.socket while preserving their current error
returns, and keep the documented set_keepalive()/close() lifecycle consistent.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 17: Update the actions/checkout@v4 step to disable credential persistence
by setting persist-credentials to false, without changing other workflow
behavior.
In `@lib/resty/ldap/asn1.lua`:
- Around line 273-292: Update decode so an unrecognized obj.tag with no entry in
decoder fails closed by returning a descriptive decode error instead of
returning a successful offset with nil. Preserve the existing decoder invocation
and error propagation for registered tags.
- Around line 232-252: Replace the ASN1_INTEGER_get and ASN1_ENUMERATED_get
conversions in the decoder[TAG.INTEGER] and decoder[TAG.ENUMERATED] handlers
with an unambiguous value-decoding approach that preserves legitimate -1 values
and detects overflow or conversion errors. Return a decoding error instead of
silently accepting an ambiguous or out-of-range result, while retaining the
existing ASN.1 object cleanup.
In `@Makefile`:
- Around line 8-11: Update the Makefile test target around the git apply, prove,
and reverse-apply commands so cleanup runs even when prove fails. Ensure the
reverse patch command always executes, then preserve and return prove’s original
exit status.
In `@t/asn1.t`:
- Around line 132-165: Extend TEST 5 in t/asn1.t with a regression case where a
child TLV exceeds its immediate parent’s declared length while remaining within
the overall buffer, such as an outer SEQUENCE length of 3 containing an OCTET
STRING claiming 5 bytes. Assert that asn1.decode returns no value and a non-nil
error, preserving the existing success response and no-error-log expectations.
In `@t/session.t`:
- Around line 19-60: Add a focused test alongside “bind and search share one
pinned connection” that connects and pins a client, calls client:close(), then
verifies the pinned socket state is cleared and a subsequent operation does not
reuse that closed session. Assert the expected close behavior and keep the test
isolated from the existing set_keepalive flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf980dfe-f183-4ba9-89be-f0dff13a9ccf
⛔ Files ignored due to path filters (2)
t/certs/localhost_slapd_cert.pemis excluded by!**/*.pemt/certs/localhost_slapd_key.pemis excluded by!**/*.pem
📒 Files selected for processing (29)
.cargo/config.toml.github/workflows/ci.yml.gitignoreCargo.tomlMakefileREADME.mdlib/resty/ldap.lualib/resty/ldap/asn1.lualib/resty/ldap/client.lualib/resty/ldap/filter.lualib/resty/ldap/ldap.lualib/resty/ldap/protocol.luarockspec/lua-resty-ldap-0.3.0-0.rockspecrockspec/lua-resty-ldap-local-0.rockspecrockspec/lua-resty-ldap-main-0.rockspecsrc/ldap_codec/decoder.rssrc/ldap_codec/encoder.rssrc/ldap_codec/mod.rssrc/lib.rst/asn1.tt/certs/mycacert.crtt/client.tt/decode.tt/filter.tt/fixtures/ad.ldift/search.tt/search_ad.tt/session.tutils/check-rust.sh
💤 Files with no reviewable changes (8)
- Cargo.toml
- .cargo/config.toml
- src/ldap_codec/encoder.rs
- utils/check-rust.sh
- .gitignore
- src/ldap_codec/mod.rs
- src/ldap_codec/decoder.rs
- src/lib.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/resty/ldap/client.lua (2)
222-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPacket-body receive failure returns a bare error, unlike the sibling timeout branch.
Line 229 returns
errdirectly, while the header-timeout branch a few lines above (211) wraps it withfmt("receive response failed: %s", err). Same failure category, inconsistent context in the message.🐛 Proposed fix
local packet, err = socket:receive(packet_len) if not packet then -- 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. _reset_socket(cli) - return nil, err + return nil, fmt("receive response failed: %s", err) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resty/ldap/client.lua` around lines 222 - 230, Update the packet-body receive failure branch in the LDAP response-reading flow to wrap the receive error with the same “receive response failed” context used by the header-timeout branch, while preserving socket reset and nil return behavior.
86-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSocket not closed on STARTTLS decode/op-mismatch/result-code failure.
Every other failure path in
_start_tls(send failure, header-receive failure, body-receive failure) closessockbefore returning. The decode-failure, op-mismatch, and result-code branches at lines 88-105 don't, leaving the socket open until GC or end of request. This runs pre-TLS, so a malformed/malicious STARTTLS response repeatedly triggers this leak path.🐛 Proposed fix
local packet = packet_header .. packet local res, err = decode_ldap(packet) if not res then -- the body can carry DNs and attribute values; keep it out of the error + sock:close() 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 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 "") end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resty/ldap/client.lua` around lines 86 - 105, Update the failure branches in _start_tls after decode_ldap, protocol_op validation, and result_code validation to close sock before returning. Preserve each existing error message and ensure all three pre-TLS STARTTLS response failure paths explicitly release the socket, matching the existing send and receive failure handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/resty/ldap/client.lua`:
- Around line 222-230: Update the packet-body receive failure branch in the LDAP
response-reading flow to wrap the receive error with the same “receive response
failed” context used by the header-timeout branch, while preserving socket reset
and nil return behavior.
- Around line 86-105: Update the failure branches in _start_tls after
decode_ldap, protocol_op validation, and result_code validation to close sock
before returning. Preserve each existing error message and ensure all three
pre-TLS STARTTLS response failure paths explicitly release the socket, matching
the existing send and receive failure handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edd2de19-26f7-48a9-a2df-3294a5696e3e
📒 Files selected for processing (11)
MakefileREADME.mdlib/resty/ldap.lualib/resty/ldap/asn1.lualib/resty/ldap/client.lualib/resty/ldap/ldap.lualib/resty/ldap/protocol.luat/asn1.tt/decode.tt/search.tt/session.t
🚧 Files skipped from review as they are similar to previous changes (9)
- t/search.t
- t/asn1.t
- lib/resty/ldap.lua
- t/decode.t
- lib/resty/ldap/asn1.lua
- lib/resty/ldap/ldap.lua
- lib/resty/ldap/protocol.lua
- Makefile
- README.md
[P1] TLS certificate verification is silently disabled for APISIX
|
Harden the ASN.1 and LDAP message layers against malformed and hostile input, and add a pure-Lua test suite covering the new behaviour. asn1.lua: - reject non-canonical tag forms, enforce primitive/constructed form per universal type, and validate decode offsets before pointer arithmetic - refuse integers outside the exactly-representable 2^53 range on both encode and decode - reject non-string/non-boolean payloads instead of emitting a header that disagrees with the bytes; return errors rather than bare nils - free the buffer i2d_* allocates (fixes a per-encode leak in the worker) protocol.lua: - pin the RFC 4511 ASN.1 type of every decoded field - enforce a single protocolOp per LDAPMessage (plus optional controls) and an explicit response-op allowlist - type-guard the bind and search request builders client.lua / ldap.lua: - validate the BER length header, drop and unpin the socket on error, and guard bind argument types Also gitignore the local deps/ luarocks build tree.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@t/lib/ldap_hex.lua`:
- Around line 4-6: Update the hex-decoding function in t/lib/ldap_hex.lua to
validate the whitespace-stripped input before decoding: reject odd-length
strings and any non-hex characters instead of preserving unmatched text. Keep
valid pairs decoded through string.char and surface malformed input immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e037b39f-4c81-4e94-8176-c5cb4d9180d2
📒 Files selected for processing (25)
.gitignorelib/resty/ldap/asn1.lualib/resty/ldap/client.lualib/resty/ldap/ldap.lualib/resty/ldap/protocol.luat/asn1.tt/asn1_bounds.tt/asn1_class.tt/asn1_constructed.tt/asn1_depth.tt/asn1_encode.tt/asn1_encode_leak.tt/asn1_ffi.tt/asn1_int_precision.tt/asn1_integer.tt/asn1_nil_member.tt/asn1_oracle.tt/asn1_vectors.tt/decode.tt/decode_hostile.tt/decode_member_count.tt/decode_op_dispatch.tt/decode_protocol.tt/decode_rfc4511.tt/lib/ldap_hex.lua
🚧 Files skipped from review as they are similar to previous changes (5)
- t/decode.t
- t/asn1.t
- lib/resty/ldap/client.lua
- lib/resty/ldap/ldap.lua
- lib/resty/ldap/asn1.lua
…nd TLS verification handling
Delete 41 duplicate test blocks and dissolve t/decode_member_count.t and t/asn1_int_precision.t into their canonical homes; strengthen each home first (offset assertions, migrated vectors, provenance headers) so no coverage is lost. Verified by mutation testing: re-introducing each hardened-away bug still fails a surviving test.
[P1] Keepalive pool reuse can bypass TLS certificate verificationThis is a merge blocker on the current head Both the compatibility path and As a result, a route that explicitly requires certificate verification can unknowingly send LDAP credentials over a connection that was established without certificate validation. Please partition the pool by both transport mode and verification policy (for example, |
[P2] Bind DN and APISIX Consumer DN still divergeFollow-up on the current head
Please use one canonical DN construction on both sides, or return and use the actual LDAP entry DN instead of reconstructing it independently. |
|
no ci? |
…ve TLS connection handling
Loosen the TLS verify error_log pattern to a substring stable across lua-nginx-module versions, and raise the client timeout on the encode leak tests, which exceed Test::Nginx's 3s default on slow runners.
|
@membphis addressed all comments, CI just need approval to run |
LDAP framing is BER length-driven; receiveuntil("\0") silently consumed
0x00 bytes at the header position and crashed calculate_payload_length on
hostile responses.
[P1][New] Authenticated LDAP sockets can leak into the generic keepalive poolThis is a merge blocker on the current head LDAP Bind state persists on a connection, but both the single-shot Please track whether a socket has been bound and never return an authenticated connection to the generic pool. Closing bound connections is the safest default. Please also add a same-worker regression covering: admin Bind -> release -> new anonymous client -> search must not inherit the admin identity. Non-blocking follow-ups for awareness:
The P2 items are FYI and do not need to block this PR. |
A Bind pins an identity to the connection, but the keepalive pool key only encodes endpoint and TLS policy, so a pooled bound socket could serve a later client's requests under the previous identity. Track bind state and close such sockets on release instead of pooling them. Both module-level message ID counters now wrap back to 1 past RFC 4511 maxInt. Also renames the file-local _send_recieve to _send_receive.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P2] Use the canonical authenticated DN for APISIX Consumer lookup
The latest head fixes the bound-socket pooling and message-ID issues, but this identity mismatch remains. ldap_authenticate() now RFC 4514-escapes the username and returns the canonical Bind DN as its third result. APISIX still ignores that value and rebuilds user_dn from the raw username, so a username containing DN metacharacters can bind successfully but fail Consumer lookup.
Please have APISIX consume the returned canonical DN (or the actual LDAP entry DN) and add an end-to-end case covering a DN-special-character username.
|
@membphis will track that issue in APISIX repo upon version release. |
|
|
||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Please use the latest version and pin it using a hash.
For example:
https://github.com/api7/adc/blob/main/.github/workflows/e2e.yaml#L18
There was a problem hiding this comment.
What's the difference between this and client.lua?
It looks like we have a lot of entrypoint files right now:
resty.ldap
resty.ldap.ldap
resty.ldap.client
I'm a little confused.
There was a problem hiding this comment.
For backward compatibality since I'm restoring file from v0.1.0 to ensure zero config change for existing ldap-auth, but i do agree it seems confusing. I think this can be refactored.
…rk-only) Temporary fork-CI branch: overlay api7/lua-resty-ldap#32 into deps after make deps, and trigger CI on push of this branch. Not for upstream.
…rk-only) Temporary fork-CI branch: overlay api7/lua-resty-ldap#32 into deps after make deps, and trigger CI on push of this branch. Not for upstream.
resty.ldap.ldap duplicated the client transport stack line for line (calculate_payload_length, bind/start_tls send-receive loops, error tables, message-id counter). ldap_authenticate is now a thin wrapper over resty.ldap.client, leaving two entrypoints: resty.ldap.client (the API) and resty.ldap (compat facade for APISIX ldap-auth). - a socket that carried a Bind is always closed, never pooled, whether the bind succeeded or failed - client:simple_bind returns nil on transport/decode failures and false only when the server rejects the bind, so the facade keeps its nil=unreachable / false=rejected contract - header-timeout and truncated-body seams ported to client.t; the dead-socket and message-id-wrap seams were already covered by session.t and asn1_encode.t
Post-merge follow-upsAfter this PR is merged, two follow-up areas remain:
These are post-merge production-readiness follow-ups and do not block this refactor. |
3 participants
Replaces the Rust
rasndecoder with pure Lua and drops the Rust toolchain entirely — installing now needs onlylua_packandlpeg(build.type = "builtin", v0.3.0 rockspec).Changes
asn1.get_object/asn1.decode) and message decoder (protocol.decode_message) producing the same output shape as the Rust path;SearchResultReferenceis now actually decoded.OCTET STRINGrejected (RFC 4511 §5.1), INTEGER exact-or-error beyond 2^53, recursion-depth and 16 MiB message caps, trailing bytes rejected, decoder errors surface instead of returning empty success. Non-minimal BER lengths stay accepted on purpose — AD/slapd interop.put_objectno longer truncates length headers containing0x00octets (corrupted requests at 256-byte boundaries).client:connect()/set_keepalive()/close()) so bind + search share one socket;filter.escape()per RFC 4515 §3, including<and>.resty.ldap/resty.ldap.ldaprestored so the existingldap-authplugin keeps working;ldap_authenticatehardened (DN-metacharacter escaping, TLS verification options).Tests
Unit suites cover the ASN.1 primitives, message decoder, hardening regressions, and encoding invariants; e2e suites run against a live slapd. The suite was deduplicated into one canonical home per invariant (−1,324 lines), with zero coverage loss verified by mutation testing: re-introducing each hardened-away bug still fails a surviving test.