Copilot commented on code in PR #13762: URL: https://github.com/apache/apisix/pull/13762#discussion_r3688703304
########## ci/pod/openldap/enable-anon-bind.sh: ########## @@ -0,0 +1,67 @@ +#!/bin/bash +# +# 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. +# +# Boot hook (docker-entrypoint-initdb.d). +# +# olcAllows bind_anon_dn: a simple bind with a DN and an EMPTY password +# succeeds at the server (RFC 4513 5.1.2, result: anonymous) -- lets the +# tests prove the plugin itself rejects empty passwords. +# +# The olcAccess rules make the bind identity observable: +# * cn=Secret User: invisible to an anonymous search, readable by any +# authenticated identity (userPassword keeps `auth` so the entry can +# still be bound once found) -- the bind-state-leak tripwire. +# * The catch-all keeps everything else readable, so the ldap-auth +# regression is unaffected. + +set -o errexit +set -o nounset +set -o pipefail + +. /opt/bitnami/scripts/liblog.sh +. /opt/bitnami/scripts/libopenldap.sh + +eval "$(ldap_env)" + +info "Enabling RFC 4513 unauthenticated bind (olcAllows: bind_anon_dn)" + +ldap_start_bg + +ldapmodify -Y EXTERNAL -H "ldapi:///" <<EOF +dn: cn=config +changetype: modify +add: olcAllows +olcAllows: bind_anon_dn +EOF + +info "Installing the bind-identity olcAccess rules on the data database" + +# Resolve the data database DN by suffix so we do not hard-code the {N} index. +DATA_DB_DN="$(ldapsearch -Y EXTERNAL -H "ldapi:///" -b cn=config \ + "(olcSuffix=dc=example,dc=org)" dn 2>/dev/null \ + | awk '/^dn:/ { $1=""; sub(/^ /,""); print; exit }')" + Review Comment: `DATA_DB_DN` is derived from an `ldapsearch`/`awk` pipeline, but the script doesn't validate that it resolved to a non-empty DN before using it in `ldapmodify`. If the lookup fails (image change, suffix mismatch, transient startup issue), this will attempt to modify an empty DN and fail with a confusing error. Add an explicit check with a clear message before proceeding. ########## apisix/plugins/ldap-auth-advanced.lua: ########## @@ -0,0 +1,390 @@ +-- +-- 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]" + 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 Review Comment: The default LDAP socket timeout is set to 3000ms, but the existing `ldap-auth` plugin uses 10000ms and the referenced design in #8958 lists 10000ms as the default. A shorter default can cause unexpected auth failures under moderate latency; consider aligning the default to 10000ms for consistency/backward expectations. ########## apisix/plugins/ldap-auth-advanced.lua: ########## @@ -0,0 +1,390 @@ +-- +-- 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]" + 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 .. "\"") Review Comment: When `header_type` is set to `basic`, this emits a lowercase `basic` scheme in `WWW-Authenticate`. Elsewhere in APISIX (e.g. `basic-auth` and `ldap-auth`) the Basic scheme is emitted as `Basic`, which is the conventional/standard form and improves interoperability with some clients. -- 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]
