From 2759a8b890076a11068e655c71bd56739a24f5f1 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 8 Sep 2021 12:05:08 +0200 Subject: [PATCH] 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. --- changelog/unreleased/ldap-tls-insecure.md | 7 +++++++ pkg/auth/manager/ldap/ldap.go | 3 ++- pkg/group/manager/ldap/ldap.go | 9 +++++---- pkg/user/manager/ldap/ldap.go | 9 +++++---- tests/oc-integration-tests/drone/ldap-users.toml | 3 +++ tests/oc-integration-tests/local/ldap-users.toml | 3 +++ 6 files changed, 25 insertions(+), 9 deletions(-) create mode 100644 changelog/unreleased/ldap-tls-insecure.md diff --git a/changelog/unreleased/ldap-tls-insecure.md b/changelog/unreleased/ldap-tls-insecure.md new file mode 100644 index 00000000000..332187e89e7 --- /dev/null +++ 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 index b136a05f5db..baeb32419c0 100644 --- 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 index 196d608ade8..3bd848349c7 100644 --- 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 index c2dcde670fe..8fe68fd98d1 100644 --- 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 index cce99f82c3d..7bb96e0c9a0 100644 --- 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 index 3124ba21c76..d068a0eaca7 100644 --- 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}}*)))"