Use safer defaults for TLS verification on LDAP connections

The LDAP client connections were hardcoded to ignore certificate validation errors everywhere. This commit changes that to uses a secure default, which can be overridden by the new config parameter 'insecure'. Also the LDAP related test configs are updated to set that override for the tests.
cs3org · Sep 8, 2021 · 2759a8b · 2759a8b
1 parent f2109fc
commit 2759a8b
Show file tree

Hide file tree

Showing 6 changed files with 25 additions and 9 deletions.
diff --git a/changelog/unreleased/ldap-tls-insecure.md b/changelog/unreleased/ldap-tls-insecure.md
@@ -0,0 +1,7 @@
+Enhancement: Safer defaults for TLS verification on LDAP connections
+
+The LDAP client connections where hardcoded to ignore certificate validation
+errors. Now verification is enable by default and a new config parameter 'insecure'
+is introduced to override that default.
+
+https://github.com/cs3org/reva/pull/2053
diff --git a/pkg/auth/manager/ldap/ldap.go b/pkg/auth/manager/ldap/ldap.go
@@ -52,6 +52,7 @@ type mgr struct {
 type config struct {
 	Hostname     string     `mapstructure:"hostname"`
 	Port         int        `mapstructure:"port"`
+	Insecure     bool       `mapstructure:"insecure"`
 	BaseDN       string     `mapstructure:"base_dn"`
 	UserFilter   string     `mapstructure:"userfilter"`
 	LoginFilter  string     `mapstructure:"loginfilter"`
@@ -138,7 +139,7 @@ func (am *mgr) Configure(m map[string]interface{}) error {
 func (am *mgr) Authenticate(ctx context.Context, clientID, clientSecret string) (*user.User, map[string]*authpb.Scope, error) {
 	log := appctx.GetLogger(ctx)
 
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", am.c.Hostname, am.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", am.c.Hostname, am.c.Port), &tls.Config{InsecureSkipVerify: am.c.Insecure})
 	if err != nil {
 		return nil, nil, err
 	}

diff --git a/pkg/group/manager/ldap/ldap.go b/pkg/group/manager/ldap/ldap.go
@@ -52,6 +52,7 @@ type manager struct {
 type config struct {
 	Hostname        string     `mapstructure:"hostname"`
 	Port            int        `mapstructure:"port"`
+	Insecure        bool       `mapstructure:"insecure"`
 	BaseDN          string     `mapstructure:"base_dn"`
 	GroupFilter     string     `mapstructure:"groupfilter"`
 	MemberFilter    string     `mapstructure:"memberfilter"`
@@ -134,7 +135,7 @@ func New(m map[string]interface{}) (group.Manager, error) {
 
 func (m *manager) GetGroup(ctx context.Context, gid *grouppb.GroupId) (*grouppb.Group, error) {
 	log := appctx.GetLogger(ctx)
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}
@@ -211,7 +212,7 @@ func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*gr
 	}
 
 	log := appctx.GetLogger(ctx)
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}
@@ -269,7 +270,7 @@ func (m *manager) GetGroupByClaim(ctx context.Context, claim, value string) (*gr
 }
 
 func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Group, error) {
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}
@@ -321,7 +322,7 @@ func (m *manager) FindGroups(ctx context.Context, query string) ([]*grouppb.Grou
 }
 
 func (m *manager) GetMembers(ctx context.Context, gid *grouppb.GroupId) ([]*userpb.UserId, error) {
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/user/manager/ldap/ldap.go b/pkg/user/manager/ldap/ldap.go
@@ -51,6 +51,7 @@ type manager struct {
 type config struct {
 	Hostname        string     `mapstructure:"hostname"`
 	Port            int        `mapstructure:"port"`
+	Insecure        bool       `mapstructure:"insecure"`
 	BaseDN          string     `mapstructure:"base_dn"`
 	UserFilter      string     `mapstructure:"userfilter"`
 	AttributeFilter string     `mapstructure:"attributefilter"`
@@ -146,7 +147,7 @@ func (m *manager) Configure(ml map[string]interface{}) error {
 
 func (m *manager) GetUser(ctx context.Context, uid *userpb.UserId) (*userpb.User, error) {
 	log := appctx.GetLogger(ctx)
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}
@@ -234,7 +235,7 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use
 	}
 
 	log := appctx.GetLogger(ctx)
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}
@@ -306,7 +307,7 @@ func (m *manager) GetUserByClaim(ctx context.Context, claim, value string) (*use
 }
 
 func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User, error) {
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return nil, err
 	}
@@ -376,7 +377,7 @@ func (m *manager) FindUsers(ctx context.Context, query string) ([]*userpb.User,
 }
 
 func (m *manager) GetUserGroups(ctx context.Context, uid *userpb.UserId) ([]string, error) {
-	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: true})
+	l, err := ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", m.c.Hostname, m.c.Port), &tls.Config{InsecureSkipVerify: m.c.Insecure})
 	if err != nil {
 		return []string{}, err
 	}

diff --git a/tests/oc-integration-tests/drone/ldap-users.toml b/tests/oc-integration-tests/drone/ldap-users.toml
@@ -13,6 +13,7 @@ auth_manager = "ldap"
 [grpc.services.authprovider.auth_managers.ldap]
 hostname="ldap"
 port=636
+insecure=true
 base_dn="dc=owncloud,dc=com"
 loginfilter="(&(objectclass=posixAccount)(|(cn={{login}}))(uid={{login}}))"
 bind_username="cn=admin,dc=owncloud,dc=com"
@@ -30,6 +31,7 @@ driver = "ldap"
 [grpc.services.userprovider.drivers.ldap]
 hostname="ldap"
 port=636
+insecure=true
 base_dn="dc=owncloud,dc=com"
 userfilter="(&(objectclass=posixAccount)(|(uid={{.OpaqueId}})(cn={{.OpaqueId}})))"
 findfilter="(&(objectclass=posixAccount)(|(cn={{query}}*)(displayname={{query}}*)(mail={{query}}*)))"
@@ -51,6 +53,7 @@ driver = "ldap"
 [grpc.services.groupprovider.drivers.ldap]
 hostname="ldap"
 port=636
+insecure=true
 base_dn="dc=owncloud,dc=com"
 groupfilter="(&(objectclass=posixGroup)(|(gid={{.OpaqueId}})(cn={{.OpaqueId}})))"
 findfilter="(&(objectclass=posixGroup)(|(cn={{query}}*)(displayname={{query}}*)(mail={{query}}*)))"

diff --git a/tests/oc-integration-tests/local/ldap-users.toml b/tests/oc-integration-tests/local/ldap-users.toml
@@ -13,6 +13,7 @@ auth_manager = "ldap"
 [grpc.services.authprovider.auth_managers.ldap]
 hostname="localhost"
 port=636
+insecure=true
 base_dn="dc=owncloud,dc=com"
 loginfilter="(&(objectclass=posixAccount)(|(cn={{login}}))(uid={{login}}))"
 bind_username="cn=admin,dc=owncloud,dc=com"
@@ -30,6 +31,7 @@ driver = "ldap"
 [grpc.services.userprovider.drivers.ldap]
 hostname="localhost"
 port=636
+insecure=true
 base_dn="dc=owncloud,dc=com"
 userfilter="(&(objectclass=posixAccount)(|(uid={{.OpaqueId}})(cn={{.OpaqueId}})))"
 findfilter="(&(objectclass=posixAccount)(|(cn={{query}}*)(displayname={{query}}*)(mail={{query}}*)))"
@@ -51,6 +53,7 @@ driver = "ldap"
 [grpc.services.groupprovider.drivers.ldap]
 hostname="localhost"
 port=636
+insecure=true
 base_dn="dc=owncloud,dc=com"
 groupfilter="(&(objectclass=posixGroup)(|(gid={{.OpaqueId}})(cn={{.OpaqueId}})))"
 findfilter="(&(objectclass=posixGroup)(|(cn={{query}}*)(displayname={{query}}*)(mail={{query}}*)))"