You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/08/02 06:29:02 UTC
[GitHub] [apisix] kingluo opened a new pull request, #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
kingluo opened a new pull request, #7590:
URL: https://github.com/apache/apisix/pull/7590
### Description
Use lua-resty-ldap to do nonblocking ldap auth.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r936165018
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -30,13 +30,12 @@ local schema = {
title = "work with route or service object",
properties = {
base_dn = { type = "string" },
- ldap_host = { type = "string" },
- ldap_port = { type = "number" },
+ ldap_uri = { type = "string" },
Review Comment:
`ldap_uri` is the original configuration item. Nothing change here.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r935300873
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -30,11 +30,13 @@ local schema = {
title = "work with route or service object",
properties = {
base_dn = { type = "string" },
- ldap_uri = { type = "string" },
+ ldap_host = { type = "string" },
Review Comment:
We should keep the compatibility of the configuration
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -139,8 +141,20 @@ function _M.rewrite(conf, ctx)
local uid = conf.uid or "cn"
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 = conf.ldap_host,
+ ldap_port = conf.ldap_port,
+ ldaps = conf.use_tls,
+ verify_ldap_host = conf.verify_ldap_host,
+ base_dn = conf.base_dn,
+ attribute = uid,
+ keepalive = 60000,
Review Comment:
Let's doc the newly added timeout and keepalive
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -139,8 +141,20 @@ function _M.rewrite(conf, ctx)
local uid = conf.uid or "cn"
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 = conf.ldap_host,
+ ldap_port = conf.ldap_port,
+ 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.error(err)
Review Comment:
We use warning level log for this situation:
https://github.com/apache/apisix/blob/5cbcf246021b9f5a97ce98e999a7ed65a1cbc117/apisix/plugins/jwt-auth.lua#L369
and it would be better to add a prefix in the error message
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7590:
URL: https://github.com/apache/apisix/pull/7590
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r935341474
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -139,8 +141,20 @@ function _M.rewrite(conf, ctx)
local uid = conf.uid or "cn"
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 = conf.ldap_host,
+ ldap_port = conf.ldap_port,
+ ldaps = conf.use_tls,
+ verify_ldap_host = conf.verify_ldap_host,
+ base_dn = conf.base_dn,
+ attribute = uid,
+ keepalive = 60000,
Review Comment:
@spacewander These items are not exported to admin api, we only use default values. So we need to doc the default values? But it's kind of internal parameters.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r935341474
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -139,8 +141,20 @@ function _M.rewrite(conf, ctx)
local uid = conf.uid or "cn"
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 = conf.ldap_host,
+ ldap_port = conf.ldap_port,
+ ldaps = conf.use_tls,
+ verify_ldap_host = conf.verify_ldap_host,
+ base_dn = conf.base_dn,
+ attribute = uid,
+ keepalive = 60000,
Review Comment:
These items are not exported to admin api, we only use default values. So we need to doc the default values? But it's kind of internal parameters.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r935619468
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -30,13 +30,12 @@ local schema = {
title = "work with route or service object",
properties = {
base_dn = { type = "string" },
- ldap_host = { type = "string" },
- ldap_port = { type = "number" },
+ ldap_uri = { type = "string" },
Review Comment:
What do you mean? Those are just new config items which make it incompatible, so I revert to use the old one.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r935603150
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -30,13 +30,12 @@ local schema = {
title = "work with route or service object",
properties = {
base_dn = { type = "string" },
- ldap_host = { type = "string" },
- ldap_port = { type = "number" },
+ ldap_uri = { type = "string" },
Review Comment:
Deleting these two options seems incompatible, can't we keep them?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r936156647
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -30,13 +30,12 @@ local schema = {
title = "work with route or service object",
properties = {
base_dn = { type = "string" },
- ldap_host = { type = "string" },
- ldap_port = { type = "number" },
+ ldap_uri = { type = "string" },
Review Comment:
I thought `ldap_host` and `ldap_port` became `ldap_uri` now, and I was wondering if they were compatible. For example, there is now an old copy of the data in etcd and they contain `ldap_host` and `ldap_port`. when updating the APISIX version, the user needs to reset the `ldap_uri`?
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7590:
URL: https://github.com/apache/apisix/pull/7590#discussion_r936256460
##########
apisix/plugins/ldap-auth.lua:
##########
@@ -30,13 +30,12 @@ local schema = {
title = "work with route or service object",
properties = {
base_dn = { type = "string" },
- ldap_host = { type = "string" },
- ldap_port = { type = "number" },
+ ldap_uri = { type = "string" },
Review Comment:
got it.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7590: feat(ldap-auth): use lua-resty-ldap instead of lualdap
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org