Use safer defaults for TLS verification on LDAP connections (#2053)

cs3org · Sep 16, 2021 · af7f2a2 · af7f2a2
1 parent de30aee
commit af7f2a2
Show file tree

Hide file tree

Showing 7 changed files with 94 additions and 29 deletions.
diff --git a/changelog/unreleased/ldap-tls-insecure.md b/changelog/unreleased/ldap-tls-insecure.md
@@ -0,0 +1,8 @@
+Enhancement: Safer defaults for TLS verification on LDAP connections
+
+The LDAP client connections were hardcoded to ignore certificate validation
+errors. Now verification is enabled by default and a new config parameter 'insecure'
+is introduced to override that default. It is also possible to add trusted Certificates
+by using the new 'cacert' config paramter.
+
+https://github.com/cs3org/reva/pull/2053
diff --git a/pkg/auth/manager/ldap/ldap.go b/pkg/auth/manager/ldap/ldap.go
@@ -20,8 +20,6 @@ package ldap
 
 import (
 	"context"
-	"crypto/tls"
-	"fmt"
 	"strconv"
 	"strings"
 
@@ -36,6 +34,7 @@ import (
 	"github.com/cs3org/reva/pkg/logger"
 	"github.com/cs3org/reva/pkg/rgrpc/todo/pool"
 	"github.com/cs3org/reva/pkg/sharedconf"
+	"github.com/cs3org/reva/pkg/utils"
 	"github.com/go-ldap/ldap/v3"
 	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
@@ -50,17 +49,16 @@ type mgr struct {
 }
 
 type config struct {
-	Hostname     string     `mapstructure:"hostname"`
-	Port         int        `mapstructure:"port"`
-	BaseDN       string     `mapstructure:"base_dn"`
-	UserFilter   string     `mapstructure:"userfilter"`
-	LoginFilter  string     `mapstructure:"loginfilter"`
-	BindUsername string     `mapstructure:"bind_username"`
-	BindPassword string     `mapstructure:"bind_password"`
-	Idp          string     `mapstructure:"idp"`
-	GatewaySvc   string     `mapstructure:"gatewaysvc"`
-	Schema       attributes `mapstructure:"schema"`
-	Nobody       int64      `mapstructure:"nobody"`
+	utils.LDAPConn `mapstructure:",squash"`
+	BaseDN         string     `mapstructure:"base_dn"`
+	UserFilter     string     `mapstructure:"userfilter"`
+	LoginFilter    string     `mapstructure:"loginfilter"`
+	BindUsername   string     `mapstructure:"bind_username"`
+	BindPassword   string     `mapstructure:"bind_password"`
+	Idp            string     `mapstructure:"idp"`
+	GatewaySvc     string     `mapstructure:"gatewaysvc"`
+	Schema         attributes `mapstructure:"schema"`
+	Nobody         int64      `mapstructure:"nobody"`
 }
 
 type attributes struct {
@@ -137,8 +135,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 := utils.GetLDAPConnection(&am.c.LDAPConn)
 	if err != nil {
 		return nil, nil, err
 	}

diff --git a/pkg/group/manager/ldap/ldap.go b/pkg/group/manager/ldap/ldap.go
@@ -21,7 +21,6 @@ package ldap
 import (
 	"bytes"
 	"context"
-	"crypto/tls"
 	"fmt"
 	"strconv"
 	"strings"
@@ -34,6 +33,7 @@ import (
 	"github.com/cs3org/reva/pkg/errtypes"
 	"github.com/cs3org/reva/pkg/group"
 	"github.com/cs3org/reva/pkg/group/manager/registry"
+	"github.com/cs3org/reva/pkg/utils"
 	"github.com/go-ldap/ldap/v3"
 	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
@@ -50,8 +50,7 @@ type manager struct {
 }
 
 type config struct {
-	Hostname        string     `mapstructure:"hostname"`
-	Port            int        `mapstructure:"port"`
+	utils.LDAPConn  `mapstructure:",squash"`
 	BaseDN          string     `mapstructure:"base_dn"`
 	GroupFilter     string     `mapstructure:"groupfilter"`
 	MemberFilter    string     `mapstructure:"memberfilter"`
@@ -134,7 +133,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}
@@ -211,7 +210,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}
@@ -269,7 +268,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}
@@ -321,7 +320,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/user/manager/ldap/ldap.go b/pkg/user/manager/ldap/ldap.go
@@ -21,7 +21,6 @@ package ldap
 import (
 	"bytes"
 	"context"
-	"crypto/tls"
 	"fmt"
 	"strconv"
 	"strings"
@@ -33,6 +32,7 @@ import (
 	"github.com/cs3org/reva/pkg/errtypes"
 	"github.com/cs3org/reva/pkg/user"
 	"github.com/cs3org/reva/pkg/user/manager/registry"
+	"github.com/cs3org/reva/pkg/utils"
 	"github.com/go-ldap/ldap/v3"
 	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
@@ -49,8 +49,7 @@ type manager struct {
 }
 
 type config struct {
-	Hostname        string     `mapstructure:"hostname"`
-	Port            int        `mapstructure:"port"`
+	utils.LDAPConn  `mapstructure:",squash"`
 	BaseDN          string     `mapstructure:"base_dn"`
 	UserFilter      string     `mapstructure:"userfilter"`
 	AttributeFilter string     `mapstructure:"attributefilter"`
@@ -146,7 +145,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}
@@ -234,7 +233,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}
@@ -306,7 +305,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return nil, err
 	}
@@ -376,7 +375,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 := utils.GetLDAPConnection(&m.c.LDAPConn)
 	if err != nil {
 		return []string{}, err
 	}

diff --git a/pkg/utils/ldap.go b/pkg/utils/ldap.go
@@ -0,0 +1,56 @@
+// Copyright 2021 CERN
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// In applying this license, CERN does not waive the privileges and immunities
+// granted to it by virtue of its status as an Intergovernmental Organization
+// or submit itself to any jurisdiction.
+
+package utils
+
+import (
+	"crypto/tls"
+	"crypto/x509"
+	"fmt"
+	"io/ioutil"
+
+	"github.com/go-ldap/ldap/v3"
+	"github.com/pkg/errors"
+)
+
+// LDAPConn holds the basic parameter for setting up an
+// LDAP connection.
+type LDAPConn struct {
+	Hostname string `mapstructure:"hostname"`
+	Port     int    `mapstructure:"port"`
+	Insecure bool   `mapstructure:"insecure"`
+	CACert   string `mapstructure:"cacert"`
+}
+
+// GetLDAPConnection initializes an LDAPS connection and allows
+// to set TLS options e.g. to add trusted Certificates or disable
+// Certificate verification
+func GetLDAPConnection(c *LDAPConn) (*ldap.Conn, error) {
+	tlsconfig := &tls.Config{InsecureSkipVerify: c.Insecure}
+
+	if !c.Insecure && c.CACert != "" {
+		if pemBytes, err := ioutil.ReadFile(c.CACert); err == nil {
+			rpool, _ := x509.SystemCertPool()
+			rpool.AppendCertsFromPEM(pemBytes)
+			tlsconfig.RootCAs = rpool
+		} else {
+			return nil, errors.Wrapf(err, "Error reading LDAP CA Cert '%s.'", c.CACert)
+		}
+	}
+	return ldap.DialTLS("tcp", fmt.Sprintf("%s:%d", c.Hostname, c.Port), tlsconfig)
+}
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}}*)))"