opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.36k stars 754 forks source link

Feature Request: "Remote" Groups #5303

Closed kaysond closed 2 years ago

kaysond commented 3 years ago

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Is your feature request related to a problem? Please describe.

Right now, functionality of Groups is somewhat hampered by the fact that they are tied to local system groups. This means group names have length limits and can't accommodate all characters used in LDAP groups (esp. spaces).

Additionally, it requires synchronizing groups and importing users to the local database in order to grant GUI access. This requires some level of manual intervention that is generally not desired when using a centralized user database like LDAP.

This seems to have come up a lot recently: #5295 #5116 #4660 #4963

My proposed solution would address all of these

Describe the solution you like

"Remote Groups" would be a new type of group that can be used for anything not related to OS-level groups, such as GUI privileges, and can bypass naming restrictions.

pfSense implements this and I think their description is useful:

Groups from remote sources, such as authentication servers (RADIUS or LDAP). These groups are not exposed to the operating system, and thus are only available for use in the GUI and other similar uses not involving the operating system layer. This scope has relaxed name restrictions, for example, group names may be longer and may contain spaces.

pfSense Docs - User Management

Describe alternatives you considered

An alternative could be group aliases, as described here, but I think Remote Groups is better because it addresses other issues with group sync and user importing.

Additional context

I posted a thread in the forums about my experience setting up LDAP, which was frustrating compared to the ease of the rest of OPNsense setup: https://forum.opnsense.org/index.php?topic=25234.0

For the use case of using LDAP and existing users/groups to grant GUI access, Remote Groups would make the process much easier and completely automatic.

Example implementation from pfSense interface: image

kaysond commented 3 years ago

I've been digging through the source, and I don't think this would be terribly difficult to implement.

system_groupmanager.php would have to be updated to include the scope field, and it looks like there is already an unused "Scope" variable.

ACL.php would need to be updated since it looks like it pulls group info from the system config. This would probably require an LDAP search in hasPrivileges, inGroup, and getLandingPage. I'm not 100% sure if AuthenticationFactory and the LDAP IAuthConnector are suited to this, but it seems so. There's also a tradeoff between caching the LDAP group results for speed and to avoid hammering LDAP, but then if a user was removed from the group, they'd still have access until a cache flush.

Most functions that use system groups should be unaffected, but it might be necessary to do some filter to exclude remote groups.

AdSchellevis commented 3 years ago

We're not considering to use groups as a placeholder for other usages, but adding an alias (or external name) as I've mentioned here https://github.com/opnsense/core/issues/5295#issuecomment-948315582 might be an option.

The disadvantage of groups that are only groups in a specific scope is that it will always get convoluted somewhere in the long run (it doesn't scale well).

kaysond commented 3 years ago

We're not considering to use groups as a placeholder for other usages, but adding an alias (or external name) as I've mentioned here #5295 (comment) might be an option.

Would this still require group synchronization and automatic import of users?

The disadvantage of groups that are only groups in a specific scope is that it will always get convoluted somewhere in the long run (it doesn't scale well).

You could address this by doing the reverse of your alias suggestion (i.e. mapping a remote group to a local group).

AdSchellevis commented 3 years ago

Would this still require group synchronization and automatic import of users?

Yes, without an anchor ACL's won't work.

You could address this by doing the reverse of your alias suggestion (i.e. mapping a remote group to a local group).

A mapping or an alias are indeed quite similar in this case, in cases where different authenticators are allowed to have different mappings it would make some sense, but currently this feels like an over complication to be honest (to offert this in a user friendly manner, one would also need to be able to query the other end for options and map them accordingly, which is quite some work for little value).

kaysond commented 3 years ago

Yes, without an anchor ACL's won't work.

I see. I think this methodology would be better, then. The goal is to remove the need for synchronization and user importing, which is still prone to require manual intervention.

but currently this feels like an over complication to be honest

Well the idea is actually to simplify things... Most if not all services I've used that implement LDAP authentication (including pfSense!) do not require you to synchronize anything from the directory to a local database. Everything happens directly with the remote directory, and it's just so much cleaner.

You mentioned previously groups not being groups in some contexts not scaling well, which I agree with. What OPNsense features require a local OS group?

Lastly, I suppose you could separately address limitations in group names with the aliases, and remove the requirement for local groups by updating the ACL class to query LDAP directly.

fichtner commented 3 years ago

You mentioned previously groups not being groups in some contexts not scaling well, which I agree with. What OPNsense features require a local OS group?

I suppose using an existing feature that has a specific purpose adding a "similar" functionality there that does not really fit the original approach ends up adding a lot of conditionals now and later on as more edge cases are discovered.

Well the idea is actually to simplify things... Most if not all services I've used that implement LDAP authentication (including pfSense!) do not require you to synchronize anything from the directory to a local database. Everything happens directly with the remote directory, and it's just so much cleaner.

As per my previous comment I cannot see any other simplification than for the user which is a good start, but adding more code than necessary is not as stated above. It really needs a balance to work nicely and reliable and we don't want to go back and fix it every couple of months because something was forgotten or the whole thing not designed well enough. This happens all the time in software development and it's annoying but still avoidable with caution.

It's sort of unintuitive to require group mappings without the need to actually bootstrap groups in any way. I always thought that was what remote queries were there for. So at least you need to add one group even if just a local placeholder and you give it a remote designation and the whole things starts to fly sounds pretty simple to me without changing the world of how groups are currently being used in the system.

Cheers, Franco

kaysond commented 3 years ago

As per my previous comment I cannot see any other simplification than for the user which is a good start, but adding more code than necessary is not as stated above.

That's kind of my point. From what I could see, it's actually more code to query from LDAP, go through all the conditionals you've had to set up for the myriad of configuration options required to support your current flow - import users, synchronize groups, etc - then bind to check authentication, but do the actual log in, ACL checking, etc using a local copy of the user, which again requires conditionals to see about auto import, etc.

In what I'd call a "typical" LDAP auth implementation, you simply bind with user credentials for authentication, search the directory for groups (which your LDAP class already does), then check ACL privileges. Aliases would indeed address the issues with group naming here, but does not address the complexity of having two copies of a user database.

I'd also point out that the current flow introduces a small security risk - because you have a local copy of every user and their groups, it's not guaranteed to be up to date with whatever is in LDAP, which is generally used as the single source of truth. I do note that you can auto import users and synchronize groups (requires options which can be turned off accidentally), and according to your docs the business edition will remove users that are no longer in LDAP (not available to community users). But pfSense community edition uses LDAP directly and so is always guaranteed to be up to date. In that way, OPNsense is less competitive.

fichtner commented 3 years ago

I think without the code to verify your code size assumptions you won’t be able to get an inch further in this discussion. Adding half-truth about sibling projects with a shared code base also doesn’t really help. I’m much surprised water is being cooked differently over there apparently.

kaysond commented 3 years ago

I think without the code to verify your code size assumptions you won’t be able to get an inch further in this discussion.

It's not just about size, right? Complexity matters too. I'd definitely be interested in helping out with a more in depth analysis if you're actually open to considering it.

Adding half-truth about sibling projects with a shared code base also doesn’t really help.

How is it a half-truth?

I’m much surprised water is being cooked differently over there apparently.

I don't follow...

cmprmsd commented 2 years ago

When testing LDAP integration with OPNSense we also wondered how the deletion of group memberships on the domain is handled. Currently it seems that the user is not revoked from the group, when I remove the user from the domain group.

Synchronization only adds people to the local group but does not revoke the access.

At least this feature should be implemented in order to make sure no access is left when e.g. the Active Directory team does remove people from e.g. a ProxyUser group.

AdSchellevis commented 2 years ago

@cmprmsd ldap group membership is enforced upon login when "Synchronize groups" is used (https://docs.opnsense.org/manual/how-tos/user-ldap.html#step-1-add-new-ldap-server).

cmprmsd commented 2 years ago

We use synchronize groups. But when the user is removed from the group the user is not removed from the local opnsense group.

I only tested it with the "test user" functionality. Maybe that's a workflow issue with the test user menu?

The user should be removed from the local group when the user is removed in active directory right?

AdSchellevis commented 2 years ago

when the user is successfully logged in, the groups will be synced first. cleaning up (removing) users automatically is only offered in the Business Edition (as mentioned in the docs)

cmprmsd commented 2 years ago

Oh yeah, there it is:

As of version business edition 21.10, the system will automatically query the ldap servers and remove unexisting users. (not available in the community version of OPNsense)

Sad. Didn't know there are features that are available only in a paid version. 😿

Thanks for the clarification though!

AdSchellevis commented 2 years ago

You can also delete users manually on periodic bases, if there's no user in LDAP, it can't login anyway so usually there's no real rush... but on the other end, it's not that odd paying a few bucks for software your using every day ;)

cmprmsd commented 2 years ago

That is true. The pricing seems reasonable :) Have a great new year 😊

OPNsense-bot commented 2 years ago

This issue has been automatically timed-out (after 180 days of inactivity).

For more information about the policies for this repository, please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

If someone wants to step up and work on this issue, just let us know, so we can reopen the issue and assign an owner to it.