feat: Add ldap-auth plugin by jp-gouin · Pull Request #3894 · apache/apisix

jp-gouin · 2021-03-23T12:44:53Z

What this PR does / why we need it:

This PR add a new authentication plugin that use a LDAP server to authenticate the user

This add a new type of consumer

"ldap-auth": {
    "userdn": "cn=user01,dc=example,dc=org"
 }

This plugin can also automatically create a consumer upon request if auto_create_consumer = true
This fixes : #3861 and #1128

Test cases are not fully operational yet.

Help needed

Also need help to add the deployment of an Openldap server on Github Action.
FYI i used the (bitnami/openldap:2)(https://hub.docker.com/r/bitnami/openldap/) docker image

openldap:
    image: bitnami/openldap:2
    ports:
      - '389:1389'
      - '636:1636'
    environment:
      - LDAP_ADMIN_USERNAME=admin
      - LDAP_ADMIN_PASSWORD=adminpassword
      - LDAP_USERS=user01,user02
      - LDAP_PASSWORDS=password1,password2

Next feature

Add a group variable in the consumer , this group would come from the memberOf (configurable) attribute of the ldap user and add it in the consumer.
Then plugins such as consumer_restriction could be updated to take into account groups -> (which can also be used on basic-auth (and more) plugin where a consumer can also be part of a group)

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
[-] Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

spacewander · 2021-03-24T02:10:31Z

+
+`ldap-auth` is an authentication plugin that can works with `consumer`. Add Ldap Authentication to a `service` or `route`.
+
+The `consumer` then authenticate against the Ldap server using Basic authentication.


"authenticates"?

Yes corrected

spacewander · 2021-03-24T02:12:27Z

+        ok, err = core.schema.check(schema, conf)
+    end
+
+    if not ok then


return ok, err is enough.

spacewander · 2021-03-24T02:14:20Z

+    end
+
+    -- 3. Retreive consumer for authorization plugin
+    if conf.auto_create_consumer then 


Better to avoid creating temporary consumers. The temporary consumer may be broken somewhere else.

Ok remove this feature .
It was more to act like the oidc plugin , that does not require a consumer to be created , but i see your point

spacewander · 2021-03-24T02:14:38Z

+| basedn | string | required    |         |       | the base dn of the `ldap` server (example : `ou=users,dc=example,dc=org`)   |
+| ldapuri | string | required    |         |       | the uri of the ldap server  |
+| usetls | boolean | optional    |    `true`     |       | Boolean flag indicating if Transport Layer Security (TLS) should be used. |
+| uid | string | optional    |     `cn`      |     | the user's password |


Is it the password?

No indeed it's the uid attribute that changes depending of the ldap server solution

spacewander · 2021-03-24T02:50:32Z

+    properties = {
+        basedn = { type = "string" },
+        ldapuri = { type = "string" },
+        usetls = { type = "boolean" },


Would you use a snake case for the field name, like base_dn / ldap_uri?

spacewander · 2021-03-24T02:56:11Z

@jp-gouin
You can install the docker in:

apisix/.travis/linux_openresty_common_runner.sh

Line 21 in 654b1a9

before_install() {

And the libldap in:

apisix/utils/linux-install-openresty.sh

Line 43 in 654b1a9

sudo apt-get install "$openresty" lua5.1 liblua5.1-0-dev

spacewander · 2021-03-24T02:59:08Z

@membphis @moonming
Can we introduce libldap as an extern dependency? This library needs to be installed manually.

spacewander · 2021-03-24T03:00:06Z

    "luasec = 0.9-1",
    "lua-resty-consul = 0.3-2",
    "penlight = 1.9.2-1",
+    "lualdap = 1.2.3-1",


Why not use the latest release: 1.2.6?

membphis · 2021-03-24T04:56:22Z

@membphis @moonming
Can we introduce libldap as an extern dependency? This library needs to be installed manually.

https://github.com/lualdap/lualdap does not declare the license, the author needs to declare the license first, otherwise APISIX cannot use this open-source project.

jp-gouin · 2021-03-24T07:03:19Z

@membphis there is a licence in their website https://lualdap.github.io/lualdap/license
We can open a ticket if the licence needs to be at project level

spacewander · 2021-03-24T09:04:10Z

The spirit of the license is that you are free to use LuaLDAP for any purpose at no cost without having to ask us. The only requirement is that if you do use LuaLDAP, then you should give us credit by including the appropriate copyright notice somewhere in your product or its documentation.

IMHO, it seems we can use it after adding the copyright notice.

There is the raw file of the License page: https://github.com/lualdap/lualdap/blob/master/docs/license.md.

membphis · 2021-03-29T08:21:30Z

@membphis there is a licence in their website https://lualdap.github.io/lualdap/license
We can open a ticket if the licence needs to be at project level

https://github.com/lualdap/lualdap

there is no information about license: https://github.com/lualdap/lualdap/search?q=license

I created a new issue right now: lualdap/lualdap#21

…, update rockspec to add libldap, update readme, remove auto_createconsumer feature

jp-gouin · 2021-04-06T12:56:06Z

@jp-gouin
You can install the docker in:

apisix/.travis/linux_openresty_common_runner.sh

Line 21 in 654b1a9

before_install() {

And the libldap in:

apisix/utils/linux-install-openresty.sh

Line 43 in 654b1a9

sudo apt-get install "$openresty" lua5.1 liblua5.1-0-dev

The CI doesn't seems to install libldap2-dev before installing apisix. Am i missing something ?

spacewander · 2021-04-07T01:18:20Z

@jp-gouin
Thank you for your contribution.
However, as @membphis mentioned, we need to wait for the author's confirmation in
lualdap/lualdap#21

spacewander · 2021-04-07T01:24:49Z

@jp-gouin
You can install the docker in:

apisix/.travis/linux_openresty_common_runner.sh

Line 21 in 654b1a9

before_install() {

And the libldap in:

apisix/utils/linux-install-openresty.sh

Line 43 in 654b1a9

sudo apt-get install "$openresty" lua5.1 liblua5.1-0-dev

The CI doesn't seems to install libldap2-dev before installing apisix. Am i missing something ?

Some ci cases have their own way to install the library, like:

apisix/utils/centos7-ci.sh

Line 25 in 4156a73

yum install -y wget tar gcc automake autoconf libtool make unzip \

apisix/ci/linux_tengine_runner.sh

Line 218 in 4156a73

sudo apt-get install lua5.1 liblua5.1-0-dev

tzssangglass · 2021-07-12T17:41:06Z

@jp-gouin hi, are you still interested in continuing this PR?

jp-gouin · 2021-07-13T07:48:48Z

I am , however there is still the licence issue on the lualdap/lualdap#21
As soon has the licence has been validated by @membphis , i will update this PR

membphis · 2021-07-14T03:31:01Z

@membphis @moonming
Can we introduce libldap as an extern dependency? This library needs to be installed manually.

@moonming need your help. ^_^

jp-gouin · 2021-08-18T12:30:53Z

@membphis is the information provided in the lualdap/lualdap#21 enough for the use of lualdap in Apisix ?
If so i would be happy to continue working on the PR

membphis · 2021-08-21T02:12:40Z

@membphis is the information provided in the lualdap/lualdap#21 enough for the use of lualdap in Apisix ?
If so i would be happy to continue working on the PR

cool, I think you can continue this PR, it fine now.

membphis · 2021-08-21T02:13:25Z

I think you need to resolve the conflicting files ^_^

…on for ldap-auth.t, merge apisix-master-0.rockspec conflict

jp-gouin · 2021-10-02T19:22:26Z

Failed tests do not seems to be related to ldap-auth plugin, do you confirm ?
Chaos test should be ok once PR apache/apisix-docker#222 is merged

spacewander · 2021-10-03T14:24:16Z

+    local decoded = ngx.decode_base64(m[1])
+
+    local res
+    res, err = ngx_re.split(decoded, ":")


Better to check if decoded is not nil and the split result has two elements.

Actually it’s a copy paste from

apisix/apisix/plugins/basic-auth.lua

Line 83 in 34df010

local decoded = ngx.decode_base64(m[1])

, I’ll try to update the code

Feel free to submit another PR to update basic-auth.

spacewander · 2021-10-03T14:26:37Z

+}'
+```
+
+you can visit Dashboard `http://127.0.0.1:9080/apisix/dashboard/` and add a Consumer through the web console.


We can remove this part now. The dashboard is not shipped by default.

Ok will remove , actually it’s a copy paste from

apisix/docs/en/latest/plugins/basic-auth.md

Line 64 in 34df010

you also can add a Consumer through the web console:

spacewander · 2021-10-03T14:31:14Z

Failed tests do not seems to be related to ldap-auth plugin, do you confirm ? Chaos test should be ok once PR apache/apisix-docker#222 is merged

Look like they are just flaky test results.

…ard in the readme

membphis · 2021-10-07T06:03:21Z

@membphis @moonming Can we introduce libldap as an extern dependency? This library needs to be installed manually.

LGTM ^_^

membphis · 2021-10-07T06:08:04Z

Needs to install the libldap2-dev in ChaosMesh case.

https://github.com/apache/apisix/pull/3894/checks?check_run_id=3788501510#step:5:1004

Error: Failed installing dependency: https://luarocks.org/lualdap-1.2.6-1.src.rock - Could not find header file for LDAP
  No file ldap.h in /usr/local/include
  No file ldap.h in /usr/include
  No file ldap.h in /include

spacewander · 2021-10-07T11:53:45Z

+        return nil, "split authorization err:" .. err
+    end
+    if #res < 2 then
+        return nil, "split authorization length is invalid"


Suggested change

return nil, "split authorization length is invalid"

return nil, "split authorization err: invalid decoded data: " .. decoded

spacewander · 2021-10-07T11:53:49Z

+
+    if err then
+        return nil, "failed to decode authentication header"
+    end


if not decoded then return nil, "failed to decode authentication header: " .. m[1] end

jp-gouin · 2021-10-07T13:52:30Z

@membphis , the chaos mesh test is using the apisix-docker project to build apisix :

apisix/.github/workflows/chaos.yml

Line 41 in aa375ab

    
                     docker build -t apache/apisix:alpine-local --build-arg APISIX_PATH=. -f Dockerfile .

I submitted a PR to add the library in the apisix-docker apache/apisix-docker#222

spacewander · 2021-10-10T04:13:19Z

@jp-gouin
Would you update this PR? It is almost done.

jp-gouin · 2021-10-10T08:07:29Z

@jp-gouin Would you update this PR? It is almost done.

Done :)

tzssangglass · 2021-10-12T16:49:59Z

LGTM

lijing-21 · 2021-10-13T02:30:38Z

@jp-gouin,
Hi, Thank you for your contribution! We are excited to hear about your story with APISIX, so we want to invite you to write an article, for reference，you could introduce the reason why you choose to use APISIX, and why you wrote this plugin, or show the readers how to use this plugin. We will publish this article on the official website of Apahce APISIX, I believe it will be of great help to our readers.
Here is my e-mail: [email protected]. Looking forward to your reply！~

Zeeeeta · 2024-01-11T07:32:04Z

Thanks for the effort.
May I know if base_dn only support searching in one level of directory and not searching for the AD objects in subdirectory?

jp-gouin added 6 commits March 16, 2021 09:28

add lualdap

a5d1ab9

add ldap plugin

2e2ffb0

add ldap plugin

6cfe4cd

add ldap plugin

7b01646

add ldap-auth plugin , add test case , need to debug the test case

979e46b

correct typo

d46d980

jp-gouin changed the title ~~feat : Add ldap-auth plugin~~ feat: Add ldap-auth plugin Mar 23, 2021

update of readme and test case

fad0827

spacewander reviewed Mar 24, 2021

View reviewed changes

jp-gouin added 5 commits April 6, 2021 14:03

add test cases for ldap-auth plugin, update travis to deploy openldap…

d410f56

…, update rockspec to add libldap, update readme, remove auto_createconsumer feature

resolve merge conflict

8a43bc3

resolve merge conflict

6c29529

remove trailing space, use libldap libldap-2.4-2

3946036

change libldap to libldap2-dev to avoid error upon lualdap installation

b67b69d

jp-gouin added 3 commits October 2, 2021 18:00

Add openldap reference in the install-dependencies.md, add sub foncti…

89f8d6a

…on for ldap-auth.t, merge apisix-master-0.rockspec conflict

resolve merge conflict

15cf6ee

remove trailing space, reindex test case

9c87c10

spacewander reviewed Oct 3, 2021

View reviewed changes

jp-gouin added 3 commits October 4, 2021 10:30

add verification on extract_header, remove reference to apisix dashbo…

9663476

…ard in the readme

revert wrong commit, add check on extract_header

abe1090

remove unnecessary check

10a4d87

jp-gouin requested review from membphis and spacewander October 6, 2021 09:52

spacewander reviewed Oct 7, 2021

View reviewed changes

commit change per review

8ab4df8

add libldap2-dev dependency in .github/workflows/cli.yml

79b01a6

jp-gouin commented Oct 10, 2021

View reviewed changes

Comment thread .github/workflows/cli.yml

spacewander reviewed Oct 11, 2021

View reviewed changes

Comment thread apisix/plugins/ldap-auth.lua Outdated

remove unexisting err from ngx.decode_base64(m[1])

78032ba

spacewander approved these changes Oct 11, 2021

View reviewed changes

tzssangglass approved these changes Oct 12, 2021

View reviewed changes

spacewander merged commit d1c178d into apache:master Oct 13, 2021

juzhiyuan mentioned this pull request Jan 10, 2022

request help: building failed on Mac(2.11 ldap.h) #6068

Closed

AlinsRan mentioned this pull request Dec 16, 2024

fix(ci): remove lualdap deps #11829

Merged

5 tasks


		`ldap-auth` is an authentication plugin that can works with `consumer`. Add Ldap Authentication to a `service` or `route`.

		The `consumer` then authenticate against the Ldap server using Basic authentication.

	return nil, "split authorization length is invalid"
	return nil, "split authorization err: invalid decoded data: " .. decoded

Uh oh!

Conversation

jp-gouin commented Mar 23, 2021

What this PR does / why we need it:

Help needed

Next feature

Pre-submission checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jp-gouin Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander commented Mar 24, 2021

Uh oh!

spacewander commented Mar 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

membphis commented Mar 24, 2021

Uh oh!

jp-gouin commented Mar 24, 2021

Uh oh!

spacewander commented Mar 24, 2021

Uh oh!

membphis commented Mar 29, 2021

Uh oh!

jp-gouin commented Apr 6, 2021

Uh oh!

spacewander commented Apr 7, 2021

Uh oh!

spacewander commented Apr 7, 2021

Uh oh!

tzssangglass commented Jul 12, 2021

Uh oh!

jp-gouin commented Jul 13, 2021

Uh oh!

membphis commented Jul 14, 2021

Uh oh!

jp-gouin commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

membphis commented Aug 21, 2021

Uh oh!

membphis commented Aug 21, 2021

Uh oh!

jp-gouin commented Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander commented Oct 3, 2021

Uh oh!

membphis commented Oct 7, 2021

Uh oh!

jp-gouin Apr 2, 2021 •

edited

Loading

jp-gouin commented Aug 18, 2021 •

edited

Loading

jp-gouin commented Oct 2, 2021 •

edited

Loading