From 426f5260ad57c3783a2a110ac0c3479ac1bc2b67 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Thu, 3 Dec 2020 16:23:52 +1100 Subject: [PATCH] [FEATURE] LDAP StartTLS (#1500) * add start_tls config option * add StartTLS method to the LDAP conn factory and the mock * implemented use of the StartTLS method when the config is set to true * add mock unit tests * add docs * add TLS min version support * add tests to tls version method * fix lint issues * minor adjustments * remove SSL3.0 * add tls consts * deprecate old filter placeholders * remove redundant fake hashing in file auth provider (to delay username enumeration, was replaced by #993 * make suite ActiveDirectory use StartTLS * misc adjustments to docs * suggested changes from code review * deprecation notice conformity * add mock test for LDAPS plus StartTLS --- BREAKING.md | 8 + config.template.yml | 19 +- docs/configuration/authentication/ldap.md | 39 +++- internal/authentication/const.go | 4 + internal/authentication/file_user_provider.go | 22 --- .../authentication/ldap_connection_factory.go | 6 + .../ldap_connection_factory_mock.go | 29 ++- internal/authentication/ldap_user_provider.go | 85 ++++---- .../authentication/ldap_user_provider_test.go | 187 +++++++++++++++++- .../configuration/schema/authentication.go | 3 + .../configuration/validator/authentication.go | 6 + .../validator/authentication_test.go | 89 +++++++++ internal/configuration/validator/const.go | 2 + .../suites/ActiveDirectory/configuration.yml | 3 +- internal/utils/const.go | 15 ++ internal/utils/strings.go | 18 ++ internal/utils/strings_test.go | 52 +++++ 17 files changed, 507 insertions(+), 80 deletions(-) diff --git a/BREAKING.md b/BREAKING.md index bdeb5d1d..c4864b06 100644 --- a/BREAKING.md +++ b/BREAKING.md @@ -6,6 +6,14 @@ recommended not to use the 'latest' Docker image tag blindly but pick a version and read this documentation before upgrading. This is where you will get information about breaking changes and about what you should do to overcome those changes. +## Breaking in v4.24.0 + +### Deprecation Notice(s) +* LDAP User Provider Filters (final removal in 4.27.0): + * User Filters containing `{0}` are being deprecated and will generate warnings. Replaced with `{input}`. + * Group Filters containing `{0}` or `{1}` are being deprecated and will generate warnings. + Replaced with `{input}` and `{username}` respectively. + ## Breaking in v4.21.0 * New LDAP attribute `display_name_attribute` has been introduced, defaults to value: `displayname`. * New key `displayname` has been introduced into the file based user database. diff --git a/config.template.yml b/config.template.yml index 98c2aa92..004ec910 100644 --- a/config.template.yml +++ b/config.template.yml @@ -102,16 +102,21 @@ authentication_backend: # Depending on the option here certain other values in this section have a default value, notably all # of the attribute mappings have a default value that this config overrides, you can read more # about these default values at https://docs.authelia.com/configuration/authentication/ldap.html#defaults - implementation: custom - # The url to the ldap server. Scheme can be ldap:// or ldaps:// + # The url to the ldap server. Scheme can be ldap or ldaps in the format (port optional) ://
[:]. url: ldap://127.0.0.1 - # Skip verifying the server certificate (to allow self-signed certificate). + # Skip verifying the server certificate (to allow a self-signed certificate). skip_verify: false - - # The base dn for every entries + + # Use StartTLS with the LDAP connection. + start_tls: false + + # Minimum TLS version for either Secure LDAP or LDAP StartTLS. + minimum_tls_version: TLS1.2 + + # The base dn for every entries. base_dn: dc=example,dc=com # The attribute holding the username of the user. This attribute is used to populate @@ -127,7 +132,7 @@ authentication_backend: # https://www.ietf.org/rfc/rfc2307.txt. # username_attribute: uid - # An additional dn to define the scope to all users + # An additional dn to define the scope to all users. additional_users_dn: ou=users # The users filter used in search queries to find the user profile based on input filled in login form. @@ -145,7 +150,7 @@ authentication_backend: # (&(|({username_attribute}={input})({mail_attribute}={input}))(objectClass=person)) users_filter: (&({username_attribute}={input})(objectClass=person)) - # An additional dn to define the scope of groups + # An additional dn to define the scope of groups. additional_groups_dn: ou=groups # The groups filter used in search queries to find the groups of the user. diff --git a/docs/configuration/authentication/ldap.md b/docs/configuration/authentication/ldap.md index ae76f307..654435de 100644 --- a/docs/configuration/authentication/ldap.md +++ b/docs/configuration/authentication/ldap.md @@ -50,13 +50,19 @@ authentication_backend: # about these default values at https://docs.authelia.com/configuration/authentication/ldap.html#defaults implementation: custom - # The url to the ldap server. Scheme can be ldap:// or ldaps:// + # The url to the ldap server. Scheme can be ldap or ldaps in the format (port optional) ://
[:]. url: ldap://127.0.0.1 - # Skip verifying the server certificate (to allow self-signed certificate). + # Skip verifying the server certificate (to allow a self-signed certificate). skip_verify: false - - # The base dn for every entries + + # Use StartTLS with the LDAP connection. + start_tls: false + + # Minimum TLS version for either Secure LDAP or LDAP StartTLS. + minimum_tls_version: TLS1.2 + + # The base dn for every entries. base_dn: dc=example,dc=com # The attribute holding the username of the user. This attribute is used to populate @@ -72,7 +78,7 @@ authentication_backend: # https://www.ietf.org/rfc/rfc2307.txt. # username_attribute: uid - # An additional dn to define the scope to all users + # An additional dn to define the scope to all users. additional_users_dn: ou=users # The users filter used in search queries to find the user profile based on input filled in login form. @@ -90,7 +96,7 @@ authentication_backend: # (&(|({username_attribute}={input})({mail_attribute}={input}))(objectClass=person)) users_filter: (&({username_attribute}={input})(objectClass=person)) - # An additional dn to define the scope of groups + # An additional dn to define the scope of groups. additional_groups_dn: ou=groups # The groups filter used in search queries to find the groups of the user. @@ -123,6 +129,27 @@ The user must have an email address in order for Authelia to perform identity verification when a user attempts to reset their password or register a second factor device. +## TLS Settings + +### Skip Verify + +The key `skip_verify` disables checking the authenticity of the TLS certificate. You should not disable this, instead +you should add the certificate that signed the certificate of your LDAP server to the machines certificate PKI trust. +For docker you can just add this to the hosts trusted store. + +### Start TLS + +The key `start_tls` enables use of the LDAP StartTLS process which is not commonly used. You should only configure this +if you know you need it. The initial connection will be over plain text, and Authelia will try to upgrade it with the +LDAP server. LDAPS URL's are slightly more secure. + +### Minimum TLS Version + +The key `minimum_tls_version` controls the minimum TLS version Authelia will use when opening LDAP connections. +The possible values are `TLS1.3`, `TLS1.2`, `TLS1.1`, `TLS1.0`. Anything other than `TLS1.3` or `TLS1.2` +are very old and deprecated. You should avoid using these and upgrade your LDAP solution instead of decreasing +this value. + ## Implementation There are currently two implementations, `custom` and `activedirectory`. The `activedirectory` implementation diff --git a/internal/authentication/const.go b/internal/authentication/const.go index 0be0af1a..c8cb3738 100644 --- a/internal/authentication/const.go +++ b/internal/authentication/const.go @@ -60,3 +60,7 @@ const sha512 = "sha512" const testPassword = "my;secure*password" const fileAuthenticationMode = 0600 + +// OWASP recommends to escape some special characters. +// https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/LDAP_Injection_Prevention_Cheat_Sheet.md +const specialLDAPRunes = ",#+<>;\"=" diff --git a/internal/authentication/file_user_provider.go b/internal/authentication/file_user_provider.go index dc6f46b4..130a534c 100644 --- a/internal/authentication/file_user_provider.go +++ b/internal/authentication/file_user_provider.go @@ -8,12 +8,10 @@ import ( "sync" "github.com/asaskevich/govalidator" - "github.com/simia-tech/crypt" "gopkg.in/yaml.v2" "github.com/authelia/authelia/internal/configuration/schema" "github.com/authelia/authelia/internal/logging" - "github.com/authelia/authelia/internal/utils" ) // FileUserProvider is a provider reading details from a file. @@ -21,9 +19,6 @@ type FileUserProvider struct { configuration *schema.FileAuthenticationBackendConfiguration database *DatabaseModel lock *sync.Mutex - - // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. - fakeHash string } // UserDetailsModel is the model of user details in the file database. @@ -62,24 +57,10 @@ func NewFileUserProvider(configuration *schema.FileAuthenticationBackendConfigur panic(err) } - var cryptAlgo CryptAlgo = HashingAlgorithmArgon2id - // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. - // This generates a hash that should be usable to do a fake CheckUserPassword - if configuration.Password.Algorithm == sha512 { - cryptAlgo = HashingAlgorithmSHA512 - } - - settings := getCryptSettings(utils.RandomString(configuration.Password.SaltLength, HashingPossibleSaltCharacters), - cryptAlgo, configuration.Password.Iterations, configuration.Password.Memory*1024, configuration.Password.Parallelism, - configuration.Password.KeyLength) - data := crypt.Base64Encoding.EncodeToString([]byte(utils.RandomString(configuration.Password.KeyLength, HashingPossibleSaltCharacters))) - fakeHash := fmt.Sprintf("%s$%s", settings, data) - return &FileUserProvider{ configuration: configuration, database: database, lock: &sync.Mutex{}, - fakeHash: fakeHash, } } @@ -174,9 +155,6 @@ func (p *FileUserProvider) CheckUserPassword(username string, password string) ( return ok, nil } - // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. - _, _ = CheckPassword(password, p.fakeHash) - return false, ErrUserNotFound } diff --git a/internal/authentication/ldap_connection_factory.go b/internal/authentication/ldap_connection_factory.go index 7d635b4b..168dba46 100644 --- a/internal/authentication/ldap_connection_factory.go +++ b/internal/authentication/ldap_connection_factory.go @@ -15,6 +15,7 @@ type LDAPConnection interface { Search(searchRequest *ldap.SearchRequest) (*ldap.SearchResult, error) Modify(modifyRequest *ldap.ModifyRequest) error + StartTLS(config *tls.Config) error } // LDAPConnectionImpl the production implementation of an ldap connection. @@ -47,6 +48,11 @@ func (lc *LDAPConnectionImpl) Modify(modifyRequest *ldap.ModifyRequest) error { return lc.conn.Modify(modifyRequest) } +// StartTLS requests the LDAP server upgrades to TLS encryption. +func (lc *LDAPConnectionImpl) StartTLS(config *tls.Config) error { + return lc.conn.StartTLS(config) +} + // ********************* FACTORY ***********************. // LDAPConnectionFactory an interface of factory of ldap connections. diff --git a/internal/authentication/ldap_connection_factory_mock.go b/internal/authentication/ldap_connection_factory_mock.go index 5e48daa6..7e662c8e 100644 --- a/internal/authentication/ldap_connection_factory_mock.go +++ b/internal/authentication/ldap_connection_factory_mock.go @@ -1,15 +1,12 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: internal/authentication/ldap_connection_factory.go - -// Package authentication is a generated GoMock package. +// Source: ldap_connection_factory.go package authentication import ( tls "crypto/tls" - reflect "reflect" - - ldap_v3 "github.com/go-ldap/ldap/v3" + ldap "github.com/go-ldap/ldap/v3" gomock "github.com/golang/mock/gomock" + reflect "reflect" ) // MockLDAPConnection is a mock of LDAPConnection interface @@ -62,10 +59,10 @@ func (mr *MockLDAPConnectionMockRecorder) Close() *gomock.Call { } // Search mocks base method -func (m *MockLDAPConnection) Search(searchRequest *ldap_v3.SearchRequest) (*ldap_v3.SearchResult, error) { +func (m *MockLDAPConnection) Search(searchRequest *ldap.SearchRequest) (*ldap.SearchResult, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Search", searchRequest) - ret0, _ := ret[0].(*ldap_v3.SearchResult) + ret0, _ := ret[0].(*ldap.SearchResult) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -77,7 +74,7 @@ func (mr *MockLDAPConnectionMockRecorder) Search(searchRequest interface{}) *gom } // Modify mocks base method -func (m *MockLDAPConnection) Modify(modifyRequest *ldap_v3.ModifyRequest) error { +func (m *MockLDAPConnection) Modify(modifyRequest *ldap.ModifyRequest) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Modify", modifyRequest) ret0, _ := ret[0].(error) @@ -90,6 +87,20 @@ func (mr *MockLDAPConnectionMockRecorder) Modify(modifyRequest interface{}) *gom return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Modify", reflect.TypeOf((*MockLDAPConnection)(nil).Modify), modifyRequest) } +// StartTLS mocks base method +func (m *MockLDAPConnection) StartTLS(config *tls.Config) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "StartTLS", config) + ret0, _ := ret[0].(error) + return ret0 +} + +// StartTLS indicates an expected call of StartTLS +func (mr *MockLDAPConnectionMockRecorder) StartTLS(config interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StartTLS", reflect.TypeOf((*MockLDAPConnection)(nil).StartTLS), config) +} + // MockLDAPConnectionFactory is a mock of LDAPConnectionFactory interface type MockLDAPConnectionFactory struct { ctrl *gomock.Controller diff --git a/internal/authentication/ldap_user_provider.go b/internal/authentication/ldap_user_provider.go index 190ed497..eb6a7609 100644 --- a/internal/authentication/ldap_user_provider.go +++ b/internal/authentication/ldap_user_provider.go @@ -11,47 +11,76 @@ import ( "github.com/authelia/authelia/internal/configuration/schema" "github.com/authelia/authelia/internal/logging" + "github.com/authelia/authelia/internal/utils" ) // LDAPUserProvider is a provider using a LDAP or AD as a user database. type LDAPUserProvider struct { configuration schema.LDAPAuthenticationBackendConfiguration + tlsConfig *tls.Config connectionFactory LDAPConnectionFactory } // NewLDAPUserProvider creates a new instance of LDAPUserProvider. func NewLDAPUserProvider(configuration schema.LDAPAuthenticationBackendConfiguration) *LDAPUserProvider { + minimumTLSVersion, _ := utils.TLSStringToTLSConfigVersion(configuration.MinimumTLSVersion) + + // TODO: RELEASE-4.27.0 Deprecated Completely in this release. + logger := logging.Logger() + + if strings.Contains(configuration.UsersFilter, "{0}") { + logger.Warnf("DEPRECATION NOTICE: LDAP Users Filter will no longer support replacing `{0}` in 4.27.0. Please use `{input}` instead.") + + configuration.UsersFilter = strings.ReplaceAll(configuration.UsersFilter, "{0}", "{input}") + } + + if strings.Contains(configuration.GroupsFilter, "{0}") { + logger.Warnf("DEPRECATION NOTICE: LDAP Groups Filter will no longer support replacing `{0}` in 4.27.0. Please use `{input}` instead.") + + configuration.GroupsFilter = strings.ReplaceAll(configuration.GroupsFilter, "{0}", "{input}") + } + + if strings.Contains(configuration.GroupsFilter, "{1}") { + logger.Warnf("DEPRECATION NOTICE: LDAP Groups Filter will no longer support replacing `{1}` in 4.27.0. Please use `{username}` instead.") + + configuration.GroupsFilter = strings.ReplaceAll(configuration.GroupsFilter, "{1}", "{username}") + } + // TODO: RELEASE-4.27.0 Deprecated Completely in this release. + + configuration.UsersFilter = strings.ReplaceAll(configuration.UsersFilter, "{username_attribute}", configuration.UsernameAttribute) + configuration.UsersFilter = strings.ReplaceAll(configuration.UsersFilter, "{mail_attribute}", configuration.MailAttribute) + configuration.UsersFilter = strings.ReplaceAll(configuration.UsersFilter, "{display_name_attribute}", configuration.DisplayNameAttribute) + return &LDAPUserProvider{ - configuration: configuration, + configuration: configuration, + tlsConfig: &tls.Config{InsecureSkipVerify: configuration.SkipVerify, MinVersion: minimumTLSVersion}, //nolint:gosec // Disabling InsecureSkipVerify is an informed choice by users. + connectionFactory: NewLDAPConnectionFactoryImpl(), } } // NewLDAPUserProviderWithFactory creates a new instance of LDAPUserProvider with existing factory. -func NewLDAPUserProviderWithFactory(configuration schema.LDAPAuthenticationBackendConfiguration, - connectionFactory LDAPConnectionFactory) *LDAPUserProvider { - return &LDAPUserProvider{ - configuration: configuration, - connectionFactory: connectionFactory, - } +func NewLDAPUserProviderWithFactory(configuration schema.LDAPAuthenticationBackendConfiguration, connectionFactory LDAPConnectionFactory) *LDAPUserProvider { + provider := NewLDAPUserProvider(configuration) + provider.connectionFactory = connectionFactory + + return provider } func (p *LDAPUserProvider) connect(userDN string, password string) (LDAPConnection, error) { var newConnection LDAPConnection - url, err := url.Parse(p.configuration.URL) + ldapURL, err := url.Parse(p.configuration.URL) if err != nil { - return nil, fmt.Errorf("Unable to parse URL to LDAP: %s", url) + return nil, fmt.Errorf("Unable to parse URL to LDAP: %s", ldapURL) } - if url.Scheme == "ldaps" { + if ldapURL.Scheme == "ldaps" { logging.Logger().Trace("LDAP client starts a TLS session") - conn, err := p.connectionFactory.DialTLS("tcp", url.Host, &tls.Config{ - InsecureSkipVerify: p.configuration.SkipVerify, //nolint:gosec // This is a configurable option, is desirable in some situations and is off by default. - }) + conn, err := p.connectionFactory.DialTLS("tcp", ldapURL.Host, p.tlsConfig) if err != nil { return nil, err } @@ -59,13 +88,19 @@ func (p *LDAPUserProvider) connect(userDN string, password string) (LDAPConnecti newConnection = conn } else { logging.Logger().Trace("LDAP client starts a session over raw TCP") - conn, err := p.connectionFactory.Dial("tcp", url.Host) + conn, err := p.connectionFactory.Dial("tcp", ldapURL.Host) if err != nil { return nil, err } newConnection = conn } + if p.configuration.StartTLS { + if err := newConnection.StartTLS(p.tlsConfig); err != nil { + return nil, err + } + } + if err := newConnection.Bind(userDN, password); err != nil { return nil, err } @@ -95,10 +130,6 @@ func (p *LDAPUserProvider) CheckUserPassword(inputUsername string, password stri return true, nil } -// OWASP recommends to escape some special characters. -// https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/LDAP_Injection_Prevention_Cheat_Sheet.md -const specialLDAPRunes = ",#+<>;\"=" - func (p *LDAPUserProvider) ldapEscape(inputUsername string) string { inputUsername = ldap.EscapeFilter(inputUsername) for _, c := range specialLDAPRunes { @@ -118,18 +149,9 @@ type ldapUserProfile struct { func (p *LDAPUserProvider) resolveUsersFilter(userFilter string, inputUsername string) string { inputUsername = p.ldapEscape(inputUsername) - // We temporarily keep placeholder {0} for backward compatibility. - userFilter = strings.ReplaceAll(userFilter, "{0}", inputUsername) - - // The {username} placeholder is equivalent to {0}, it's the new way, a named placeholder. + // The {input} placeholder is replaced by the users username input. userFilter = strings.ReplaceAll(userFilter, "{input}", inputUsername) - // {username_attribute} and {mail_attribute} are replaced by the content of the attribute defined - // in configuration. - userFilter = strings.ReplaceAll(userFilter, "{username_attribute}", p.configuration.UsernameAttribute) - userFilter = strings.ReplaceAll(userFilter, "{mail_attribute}", p.configuration.MailAttribute) - userFilter = strings.ReplaceAll(userFilter, "{display_name_attribute}", p.configuration.DisplayNameAttribute) - return userFilter } @@ -199,13 +221,10 @@ func (p *LDAPUserProvider) getUserProfile(conn LDAPConnection, inputUsername str func (p *LDAPUserProvider) resolveGroupsFilter(inputUsername string, profile *ldapUserProfile) (string, error) { //nolint:unparam inputUsername = p.ldapEscape(inputUsername) - // We temporarily keep placeholder {0} for backward compatibility. - groupFilter := strings.ReplaceAll(p.configuration.GroupsFilter, "{0}", inputUsername) - groupFilter = strings.ReplaceAll(groupFilter, "{input}", inputUsername) + // The {input} placeholder is replaced by the users username input. + groupFilter := strings.ReplaceAll(p.configuration.GroupsFilter, "{input}", inputUsername) if profile != nil { - // We temporarily keep placeholder {1} for backward compatibility. - groupFilter = strings.ReplaceAll(groupFilter, "{1}", ldap.EscapeFilter(profile.Username)) groupFilter = strings.ReplaceAll(groupFilter, "{username}", ldap.EscapeFilter(profile.Username)) groupFilter = strings.ReplaceAll(groupFilter, "{dn}", ldap.EscapeFilter(profile.DN)) } diff --git a/internal/authentication/ldap_user_provider_test.go b/internal/authentication/ldap_user_provider_test.go index d831c11a..b4c3ab6e 100644 --- a/internal/authentication/ldap_user_provider_test.go +++ b/internal/authentication/ldap_user_provider_test.go @@ -1,6 +1,7 @@ package authentication import ( + "errors" "testing" "github.com/go-ldap/ldap/v3" @@ -151,7 +152,9 @@ func TestShouldEscapeUserInput(t *testing.T) { Search(NewSearchRequestMatcher("(|(uid=john\\=abc)(mail=john\\=abc))")). Return(&ldap.SearchResult{}, nil) - ldapClient.getUserProfile(mockConn, "john=abc") //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. + _, err := ldapClient.getUserProfile(mockConn, "john=abc") + require.Error(t, err) + assert.EqualError(t, err, "user not found") } func TestShouldCombineUsernameFilterAndUsersFilter(t *testing.T) { @@ -177,7 +180,9 @@ func TestShouldCombineUsernameFilterAndUsersFilter(t *testing.T) { Search(NewSearchRequestMatcher("(&(uid=john)(&(objectCategory=person)(objectClass=user)))")). Return(&ldap.SearchResult{}, nil) - ldapClient.getUserProfile(mockConn, "john") //nolint:errcheck // TODO: Legacy code, consider refactoring time permitting. + _, err := ldapClient.getUserProfile(mockConn, "john") + require.Error(t, err) + assert.EqualError(t, err, "user not found") } func createSearchResultWithAttributes(attributes ...*ldap.EntryAttribute) *ldap.SearchResult { @@ -386,3 +391,181 @@ func TestShouldReturnUsernameFromLDAP(t *testing.T) { assert.Equal(t, details.DisplayName, "John Doe") assert.Equal(t, details.Username, "John") } + +func TestShouldCallStartTLSWhenEnabled(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockFactory := NewMockLDAPConnectionFactory(ctrl) + mockConn := NewMockLDAPConnection(ctrl) + + ldapClient := NewLDAPUserProviderWithFactory(schema.LDAPAuthenticationBackendConfiguration{ + URL: "ldap://127.0.0.1:389", + User: "cn=admin,dc=example,dc=com", + Password: "password", + UsernameAttribute: "uid", + MailAttribute: "mail", + DisplayNameAttribute: "displayname", + UsersFilter: "uid={input}", + AdditionalUsersDN: "ou=users", + BaseDN: "dc=example,dc=com", + StartTLS: true, + }, mockFactory) + + mockFactory.EXPECT(). + Dial(gomock.Eq("tcp"), gomock.Eq("127.0.0.1:389")). + Return(mockConn, nil) + + mockConn.EXPECT(). + Bind(gomock.Eq("cn=admin,dc=example,dc=com"), gomock.Eq("password")). + Return(nil) + + mockConn.EXPECT(). + StartTLS(ldapClient.tlsConfig) + + mockConn.EXPECT(). + Close() + + searchGroups := mockConn.EXPECT(). + Search(gomock.Any()). + Return(createSearchResultWithAttributes(), nil) + searchProfile := mockConn.EXPECT(). + Search(gomock.Any()). + Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: "uid=test,dc=example,dc=com", + Attributes: []*ldap.EntryAttribute{ + { + Name: "displayname", + Values: []string{"John Doe"}, + }, + { + Name: "mail", + Values: []string{"test@example.com"}, + }, + { + Name: "uid", + Values: []string{"john"}, + }, + }, + }, + }, + }, nil) + + gomock.InOrder(searchProfile, searchGroups) + + details, err := ldapClient.GetDetails("john") + require.NoError(t, err) + + assert.ElementsMatch(t, details.Groups, []string{}) + assert.ElementsMatch(t, details.Emails, []string{"test@example.com"}) + assert.Equal(t, details.DisplayName, "John Doe") + assert.Equal(t, details.Username, "john") +} + +func TestShouldCallStartTLSWithInsecureSkipVerifyWhenSkipVerifyTrue(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockFactory := NewMockLDAPConnectionFactory(ctrl) + mockConn := NewMockLDAPConnection(ctrl) + + ldapClient := NewLDAPUserProviderWithFactory(schema.LDAPAuthenticationBackendConfiguration{ + URL: "ldap://127.0.0.1:389", + User: "cn=admin,dc=example,dc=com", + Password: "password", + UsernameAttribute: "uid", + MailAttribute: "mail", + DisplayNameAttribute: "displayname", + UsersFilter: "uid={input}", + AdditionalUsersDN: "ou=users", + BaseDN: "dc=example,dc=com", + StartTLS: true, + SkipVerify: true, + }, mockFactory) + + mockFactory.EXPECT(). + Dial(gomock.Eq("tcp"), gomock.Eq("127.0.0.1:389")). + Return(mockConn, nil) + + mockConn.EXPECT(). + Bind(gomock.Eq("cn=admin,dc=example,dc=com"), gomock.Eq("password")). + Return(nil) + + mockConn.EXPECT(). + StartTLS(ldapClient.tlsConfig) + + mockConn.EXPECT(). + Close() + + searchGroups := mockConn.EXPECT(). + Search(gomock.Any()). + Return(createSearchResultWithAttributes(), nil) + searchProfile := mockConn.EXPECT(). + Search(gomock.Any()). + Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: "uid=test,dc=example,dc=com", + Attributes: []*ldap.EntryAttribute{ + { + Name: "displayname", + Values: []string{"John Doe"}, + }, + { + Name: "mail", + Values: []string{"test@example.com"}, + }, + { + Name: "uid", + Values: []string{"john"}, + }, + }, + }, + }, + }, nil) + + gomock.InOrder(searchProfile, searchGroups) + + details, err := ldapClient.GetDetails("john") + require.NoError(t, err) + + assert.ElementsMatch(t, details.Groups, []string{}) + assert.ElementsMatch(t, details.Emails, []string{"test@example.com"}) + assert.Equal(t, details.DisplayName, "John Doe") + assert.Equal(t, details.Username, "john") +} + +func TestShouldReturnLDAPSAlreadySecuredWhenStartTLSAttempted(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockFactory := NewMockLDAPConnectionFactory(ctrl) + mockConn := NewMockLDAPConnection(ctrl) + + ldapClient := NewLDAPUserProviderWithFactory(schema.LDAPAuthenticationBackendConfiguration{ + URL: "ldaps://127.0.0.1:389", + User: "cn=admin,dc=example,dc=com", + Password: "password", + UsernameAttribute: "uid", + MailAttribute: "mail", + DisplayNameAttribute: "displayname", + UsersFilter: "uid={input}", + AdditionalUsersDN: "ou=users", + BaseDN: "dc=example,dc=com", + StartTLS: true, + SkipVerify: true, + }, mockFactory) + + mockFactory.EXPECT(). + DialTLS(gomock.Eq("tcp"), gomock.Eq("127.0.0.1:389"), gomock.Any()). + Return(mockConn, nil) + + mockConn.EXPECT(). + StartTLS(ldapClient.tlsConfig). + Return(errors.New("LDAP Result Code 200 \"Network Error\": ldap: already encrypted")) + + _, err := ldapClient.GetDetails("john") + assert.EqualError(t, err, "LDAP Result Code 200 \"Network Error\": ldap: already encrypted") +} diff --git a/internal/configuration/schema/authentication.go b/internal/configuration/schema/authentication.go index 3b836ba0..ee1c1d62 100644 --- a/internal/configuration/schema/authentication.go +++ b/internal/configuration/schema/authentication.go @@ -5,6 +5,8 @@ type LDAPAuthenticationBackendConfiguration struct { Implementation string `mapstructure:"implementation"` URL string `mapstructure:"url"` SkipVerify bool `mapstructure:"skip_verify"` + StartTLS bool `mapstructure:"start_tls"` + MinimumTLSVersion string `mapstructure:"minimum_tls_version"` BaseDN string `mapstructure:"base_dn"` AdditionalUsersDN string `mapstructure:"additional_users_dn"` UsersFilter string `mapstructure:"users_filter"` @@ -76,6 +78,7 @@ var DefaultLDAPAuthenticationBackendConfiguration = LDAPAuthenticationBackendCon MailAttribute: "mail", DisplayNameAttribute: "displayname", GroupNameAttribute: "cn", + MinimumTLSVersion: "TLS1.2", } // DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration represents the default LDAP config for the MSAD Implementation. diff --git a/internal/configuration/validator/authentication.go b/internal/configuration/validator/authentication.go index abba521a..98f5f209 100644 --- a/internal/configuration/validator/authentication.go +++ b/internal/configuration/validator/authentication.go @@ -104,6 +104,12 @@ func validateLdapAuthenticationBackend(configuration *schema.LDAPAuthenticationB configuration.Implementation = schema.DefaultLDAPAuthenticationBackendConfiguration.Implementation } + if configuration.MinimumTLSVersion == "" { + configuration.MinimumTLSVersion = schema.DefaultLDAPAuthenticationBackendConfiguration.MinimumTLSVersion + } else if _, err := utils.TLSStringToTLSConfigVersion(configuration.MinimumTLSVersion); err != nil { + validator.Push(fmt.Errorf("error occurred validating the LDAP minimum_tls_version key with value %s: %v", configuration.MinimumTLSVersion, err)) + } + switch configuration.Implementation { case schema.LDAPImplementationCustom: setDefaultImplementationCustomLdapAuthenticationBackend(configuration) diff --git a/internal/configuration/validator/authentication_test.go b/internal/configuration/validator/authentication_test.go index 9f85955a..fcb9aad6 100644 --- a/internal/configuration/validator/authentication_test.go +++ b/internal/configuration/validator/authentication_test.go @@ -312,6 +312,95 @@ func (suite *LdapAuthenticationBackendSuite) TestShouldAdaptLDAPURL() { assert.Equal(suite.T(), "ldaps://127.0.0.1:636", validateLdapURL("ldaps://127.0.0.1", suite.validator)) } +func (suite *LdapAuthenticationBackendSuite) TestShouldDefaultTLS12() { + ValidateAuthenticationBackend(&suite.configuration, suite.validator) + assert.Len(suite.T(), suite.validator.Errors(), 0) + assert.Equal(suite.T(), schema.DefaultLDAPAuthenticationBackendConfiguration.MinimumTLSVersion, suite.configuration.Ldap.MinimumTLSVersion) +} + +func (suite *LdapAuthenticationBackendSuite) TestShouldNotAllowInvalidTLSValue() { + suite.configuration.Ldap.MinimumTLSVersion = "SSL2.0" + ValidateAuthenticationBackend(&suite.configuration, suite.validator) + require.Len(suite.T(), suite.validator.Errors(), 1) + assert.EqualError(suite.T(), suite.validator.Errors()[0], "error occurred validating the LDAP minimum_tls_version key with value SSL2.0: supplied TLS version isn't supported") +} + func TestLdapAuthenticationBackend(t *testing.T) { suite.Run(t, new(LdapAuthenticationBackendSuite)) } + +type ActiveDirectoryAuthenticationBackendSuite struct { + suite.Suite + configuration schema.AuthenticationBackendConfiguration + validator *schema.StructValidator +} + +func (suite *ActiveDirectoryAuthenticationBackendSuite) SetupTest() { + suite.validator = schema.NewStructValidator() + suite.configuration = schema.AuthenticationBackendConfiguration{} + suite.configuration.Ldap = &schema.LDAPAuthenticationBackendConfiguration{} + suite.configuration.Ldap.Implementation = schema.LDAPImplementationActiveDirectory + suite.configuration.Ldap.URL = "ldap://ldap" + suite.configuration.Ldap.User = "user" + suite.configuration.Ldap.Password = "password" + suite.configuration.Ldap.BaseDN = "base_dn" +} + +func (suite *ActiveDirectoryAuthenticationBackendSuite) TestShouldSetActiveDirectoryDefaults() { + ValidateAuthenticationBackend(&suite.configuration, suite.validator) + + assert.Len(suite.T(), suite.validator.Errors(), 0) + + assert.Equal(suite.T(), + suite.configuration.Ldap.UsersFilter, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.UsersFilter) + assert.Equal(suite.T(), + suite.configuration.Ldap.UsernameAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.UsernameAttribute) + assert.Equal(suite.T(), + suite.configuration.Ldap.DisplayNameAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.DisplayNameAttribute) + assert.Equal(suite.T(), + suite.configuration.Ldap.MailAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.MailAttribute) + assert.Equal(suite.T(), + suite.configuration.Ldap.GroupsFilter, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.GroupsFilter) + assert.Equal(suite.T(), + suite.configuration.Ldap.GroupNameAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.GroupNameAttribute) +} + +func (suite *ActiveDirectoryAuthenticationBackendSuite) TestShouldOnlySetDefaultsIfNotManuallyConfigured() { + suite.configuration.Ldap.UsersFilter = "(&({username_attribute}={input})(objectCategory=person)(objectClass=user)(!userAccountControl:1.2.840.113556.1.4.803:=2))" + suite.configuration.Ldap.UsernameAttribute = "cn" + suite.configuration.Ldap.MailAttribute = "userPrincipalName" + suite.configuration.Ldap.DisplayNameAttribute = "name" + suite.configuration.Ldap.GroupsFilter = "(&(member={dn})(objectClass=group)(objectCategory=group))" + suite.configuration.Ldap.GroupNameAttribute = "distinguishedName" + + ValidateAuthenticationBackend(&suite.configuration, suite.validator) + + assert.NotEqual(suite.T(), + suite.configuration.Ldap.UsersFilter, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.UsersFilter) + assert.NotEqual(suite.T(), + suite.configuration.Ldap.UsernameAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.UsernameAttribute) + assert.NotEqual(suite.T(), + suite.configuration.Ldap.DisplayNameAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.DisplayNameAttribute) + assert.NotEqual(suite.T(), + suite.configuration.Ldap.MailAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.MailAttribute) + assert.NotEqual(suite.T(), + suite.configuration.Ldap.GroupsFilter, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.GroupsFilter) + assert.NotEqual(suite.T(), + suite.configuration.Ldap.GroupNameAttribute, + schema.DefaultLDAPAuthenticationBackendImplementationActiveDirectoryConfiguration.GroupNameAttribute) +} + +func TestActiveDirectoryAuthenticationBackend(t *testing.T) { + suite.Run(t, new(ActiveDirectoryAuthenticationBackendSuite)) +} diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 45a67074..5e7ee5d6 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -94,6 +94,8 @@ var validKeys = []string{ "authentication_backend.ldap.implementation", "authentication_backend.ldap.url", "authentication_backend.ldap.skip_verify", + "authentication_backend.ldap.start_tls", + "authentication_backend.ldap.minimum_tls_version", "authentication_backend.ldap.base_dn", "authentication_backend.ldap.username_attribute", "authentication_backend.ldap.additional_users_dn", diff --git a/internal/suites/ActiveDirectory/configuration.yml b/internal/suites/ActiveDirectory/configuration.yml index 363a2b37..fc84792c 100644 --- a/internal/suites/ActiveDirectory/configuration.yml +++ b/internal/suites/ActiveDirectory/configuration.yml @@ -15,8 +15,9 @@ jwt_secret: very_important_secret authentication_backend: ldap: implementation: activedirectory - url: ldaps://sambaldap + url: ldap://sambaldap skip_verify: true + start_tls: true base_dn: DC=example,DC=com username_attribute: sAMAccountName additional_users_dn: OU=Users diff --git a/internal/utils/const.go b/internal/utils/const.go index ba9a056f..fa10d368 100644 --- a/internal/utils/const.go +++ b/internal/utils/const.go @@ -32,3 +32,18 @@ const testStringInput = "abcdefghijkl" // AlphaNumericCharacters are literally just valid alphanumeric chars. var AlphaNumericCharacters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") + +// ErrTLSVersionNotSupported returned when an unknown TLS version supplied. +var ErrTLSVersionNotSupported = errors.New("supplied TLS version isn't supported") + +// TLS13 is the textual representation of TLS 1.3. +const TLS13 = "1.3" + +// TLS12 is the textual representation of TLS 1.2. +const TLS12 = "1.2" + +// TLS11 is the textual representation of TLS 1.1. +const TLS11 = "1.1" + +// TLS10 is the textual representation of TLS 1.0. +const TLS10 = "1.0" diff --git a/internal/utils/strings.go b/internal/utils/strings.go index 9b8789e4..2de98274 100644 --- a/internal/utils/strings.go +++ b/internal/utils/strings.go @@ -1,7 +1,9 @@ package utils import ( + "crypto/tls" "math/rand" + "strings" "time" "unicode" ) @@ -91,3 +93,19 @@ func RandomString(n int, characters []rune) (randomString string) { return string(b) } + +// TLSStringToTLSConfigVersion returns a go crypto/tls version for a tls.Config based on string input. +func TLSStringToTLSConfigVersion(input string) (version uint16, err error) { + switch strings.ToUpper(input) { + case "TLS1.3", TLS13: + return tls.VersionTLS13, nil + case "TLS1.2", TLS12: + return tls.VersionTLS12, nil + case "TLS1.1", TLS11: + return tls.VersionTLS11, nil + case "TLS1.0", TLS10: + return tls.VersionTLS10, nil + } + + return 0, ErrTLSVersionNotSupported +} diff --git a/internal/utils/strings_test.go b/internal/utils/strings_test.go index fcfc240a..22b44032 100644 --- a/internal/utils/strings_test.go +++ b/internal/utils/strings_test.go @@ -1,6 +1,7 @@ package utils import ( + "crypto/tls" "testing" "github.com/stretchr/testify/assert" @@ -68,3 +69,54 @@ func TestShouldNotFindSliceDifferences(t *testing.T) { diff := IsStringSlicesDifferent(a, b) assert.False(t, diff) } + +func TestShouldReturnCorrectTLSVersions(t *testing.T) { + tls13 := uint16(tls.VersionTLS13) + tls12 := uint16(tls.VersionTLS12) + tls11 := uint16(tls.VersionTLS11) + tls10 := uint16(tls.VersionTLS10) + + version, err := TLSStringToTLSConfigVersion(TLS13) + assert.Equal(t, tls13, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion("TLS" + TLS13) + assert.Equal(t, tls13, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion(TLS12) + assert.Equal(t, tls12, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion("TLS" + TLS12) + assert.Equal(t, tls12, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion(TLS11) + assert.Equal(t, tls11, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion("TLS" + TLS11) + assert.Equal(t, tls11, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion(TLS10) + assert.Equal(t, tls10, version) + assert.NoError(t, err) + + version, err = TLSStringToTLSConfigVersion("TLS" + TLS10) + assert.Equal(t, tls10, version) + assert.NoError(t, err) +} + +func TestShouldReturnZeroAndErrorOnInvalidTLSVersions(t *testing.T) { + version, err := TLSStringToTLSConfigVersion("TLS1.4") + assert.Error(t, err) + assert.Equal(t, uint16(0), version) + assert.EqualError(t, err, "supplied TLS version isn't supported") + + version, err = TLSStringToTLSConfigVersion("SSL3.0") + assert.Error(t, err) + assert.Equal(t, uint16(0), version) + assert.EqualError(t, err, "supplied TLS version isn't supported") +}