From 42bc6caee038d0abcb8636182c2c0eac70dae8e8 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Sun, 17 Sep 2017 18:07:29 -0700 Subject: [PATCH] Trim extraneous spaces from DNs --- lib/gitlab/ldap/auth_hash.rb | 4 + lib/gitlab/ldap/person.rb | 41 +++++++- spec/lib/gitlab/ldap/auth_hash_spec.rb | 16 ++- spec/lib/gitlab/ldap/person_spec.rb | 130 +++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb index 4fbc5fa5262..3123da17fd9 100644 --- a/lib/gitlab/ldap/auth_hash.rb +++ b/lib/gitlab/ldap/auth_hash.rb @@ -3,6 +3,10 @@ module Gitlab module LDAP class AuthHash < Gitlab::OAuth::AuthHash + def uid + Gitlab::LDAP::Person.normalize_dn(super) + end + private def get_info(key) diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 9a6f7827b16..4299d35fabc 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -36,6 +36,12 @@ module Gitlab ] end + def self.normalize_dn(dn) + dn.split(/([,+=])/).map do |part| + normalize_dn_part(part) + end.join('') + end + def initialize(entry, provider) Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" } @entry = entry @@ -58,10 +64,43 @@ module Gitlab attribute_value(:email) end - delegate :dn, to: :entry + def dn + self.class.normalize_dn(entry.dn) + end private + def self.normalize_dn_part(part) + cleaned = part.strip + + if cleaned.ends_with?('\\') + # If it ends with an escape character that is not followed by a + # character to be escaped, then this part may be malformed. But let's + # not worry too much about it, and just return it unmodified. + # + # Why? Because the reason we clean DNs is to make our simplistic + # string comparisons work better, even though there are all kinds of + # ways that equivalent DNs can vary as strings. If we run into a + # strange DN, we should just try to work with it. + # + # See https://www.ldap.com/ldap-dns-and-rdns for more. + return part unless part.ends_with?(' ') + + # Ends with an escaped space (which is valid). + cleaned = cleaned + ' ' + end + + # Get rid of blanks. This can happen if a split character is followed by + # whitespace and then another split character. + # + # E.g. this DN: 'uid=john+telephoneNumber= +1 555-555-5555' + # + # Should be returned as: 'uid=john+telephoneNumber=+1 555-555-5555' + cleaned = '' if cleaned.blank? + + cleaned + end + def entry @entry end diff --git a/spec/lib/gitlab/ldap/auth_hash_spec.rb b/spec/lib/gitlab/ldap/auth_hash_spec.rb index 8370adf9211..a4bd40705df 100644 --- a/spec/lib/gitlab/ldap/auth_hash_spec.rb +++ b/spec/lib/gitlab/ldap/auth_hash_spec.rb @@ -4,7 +4,7 @@ describe Gitlab::LDAP::AuthHash do let(:auth_hash) do described_class.new( OmniAuth::AuthHash.new( - uid: '123456', + uid: given_uid, provider: 'ldapmain', info: info, extra: { @@ -32,6 +32,8 @@ describe Gitlab::LDAP::AuthHash do end context "without overridden attributes" do + let(:given_uid) { 'uid=John Smith,ou=People,dc=example,dc=com' } + it "has the correct username" do expect(auth_hash.username).to eq("123456") end @@ -42,6 +44,8 @@ describe Gitlab::LDAP::AuthHash do end context "with overridden attributes" do + let(:given_uid) { 'uid=John Smith,ou=People,dc=example,dc=com' } + let(:attributes) do { 'username' => %w(mail email), @@ -61,4 +65,14 @@ describe Gitlab::LDAP::AuthHash do expect(auth_hash.name).to eq("John Smith") end end + + describe '#uid' do + context 'when there is extraneous (but valid) whitespace' do + let(:given_uid) { 'uid =John Smith , ou = People, dc= example,dc =com' } + + it 'removes the extraneous whitespace' do + expect(auth_hash.uid).to eq('uid=John Smith,ou=People,dc=example,dc=com') + end + end + end end diff --git a/spec/lib/gitlab/ldap/person_spec.rb b/spec/lib/gitlab/ldap/person_spec.rb index 087c4d8c92c..74d6979cf61 100644 --- a/spec/lib/gitlab/ldap/person_spec.rb +++ b/spec/lib/gitlab/ldap/person_spec.rb @@ -16,6 +16,136 @@ describe Gitlab::LDAP::Person do ) end + describe '.normalize_dn' do + context 'when there is extraneous (but valid) whitespace' do + it 'removes the extraneous whitespace' do + given = 'uid =John Smith , ou = People, dc= example,dc =com' + expected = 'uid=John Smith,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + + context 'for a DN with a single RDN' do + it 'removes the extraneous whitespace' do + given = 'uid = John Smith' + expected = 'uid=John Smith' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there are escaped characters' do + it 'removes extraneous whitespace without changing the escaped characters' do + given = 'uid = Sebasti\\c3\\a1n\\ C.\\20Smith\\ , ou=People (aka. \\22humans\\") ,dc=example, dc=com' + expected = 'uid=Sebasti\\c3\\a1n\\ C.\\20Smith\\ ,ou=People (aka. \\22humans\\"),dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'with a multivalued RDN' do + it 'removes extraneous whitespace without modifying the multivalued RDN' do + given = 'uid = John Smith + telephoneNumber = +1 555-555-5555 , ou = People,dc=example,dc=com' + expected = 'uid=John Smith+telephoneNumber=+1 555-555-5555,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + + context 'with a telephoneNumber with a space after the plus sign' do + # I am not sure whether a space after the telephoneNumber plus sign is valid, + # and I am not sure if this is "proper" behavior under these conditions, and + # I am not sure if it matters to us or anyone else, so rather than dig + # through RFCs, I am only documenting the behavior here. + it 'removes the space after the plus sign in the telephoneNumber' do + given = 'uid = John Smith + telephoneNumber = + 1 555-555-5555 , ou = People,dc=example,dc=com' + expected = 'uid=John Smith+telephoneNumber=+1 555-555-5555,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + end + end + + context 'for a null DN (empty string)' do + it 'returns empty string and does not error' do + given = '' + expected = '' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there is an escaped leading space in an attribute value' do + it 'does not remove the escaped leading space (and does not error like Net::LDAP::DN.new does)' do + given = 'uid=\\ John Smith,ou=People,dc=example,dc=com' + expected = 'uid=\\ John Smith,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there is an escaped trailing space in an attribute value' do + it 'does not remove the escaped trailing space' do + given = 'uid=John Smith\\ ,ou=People,dc=example,dc=com' + expected = 'uid=John Smith\\ ,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there is an escaped leading newline in an attribute value' do + it 'does not remove the escaped leading newline' do + given = 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com' + expected = 'uid=\\\nJohn Smith,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there is an escaped trailing newline in an attribute value' do + it 'does not remove the escaped trailing newline' do + given = 'uid=John Smith\\\n,ou=People,dc=example,dc=com' + expected = 'uid=John Smith\\\n,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there is an unescaped leading newline in an attribute value' do + it 'does not remove the unescaped leading newline' do + given = 'uid=\nJohn Smith,ou=People,dc=example,dc=com' + expected = 'uid=\nJohn Smith,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when there is an unescaped trailing newline in an attribute value' do + it 'does not remove the unescaped trailing newline' do + given = 'uid=John Smith\n ,ou=People,dc=example,dc=com' + expected = 'uid=John Smith\n,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'with uppercase characters' do + # We may need to normalize casing at some point. + # I am just making it explicit that we don't at this time. + it 'returns the DN with unmodified casing' do + given = 'UID=John Smith,ou=People,dc=example,dc=com' + expected = 'UID=John Smith,ou=People,dc=example,dc=com' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'with a malformed DN' do + context 'when passed a UID instead of a DN' do + it 'returns the UID (with whitespace stripped)' do + given = ' John C. Smith ' + expected = 'John C. Smith' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + + context 'when an equal sign is escaped' do + it 'returns the DN completely unmodified' do + given = 'uid= foo\\=bar' + expected = 'uid= foo\\=bar' + expect(described_class.normalize_dn(given)).to eq(expected) + end + end + end + end + describe '#name' do it 'uses the configured name attribute and handles values as an array' do name = 'John Doe'