Skip to content

Allow OAuth to auto link LDAP users via email address

Niko requested to merge (removed):search_ldap_by_email into master

What does this MR do?

This merge requests changes that after a succesful oauth login a search for a user by their email-address will be performed against the mail of the user.

Lookups for the mail-address have previously only been made against the uid which only works if this is the e-mail-address as well. This MR takes the email-address attribute into consideration.

It implements a fix suggest by @juwo here.

Background

In an instance SAML is used for authentication. The idP returns a persistent ID after successful auth which is not in the format of an e-mail-address, nor ID, nor DN. LDAP is used in addition to sync user name and e-mail as well as blocking users. LDAP is not available for logging in.

Behaviour without this MR

User logs in through SAML. No user attributes are received through LDAP. An error is displayed in production.log.

User is able to change name and e-mail in his profile.

Excerpt from production.log

See the LDAP search error below.

Started POST "/users/auth/saml" for 10.10.10.202 at 2020-06-03 13:44:23 +0200
Processing by Gitlab::RequestForgeryProtection::Controller#index as HTML
Parameters: {"authenticity_token"=>"[FILTERED]"}
Completed 200 OK in 0ms (ActiveRecord: 0.0ms | Elasticsearch: 0.0ms | Allocations: 162)
Started POST "/users/auth/saml/callback" for 10.10.10.202 at 2020-06-03 13:44:24 +0200
Processing by OmniauthCallbacksController#saml as HTML
Parameters: {"SAMLResponse"=>"PHN...2U+"}
LDAP search error: Invalid DN Syntax
Redirected to https://gitlab.domain.tld/
Completed 302 Found in 237ms (ActiveRecord: 54.0ms | Elasticsearch: 0.0ms | Allocations: 92967)

Behaviour with this MR

Users log in through SAML and afterwards user attributes are received through LDAP. Users are not able to change their name or e-mail-address. The logs contain no error.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

The test only affects backend components without altering any APIs. Testing with browsers can be skipped with this.

I tested the change on the affected instance and it worked as desired. I'm not confident enough in Ruby to provide tests for this myself.

Configuration

To make testing easier these are settings used in the affected instance.

Configuration used in /etc/gitlab/gitlab.rb

Relevant settings from /etc/gitlab/gitlab.rb.

gitlab_rails['ldap_servers'] = {
'main' => {
	...
	'attributes' => {
		'username' => ['userPrincipalName'],
		'email' => ['email'],
		'name' => 'cn',
		'first_name' => 'givenName',
		'last_name' => 'sn'
		},
	}
}
gitlab_rails['sync_profile_attributes'] = ['name', 'email']
gitlab_rails['prevent_ldap_sign_in'] = true

gitlab_rails['omniauth_allow_single_sign_on'] = ['saml']
gitlab_rails['omniauth_block_auto_created_users'] = false
gitlab_rails['omniauth_auto_link_saml_user'] = true
gitlab_rails['omniauth_auto_link_ldap_user'] = true

gitlab_rails['omniauth_providers'] = [
	{
		name: 'saml',
		args: {
			...
			name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'
		},
		label: 'SAML'
	}
]

Security

This touches retrieval of data from LDAP after the authentication. The way I can think this can be abused is if an user is able to change their e-mail-address in idP and then impersonate another user in LDAP through this.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Michael Kozono

Merge request reports