Ldap authentication by jdoucerain · Pull Request #369 · jpillora/chisel

jdoucerain · 2022-06-30T00:01:22Z

This PR is to allow LDAP authentication of chisel client.
The chisel server ldap configuration is maintained in a file pointed by the --ldap-config option
The ldap-config file structure is as follow:
BindDN for non anonymous search
BindPassword for non anonymous search
Url ldap server url
BaseDN for search
Filter ldap objects filter
IDMapTo ldap attribute used as id
CA file containing the PEM formatted certificates to trust when connecting to ldap server
Insecure true if we skip the certificates validation

The users are authentication against the ldap server.
The authorization pattern is unchanged (in authfile)
In the auth file we maintain the userid and the remotes. The password is left to an empty string

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

kblanckaert · 2022-10-18T11:21:00Z

Hi,

Interesting feature!
Would really like to see this added as well, but I wouldn't completely disable the password authentication functionality once LDAP configuration is in place.

The documentation about how to use this could also be made more clear:

An example of the ldap.json configuration file
The fact that the LDAP users have to be added to the users.json file
The fact that LDAP over port 389 doesn't seem to work (TLS issues when trying)

I've added some comments about the above mentioned improvements in your code.

…-config example

kblanckaert · 2022-10-19T07:12:02Z

@jpillora Can you merge this please? Thanks!

jpillora

Looks reasonable from a quick glance

Would be great if we could flatten the 5 deep if-else blocks

Also what’s the binary size difference after importing ldap v3?

kblanckaert · 2022-10-19T10:09:47Z

binary size difference: 12214.113 KB (master) -> 12397.106 KB (ldap-authentication)

… to both rely on local or LDAP authentication

jdoucerain · 2022-10-19T16:08:52Z

@jpillora could you please have a look at the bit improved version of the password validation ?
Thanks

jpillora · 2022-10-31T04:13:26Z


 <!--tmpl,code=plain:echo "$ chisel client --help" && go run main.go client --help | sed 's#0.0.0-src (go1\..*)#X.Y.Z#' -->
-``` plain 
+``` plain


these spacing edits come from your IDE i think,
can we revert them to reduce the size of the change set

easiest way is to execute the markdown template https://github.com/jpillora/md-tmpl

jpillora · 2022-10-31T04:34:57Z

+}
+
+// Ldap Connection with TLS
+func ConnectTLS(ldapconfig LdapConfig) (*ldap.Conn, error) {


can we unexport ConnectTLS and any other functions that arent used

jpillora · 2022-10-31T04:41:07Z

-	if !found || user.Pass != string(password) {
-		s.Debugf("Login failed for user: %s", n)
-		return nil, errors.New("Invalid authentication for username: %s")
+	if (found && string(password) != "") {


although the users config file acts as a nice whitelist and access control, its not technically required - ldap can tell us both, if a user exists, and if the password is correct. should it work no users defined?

My opinion on this is the following:
if we want to use LDAP to only authenticate and forget about authorization, I think it should be explicitly mentioned with an additional flag to avoid confusion.
Now from the enterprise perspective (which was coming with the LDAP authentication) I am not seeing the only authentication and no authorization as a use-case and if I may I would deal with this as a separate change.

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

…-config example

… to both rely on local or LDAP authentication

…hisel into LDAP-authentication

jdoucerain · 2022-11-25T21:46:20Z

@jpillora thanks for the review you did and I tried to follow what you said.
Could you please review this change again ?
Thanks again

jpillora · 2022-11-30T10:11:26Z

Sorry @jdoucerain I was going to merge a bunch of PRs and release though tests are broken for latest versions of Go and I haven't had time to fix them #390

jdoucerain · 2023-03-07T19:41:31Z

Hi @jpillora would you have some time to check if you could merge this branch ?

…-authentication

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

jdoucerain · 2023-07-07T21:55:29Z

Hi @jpillora could you please check if you could merge this branch ? Thanks

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

jpillora · 2023-07-08T04:04:19Z

Looks pretty good, will try take a look toknight

I think the server config should only have the ldap config, the path string should be in main (cli code) - settings can export a LoadLDAP(path) LDAPConfig which is used my main, and by server package consumers

I’m happy to do this change if you’d like

jdoucerain · 2023-07-08T21:51:46Z

@jpillora please go ahead if you have some bandwidth. Thanks

jpillora · 2023-07-09T11:41:50Z

looking at the is PR now. it appears you still have to define users in the --auth or --authfile, and the "local" password still works. ldap just adds an additional password that will now work. can you confirm this is correct and intended?

jpillora · 2023-07-09T11:46:42Z

IMO, i think ldap should work in either one of two ways:

using ldap should be used if the local user is not found. so by default all user attempts use ldap. config.AllowedUsers []string could be added to whitelist users. maybe you could place remote configuration as a user attribute? otherwise all remotes are allowed
using ldap should force all "local" passwords to be blank. must be --auth foo if --auth foo:bar then fails to start. similarly with --authfile

commit 8786463 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Jul 7 19:16:27 2023 -0400 fix unreachable code Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca> commit 3a6f13c Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Jul 7 17:22:12 2023 -0400 update README.md with features from master upstream branch Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca> commit 3f92a77 Merge: 6b87e3f ce307e5 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Jul 7 16:45:51 2023 -0400 Merge branch 'master' of https://github.com/jpillora/chisel into LDAP-authentication commit 6b87e3f Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:52:56 2022 -0500 MD again commit b66e317 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:50:20 2022 -0500 markdown adjustments commit 62920af Merge: 3e05ec5 8f96052 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:34:44 2022 -0500 Merge branch 'LDAP-authentication' of https://github.com/jdoucerain/chisel into LDAP-authentication commit 3e05ec5 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:14:53 2022 -0500 markdown adjustments commit 659c06d Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:10:52 2022 -0500 markdown adjustments commit cfb1d00 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 15:58:32 2022 -0500 Changes as recommended by J.Pillora commit 69a8ce0 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Oct 19 12:04:33 2022 -0400 a bit improved version of the password validation keeping the ability to both rely on local or LDAP authentication commit 40da157 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Tue Oct 18 10:59:02 2022 -0400 allow combination of both LDAP and local authentication and give ldap-config example commit 4708fe1 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 19:33:27 2022 -0400 ca file debug commit 92b20ba Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:51:14 2022 -0400 add user in debugging log when too many entries commit ba54e8d Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:43:24 2022 -0400 remove comments in ldap.go commit 505ba02 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:39:23 2022 -0400 new --ldap-config in README.md commit 111b545 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:33:57 2022 -0400 ldap authentication Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca> commit 8f96052 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:14:53 2022 -0500 markdown adjustments commit 2d7c977 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 16:10:52 2022 -0500 markdown adjustments commit 7c0e741 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Fri Nov 25 15:58:32 2022 -0500 Changes as recommended by J.Pillora commit f7cdb96 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Oct 19 12:04:33 2022 -0400 a bit improved version of the password validation keeping the ability to both rely on local or LDAP authentication commit 00836b1 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Tue Oct 18 10:59:02 2022 -0400 allow combination of both LDAP and local authentication and give ldap-config example commit 6f7bc5d Merge: 79c0a67 bea4540 Author: jdoucerain <jerome.doucerain@bell.ca> Date: Tue Oct 18 09:53:06 2022 -0400 Merge branch 'jpillora:master' into LDAP-authentication commit 79c0a67 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 19:33:27 2022 -0400 ca file debug commit 5b77520 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:51:14 2022 -0400 add user in debugging log when too many entries commit 6c50813 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:43:24 2022 -0400 remove comments in ldap.go commit 4bc9fc4 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:39:23 2022 -0400 new --ldap-config in README.md commit 6c1f269 Author: Jerome Doucerain <jerome.doucerain@bell.ca> Date: Wed Jun 29 18:33:57 2022 -0400 ldap authentication Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

jdoucerain · 2023-07-09T12:33:41Z

looking at the is PR now. it appears you still have to define users in the --auth or --authfile, and the "local" password still works. ldap just adds an additional password that will now work. can you confirm this is correct and intended?

jdoucerain · 2023-07-09T12:35:21Z

Yes this is intentional. We have to cover case where the users could or could not be in the enterprise directory.

jdoucerain · 2023-07-09T12:39:05Z

And the presence in the directory is not enough for us to allow anyone to use chisel. This is why we are maintaining the authfile for each individual. This is our use case.

jdoucerain · 2023-07-09T12:43:27Z

I think using ldap as authentication only and applying a default parameterized remote is another use case to be developed.
And limiting the authentication to ldap only and blocking the ability to authenticate with local password when ldap is active is also another use case.

jpillora · 2023-07-09T12:44:53Z

gotcha, would you prefer option 2?

I’m finding it hard to see a good use case for two passwords

jdoucerain · 2023-07-09T12:52:23Z

Yes I agree with option 2
I will update the PR consequently

jdoucerain · 2023-07-11T14:24:34Z

@jpillora option 2 (using ldap should force all "local" passwords to be blank. must be --auth foo if --auth foo:bar then fails to start. similarly with --authfile) is implemented.

jpillora · 2023-07-11T14:46:21Z

Thanks! Hopefully will get some more time soon

…

On Wed, 12 Jul 2023 at 12:24 am jdoucerain ***@***.***> wrote: @jpillora <https://github.com/jpillora> option 2 (using ldap should force all "local" passwords to be blank. must be --auth foo if --auth foo:bar then fails to start. similarly with --authfile) is implemented. — Reply to this email directly, view it on GitHub <#369 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE2X4YXRVOQFGEFJUDMOBTXPVO23ANCNFSM52HPYV4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jdoucerain added 5 commits June 29, 2022 18:33

ldap authentication

6c1f269

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

new --ldap-config in README.md

4bc9fc4

remove comments in ldap.go

6c50813

add user in debugging log when too many entries

5b77520

ca file debug

79c0a67

kblanckaert reviewed Oct 18, 2022

View reviewed changes

Comment thread README.md

kblanckaert reviewed Oct 18, 2022

View reviewed changes

Comment thread server/server.go Outdated

jdoucerain added 2 commits October 18, 2022 09:53

Merge branch 'jpillora:master' into LDAP-authentication

6f7bc5d

allow combination of both LDAP and local authentication and give ldap…

00836b1

…-config example

jpillora reviewed Oct 19, 2022

View reviewed changes

kblanckaert reviewed Oct 19, 2022

View reviewed changes

Comment thread server/server.go

a bit improved version of the password validation keeping the ability…

f7cdb96

… to both rely on local or LDAP authentication

jpillora reviewed Oct 31, 2022

View reviewed changes

jpillora added enhancement awaiting-update labels Oct 31, 2022

jdoucerain added 11 commits November 25, 2022 15:58

Changes as recommended by J.Pillora

7c0e741

markdown adjustments

2d7c977

markdown adjustments

8f96052

ldap authentication

111b545

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

new --ldap-config in README.md

505ba02

remove comments in ldap.go

ba54e8d

add user in debugging log when too many entries

92b20ba

ca file debug

4708fe1

allow combination of both LDAP and local authentication and give ldap…

40da157

…-config example

a bit improved version of the password validation keeping the ability…

69a8ce0

… to both rely on local or LDAP authentication

Changes as recommended by J.Pillora

cfb1d00

jdoucerain added 3 commits November 25, 2022 16:29

markdown adjustments

659c06d

markdown adjustments

3e05ec5

Merge branch 'LDAP-authentication' of https://github.com/jdoucerain/c…

62920af

…hisel into LDAP-authentication

jdoucerain added 2 commits November 25, 2022 16:50

markdown adjustments

b66e317

MD again

6b87e3f

jpillora removed the awaiting-update label Nov 30, 2022

jdoucerain added 2 commits July 7, 2023 16:45

Merge branch 'master' of https://github.com/jpillora/chisel into LDAP…

3f92a77

…-authentication

update README.md with features from master upstream branch

3a6f13c

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

fix unreachable code

8786463

Signed-off-by: Jerome Doucerain <jerome.doucerain@bell.ca>

exclusion between local passwords and LDAP authentication

fc5387c

Conversation

jdoucerain commented Jun 30, 2022

Uh oh!

kblanckaert commented Oct 18, 2022

Uh oh!

Uh oh!

Uh oh!

kblanckaert commented Oct 19, 2022

Uh oh!

jpillora left a comment

Choose a reason for hiding this comment

Uh oh!

kblanckaert commented Oct 19, 2022

Uh oh!

Uh oh!

jdoucerain commented Oct 19, 2022

Uh oh!

Uh oh!

jpillora Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpillora Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpillora Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

jdoucerain Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

jdoucerain commented Nov 25, 2022

Uh oh!

jpillora commented Nov 30, 2022

Uh oh!

jdoucerain commented Mar 7, 2023

Uh oh!

jdoucerain commented Jul 7, 2023

Uh oh!

jpillora commented Jul 8, 2023

Uh oh!

jdoucerain commented Jul 8, 2023

Uh oh!

jpillora commented Jul 9, 2023

Uh oh!

jpillora commented Jul 9, 2023

Uh oh!

jdoucerain commented Jul 9, 2023

Uh oh!

jdoucerain commented Jul 9, 2023

Uh oh!

jdoucerain commented Jul 9, 2023

Uh oh!

jdoucerain commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpillora commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdoucerain commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdoucerain commented Jul 11, 2023

Uh oh!

jpillora commented Jul 11, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jdoucerain commented Jul 9, 2023 •

edited

Loading

jpillora commented Jul 9, 2023 •

edited

Loading

jdoucerain commented Jul 9, 2023 •

edited

Loading