shaj13 / go-guardian

Go-Guardian is a golang library that provides a simple, clean, and idiomatic way to create powerful modern API and web authentication.
MIT License
543 stars 55 forks source link

LDAPS Schema does not work with 389 + Start TLS Server #116

Closed m87carlson closed 2 years ago

m87carlson commented 2 years ago

What version of Go are you using (go version)?

$ go version 1.17

Does this issue reproduce with the latest release?

yes

What version of Go-Guardian are you using ?

Go-Guardian Version: 2.11.4

What did you do?

I configured ldap against a server that has port 636 "ldaps" disabled, and only uses StartTLS on port 389.

What did you expect to see?

I expected to connect and authenticate with ldap credentials

What did you see instead?

"Network Error": read tcp x.x.x.x:60677->x.x.x.x:389: read: connection reset by peer

I believe this should still be scheme ldap, and ldaps should be used against an LDAP server with SSL enabled on port 636.

This would remediate the issue:

diff --git a/auth/strategies/ldap/ldap.go b/auth/strategies/ldap/ldap.go
index ec9b920..cc00906 100644
--- a/auth/strategies/ldap/ldap.go
+++ b/auth/strategies/ldap/ldap.go
@@ -56,10 +56,13 @@ func dial(cfg *Config) (conn, error) {
    opts := []ldap.DialOpt{}

    if cfg.TLS != nil {
-       scheme = "ldaps"
        opts = append(opts, ldap.DialWithTLSConfig(cfg.TLS))
    }

+  if cfg.Port == "636" {
+    scheme = "ldaps"
+  }
+
    addr := fmt.Sprintf("%s://%s:%s", scheme, cfg.Host, cfg.Port)
    return ldap.DialURL(addr, opts...)
 }
m87carlson commented 2 years ago

my employer has an internal LDAP server that has plaintext disabled on 389, and you can only connect with starttls. It also has what is referred to as "ldaps" on port 636 disabled (as that has been considered deprecated and insecure).

this was my setup, I have redacted any sensitive information such as user names, actual ldap servers, and groups.

func setupGoGuardian() {
    keeper = jwt.StaticSecret{
        Secret:    []byte(secret),
        ID:        "secret-id",
        Algorithm: jwt.HS256,
    }

    tlsCfg := &tls.Config{
        ServerName:         "ldap.company.com",
        MaxVersion:         tls.VersionTLS13,
        InsecureSkipVerify: false,
    }

    ldapBind := os.Getenv("LDAP_BIND_ACCOUNT")
    ldapBindPw := os.Getenv("LDAP_BIND_PASSWORD")
    cfg := &ldap.Config{
        BaseDN: "dc=company,dc=com",
        BindDN: ldapBind,
        Port: "389",
        TLS: tlsCfg,
        BindPassword: ldapBindPw,
        Host: "ldap.company.com",
        Filter: "(&(user=%s)(memberOf=cn=it,ou=groups,dc=company,dc=com))",
    }

    cache := libcache.FIFO.New(0)
    cache.SetTTL(time.hour * 1)
    cache.RegisterOnExpired(func(key,_ interface{}){
        cache.Peek(key)
    })
    jwtStrategy := jwt.New(cache, keeper)
    ldapStrategy := ldap.NewCached(cfg, cache)
    strategy = union.New(ldapStrategy, jwtStrategy)
}

With schema being forced to ldaps this would happen:

2021/12/14 09:05:47 server started and listening on http://localhost:8080
2021/12/14 09:05:50 executing auth middleware
2021/12/14 09:05:50 Network Error": read tcp x.x.x.x:60677->x.x.x.x:389: read: connection reset by peer

however, with my fork (which I again sincerely apologize for PR updates, I forgot I did not use a branch but master...)

2021/12/14 09:05:47 server started and listening on http://localhost:8080
2021/12/14 09:05:50 executing auth middleware
2021/12/14 09:05:50 user foo authenticated

If using the provided diff is not sufficient, then perhaps you can pass the schema from the ldap config object into ldap.go, so a end user like myself can override ldaps when cfg.TLS is in use.

shaj13 commented 2 years ago

@m87carlson can't use the port to distinguish SSL and non SSL LDAP server. 636 is the default but users can deploy SSL LDAP server on any desired port.

 if cfg.Port == "636" {
   scheme = "ldaps"
  }

I'd like to see your configuration, make sure to mask sensitive data with XXX. e.g

    cfg := &ldap.Config{
        BaseDN:       "dc=example,dc=com",
        BindDN:       "cn=read-only-admin,dc=example,dc=com",
        Port:         "389",
        Host:         "XXX.xxx.XXX",
        BindPassword: "XXXX",
        Filter:       "(uid=%s)",
    } 
m87carlson commented 2 years ago

I added the if statement for port being 636 just to allow some continuity, but honestly no one should be using ldaps: https://www.openldap.org/faq/data/cache/605.html

In fact, ldaps isn't even part of the RFC for ldap.

In any case, if I could either have schema exposed so I can override it in my config, or not force ldaps as the scheme when TLS is defined that would be fine with me.

shaj13 commented 2 years ago

@m87carlson agree, https://datatracker.ietf.org/doc/html/rfc2830 ldaps used with sslV2 and is deprecated in OpenLDAP but still supported.

shaj13 commented 2 years ago

looks like need to deprecate the port and host not sure why the separation was introduced. However, the user can provide a full URL, and it's more idiomatic. obviously, we cant add any breaking change so the idealistic fix will be by deprecating port and host while keeping backward compatibility so the applications keep working even after upgrading the library.

// Config define the configuration to connect to LDAP.
type Config struct {
    // Port LDAP server port.
        // 
    // Deprecated: Use URL instead.
    Port string
    // Host LDAP server host.
        // 
    // Deprecated: Use URL instead.
    Host string
        // URL specifies LDAP server URL. 
       URL string
........
}

func dial(cfg *Config) (conn, error) {
    scheme := "ldap"
    opts := []ldap.DialOpt{}

    if cfg.TLS != nil {
        scheme = "ldaps"
        opts = append(opts, ldap.DialWithTLSConfig(cfg.TLS))
    }

         if cfg.URL != "" {
                cfg.URL = fmt.Sprintf("%s://%s:%s", scheme, cfg.Host, cfg.Port)        
         } 

    return ldap.DialURL(cfg.URL, opts...)
}

TODO:

@m87carlson I'd be glad to see PR.

m87carlson commented 2 years ago

Hmm, we still might need to remove the scheme = "ldaps" here:

        if cfg.TLS != nil {
        scheme = "ldaps"
        opts = append(opts, ldap.DialWithTLSConfig(cfg.TLS))
    }

or, make sure ldap.DialWithTLSConfig is called by another means.

Otherwise, I'm more than happy to get a PR going with the listed requirements!

shaj13 commented 2 years ago

@m87carlson PTAL https://go.dev/play/p/IAFuN13L0et the user has the ability to override the scheme by the url.

shaj13 commented 2 years ago

Fixed by #117