feat: implement LDAP service discovery (RFC 2782) #362

tonyhhyip · 2022-03-21T14:06:08Z

feat(discovery): implement LDAP service discovery (RFC 2782)

Closes #329

vetinari · 2022-03-22T08:57:04Z

Please also add some documentation for the users describing what this does. From taking a quick look at the code I'd guess it allows to use ldaps:// as URL and it tries to find the server(s) from SRV records. Without docs no-one will discover and/or use this feature.

v3/conn.go

tonyhhyip · 2022-03-28T10:47:28Z

A few more to implement before finalize and write document:

a way to append baseDN for every operation
decide a way to deal with ldap:///dc=example,dc=com as the standard does not defined this and AD have no implement for LDAPS

tonyhhyip · 2022-05-31T03:54:20Z

Not going to special deal with LDAPS for service discovery for now, as the RFC does not define the way of implement for LDAPS

tonyhhyip · 2022-05-31T03:54:47Z

Document will be updated once the code are fine and ready

cpuschma · 2022-06-01T20:55:44Z

I'm sorry for my delay in responding, alot going on currently. I'll have a look at your changes tomorrow!

cpuschma

Everything mentioned also counts for v3

cpuschma · 2022-06-12T20:05:04Z

search.go

@@ -231,6 +231,20 @@ type SearchRequest struct {
 	Controls     []Control
 }

+func (req *SearchRequest) appendBaseDN(dn string) appendDnRequest {
+	return &SearchRequest{


This is an unnecessary allocation. You're working with a pointer to the object anyway, why not edit object in place and simply change the field? E.g.

func (req *SearchRequest) appendBaseDN(dn string) { req.BaseDN = appendDN(req.BaseDN, dn) }

This also woldn't require us to constantly update this method in case new fields are added/changed.

As a user, I don't expect my input to be modified by the library, at least, the original version won't. However, there is a way to implement a clone and edit method

Most of the operations in this library mutate the given input. Let's stick with the same approach for now, and if we decide to clone and edit user input in the future we can do that everywhere, rather than inconsistently.

cpuschma · 2022-06-12T20:05:34Z

modify.go

@@ -82,6 +82,14 @@ func (req *ModifyRequest) appendChange(operation uint, attrType string, attrVals
 	req.Changes = append(req.Changes, Change{operation, PartialAttribute{Type: attrType, Vals: attrVals}})
 }

+func (req *ModifyRequest) appendBaseDN(dn string) appendDnRequest {
+	return &ModifyRequest{


Same goes here

cpuschma · 2022-06-12T20:05:42Z

moddn.go

@@ -50,6 +50,16 @@ func NewModifyDNWithControlsRequest(dn string, rdn string, delOld bool,
 	}
 }

+func (req *ModifyDNRequest) appendBaseDN(dn string) appendDnRequest {
+	return &ModifyDNRequest{


Same goes here

cpuschma · 2022-06-12T20:10:13Z

dn.go

@@ -268,3 +268,15 @@ func (r *RelativeDN) hasAllAttributesFold(attrs []*AttributeTypeAndValue) bool {
 func (a *AttributeTypeAndValue) EqualFold(other *AttributeTypeAndValue) bool {
 	return strings.EqualFold(a.Type, other.Type) && strings.EqualFold(a.Value, other.Value)
 }
+
+func appendDN(baseDN, rootDN string) string {


baseDN isn't appended by rootDN if it isn't empty.

appendDN("ou=base", "dc=test,dc=org")

would result in ,dc=test,dc=org, which in turn is a invalid DN syntax.

Also, may you add some documentation what this function is exactly for and what baseDN and rootDN stand for

cpuschma · 2022-06-12T20:11:21Z

del.go

@@ -12,6 +12,13 @@ type DelRequest struct {
 	Controls []Control
 }

+func (req *DelRequest) appendBaseDN(dn string) appendDnRequest {
+	return &DelRequest{


Same goes here

cpuschma · 2022-06-12T20:11:29Z

add.go

@@ -33,6 +33,14 @@ type AddRequest struct {
 	Controls []Control
 }

+func (req *AddRequest) appendBaseDN(dn string) appendDnRequest {
+	return &AddRequest{


Same goes here

cpuschma · 2022-06-12T20:12:50Z

conn.go

@@ -144,35 +148,87 @@ type DialContext struct {
 	tc *tls.Config
 }

-func (dc *DialContext) dial(u *url.URL) (net.Conn, error) {
+func (dc *DialContext) dial(u *url.URL) (conn net.Conn, err error) {


May you update the related tests and add some comments for documentation. The newly added functionality isn't really self-explanatory, especially when it comes down to the rootDN and baseDN

cpuschma · 2022-06-12T20:27:40Z

I also want to mention that this PR doesn't satisfy RFC 2782, as it only covers using the SRV records when using the distinguishedName of the directory server as hostname to connect to, which isn't covered by the RFC document or by any other. May you please point us to the correlated document here?

Sorry if this might sound a bit harsh, but I really didn't quite get what you tried to do here, because I was constantly comparing your changes with the mentioned RFC document and was like: What are you trying here.. especially because:

There are no updated tests
No code comments
Service discovery only works when using a distinguishedName to connect
An entirely new topic was added (feat: implement having rootDN for dialURL)
A faulty function which didn't make sense (appendDN)

// Edit:
And perhaps I'm a bit biased now and maybe even a little bit pissed, but I really dislike having one PR entirely missing the point (implementing the RFC) and even adding a completely new unrelated topic:

feat(discovery): implement LDAP service discovery (RFC 2782)
feat: implement having rootDN for dialURL

Maybe if another maintainer could step in and give in his 5 cents please, if we should continue adding work to this PR or if this is "burned grounds". @stefanmcshane @johnweldon @vetinari

tonyhhyip · 2022-06-13T04:11:31Z

I implement this feature based on the description of DNS SRV Records for LDAP. What I would like to implement is for allowing ldap:///dc=example,dc=com could use SRV record _ldap._tcp.example.com

tonyhhyip · 2022-06-13T04:49:13Z

And document will be written after the code is ready to accept, I don't want the document is out of sync with code

johnweldon · 2022-06-17T20:46:24Z

Maybe if another maintainer could step in and give in his 5 cents please, if we should continue adding work to this PR or if this is "burned grounds". @stefanmcshane @johnweldon @vetinari

I'm inclined to agree about not accepting a PR that 1) mixes several changes together, and 2) implements an RFC incorrectly, or incompletely to the point of being detrimental.

@tonyhhyip it seems like you opened this PR as a starting point for discussion? Let's take some time and figure out what needs to change.

First, let's move the append base DN to a new PR (I'd rather see it exposed as a user-optional feature, than have it baked in), and get that implemented to satisfaction, then let's revisit this to see that RFC 2782 is adequately considered.

cpuschma requested changes Mar 22, 2022

View reviewed changes

v3/conn.go Show resolved Hide resolved

v3/conn.go Show resolved Hide resolved

v3/conn.go Show resolved Hide resolved

vetinari changed the title ~~Closes #329~~ implement LDAP service discovery (RFC 2782) Mar 22, 2022

tonyhhyip changed the title ~~implement LDAP service discovery (RFC 2782)~~ Draft: implement LDAP service discovery (RFC 2782) Mar 28, 2022

cpuschma mentioned this pull request Apr 25, 2022

Connect to multiple instances (clusters) at the same time #314

Open

tonyhhyip requested a review from cpuschma May 31, 2022 02:53

tonyhhyip force-pushed the dns-discovery branch from 52f3871 to eb13da1 Compare May 31, 2022 02:58

tonyhhyip changed the title ~~Draft: implement LDAP service discovery (RFC 2782)~~ feat: implement LDAP service discovery (RFC 2782) May 31, 2022

cpuschma requested changes Jun 12, 2022

View reviewed changes

feat(discovery): implement LDAP service discovery (RFC 2782)

6d25a80

tonyhhyip force-pushed the dns-discovery branch from cf793d4 to 6d25a80 Compare June 13, 2022 04:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement LDAP service discovery (RFC 2782) #362

feat: implement LDAP service discovery (RFC 2782) #362

tonyhhyip commented Mar 21, 2022 •

edited

vetinari commented Mar 22, 2022

tonyhhyip commented Mar 28, 2022 •

edited

tonyhhyip commented May 31, 2022

tonyhhyip commented May 31, 2022

cpuschma commented Jun 1, 2022

cpuschma left a comment

cpuschma Jun 12, 2022 •

edited

tonyhhyip Jun 13, 2022

johnweldon Jun 17, 2022

cpuschma Jun 12, 2022

cpuschma Jun 12, 2022

cpuschma Jun 12, 2022

cpuschma Jun 12, 2022

cpuschma Jun 12, 2022

cpuschma Jun 12, 2022

cpuschma commented Jun 12, 2022 •

edited

tonyhhyip commented Jun 13, 2022

tonyhhyip commented Jun 13, 2022

johnweldon commented Jun 17, 2022

feat: implement LDAP service discovery (RFC 2782) #362

Are you sure you want to change the base?

feat: implement LDAP service discovery (RFC 2782) #362

Conversation

tonyhhyip commented Mar 21, 2022 • edited

vetinari commented Mar 22, 2022

tonyhhyip commented Mar 28, 2022 • edited

tonyhhyip commented May 31, 2022

tonyhhyip commented May 31, 2022

cpuschma commented Jun 1, 2022

cpuschma left a comment

Choose a reason for hiding this comment

cpuschma Jun 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuschma commented Jun 12, 2022 • edited

tonyhhyip commented Jun 13, 2022

tonyhhyip commented Jun 13, 2022

johnweldon commented Jun 17, 2022

tonyhhyip commented Mar 21, 2022 •

edited

tonyhhyip commented Mar 28, 2022 •

edited

cpuschma Jun 12, 2022 •

edited

cpuschma commented Jun 12, 2022 •

edited