Merge pull request #483 from ericchiang/ldap-groups

Clean up LDAP Connector

.travis.yml

@@ -12,6 +12,7 @@ go:
 env:
   - DEX_TEST_DSN="postgres://postgres@127.0.0.1:15432/postgres?sslmode=disable" ISOLATED=true
     DEX_TEST_LDAP_HOST="tlstest.local:1389"
+    DEX_TEST_LDAPS_HOST="tlstest.local:1636"
     DEX_TEST_LDAP_BINDNAME="cn=admin,dc=example,dc=org"
     DEX_TEST_LDAP_BINDPASS="admin"
 

connector/connector_ldap_test.go

@@ -21,6 +21,7 @@ func init() {
 func TestLDAPConnectorConfigValidTLS(t *testing.T) {
 	cc := LDAPConnectorConfig{
 		ID:     "ldap",
+		Host:   "example.com:636",
 		UseTLS: true,
 		UseSSL: false,
 	}
@@ -34,6 +35,7 @@ func TestLDAPConnectorConfigValidTLS(t *testing.T) {
 func TestLDAPConnectorConfigInvalidSSLandTLS(t *testing.T) {
 	cc := LDAPConnectorConfig{
 		ID:     "ldap",
+		Host:   "example.com:636",
 		UseTLS: true,
 		UseSSL: true,
 	}
@@ -47,6 +49,7 @@ func TestLDAPConnectorConfigInvalidSSLandTLS(t *testing.T) {
 func TestLDAPConnectorConfigValidSearchScope(t *testing.T) {
 	cc := LDAPConnectorConfig{
 		ID:          "ldap",
+		Host:        "example.com:636",
 		SearchScope: "one",
 	}
 
@@ -59,6 +62,7 @@ func TestLDAPConnectorConfigValidSearchScope(t *testing.T) {
 func TestLDAPConnectorConfigInvalidSearchScope(t *testing.T) {
 	cc := LDAPConnectorConfig{
 		ID:          "ldap",
+		Host:        "example.com:636",
 		SearchScope: "three",
 	}
 
@@ -71,6 +75,7 @@ func TestLDAPConnectorConfigInvalidSearchScope(t *testing.T) {
 func TestLDAPConnectorConfigInvalidCertFileNoKeyFile(t *testing.T) {
 	cc := LDAPConnectorConfig{
 		ID:       "ldap",
+		Host:     "example.com:636",
 		CertFile: "/tmp/ldap.crt",
 	}
 
@@ -83,6 +88,7 @@ func TestLDAPConnectorConfigInvalidCertFileNoKeyFile(t *testing.T) {
 func TestLDAPConnectorConfigValidCertFileAndKeyFile(t *testing.T) {
 	cc := LDAPConnectorConfig{
 		ID:       "ldap",
+		Host:     "example.com:636",
 		CertFile: "/tmp/ldap.crt",
 		KeyFile:  "/tmp/ldap.key",
 	}

connector/connector_local.go

@@ -87,7 +87,7 @@ func (c *LocalConnector) LoginURL(sessionKey, prompt string) (string, error) {
 
 func (c *LocalConnector) Register(mux *http.ServeMux, errorURL url.URL) {
 	route := c.namespace.Path + "/login"
-	mux.Handle(route, handleLoginFunc(c.loginFunc, c.loginTpl, c.idp, route, errorURL))
+	mux.Handle(route, handlePasswordLogin(c.loginFunc, c.loginTpl, c.idp, route, errorURL))
 }
 
 func (c *LocalConnector) Sync() chan struct{} {

connector/interface.go

@@ -65,7 +65,3 @@ type ConnectorConfigRepo interface {
 	GetConnectorByID(repo.Transaction, string) (ConnectorConfig, error)
 	Set(cfgs []ConnectorConfig) error
 }
-
-type IdentityProvider interface {
-	Identity(email, password string) (*oidc.Identity, error)
-}

connector/login_local.go

@@ -18,7 +18,12 @@ func redirectPostError(w http.ResponseWriter, errorURL url.URL, q url.Values) {
 	w.WriteHeader(http.StatusSeeOther)
 }
 
-func handleLoginFunc(lf oidc.LoginFunc, tpl *template.Template, idp IdentityProvider, localErrorPath string, errorURL url.URL) http.HandlerFunc {
+// passwordLoginProvider is a provider which requires a username and password to identify the user.
+type passwordLoginProvider interface {
+	Identity(email, password string) (*oidc.Identity, error)
+}
+
+func handlePasswordLogin(lf oidc.LoginFunc, tpl *template.Template, idp passwordLoginProvider, localErrorPath string, errorURL url.URL) http.HandlerFunc {
 	handleGET := func(w http.ResponseWriter, r *http.Request, errMsg string) {
 		q := r.URL.Query()
 		sessionKey := q.Get("session_key")
@@ -54,9 +59,7 @@ func handleLoginFunc(lf oidc.LoginFunc, tpl *template.Template, idp IdentityProv
 		}
 
 		ident, err := idp.Identity(userid, password)
-		log.Errorf("IDENTITY: err: %v", err)
-
-		if ident == nil || err != nil {
+		if err != nil {
 			handleGET(w, r, "invalid login")
 			return
 		}

functional/ldap_test.go

 package functional
 
 import (
-	"fmt"
 	"html/template"
-	"net"
 	"net/url"
 	"os"
-	"strconv"
 	"sync"
 	"testing"
 	"time"
@@ -16,45 +13,41 @@ import (
 	"gopkg.in/ldap.v2"
 )
 
-var (
-	ldapHost   string
-	ldapPort   uint16
-	ldapBindDN string
-	ldapBindPw string
-)
-
 type LDAPServer struct {
-	Host   string
-	Port   uint16
-	BindDN string
-	BindPw string
+	Host      string // Address (host:port) of LDAP service.
+	LDAPSHost string // Address (host:port) of LDAPS service (TLS port).
+	BindDN    string
+	BindPw    string
 }
 
 const (
-	ldapEnvHost     = "DEX_TEST_LDAP_HOST"
+	ldapEnvHost  = "DEX_TEST_LDAP_HOST"
+	ldapsEnvHost = "DEX_TEST_LDAPS_HOST"
+
 	ldapEnvBindName = "DEX_TEST_LDAP_BINDNAME"
 	ldapEnvBindPass = "DEX_TEST_LDAP_BINDPASS"
 )
 
 func ldapServer(t *testing.T) LDAPServer {
-	host := os.Getenv(ldapEnvHost)
-	if host == "" {
-		t.Fatalf("%s not set", ldapEnvHost)
-	}
-	var port uint64 = 389
-	if h, p, err := net.SplitHostPort(host); err == nil {
-		port, err = strconv.ParseUint(p, 10, 16)
-		if err != nil {
-			t.Fatalf("failed to parse port: %v", err)
+	getenv := func(key string) string {
+		val := os.Getenv(key)
+		if val == "" {
+			t.Fatalf("%s not set", key)
 		}
-		host = h
+		t.Logf("%s=%v", key, val)
+		return val
+	}
+	return LDAPServer{
+		Host:      getenv(ldapEnvHost),
+		LDAPSHost: getenv(ldapsEnvHost),
+		BindDN:    os.Getenv(ldapEnvBindName),
+		BindPw:    os.Getenv(ldapEnvBindPass),
 	}
-	return LDAPServer{host, uint16(port), os.Getenv(ldapEnvBindName), os.Getenv(ldapEnvBindPass)}
 }
 
 func TestLDAPConnect(t *testing.T) {
 	server := ldapServer(t)
-	l, err := ldap.Dial("tcp", fmt.Sprintf("%s:%d", server.Host, server.Port))
+	l, err := ldap.Dial("tcp", server.Host)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -74,39 +67,35 @@ func TestConnectorLDAPHealthy(t *testing.T) {
 	}{
 		{
 			config: connector.LDAPConnectorConfig{
-				ID:         "ldap",
-				ServerHost: server.Host,
-				ServerPort: server.Port + 1,
+				ID:   "ldap",
+				Host: "localhost:0",
 			},
 			wantErr: true,
 		},
 		{
 			config: connector.LDAPConnectorConfig{
-				ID:         "ldap",
-				ServerHost: server.Host,
-				ServerPort: server.Port,
+				ID:   "ldap",
+				Host: server.Host,
 			},
 		},
 		{
 			config: connector.LDAPConnectorConfig{
-				ID:         "ldap",
-				ServerHost: server.Host,
-				ServerPort: server.Port,
-				UseTLS:     true,
-				CertFile:   "/tmp/ldap.crt",
-				KeyFile:    "/tmp/ldap.key",
-				CaFile:     "/tmp/openldap-ca.pem",
+				ID:       "ldap",
+				Host:     server.Host,
+				UseTLS:   true,
+				CertFile: "/tmp/ldap.crt",
+				KeyFile:  "/tmp/ldap.key",
+				CaFile:   "/tmp/openldap-ca.pem",
 			},
 		},
 		{
 			config: connector.LDAPConnectorConfig{
-				ID:         "ldap",
-				ServerHost: server.Host,
-				ServerPort: server.Port + 247, // 636
-				UseSSL:     true,
-				CertFile:   "/tmp/ldap.crt",
-				KeyFile:    "/tmp/ldap.key",
-				CaFile:     "/tmp/openldap-ca.pem",
+				ID:       "ldap",
+				Host:     server.LDAPSHost,
+				UseSSL:   true,
+				CertFile: "/tmp/ldap.crt",
+				KeyFile:  "/tmp/ldap.key",
+				CaFile:   "/tmp/openldap-ca.pem",
 			},
 		},
 	}
@@ -131,8 +120,7 @@ func TestLDAPPoolHighWatermarkAndLockContention(t *testing.T) {
 	server := ldapServer(t)
 	ldapPool := &connector.LDAPPool{
 		MaxIdleConn: 30,
-		ServerHost:  server.Host,
-		ServerPort:  server.Port,
+		Host:        server.Host,
 		UseTLS:      false,
 		UseSSL:      false,
 	}
@@ -151,17 +139,16 @@ func TestLDAPPoolHighWatermarkAndLockContention(t *testing.T) {
 				case <-ctx.Done():
 					return
 				default:
-					ldapConn, err := ldapPool.Acquire()
-					if err != nil {
-						t.Errorf("Unable to acquire LDAP Connection: %v", err)
-					}
-					s := ldap.NewSearchRequest("", ldap.ScopeBaseObject, ldap.NeverDerefAliases, 0, 0, false, "(objectClass=*)", []string{}, nil)
-					_, err = ldapConn.Search(s)
+					err := ldapPool.Do(func(conn *ldap.Conn) error {
+						s := &ldap.SearchRequest{
+							Scope:  ldap.ScopeBaseObject,
+							Filter: "(objectClass=*)",
+						}
+						_, err := conn.Search(s)
+						return err
+					})
 					if err != nil {
 						t.Errorf("Search request failed. Dead/invalid LDAP connection from pool?: %v", err)
-						ldapConn.Close()
-					} else {
-						ldapPool.Put(ldapConn)
 					}
 					_, _ = ldapPool.CheckConnections()
 				}

static/fixtures/connectors.json.sample

@@ -25,8 +25,7 @@
 	{
 		"type": "ldap",
 		"id": "ldap",
-		"serverHost": "127.0.0.1",
-		"serverPort": 389,
+		"host": "127.0.0.1:389",
 		"useTLS": true,
 		"useSSL": false,
 		"caFile": "/etc/ssl/certs/example_com_root.crt",