Introducing LDAP support #301

mssola · 2015-09-02T14:14:56Z

This is an introduction to LDAP support. There are some questions that need to
be addressed, but at least the basic structure is in place now.

Fixes #150

Signed-off-by: Miquel Sabaté Solà [email protected]

mssola · 2015-09-02T14:39:10Z

This is an introduction to LDAP support. This PR includes the code that allows an LDAP user to authenticate in Portus. Some considerations:

I've added the ldap_name column inside of the users table. This column will be non-empty for LDAP users.
The ldap_name and the username will have the same value if the LDAP name does not contain any character forbidden by Docker.
Because of the previous point, an LDAP user (that has signed in at least once) can authenticate itself with either the ldap_name or username.

The thing is that we need to register LDAP users in Portus' DB, but we want to do this transparently to LDAP users. Therefore, when an LDAP user signs in Portus for the first time, it will be created automatically in Portus' database, and a proper username will be created for said user if its username is not valid for Docker (as explained previously).

But of course we've got a problem, since we require the email column to be filled when creating a user, and LDAP makes no guarantees of providing an email for each user. The decision here is to drop this requirement for LDAP users that haven't filled up their emails yet on Portus. You can see that in the User#email_required? method, that is then picked up by Devise to do the magic. Now, isn't this dangerous ? Yes it is, that's why I've added a hook that redirects any LDAP user with an empty email to its profile page. Moreover, this profile page has been tweaked a bit, to reflect better what the user should do:

This situation can be better. Basically we can add a configuration option in the config.yml file that:

Allows the admin to specify the key on the LDIF of the user where Portus can fetch the user's email.
Allows an alternative scheme where the admin just specifies the hostname of the mailing server. Example: the admin sets "@organization.com", so Portus will automatically guess "[email protected]".

My idea is that if the admin set neither of the previous options, Portus would fallback to the current behavior. This way we would act safely, while being as clever as possible. The implementation of the above points should be done in another PR though.

There are more crazy ideas that are related to LDAP support. One that occurred to me is to automatically create namespaces/teams from the organizations defined in the LDAP server (with users also being mapped into it automatically).

Moreover, the configuration format has changed a little (and it needs further changing but I'll do that later on another PR). Notice that now each key defines a enabled key. This is in concordance with other projects like Docker's Distribution or Gitlab. This allows us to add the handy enabled? method. So from anywhere of the code, the developer can always check whether LDAP is enabled with:

APP_CONFIG.enabled?("ldap")

The specific configuration options for LDAP support are quite basic:

hostname: the hostname of the LDAP server.
port: the port of the LDAP server.
base: the base where users are located. This is usually a string like "ou=users,dc=example,dc=com". By default it's empty, which is the simplest way in which an LDAP directory can be structured.

mssola · 2015-09-02T14:43:56Z

By the way, notice that LDAP users would need to use their username in order to pull/push inside of the docker client.

mssola · 2015-09-02T14:53:24Z

Travis' failure is regarding a change from @jordimassaguerpla that will be superseded by #297. That's why I didn't fix it.

jordimassaguerpla · 2015-09-02T15:21:57Z

app/models/user.rb

-                       format: { with:    /\A[a-z0-9]{4,30}\Z/,
-                                 message: 'Accepted format: "\A[a-z0-9]{4,30}\Z"' }
+                       format: { with:    USERNAME_FORMAT,
+                                 message: "Only alphanumeric characters are allowed" }


shouldn't we say also "minimum 4 and maximum 30"?

jordimassaguerpla · 2015-09-02T15:28:48Z

a part from the small issue with the message, all the rest lgtm, but I am no expert. Let's see what flavio says.

flavio · 2015-09-02T15:35:36Z

db/schema.rb

@@ -136,6 +136,7 @@
    t.datetime "updated_at"
    t.boolean  "admin",                  limit: 1,   default: false
    t.boolean  "enabled",                limit: 1,   default: true
+    t.string   "ldap_name",              limit: 255, default: ""


Do you think two different users can end up having the same ldap_name? I think we should add a unique index on it.

flavio · 2015-09-02T15:45:24Z

We have to make sure the LDAP server is always contacted, the password inside of our DB should be omitted for users registered via LDAP

flavio · 2015-09-02T15:46:44Z

We should also notice clashes inside of the ldap_name and prompt the user to provide a custom name. We should propose a unique name, like "#{name_already_taken}#{counter}".

flavio · 2015-09-02T15:46:58Z

LGTM 👏

flavio · 2015-09-02T15:48:22Z

One last thing about the screenshot you posted: IMHO the form should fill the entire page, right now there's a lot of empty space on the right side of the screen. However I think that's a problem that has always afflicted the page.

mssola · 2015-09-03T08:01:14Z

@flavio on the layout of the profile page, yes, this has been like that since always. We can fix that in another PR.

mssola · 2015-09-03T11:52:54Z

Implemented the code to avoid name clashes:

On creation, if the name clashes it will call generate_random_name. This function will try to get a free name with the format "#{username}#{random_number}". If it doesn't work, then it will raise an exception as expected by Devise.
I've added a unique index to ldap_name.

flavio · 2015-09-03T12:19:27Z

generate_randome_name looks good to me. I just realized another possible problem: what about ldap names that are shorter than 4 characters? These would not be accepted by Docker.

mssola · 2015-09-03T12:27:49Z

@flavio I say let the generate_random_name method handle that too. So if the LDAP name is just "a", this method would produce something like "a123". Of course, this is a bit ugly, but it would be fixed if we allow users to change their user name.

flavio · 2015-09-03T12:28:12Z

Sorry, I missed that. LGTM

mssola · 2015-09-03T12:31:21Z

@flavio you didn't miss it, it's not implemented yet :D

mssola · 2015-09-03T14:31:18Z

This should be it. Last changes:

LDAP users won't be able to change their passwords through Portus' profile page.
If LDAP is disabled or the current user does not exist in the given LDAP server, then LDAP authenticatable is discarded. Otherwise the other strategies are discarded.
More documentation, more tests.

flavio · 2015-09-03T14:44:44Z

lib/portus/ldap.rb

+          fail!(:invalid_login)
+        end
+      else
+        # rubocop:disable Style/SignalException


Being there means:

ldap is disabled

ldap is enabled, but the user cannot be found on the remote server

The next action is fail which moves to the next authentication strategy (portus' db). I think it would be better to distinguish the second case and fail immediately. I think that once LDAP is enable we should not allow a mixed authentication strategy.

Did I get something wrong?

I'm not sure. I'd be for with a mixed authentication strategy. For example, how would you create the admin user if LDAP is enabled if you don't allow a mixed solution ?

This is an introduction to LDAP support. There are some questions that need to be addressed, but at least the basic structure is in place now. Fixes SUSE#150 Signed-off-by: Miquel Sabaté Solà <[email protected]>

mssola · 2015-09-04T09:24:44Z

After a bit of discussion, we're going for a "strict" LDAP support. That is, if LDAP support is enabled, then only LDAP users can log in Portus. This can be put as a configurable value in the future if users request the "mixed" approach. With this, I've updated the patch so:

The sign up page is not accessible if LDAP support is enabled.
The first LDAP user to sign in will be an admin. This is stated in the login form, and the wiki will be updated accordingly.

I've updated the patch to reflect this changes.

flavio · 2015-09-04T10:35:59Z

LGTM. BTW, in the future it would be better to avoid the merge of all the commits for big pull requests like this; it's painful to review the whole diff all the times to spot only a few changes.

Proposal: for PR like this one let's merge the the commits only before doing the real merge.

mssola · 2015-09-04T11:04:31Z

@flavio sounds reasonable to me

Introducing LDAP support

mssola force-pushed the ldap branch 2 times, most recently from ccc8c66 to 10f5ae7 Compare September 2, 2015 14:17

jordimassaguerpla reviewed Sep 2, 2015
View reviewed changes

flavio reviewed Sep 2, 2015
View reviewed changes

mssola force-pushed the ldap branch from 7c534db to a457daf Compare September 2, 2015 15:38

mssola force-pushed the ldap branch from a457daf to 5398e9d Compare September 2, 2015 15:50

mssola force-pushed the ldap branch from 5398e9d to 3749b24 Compare September 3, 2015 11:46

mssola force-pushed the ldap branch 3 times, most recently from f3ecda9 to e397a8e Compare September 3, 2015 14:28

flavio reviewed Sep 3, 2015
View reviewed changes

mssola force-pushed the ldap branch 2 times, most recently from 205e5e0 to 297edab Compare September 4, 2015 08:12

Introducing LDAP support

16da5e3

This is an introduction to LDAP support. There are some questions that need to be addressed, but at least the basic structure is in place now. Fixes SUSE#150 Signed-off-by: Miquel Sabaté Solà <[email protected]>

mssola force-pushed the ldap branch from 297edab to 16da5e3 Compare September 4, 2015 09:24

mssola added a commit that referenced this pull request Sep 4, 2015

Merge pull request #301 from mssola/ldap

2da3f91

Introducing LDAP support

mssola merged commit 2da3f91 into SUSE:master Sep 4, 2015

mssola deleted the ldap branch September 4, 2015 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing LDAP support #301

Introducing LDAP support #301

mssola commented Sep 2, 2015

mssola commented Sep 2, 2015

mssola commented Sep 2, 2015

mssola commented Sep 2, 2015

jordimassaguerpla Sep 2, 2015

flavio Sep 2, 2015

jordimassaguerpla commented Sep 2, 2015

flavio Sep 2, 2015

flavio commented Sep 2, 2015

flavio commented Sep 2, 2015

flavio commented Sep 2, 2015

flavio commented Sep 2, 2015

mssola commented Sep 3, 2015

mssola commented Sep 3, 2015

flavio commented Sep 3, 2015

mssola commented Sep 3, 2015

flavio commented Sep 3, 2015

mssola commented Sep 3, 2015

mssola commented Sep 3, 2015

flavio Sep 3, 2015

mssola Sep 3, 2015

mssola commented Sep 4, 2015

flavio commented Sep 4, 2015

mssola commented Sep 4, 2015

Introducing LDAP support #301

Introducing LDAP support #301

Conversation

mssola commented Sep 2, 2015

mssola commented Sep 2, 2015

mssola commented Sep 2, 2015

mssola commented Sep 2, 2015

jordimassaguerpla Sep 2, 2015

Choose a reason for hiding this comment

flavio Sep 2, 2015

Choose a reason for hiding this comment

jordimassaguerpla commented Sep 2, 2015

flavio Sep 2, 2015

Choose a reason for hiding this comment

flavio commented Sep 2, 2015

flavio commented Sep 2, 2015

flavio commented Sep 2, 2015

flavio commented Sep 2, 2015

mssola commented Sep 3, 2015

mssola commented Sep 3, 2015

flavio commented Sep 3, 2015

mssola commented Sep 3, 2015

flavio commented Sep 3, 2015

mssola commented Sep 3, 2015

mssola commented Sep 3, 2015

flavio Sep 3, 2015

Choose a reason for hiding this comment

mssola Sep 3, 2015

Choose a reason for hiding this comment

mssola commented Sep 4, 2015

flavio commented Sep 4, 2015

mssola commented Sep 4, 2015