nic-6443 commented on code in PR #13762: URL: https://github.com/apache/apisix/pull/13762#discussion_r3689714325
########## apisix/plugins/ldap-auth-advanced.lua: ########## @@ -0,0 +1,382 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one or more +-- contributor license agreements. See the NOTICE file distributed with +-- this work for additional information regarding copyright ownership. +-- The ASF licenses this file to You under the Apache License, Version 2.0 +-- (the "License"); you may not use this file except in compliance with +-- the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. +-- +local core = require("apisix.core") +local schema_def = require("apisix.schema_def") +local auth_utils = require("apisix.utils.auth") +local consumer_mod = require("apisix.consumer") +local ldap_client = require("resty.ldap.client") +local ldap_protocol = require("resty.ldap.protocol") +local ldap_filter = require("resty.ldap.filter") +local ngx = ngx +local ipairs = ipairs +local type = type +local ngx_decode_base64 = ngx.decode_base64 +local ngx_re_match = ngx.re.match +local str_find = string.find +local str_sub = string.sub +local parse_addr = core.utils.parse_addr + +-- 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;-]*$" + +local schema = { + type = "object", + title = "work with route or service object", + properties = { + -- connection + ldap_uri = { type = "string" }, -- "host[:port]" + use_ldaps = { type = "boolean", default = false }, + use_starttls = { type = "boolean", default = false }, + ssl_verify = { type = "boolean", default = true }, + timeout = { type = "integer", minimum = 1, maximum = 60000, + default = 3000 }, -- milliseconds + + -- connection pool + keepalive = { type = "boolean", default = true }, + keepalive_timeout = { type = "integer", minimum = 1000, default = 60000 }, + keepalive_pool_size = { type = "integer", minimum = 1, default = 5 }, + keepalive_pool_name = { type = "string" }, + + -- user resolution (search-then-bind) + base_dn = { type = "string" }, -- search root + attribute = { type = "string", -- filter: (attribute=username) + default = "cn", pattern = ATTR_PATTERN }, + bind_dn = { type = "string" }, -- absent => anonymous search + ldap_password = { type = "string" }, + + -- search bounds + size_limit = { type = "integer", minimum = 2, default = 2 }, + time_limit = { type = "integer", minimum = 0, default = 5 }, -- seconds; 0 = server default + + + + -- consumer + consumer_required = { type = "boolean", default = true }, + + -- request handling + header_type = { type = "string", enum = {"ldap", "basic"}, default = "ldap" }, + realm = schema_def.get_realm_schema("ldap"), + + }, + encrypt_fields = {"ldap_password"}, + required = {"ldap_uri", "base_dn"}, +} + +local consumer_schema = { + type = "object", + title = "work with consumer object", + properties = { + user_dn = { type = "string" }, + }, + required = {"user_dn"}, +} + +local plugin_name = "ldap-auth-advanced" + + +local _M = { + version = 0.1, + priority = 2541, + type = 'auth', + name = plugin_name, + schema = schema, + consumer_schema = consumer_schema, +} + +function _M.check_schema(conf, schema_type) + if schema_type == core.schema.TYPE_CONSUMER then + return core.schema.check(consumer_schema, conf) + end + + local ok, err = core.schema.check(schema, conf) + if not ok then + return false, err + end + + if conf.use_ldaps and conf.use_starttls then + return false, "use_ldaps and use_starttls are mutually exclusive" + end + + if conf.bind_dn and not conf.ldap_password then + return false, "ldap_password is required when bind_dn is set" + end + + -- ldap_uri may omit ":port"; the effective port (636 with use_ldaps, + -- else 389) is resolved when the connection is opened. + + return true +end + + + +-- Shared 401 helper for the authentication-failure paths. +local function auth_failed(conf, ctx, reason) + + -- under multi-auth, decline quietly and let the wrapper render the 401 + if auth_utils.is_running_under_multi_auth(ctx) then + return 401 + end + + if reason then + core.log.warn(plugin_name, ": ", reason) + end + core.response.set_header("WWW-Authenticate", + conf.header_type .. " realm=\"" .. conf.realm .. "\"") + return 401, { message = "Authorization required" } +end + + + + +-- Extract username/password from the credential header: the scheme word is +-- conf.header_type ("ldap" or "basic", case-insensitive), the payload is +-- base64("username:password"). +local function extract_credentials(conf, ctx) + -- 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 Review Comment: Thanks, scheme mismatches and decode failures now fall back correctly. One unusable case still bypasses the fallback: Lua treats `""` as truthy, so `Proxy-Authorization: ldap OnBhc3M=` (`:pass`) or `ldap dXNlcjo=` (`user:`) enters this branch; the later empty-field check returns 401 without trying a valid `Authorization`. Please reject empty usernames/passwords before granting Proxy priority and cover both cases. ########## apisix/plugins/ldap-auth-advanced.lua: ########## @@ -0,0 +1,399 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one or more +-- contributor license agreements. See the NOTICE file distributed with +-- this work for additional information regarding copyright ownership. +-- The ASF licenses this file to You under the Apache License, Version 2.0 +-- (the "License"); you may not use this file except in compliance with +-- the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. +-- +local core = require("apisix.core") +local schema_def = require("apisix.schema_def") +local auth_utils = require("apisix.utils.auth") +local consumer_mod = require("apisix.consumer") +local ldap_client = require("resty.ldap.client") +local ldap_protocol = require("resty.ldap.protocol") +local ldap_filter = require("resty.ldap.filter") +local ngx = ngx +local ipairs = ipairs +local type = type +local ngx_decode_base64 = ngx.decode_base64 +local ngx_re_match = ngx.re.match +local str_find = string.find +local str_sub = string.sub +local parse_addr = core.utils.parse_addr + +-- RFC 4512 attribute-description: a descriptor ("cn", "sAMAccountName") or a +-- numeric OID ("1.2.840.113556.1.4.656"), either optionally followed by +-- ";option" suffixes ("cn;lang-en", "1.2.840.113556.1.4.656;binary"). +local ATTR_PATTERN = "^(?:[A-Za-z][A-Za-z0-9-]*" + .. "|(?:0|[1-9][0-9]*)(?:\\.(?:0|[1-9][0-9]*))+)" + .. "(?:;[A-Za-z0-9-]+)*$" + +local schema = { + type = "object", + title = "work with route or service object", + properties = { + -- connection + ldap_uri = { type = "string", -- "host[:port]" + minLength = 1, maxLength = 256 }, + use_ldaps = { type = "boolean", default = false }, + use_starttls = { type = "boolean", default = false }, + ssl_verify = { type = "boolean", default = true }, + timeout = { type = "integer", minimum = 1, maximum = 60000, + default = 10000 }, -- milliseconds + + -- connection pool + keepalive = { type = "boolean", default = true }, + keepalive_timeout = { type = "integer", minimum = 1000, default = 60000 }, + keepalive_pool_size = { type = "integer", minimum = 1, default = 5 }, + keepalive_pool_name = { type = "string", minLength = 1, maxLength = 256 }, + + -- user resolution (search-then-bind) + base_dn = { type = "string", -- search root + minLength = 1, maxLength = 4096 }, + attribute = { type = "string", maxLength = 256, -- filter: (attribute=username) + default = "cn", pattern = ATTR_PATTERN }, + bind_dn = { type = "string", -- absent => anonymous search + minLength = 1, maxLength = 4096 }, + ldap_password = { type = "string", minLength = 1, maxLength = 4096 }, + + -- search bounds + size_limit = { type = "integer", minimum = 2, default = 2 }, + time_limit = { type = "integer", minimum = 0, default = 5 }, -- seconds; 0 = server default + + + + -- consumer + consumer_required = { type = "boolean", default = true }, + + -- request handling + header_type = { type = "string", enum = {"ldap", "basic"}, default = "ldap" }, + realm = schema_def.get_realm_schema("ldap"), + + }, + encrypt_fields = {"ldap_password"}, + required = {"ldap_uri", "base_dn"}, +} + +local consumer_schema = { + type = "object", + title = "work with consumer object", + properties = { + user_dn = { type = "string", minLength = 1, maxLength = 4096 }, + }, + required = {"user_dn"}, +} + +local plugin_name = "ldap-auth-advanced" + + +local _M = { + version = 0.1, + priority = 2541, + type = 'auth', + name = plugin_name, + schema = schema, + consumer_schema = consumer_schema, +} + +function _M.check_schema(conf, schema_type) + if schema_type == core.schema.TYPE_CONSUMER then + return core.schema.check(consumer_schema, conf) + end + + local ok, err = core.schema.check(schema, conf) + if not ok then + return false, err + end + + if conf.use_ldaps and conf.use_starttls then + return false, "use_ldaps and use_starttls are mutually exclusive" + end + + if conf.bind_dn and not conf.ldap_password then + return false, "ldap_password is required when bind_dn is set" + end + + -- ldap_uri may omit ":port"; the effective port (636 with use_ldaps, + -- else 389) is resolved when the connection is opened. + + return true +end + + +local CHALLENGE_SCHEME = { + ldap = "ldap", + basic = "Basic", +} + + +-- Shared 401 helper for the authentication-failure paths. +local function auth_failed(conf, ctx, reason) + + -- under multi-auth, decline quietly and let the wrapper render the 401 + if auth_utils.is_running_under_multi_auth(ctx) then + return 401 + end + + if reason then + core.log.warn(plugin_name, ": ", reason) + end + core.response.set_header("WWW-Authenticate", + CHALLENGE_SCHEME[conf.header_type] + .. " realm=\"" .. conf.realm .. "\"") + return 401, { message = "Authorization required" } +end + + + + +-- Parse one credential header value: the scheme word is conf.header_type +-- ("ldap" or "basic", case-insensitive), the payload is +-- base64("username:password"). +local function parse_credential_header(conf, auth_header) + local m, err = ngx_re_match(auth_header, + "^(?i:" .. conf.header_type .. ")\\s+(.+)", "jo") + if err then + return nil, nil, "error matching authorization header: " .. err + end + if not m then + return nil, nil, "invalid authorization header format" + end + + local decoded = ngx_decode_base64(m[1]) + if not decoded then + return nil, nil, "failed to base64-decode authorization header" + end + + -- split on the FIRST colon only: the password may itself contain ':' + local sep = str_find(decoded, ":", 1, true) + if not sep then + return nil, nil, "invalid credential: missing ':' separator" + end + + return str_sub(decoded, 1, sep - 1), str_sub(decoded, sep + 1) +end + + +-- Proxy-Authorization takes priority, but only when it parses into +-- credentials for conf.header_type: a forward proxy may spend that header +-- on its own credentials (e.g. "Basic ...") while the end user's ride in +-- Authorization, so its mere presence must not mask a usable Authorization. +local function extract_credentials(conf, ctx) + local proxy_err + local proxy_header = core.request.header(ctx, "Proxy-Authorization") + if proxy_header then + local username, password + username, password, proxy_err = parse_credential_header(conf, proxy_header) + if username then + return username, password + end + end + + local auth_header = core.request.header(ctx, "Authorization") + if not auth_header then + return nil, nil, proxy_err or "missing authorization header" + end + + return parse_credential_header(conf, auth_header) +end + + +-- resty.ldap reports a directory result-code failure as +-- "<op> failed, error: <ERROR_MSG[code]>, details: <diagnostic>"; anything +-- else is a socket/TLS/timeout error or a protocol violation. Match against +-- the library's own message table so the strings cannot drift from it. +local RESULT_INVALID_CREDENTIALS = ldap_protocol.ERROR_MSG[49] +local RESULT_SIZE_LIMIT_EXCEEDED = ldap_protocol.ERROR_MSG[4] + +local function is_result_code(err, op, result_msg) + if type(err) ~= "string" then + return false + end + local prefix = op .. " failed, error: " .. result_msg .. ", details:" + return str_sub(err, 1, #prefix) == prefix +end + + + + +-- The LDAP round trip: resolve the user DN and authenticate the user's bind +-- on ONE pinned connection. Returns (nil, nil, user_dn) on success, or +-- (code, body) on failure. The socket is closed on every failure path and +-- released to the pool only on success, so a poisoned socket is never pooled. +local function ldap_resolve(conf, ctx, username, password) + -- The only client-controlled part of the search filter is the escaped + -- username. filter.escape leaves bytes the filter grammar rejects (e.g. + -- invalid UTF-8), and a grammar reject at search time would surface as a + -- 500 -- misclassifying a bad credential as a server fault. Pre-compile + -- the filter so any reject is a clean 401. + local search_filter = "(" .. conf.attribute .. "=" + .. ldap_filter.escape(username) .. ")" + if not ldap_filter.compile(search_filter) then Review Comment: P2: This preflight rejects a valid RFC 4515 assertion value. `~` (0x7e) belongs to `UTF1SUBSET` (`%x5D-7F`), so `admin~` is a legal username, but the bundled filter grammar rejects it at the end and TEST 46 now codifies a 401. Please fix/upgrade the parser or escape `~` as `\\7e` before compiling so valid directory users remain authenticatable. ########## apisix-master-0.rockspec: ########## @@ -79,7 +79,7 @@ dependencies = { "net-url = 1.2-1", "xml2lua = 1.6-2", "lua-resty-mediador = 0.1.2-1", - "lua-resty-ldap = 0.1.0-0", + "lua-resty-ldap = 0.3.0-0", Review Comment: P2: `lua-resty-ldap 0.3.0-0` pins `lpeg = 1.0.2-1`. In a clean APISIX dependency tree, applying this bump removes the currently resolved `lpeg 1.1.0-2` and installs 1.0.2; `graphql` and `jsonpath` then also run against the downgraded global module. Please publish/use an LDAP rock with a compatible range such as `lpeg >= 1.0.2`, so APISIX retains its current LPeg version. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
