spacewander commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r936268824
##########
docs/en/latest/plugins/ldap-auth.md:
##########
@@ -49,7 +49,8 @@ For Route:
|----------|---------|----------|---------|------------------------------------------------------------------------|
| base_dn | string | True | | Base dn of the LDAP server. For
example, `ou=users,dc=example,dc=org`. |
| ldap_uri | string | True | | URI of the LDAP server.
|
-| use_tls | boolean | False | `true` | If set to `true` uses TLS.
|
+| use_tls | boolean | False | `false` | If set to `true` uses TLS.
|
+| verify_ldap_host| boolean | False | `false` | Whether to verify
the server certificate when `use_tls` is enabled; If set to `true`, you must
set `ssl_trusted_certificate` in `config.yaml`, and make sure the host of
`ldap_uri` matches the host in server certificate. |
Review Comment:
This field doesn't have default value in the code. Why add a default value
in the doc?
##########
docs/en/latest/plugins/ldap-auth.md:
##########
@@ -49,7 +49,8 @@ For Route:
|----------|---------|----------|---------|------------------------------------------------------------------------|
| base_dn | string | True | | Base dn of the LDAP server. For
example, `ou=users,dc=example,dc=org`. |
| ldap_uri | string | True | | URI of the LDAP server.
|
-| use_tls | boolean | False | `true` | If set to `true` uses TLS.
|
+| use_tls | boolean | False | `false` | If set to `true` uses TLS.
|
+| verify_ldap_host| boolean | False | `false` | Whether to verify
the server certificate when `use_tls` is enabled; If set to `true`, you must
set `ssl_trusted_certificate` in `config.yaml`, and make sure the host of
`ldap_uri` matches the host in server certificate. |
Review Comment:
According to the description, we should name this field `tls_verify`? It
doesn't verify the host but the TLS relative stuff.
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -138,9 +139,23 @@ function _M.rewrite(conf, ctx)
-- 2. try authenticate the user against the ldap server
local uid = conf.uid or "cn"
+ local ldap_host, ldap_port = core.utils.parse_addr(conf.ldap_uri)
+
local userdn = uid .. "=" .. user.username .. "," .. conf.base_dn
- local ld = lualdap.open_simple (conf.ldap_uri, userdn, user.password,
conf.use_tls)
- if not ld then
+ local ldapconf = {
+ timeout = 10000,
+ start_tls = false,
+ ldap_host = ldap_host,
+ ldap_port = ldap_port or 389,
+ ldaps = conf.use_tls,
+ verify_ldap_host = conf.verify_ldap_host,
+ base_dn = conf.base_dn,
+ attribute = uid,
+ keepalive = 60000,
+ }
+ local res, err = ldap.ldap_authenticate(user.username, user.password,
ldapconf)
+ if not res then
+ core.log.warn("ldap-auth: ", err)
Review Comment:
The prefix should be more meaningful
--
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]
