Add client certs support and anonymous group search to LDAP by AeroNotix · Pull Request #7578 · hashicorp/vault

AeroNotix · 2019-10-05T20:27:10Z

No description provided.

* GSuite LDAP requires that clients use client TLS certificates/keys in order to authenticate against their LDAP.

hashicorp-cla · 2019-10-05T20:27:13Z

All committers have signed the CLA.

AeroNotix · 2019-10-05T20:27:58Z

This adds the ability to provide client cert/key files when using LDAP. Some LDAP providers like Google require the use of client certificates for client authentication and being able to perform searches with no bind set. This is how you are required to use the Google LDAP service.

Unknowns:

Don't really know if the UI elements are being created correctly, they appear in the UI when I add the fields but I didn't attempt to use them, any pointers will be helpful.

Refs (and probably a few others):

AeroNotix · 2019-10-17T16:52:48Z

Any feedback on this? Am I missing anything? I am willing to support this code in perpetuity (and force my children to do so as well.)

athieme · 2019-10-17T17:02:46Z

I would need this as well. Is merging this an option?

AeroNotix · 2019-12-02T10:23:50Z

Any feedback on this?

michelvocks · 2019-12-02T12:47:33Z

 		return nil, logical.ErrorResponse(err.Error()), nil, nil
 	}

+	if cfg.AnonymousGroupSearch {


Would you mind to explain this part a bit further? The below code is exactly the same we already execute on line 80:

vault/builtin/credential/ldap/backend.go

Lines 80 to 86 in 4fbf1f4

c, err := ldapClient.DialLDAP(cfg.ConfigEntry)

if err != nil {

return nil, logical.ErrorResponse(err.Error()), nil, nil

}

if c == nil {

return nil, logical.ErrorResponse("invalid connection returned from LDAP dial"), nil, nil

}

With google LDAP there's no way to query for groups a user is logged in as while logged in as that user. But there's no way to verify a user's login credentials without binding as that user. At least no way I could find to do it.

Reason I added this was so that you could do the first bind as the logged in user, verify that their credentials are correct - then drop to the anonymous bind with client certificates just for the group membership search.

Ah, gotcha! That sounds very google LDAP specific to me? I wonder if we should rename anonymous_group_search to google_anonymous_group_search or something similar? anonymous_group_search sounds to me like the credentials are never used at all.

Sure, that makes sense and I agree. I will make that change 👍

The problem is if this behavior is ever necessary with an ldap server that isn't Google, now the option name is confusing/limiting.

Yes, @michelvocks made a convincing argument for the name change. I will change it.

Yes, and I'm suggesting that it should not be name that was discussed.

Oh my bad, I misunderstood your point then. I'm happy either way, right now we only know that Google LDAP services need this option - but potentially I guess it is possible that others would need it.

michelvocks · 2019-12-02T12:49:16Z

@@ -1,8 +1,6 @@
 module github.com/hashicorp/vault

-go 1.12


As far as I can see all the changes to go.mod, go.sum and sdk/go.mod are not necessary for this PR. Would you mind to exclude them from your PR?

Sure, I can. What are the "rules" in general about PRs which bump go/dependency versions?

PRs never bump Go version. But you're out of date from master anyways, it's already updated for Go 1.13.

In general, we bump the go version in a separate PR. Dependency versions can be updated in feature/bugfix PRs if there is reason/advantage to do that.

It wasn't out of date two months ago when I submitted the PR 😛

I'll rebase with master and push up changes.

michelvocks

Hi @AeroNotix!

First of all, I want to apologize for the super late reply/review. We are doing our best to do community reviews as quickly as possible but sometimes we struggle a bit.
We really appreciate your work and effort tho!

This PR looks good overall but I have a question and one minor change request left (see in the review). I'm also a bit unsure about the UI question you have mentioned above. I CC here @noelledaley since I think she can help out here.

Cheers,
Michel

AeroNotix · 2019-12-02T13:09:25Z

Thanks for getting back to me!

The UI changes I refer to are that some of the additional configuration options I added may not be configurable from the UI properly, I did a quick poke around the JS and fragments that add the configuration options to the UI but last I checked they didn't seem to work as expected. It's been a couple of months since I fired up this PR and had it going. When I get time this week I'll rebase w/ master and go through the motions testing it with our Google LDAP set up.

@noelledaley hi - any feedback or tips regarding what's needed for the UI portions is much appreciated!

michelvocks · 2019-12-02T13:13:00Z

+		if err != nil {
+			return nil, logical.ErrorResponse("ldap operation failed"), nil, nil
+		}
+		defer c.Close()


I think you can drop this line since we already added c.Close() to the defer-stack a few lines above.

michelvocks · 2019-12-02T13:20:00Z

+		"anonymous_group_search": {
+			Type:        framework.TypeBool,
+			Default:     false,
+			Description: "Use anonymous binds when performing LDAP group searches",


I think it makes sense to add that the credentials are stilled used for the initial connection test otherwise users could be confused.

andaley · 2019-12-04T22:07:35Z

+		"anonymous_group_search": {
+			Type:        framework.TypeBool,
+			Default:     false,
+			Description: "Use anonymous binds when performing LDAP group searches",


You'll need to add another property after Description called DisplayAttrs with the field display Name and default Value to get this field to show in the UI. Here's an example of how to do that!

Thank you! Will do.

andaley · 2019-12-04T22:09:09Z


+		"client_tls_cert": {
+			Type:        framework.TypeString,
+			Description: "Client certificate to provide to the LDAP server, must be x509 PEM encoded (optional)",


You'll need the DisplayAttrs for this field as well

andaley · 2019-12-04T22:09:31Z

+
+		"client_tls_key": {
+			Type:        framework.TypeString,
+			Description: "Client certificate key to provide to the LDAP server, must be x509 PEM encoded (optional)",


....and this one, too. :)

Thank you! Will make these changes.

andaley · 2019-12-04T22:12:43Z

Hi @AeroNotix!

Thanks for including UI changes in this PR! The changes look good to me at an initial glance -- you'll just need to add the DisplayAttrs to get the fields to show up in the UI properly. Once you've done that + rebased, let us know and we can test it out.

Nice work!

Cheers,
Noelle

AeroNotix

Thanks @noelledaley appreciate the feedback. Will make the changed you requested and push up.

stephenrjohnson · 2020-02-17T10:13:42Z

So excited for this change and then we can finally connect vault to google for user authentication.

rodrigorato · 2020-02-17T10:49:29Z

So excited for this change and then we can finally connect vault to google for user authentication.

Yeah, I'm also waiting on this one. Thanks to everyone who is making this possible!

AeroNotix · 2020-02-17T11:11:39Z

My use-case for vault logins has changed somewhat - I might not be able to get to this to deal with PR comments/review comments.

If you need Vault LDAP logins working with Google - all the work is done and the changes requested shouldn't be much on top of that.

Gisson · 2020-02-17T14:00:03Z

I've rebased from master and applied the fixes mentioned in the previous review in the PR #8365 . Can we follow up on this there?

AeroNotix · 2020-02-17T14:03:32Z

@Gisson thanks for taking this on and trying to get it over the line. I'll close this PR in favour of your other.

AeroNotix added 4 commits October 5, 2019 13:15

feat: provide a way to specify client certificates

88cbae8

* GSuite LDAP requires that clients use client TLS certificates/keys in order to authenticate against their LDAP.

fix: add config fields to ldap.js and config.go

1516fd5

fix: load x509 key pair when creating LDAP TLS config

76dd7bd

fix: handle key pair errors

64bb402

AeroNotix force-pushed the add-client-certs branch from 5e5906c to 540d351 Compare October 6, 2019 16:41

AeroNotix changed the title ~~Add client certs~~ Add client certs, anonymous group search Oct 6, 2019

fix: provide a way to not bind when searching groups

10f7e13

AeroNotix force-pushed the add-client-certs branch from 540d351 to 10f7e13 Compare October 7, 2019 11:56

chrishoffman changed the title ~~Add client certs, anonymous group search~~ Add client certs support and anonymous group search to LDAP Oct 8, 2019

michelvocks reviewed Dec 2, 2019

View reviewed changes

andaley reviewed Dec 4, 2019

View reviewed changes

AeroNotix commented Dec 4, 2019

View reviewed changes

michelvocks mentioned this pull request Feb 17, 2020

LDAP rebind after authenticating does not allow listing groups with Google Cloud Identity #6111

Closed

Gisson mentioned this pull request Feb 17, 2020

Add client certs #8365

Merged

AeroNotix closed this Feb 17, 2020

AeroNotix deleted the add-client-certs branch February 17, 2020 14:03

	c, err := ldapClient.DialLDAP(cfg.ConfigEntry)
	if err != nil {
	return nil, logical.ErrorResponse(err.Error()), nil, nil
	}
	if c == nil {
	return nil, logical.ErrorResponse("invalid connection returned from LDAP dial"), nil, nil
	}

Conversation

AeroNotix commented Oct 5, 2019

Uh oh!

hashicorp-cla commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AeroNotix commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AeroNotix commented Oct 17, 2019

Uh oh!

athieme commented Oct 17, 2019

Uh oh!

AeroNotix commented Dec 2, 2019

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!

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!

michelvocks left a comment

Choose a reason for hiding this comment

Uh oh!

AeroNotix commented Dec 2, 2019

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!

Choose a reason for hiding this comment

Uh oh!

andaley commented Dec 4, 2019

Uh oh!

AeroNotix left a comment

Choose a reason for hiding this comment

Uh oh!

stephenrjohnson commented Feb 17, 2020

Uh oh!

rodrigorato commented Feb 17, 2020

Uh oh!

AeroNotix commented Feb 17, 2020

Uh oh!

Gisson commented Feb 17, 2020

Uh oh!

AeroNotix commented Feb 17, 2020

hashicorp-cla commented Oct 5, 2019 •

edited

Loading

AeroNotix commented Oct 5, 2019 •

edited

Loading