feat: add ldap-auth-advanced plugin (core authentication) - #13762
feat: add ldap-auth-advanced plugin (core authentication)#13762janiussyafiq wants to merge 2 commits into
Conversation
Add the ldap-auth-advanced plugin (priority 2541), Kong ldap-auth-advanced parity, core authentication: search-then-bind (AD sAMAccountName shape), service-account or anonymous search bind, user_dn Consumer association, LDAPS/StartTLS, LDAP filter-injection defense, strict 401-vs-500 auth/transport error split, multi-auth compatibility. Bump lua-resty-ldap 0.1.0 -> 0.3.0 (pure-Lua client/protocol/filter; the released client is lazy-connect, so the plugin binds first and classifies a (nil, err) return as a transport failure). Note: under 0.1.0 the ldap-auth plugin's tls_verify was a silent no-op (the library read verify_ldap_host); 0.3.0 fixes the field name, so tls_verify:true configs begin real certificate verification. Also regenerate t/certs/localhost_slapd_cert.pem: the old cert has CN=test.com but no subjectAltName, and nginx's hostname verification does not fall back to CN, so any real tls_verify handshake against it fails with "certificate host mismatch". The new cert (same key, same test CA) adds SAN DNS:test.com, DNS:localhost, IP:127.0.0.1. Group collection/authorization and credential caching/anonymous-consumer follow in separate PRs; docs follow up separately.
dosubot
Bot
added
size:XXL
|
|
||
| function _M.rewrite(conf, ctx) | ||
| -- Strip any client-supplied X-Authenticated-Groups before any auth work. | ||
| core.request.set_header(ctx, "X-Authenticated-Groups", nil) |
There was a problem hiding this comment.
P1: Could we also clear X-Consumer-Username, X-Consumer-Custom-ID, and X-Credential-Identifier here? With consumer_required=false, attach_consumer() is skipped, so client-supplied values for these standard identity headers pass through unchanged. An authenticated client can therefore impersonate another Consumer to an upstream that trusts APISIX identity headers. All APISIX-authenticated identity headers should be stripped before authentication, then only values derived from the matched Consumer should be added.
|
|
||
| -- RFC 4512 attribute-description shape: a leading letter, then letters, | ||
| -- digits, semicolons (option separators) or hyphens. | ||
| local ATTR_PATTERN = "^[A-Za-z][A-Za-z0-9;-]*$" |
There was a problem hiding this comment.
P2: RFC 4512 allows an AttributeType to be either a descriptor or a numeric OID. This pattern rejects valid values such as 1.2.840.113556.1.4.656, so directories configured with OID attribute names cannot enable the plugin. Could we accept the numericoid form as well while still validating any ;option suffixes?
| -- Proxy-Authorization is checked before Authorization | ||
| local header_name = "Proxy-Authorization" | ||
| local auth_header = core.request.header(ctx, header_name) | ||
| if not auth_header then |
There was a problem hiding this comment.
P2: Please fall back to Authorization when Proxy-Authorization is present but cannot produce credentials for conf.header_type. A forward proxy can send its own Proxy-Authorization: Basic ... while forwarding a valid end-user Authorization: ldap ...; currently the mere presence of the proxy header makes the valid Authorization unreachable and returns 401. Proxy credentials should take priority only after scheme, base64, and separator parsing succeeds.
| return false | ||
| end | ||
| return str_sub(err, 1, 18) == "simple bind failed" | ||
| or str_sub(err, 1, 13) == "search failed" |
There was a problem hiding this comment.
P2: This classifies every LDAP operation result as a client authentication failure. A service-account bind rejected after password rotation and search results such as busy or unavailable all have these prefixes, so the route returns 401 even though the client credentials have not been evaluated. Could we classify by operation and result code—for example, return 401 only for invalidCredentials from the final user bind (plus the not-found/ambiguous cases handled below), and treat search-bind/search operational failures as 5xx?
There was a problem hiding this comment.
I've some reserve on this but already addressed. Can u take a look at Test 62 and forward and see if this is what you expected?
https://github.com/apache/apisix/pull/13762/changes#diff-7fddff66e1de9a3b6a75a346d6bf0579d9bcad7e764425094fd3983160e56fa0R1417
| local function build_consumer_maps(consumer_conf) | ||
| local by_user_dn = {} | ||
| for _, node in ipairs(consumer_conf.nodes) do | ||
| local auth_conf = node.auth_conf |
There was a problem hiding this comment.
P2: node.auth_conf still contains literal $secret://... or $ENV://... references here. This custom map bypasses consumer_mod.consumers_kv(), which clones the auth config, resolves secrets, and skips unresolved references fail-closed. As a result, a valid Consumer whose user_dn is stored by reference can never match the DN returned by LDAP. Please build the map from resolved Consumer config or reuse consumers_kv().
| for _, node in ipairs(consumer_conf.nodes) do | ||
| local auth_conf = node.auth_conf | ||
| if auth_conf and auth_conf.user_dn then | ||
| by_user_dn[auth_conf.user_dn] = node |
There was a problem hiding this comment.
P2: Please also register ldap-auth-advanced → user_dn in plugin_unique_key_attrs in apisix/consumer.lua. Otherwise the Admin API accepts duplicate direct user_dn values and this assignment silently lets the last-loaded Consumer win, so Consumer-level ACL or rate-limit policy can run under the wrong identity.
- strip client-supplied X-Consumer-Username/X-Consumer-Custom-ID/ X-Credential-Identifier (with X-Authenticated-Groups) before any auth work, so consumer_required=false cannot pass spoofed identity upstream - accept RFC 4512 numeric-OID attribute types and tighten ;option validation in the attribute schema pattern - fall back to Authorization when Proxy-Authorization is present but does not parse into credentials for header_type - classify directory failures by operation and result code: 401 only for invalidCredentials on the user bind plus the not-found/ambiguous cases (including sizeLimitExceeded, the >size_limit ambiguity); service-bind rejections and search operational failures are 500 - look up Consumers via consumer.find_consumer() so a secret-ref user_dn resolves (and unresolved references fail closed) - register ldap-auth-advanced user_dn in plugin_unique_key_attrs so the Admin API rejects duplicate user_dn values
2 participants
Description
Adds the
ldap-auth-advancedplugin (priority 2541), bringing Kongldap-auth-advancedparity for core authentication: search-then-bind (ADsAMAccountNameshape), service-account or anonymous search bind,user_dnConsumer association, LDAPS/StartTLS, LDAP filter-injection defense, and a strict 401-vs-500 auth/transport error split. This is part 1 of 3 (core authentication); group collection/authorization and credential caching/anonymous-consumer follow in separate PRs. Prior discussion in #8958.This PR also bumps
lua-resty-ldap0.1.0 -> 0.3.0. Under 0.1.0, the existingldap-authplugin'stls_verifyoption was a silent no-op (the library read a different field name); 0.3.0 fixes this, sotls_verify: trueconfigs now perform real certificate verification. This also required regeneratingt/certs/localhost_slapd_cert.pem(same key, same test CA) to add asubjectAltName(DNS:test.com, DNS:localhost, IP:127.0.0.1) — the old cert hadCN=test.combut no SAN, and nginx's hostname verification does not fall back to CN, so the newly-realldap-authTLS-verify test started failing a certificate host-mismatch check that the 0.1.0 bug had been silently masking.Which issue(s) this PR fixes:
Related: #8958
Checklist
lua-resty-ldapbump activates realtls_verifycertificate verification, previously a silent no-op under 0.1.0 -- see description)