Add LDAP authentication #4032

immae · 2017-07-02T00:12:48Z

This adds LDAP authentication to mastodon instances.

Two ways are possible:

Either having a "manager" password for mastodon who can search for users (preferred way for LDAP)
Or using a template to login as the user ("simple" way)

This is mostly devise work (there exist an LDAP authenticator for devise, but it only supports the second way), but I don't have the required skills to write it for devise directly, maybe for another time when I understand better how it works.

I updated "en" and "fr" locales, I don't know how I should proceed to update the other locales

It fixes issue #942

immae · 2017-07-02T00:17:05Z

Oops it looks like someone did it differently in the meantime already...

ykzts · 2017-07-04T02:51:53Z

duplicated with #3103?

immae · 2017-07-04T08:21:49Z

@ykzts: I didn't see this pull request before developping my version. The PR #3103 is incomplete in my opinion: in particular, it doesn't implement the "usual" way of implementing LDAP authentication.

But if it is merged I can add the mentioned implementation afterwards, both are very similar.

It's up to you to finish with #3103 and I'll rebase mine on top of it once it's merged.

(Also, some of the problematic bullet points are adressed in my PR.)

Gargron · 2017-07-04T13:15:14Z

This PR is better than #3103, good job!

This topic has been on a bit of a backburner for me. On my own PR, I got this comment, which I think makes sense given that there are outstanding requests for CAS authentication as well as more general OAuth: #3103 (comment) So I was considering eventually implementing that. Do you think that makes sense?

nightpool · 2017-07-04T14:46:31Z

app/controllers/auth/registrations_controller.rb

+    if single_user_mode? || !Setting.open_registrations
+      redirect_to root_path
+    elsif Rails.configuration.x.use_ldap
+      redirect_to new_user_session_path


this should probably be it's own method rather then overloading this one.

nightpool · 2017-07-04T14:47:47Z

app/models/user.rb

@@ -31,6 +31,7 @@
 #  last_emailed_at           :datetime
 #  otp_backup_codes          :string           is an Array
 #  filtered_languages        :string           default([]), not null, is an Array
+#  ldap_dn                   :text


yeah, I agree with Gargron that putting this on the user model doesn't make a lot of sense. How does discourse do it with their pluggable authentication methods?

The way I understand how it works, it's not how devise see things:
Different models should correspond to different roles, and not different authentication modes. If we create a LdapUser model, then we will have two models which share the same utility. That means everywhere you check that your user is logged in, you'll have to check if user is logged in or ldap_user is logged in.

An example (given in devise's documentation) of the use to have different models is to make a distinction between a simple user and an admin user, which clearly have different roles.

In our case, an ldap user and an user would have exactly the same role, it's only the way they authenticate that change. It's sort of the same change as the two factor authentications: it lies on the user object and not outside, because once logged in you are that user.

ldap_dn is explicitely stated here, because there is no devise LDAP authenticator that "hides" the thing. If you look in the database schema, you'll see otp / 2FA related informations, which may be null in case it's not relevent to that user.

Here it's the same, except the attribute is stated explicitely in the model

nightpool · 2017-07-04T14:49:32Z

app/models/user.rb

  protected

  def send_devise_notification(notification, *args)
    devise_mailer.send(notification, self, *args).deliver_later
  end

+  def confirmation_required?
+    if ldap_user?
+      !Rails.configuration.x.ldap_skip_email_confirmation


super unless Rails.configuration.x.ldap_skip_email_configuration seems equivalent?

uh nope, it would be ldap_user? && !Rails.configuration.x.ldap_skip_email_confirmation || super, but then it's much harder to read?

nightpool · 2017-07-04T14:50:31Z

app/views/about/show.html.haml

-        - if @instance_presenter.closed_registrations_message.blank?
+        - if Rails.configuration.x.use_ldap
+          %p= t('about.ldap_registrations')
+        - elsif @instance_presenter.closed_registrations_message.blank?


so in this case, it's not possible to close registrations if you have ldap enabled? seems bad.

I don't think LDAP registrations ever need to be closed, because they're not really registrations - these people already have logins in LDAP, so it is expected that they can login on a connected Mastodon.

Yes, if you don't want you users to use mastodon, then you disable them from LDAP, it's no more up to mastodon to decide that. (At least that's how I see LDAP)

that makes sense, but if ldap_only is false, then how would you prevent people from signing up with their email addresses? or is that handled elsewhere?

Right, I just understood your point.

Gargron · 2017-07-04T14:53:07Z

app/models/user.rb

@@ -105,12 +107,24 @@ def session_active?(id)
    session_activations.active? id
  end

+  def ldap_user?
+    Rails.configuration.x.use_ldap && ldap_dn.present?


ldap_dn :text, so seems correct.

yes, sorry, I was misreading. I was confused because of the attribute :ldap_username, above.

nightpool · 2017-07-04T14:53:08Z

config/initializers/0_ldap_authenticatable.rb

+      end
+
+      def authenticate!
+        config = Rails.configuration


maybe a better way to do this would be something like delegate :ldap_host, :ldap_port, :ldap_manager_dn, :ldap_manager_password, :ldap_manager_bind, :ldap_search_filter, :ldap_search_base, :ldap_mail_attribute, :ldap_username_attribute, to: Rails.configuration.x, or something? so we have all of the expected config variables listed out explicitly.

Honestly, it doesn't feel like Rails.configuration.x is the right place to put these variables...

I believe it is probably okay to simply read the ENV variables in this very class, instead of doing so in an initializer. If nothing but this class will use them.

devise.rb uses it to decide which key to use (ldap_username or email). I need to figure out how to make it not sensible to ordering though

nightpool · 2017-07-04T14:53:58Z

config/initializers/0_ldap_authenticatable.rb

+  end
+end
+
+Rails.application.configure do


these two things should be in separate files.

nightpool · 2017-07-04T14:55:25Z

config/initializers/0_ldap_authenticatable.rb

@@ -0,0 +1,109 @@
+require 'net/ldap'


the fact that this is required to run before initializers/devise feels like a code smell and a bad hack.

Yep, but how can we do otherwise?
As far as I know (little experience with other projects), config/initializer files are supposed to be ordered, it's just luck that makes the ones in mastodon not necessarily ordered?

nightpool · 2017-07-04T14:55:57Z

config/initializers/devise.rb

@@ -50,7 +51,11 @@
  # session. If you need permissions, you should implement that in a before filter.
  # You can also supply a hash where the value is a boolean determining whether
  # or not authentication should be aborted when the value is not present.
-  # config.authentication_keys = [:email]
+  if Rails.configuration.x.use_ldap
+    config.authentication_keys = [:ldap_username]


is there a way to delay this configuration until later, so we don't have to worry about the load order? (and remove the ugly 0_ hack)

I tried in vain, but as I said I have little experience with that, if someone knows better...

nightpool · 2017-07-04T14:57:00Z

config/initializers/devise.rb

@@ -50,7 +51,11 @@
  # session. If you need permissions, you should implement that in a before filter.
  # You can also supply a hash where the value is a boolean determining whether
  # or not authentication should be aborted when the value is not present.
-  # config.authentication_keys = [:email]
+  if Rails.configuration.x.use_ldap


this doesn't handle ldap_only properly, if i'm reading this correctly.

the "ldap_username" should be named "ldap_username_or_email" actually, but it seems very long. Or yes we could have three keys depending on "ldap_only", "ldap", "no_ldap" ?

immae · 2017-07-04T15:56:18Z

@Gargron : yes, I saw the omniauth / Oauth thing, and to me it seems outside of the scope of that PR (or yours), even if of course the problem triggers now that we consider a new login process.

We have two things:

Users can be authenticated in different ways: Database, 2FA, OTP, LDAP
This is already handled by devise correctly (I think)
Users may want to be able to authenticate in more than one way (for instance, depending on their device or their mood).
This is currently not handled, but I think @gloaec's solution based on Identity is unnecessary because devise is "Omniauth"-able (http://rubydoc.info/github/plataformatec/devise/master/Devise/Models/Omniauthable) I still need to check on that though, I didn't look at this aspect.

Edit: It's hard to explain the difference in proper words. If it's not clear, it's the distinction between:

User A logs in through database while User B logs in through LDAP
and
User A want to be able to login through database or LDAP depending on the device he uses

gloaec · 2017-07-04T16:04:17Z

@immae Middleware is indeed unnecessary for LDAP authentication, you're right. But it is with protocols like CAS or SAML. I'm currently working on both, i you want to have a look and review (with devise/omniauth) :
#3148

immae · 2017-07-04T16:12:08Z

@gloaec: I have no idea what CAS or SAML are. Oauth or openid (if I understand/remember correctly) have completely different mecanisms and cannot be handled with only one POST from a form: the navigator need to contact itself the trusted third party to authenticate +challenge.

In LDAP case, it's the job of the provider (mastodon) to get the credentials and pass them to the LDAP server.

I think I understand now why you want to add an Identity for your case, but I need to dig and document more :p

gloaec · 2017-07-04T16:19:17Z

@immae, right. Unlike LDAP, SSO services like Shibboleth (SAML), CAS, facebook, google, twitter,... have their own login page, that's why there is a need for a middleware in this case :)

jcgruenhage · 2017-07-22T08:48:59Z

Is there any progress on this? I'd be really happy to see this merged, and I'm sure, that I am not the only one.

immae · 2017-07-22T09:02:27Z

@jcgruenhage : I'm waiting for #3148

You may use the commit already, it's functionnal, but most probably the database schema won't be the same after #3148 so prepare to have some migration tasks

gloaec · 2017-07-25T12:54:08Z

@immae, I need to push some of the modifications i made on #3148 to integrate SAML as well, but at last, I need to provision the .env files for docker (pretty niche too). It shouldn't take long, but I wont be able to push anything before next week unfortunately. I'll keep in touch soon.

Gargron · 2018-02-02T09:26:55Z

I have merged PAM authentication (#5303). PAM is supposed to handle integrations with LDAP and CAS. Do you think that is enough and this PR is no longer necessary?

immae · 2018-02-02T09:31:07Z

I didn’t check the pam implementation, but there are two things that LDAP provides, authentication and account informations. Does your pam implementation take those two aspects? If so I think there is nothing more to add :p, otherwise it would be good to complete the pam to handle the missing one :)

immae · 2018-02-02T09:34:15Z

(There is actually not that much information, only email and uid, but I’m almost sure pam has this kind of feature and it’s nice to have)

Gargron · 2018-02-02T09:47:53Z

account informations

I think the PAM PR only did authentication (e-mail + username), nothing like display name or bio. So if there is a way of implementing that, you're welcome to add that (but I'm assuming in a different PR... Can I close this?)

immae · 2018-02-02T09:50:37Z

Yes, thanks for the work!

Gargron · 2018-02-02T09:53:00Z

@immae You should thank @devkral and not me, I only reviewed it (and I took a long time to do it because of releases/priorities) 😋

immae · 2018-02-07T13:33:52Z

@Gargron : I’d like to reopen it (and maybe one day redo a proper PR) if you agree, as I received some e-mail pointing out that PAM was not the solution to every problem, and could be a pain to configure properly.
I agree that it makes sense to have it alongside PAM

Gargron · 2018-02-07T15:33:53Z

@immae You can open a new PR. It would be via omniauth-ldap and not net-ldap directly so not much code reuse from this PR except maybe the environment variables.

akihikodaki added security Security issues and fixes, vulnerabilities enhancement labels Jul 2, 2017

Add LDAP authentication

f2f7b40

Gargron mentioned this pull request Jul 4, 2017

Fix #942 - Add LDAP support #3103

Closed

nightpool reviewed Jul 4, 2017

View reviewed changes

Gargron reviewed Jul 4, 2017

View reviewed changes

nightpool reviewed Jul 4, 2017

View reviewed changes

config/initializers/0_ldap_authenticatable.rb

end

end

Rails.application.configure do

Copy link

Member

nightpool Jul 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two things should be in separate files.

nightpool reviewed Jul 4, 2017

View reviewed changes

immae mentioned this pull request Jul 4, 2017

CAS + SAML authentication feature #3148

Closed

jcgruenhage mentioned this pull request Sep 20, 2017

[Feature Request] LDAP integration #942

Closed

1 task

Gargron closed this Feb 2, 2018

Add LDAP authentication #4032

Add LDAP authentication #4032

Conversation

immae commented Jul 2, 2017 • edited Loading

immae commented Jul 2, 2017

ykzts commented Jul 4, 2017

immae commented Jul 4, 2017

Gargron commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightpool Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightpool Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightpool Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

immae commented Jul 4, 2017 • edited Loading

gloaec commented Jul 4, 2017

immae commented Jul 4, 2017

gloaec commented Jul 4, 2017

jcgruenhage commented Jul 22, 2017

immae commented Jul 22, 2017 • edited Loading

gloaec commented Jul 25, 2017

Gargron commented Feb 2, 2018

immae commented Feb 2, 2018

immae commented Feb 2, 2018

Gargron commented Feb 2, 2018

immae commented Feb 2, 2018

Gargron commented Feb 2, 2018

immae commented Feb 7, 2018

Gargron commented Feb 7, 2018

immae commented Jul 2, 2017 •

edited

Loading

nightpool Jul 4, 2017 •

edited

Loading

nightpool Jul 4, 2017 •

edited

Loading

nightpool Jul 4, 2017 •

edited

Loading

immae commented Jul 4, 2017 •

edited

Loading

immae commented Jul 22, 2017 •

edited

Loading