New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable PHP LDAP module #117

Closed
HittingSmoke opened this Issue Oct 25, 2016 · 32 comments

Comments

Projects
None yet
7 participants
@HittingSmoke
Copy link

HittingSmoke commented Oct 25, 2016

The LDAP app is not usable in the snap because the LDAP module is not enabled in php.ini.

The line is already there, just commented out. I'm not sure if this would make the module a dependency or if PHP will gracefully start when a module is not found.

@oparoz

This comment has been minimized.

Copy link
Member

oparoz commented Oct 25, 2016

It won't work. You need to use a snap which comes with LDAP support, because this snap focuses on home users.

@HittingSmoke

This comment has been minimized.

Copy link

HittingSmoke commented Oct 25, 2016

libldap is already included in both snappy core and the nextcloud snap. Doesn't that mean it's just a matter of including the PHP module? It's a single file and there are no other dependencies.

@oparoz

This comment has been minimized.

Copy link
Member

oparoz commented Oct 25, 2016

It doesn't work that way because PHP is compiled from source. Also, it's currently not possible to let users enable features à la carte.

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 25, 2016

How big is the ldap module?

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 25, 2016

I don't see a problem with enabling this, but I don't have a directory server setup to actually make sure it's working. @HittingSmoke are you willing to help with that?

@HittingSmoke

This comment has been minimized.

Copy link

HittingSmoke commented Oct 25, 2016

@kyrofa I'm setting one up this week on my home/office network. I'll do whatever I can to help. All I need is a snap put together with the php-ldap package. I only started learning how snaps work over the last day or two but if I'm understanding correctly it's not hard to include an existing Ubuntu package at compile time? The entire installed package size is 91KB.

@oparoz I compile my own server stack for other projects including PHP and I don't really understand how that affects enabling LDAP. When compiled from source PHP can still load modules. I don't believe there are any compile-time options required. The requirements are just libldap, and php-ldap. The only part missing from the snap is php-ldap then uncommenting ldap.so in php.ini.

@oparoz

This comment has been minimized.

Copy link
Member

oparoz commented Oct 25, 2016

When compiled from source PHP can still load modules

Sure

I don't believe there are any compile-time options required

That's incorrect. That module has to be compiled at the same time, just like an external PECL module and the problem is that with this snap, the module has to be enabled for everybody and this feature has nothing to do with a snap destined to home users who can't use the command line.
Ideally, we would compile all module and let users enable what they want, but that doesn't work yet.

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 25, 2016

@HittingSmoke indeed, but the PHP configuration in the snap is read-only, which means this would have to be enabled for all users of the snap, not just those that want it.

@oparoz are there security concerns with enabling this module?

@HittingSmoke

This comment has been minimized.

Copy link

HittingSmoke commented Oct 25, 2016

@oparoz I see what you mean now. Reading the snappy docs I was under the impression you could include packages but now I see those are just build dependencies.

I forked the snap. It was compiling when I left the house. Going to test it out when I get back to my desk. I'll report back and if it works make a pull request if it's something you'd be open to. There should be no security concerns though I'll admit it is a niche usage. The only downside would be a potential few KB increased PHP binary size and negligibly increased compile time for PHP when updating the snap.

@oparoz

This comment has been minimized.

Copy link
Member

oparoz commented Oct 25, 2016

@oparoz are there security concerns with enabling this module?

@kyrofa - Not more than usual, but it does load quite a few libraries and that's one more component to keep an eye on. One which will be used by a tiny minority. If you look at the documentation about the module, it requires some decent knowledge about LDAP to set up.
From my pov, it's out of scope as I don't see any Box owner connect it to an LDAP server. Those users asked for other feature such as SMB, Spreed.ME, snapweb.

The only downside would be a potential few KB increased PHP binary size

@HittingSmoke - It will be more than a few KB, but all together, it will just eat a few MBs and that's something even the Pi2 could manage.
If that's the only extra feature that you need, I think maintaining your fork will be easy. You will only need to rebase your branch from time to time and won't even need to rebuild the whole snap depending on what's changed.

@HittingSmoke

This comment has been minimized.

Copy link

HittingSmoke commented Oct 26, 2016

I got it working. Have not connected it to a server because I'm still building the IPA server but I have the LDAP auth app installed.

Actually, the only change to the snap that needed to be made was adding --with-ldap to snapcraft.yaml under PHP configure options. libldap2-dev becomes a build dependency but I didn't have to specify it in the build packages. snapcraft pulled it automatically. I don't know if this is expected or bad practice for building a snap.

The only real hiccup is on Ubuntu you require symlinks in /usr/lib to libdlap and liblber because they are installed into /usr/lib/x86_64-linux-gnu. That's only on the build machine though and after that it compiles fine and NextCloud allows you to install the LDAP auth app. I'm poking around in the snappy docs to see if I can make a symlink using the snapcraft file.

@oparoz It's not much work at all to maintain myself, but here's my argument for it being included.

If there were three dozen apps that had specialty PHP extension requirements I would totally get it. You'd have to pick and choose the most commonly demanded or include none. If it were some massive dependency like Calligra and libreoffice I'd understand how increasing the compile time by three hours (seriously, libreoffice takes a long time) and the size of the snap tenfold is not worth it. But in this case it's literally the only app that ships with NextCloud which has a special dependency. It's very small. People who don't use it will never be affected by its inclusion except they won't have the red "DISABLED" warning on one app on the Not Enabled app list.

In this case, it's a single line and a single build dependency package. I think the simplicity of including it in addition to the incredible functionality it offers outweighs the niche market for it.

After I determine the best way to add the symlinks to the snapcraft config I'll submit a pull request and you can look it over and do as you please with it. I understand the hesitation completely. I just think the pros outweigh the cons.

If it's not going to be included, I'd recommend no longer shipping the LDAP app with the NextCloud snap as it's just vestigial code that can never be used and taking up useless space in the app install UI.

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 26, 2016

@HittingSmoke your argument sold it to me. I agree that if it's a default app it should be possible to use out of the box.

Note that you likely don't have to make a symlink, but pass the right directory with --with-ldap=<dir>, which means you need to do it in the PHP plugin in parts/plugins/x-php.py and utilize the plugin's self.project.arch_triplet property to figure out the path.

@HittingSmoke

This comment has been minimized.

Copy link

HittingSmoke commented Oct 26, 2016

@kyrofa For some reason in Ubuntu that's not enough. If you don't set the directory it says it can't find LDAP libraries in /usr/lib. If you set the LDAP library directory manually it complains that it can't find the LDAP headers in /usr/include. I don't think there is a way to set both. There are a bunch of StackOverflow questions about it as well as at least one bug report I found but it was made in PHP, not Ubuntu. The only solution I found was to use a symlink. Would the PHP plugin expose some sort of workaround for that by pointing directly to the libraries and headers?

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 28, 2016

@HittingSmoke the symlink mod is a bit of a non-starter, we need this to be able to build on clean systems.

Can you give this patch a try for me? Change your arch if you're not on amd64, but I'm curious if this works for you:

diff --git a/snapcraft.yaml b/snapcraft.yaml
index 8838f35..072e6ef 100644
--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -150,6 +150,7 @@ parts:
       - --enable-exif
       - --enable-intl
       - --with-jpeg-dir=/usr/lib
+      - --with-ldap-libs=/usr/lib/x86_64-linux-gnu
       - --disable-rpath
     stage-packages:
       # These are only included here until the OS snap stabilizes
@@ -162,6 +163,7 @@ parts:
       - libjpeg9-dev
       - libbz2-dev
       - libmcrypt-dev
+      - libldap2-dev
     snap:
      - -sbin/
      - -etc/

@kyrofa kyrofa added the feature label Oct 28, 2016

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 28, 2016

Actually, probably easier is to just try my branch.

@oparoz

This comment has been minimized.

Copy link
Member

oparoz commented Oct 29, 2016

But in this case it's literally the only app that ships with NextCloud which has a special dependency.

Not true. There are several and even some of the core functionalities have dependencies if you want to have access to additional features or improve performance.
And you're forgetting a major external dependency such as the requirement of having an LDAP server to be able to use the ldap app.

It's very small.

Kind of, but you're forgetting the LDAP lib dependencies which are pulled in.

People who don't use it will never be affected by its inclusion except they won't have the red "DISABLED" warning on one app on the Not Enabled app list.

For the audience of this snap, having a feature they don't even begin to understand, marked as disabled is insignificant. The real solution for this is to actually remove the app from the snap, just like the updater needs to be removed.

I agree that if it's a default app it should be possible to use out of the box.

I just mentioned the updater as an example of app for which that reasoning doesn't work. SAML is probably another candidate and there will be more in the future as Enterprise features are added.
Instead of trying to support every niche use case, we should be removing all those apps which have nothing to do with the goals of this snap.
Advanced users will never be satisfied with the restrictions which come with this snap and we shouldn't try to attract them.

@HittingSmoke

This comment has been minimized.

Copy link

HittingSmoke commented Oct 29, 2016

@kyrofa Looks like there's a problem with a mirror to Apache in your branch. I'll tinker around with it or apply the patch to mine and try again after the weekend.

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 31, 2016

@HittingSmoke huh, I didn't change any of that. Maybe they were down temporarily. Note that I did neglect to load the ldap module in the php.ini though, you probably need that 😛 .

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Oct 31, 2016

I just mentioned the updater as an example of app for which that reasoning doesn't work.

We already have a bug for removing/disabling that one. It doesn't make sense to have it in the snap, not because we don't want to support it, but because the snap makes it redundant and useless. If features are easy to enable and present no security risk, I see no reason to not enable them.

Advanced users will never be satisfied with the restrictions which come with this snap and we shouldn't try to attract them.

Are you talking about @HittingSmoke? We have a user here that's asking for a new feature, and you're telling them we don't want them as a user. Just wanting enterprisey features doesn't necessarily mean that the user wants to hack on PHP and Apache configs. Show me an enterprise feature that requires abilities the snap can't provide, and I'll show you an enterprise feature we can't support. Until then, why the hostility?

@oparoz

This comment has been minimized.

Copy link
Member

oparoz commented Nov 22, 2016

you're telling them we don't want them as a user

It's not exactly that. I'm saying it's the wrong product/snap for them and that they will be disappointed down the line if they need some flexibility.

At the end of the day, you're the maintainer. If that's something you want to support, then go ahead and implement the feature. I just think it's not aligned with the goals of this snap: Home users with no IT knowledge.

@sempervictus

This comment has been minimized.

Copy link

sempervictus commented May 28, 2017

With the SAML provider not working apparently (v1474), this is the only rational way to go for central auth. Even users who aren't very tech savvy still have gool ol' active directory at their company, so this would be a welcome function for the do-it-yourself department sick of waiting on their IT guy to finish his MMORPG quest and implement these collaborative functions for them :). There are also a number of user-friendly distributions for all sorts of things exposing LDAP, and this helps tie-in.
Also, snaps are great from a security perspective - gets pretty hard to overwrite PHP files when they're in squashfs, and with MPROTECT its hard to get stagers to work properly, forcing attackers to write their payloads to the mounted portion and try to execute from there instead of staging into the memory of the vulnerable host process. So, yeah, LDAP would be useful for that use case too - where people have infrastructure, but are using this to keep what is essentially a terrifying publicly exposed endpoint constantly updated (and a bit more secure if they build the env up)

phibbs7 added a commit to phibbs7/nextcloud-snap that referenced this issue Jul 27, 2017

@phibbs7

This comment has been minimized.

Copy link

phibbs7 commented Jul 29, 2017

I got it to work. It seems that all you have to do is set php's --with-libdir configflag to the correct path and the build will complete just fine. I've also attempted to use it with a working AD which also works. About the only thing that doesn't work currently is the User Admin interface.

When user_ldap is enabled and configured, attempting to open the User administration page fails. So locally defined users can't be edited. This seems to be an issue in nextcloud not php-ldap though.

Log:
Level: Error App: index TypeError: Argument 1 passed to OC\Group\MetaData::generateGroupMetaData() must implement interface OCP\IGroup, null given, called in /snap/nextcloud/x1/htdocs/lib/private/Group/MetaData.php on line 92

/snap/nextcloud/x1/htdocs/lib/private/Group/MetaData.php - line 92: OC\Group\MetaData->generateGroupMetaData(NULL, '')
/snap/nextcloud/x1/htdocs/settings/users.php - line 69: OC\Group\MetaData->get()
/snap/nextcloud/x1/htdocs/lib/private/Route/Route.php - line 155): runtime-created function(1) require_once('/snap/nextcloud...')
[internal function] __lambda_func()
/snap/nextcloud/x1/htdocs/lib/private/Route/Router.php - line 299: call_user_func('\x00lambda_166', Array)
/snap/nextcloud/x1/htdocs/lib/base.php - line 1010: OC\Route\Router->match('/settings/users')
/snap/nextcloud/x1/htdocs/index.php - line 40: OC handleRequest()
{main}
@phibbs7

This comment has been minimized.

Copy link

phibbs7 commented Aug 3, 2017

OK it works completely now. Apparently, deleting the ldap config and re-entering it fixed the users administration page not opening properly. (I guess there needs to be some extra sanity checking done for the ldap config when loading that page, but that would be a different bug.)

With regards to this bug, is there a list of supported systems / distros to check and see if it works on? I've been looking but I haven't found one yet.

@adhami03

This comment has been minimized.

Copy link
Contributor

adhami03 commented Jan 2, 2018

This snap is perfect for our small business except that we still rely on Active Directory for managing users. I would argue that LDAP/AD integration is pretty common need once you get beyond a single user in your group, which applies to most small businesses. I have read through the entire chain here and there are references to this snap being focused on home users and not businesses. If that is the case, is there a snap that is more business-oriented that has LDAP enabled? If not, is there documentation someone could point me to that would allow me to take the latest snap and enable LDAP support? Thanks.

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Jan 2, 2018

@adhami03 this is the place to enable it, I just don't have the ability to test it so I need someone else to do the legwork. I'd start with what @phibbs7 was mentioning.

@mauriziomarini

This comment has been minimized.

Copy link

mauriziomarini commented Jan 2, 2018

Hello, I need Ldap integration for use nextcloud with
https://github.com/Zimbra-Community/owncloud-zimlet
I would have SSO between NC and ZImbra ldap, or with zimbra external ldap authentication (openldap or AD).
At the moment the app "Ldap user and group" is grayed, and it's not clear to me (after reading all this thread) what I should do into my snap to get it working.
Please try to summarize what I should do, step by step, I will be very thankful to you.

@adhami03

This comment has been minimized.

Copy link
Contributor

adhami03 commented Jan 3, 2018

@kyrofa , I tested out @phibbs7 fork of NextCloud with the code changes for enabling LDAP. It worked like a charm with our AD setup. I simply copied the configuration we have been using with our current NextCloud appliance. I also cloned the main nextcloud-snap repo and did a checkout to the latest 12 release. I applied the same code changes manually and it is working as well. What other aspects can I test?

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Jan 3, 2018

Nice! Please make your changes to a branch up-to-date with the develop branch. If that still works, open a pull request here (into develop, not master). I'll then build that pull request into a snap for all architectures. Once that's done, we have a good-sized collection of smoke tests for verifying nothing has broken. Mostly, though, I want to make sure AD works across a number of architectures, at least amd64 and armhf (e.g. a raspberry pi). Do you have the ability to test that (once I build the snap for it)?

@mauriziomarini

This comment has been minimized.

Copy link

mauriziomarini commented Jan 4, 2018

@adhami03

This comment has been minimized.

Copy link
Contributor

adhami03 commented Jan 4, 2018

@kyrofa , I tested the change in the develop branch and it works. According the the project guidelines, it says I should make a fork of the project, commit my changes there, and then make a pull request from my fork over to the develop branch of this project. Is that method I should use?

@kyrofa

This comment has been minimized.

Copy link
Member

kyrofa commented Jan 4, 2018

Yes indeed!

adhami03 added a commit to adhami03/nextcloud-snap that referenced this issue Jan 4, 2018

Fixes nextcloud#117 (credit to @phibbs7 for changes)
- Enables the LDAP module in PHP, which allows the "LDAP user and group backend" app to be enabled
@adhami03

This comment has been minimized.

Copy link
Contributor

adhami03 commented Jan 4, 2018

@kyrofa , I created a pull request. As for testing this out on different architectures, I only have access to x86_64 right now. I have a 1st gen Raspberry Pi, but it doesn't support Ubuntu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment