feat(ldap-auth): use lua-resty-ldap instead of lualdap#7590
Conversation
054b6e9 to
52b4ee9
Compare
| properties = { | ||
| base_dn = { type = "string" }, | ||
| ldap_uri = { type = "string" }, | ||
| ldap_host = { type = "string" }, |
There was a problem hiding this comment.
We should keep the compatibility of the configuration
| verify_ldap_host = conf.verify_ldap_host, | ||
| base_dn = conf.base_dn, | ||
| attribute = uid, | ||
| keepalive = 60000, |
There was a problem hiding this comment.
Let's doc the newly added timeout and keepalive
There was a problem hiding this 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.
| } | ||
| local res, err = ldap.ldap_authenticate(user.username, user.password, ldapconf) | ||
| if not res then | ||
| core.log.error(err) |
There was a problem hiding this comment.
We use warning level log for this situation:
apisix/apisix/plugins/jwt-auth.lua
Line 369 in 5cbcf24
and it would be better to add a prefix in the error message
| } | ||
| local res, err = ldap.ldap_authenticate(user.username, user.password, ldapconf) | ||
| if not res then | ||
| core.log.warn("ldap-auth: ", err) |
There was a problem hiding this comment.
The prefix should be more meaningful
| | 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. | |
There was a problem hiding this comment.
This field doesn't have default value in the code. Why add a default value in the doc?
| | 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. | |
There was a problem hiding this comment.
According to the description, we should name this field tls_verify? It doesn't verify the host but the TLS relative stuff.
3 participants
Description
Use lua-resty-ldap to do nonblocking ldap auth.
Checklist