Auth Base has LDAP specific group handling, which is incompatible with the Radius Auth provider

[peter-dolkens]

Important notices

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

• [ ] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md
• [ ] I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/core/issues?q=is%3Aissue

Describe the bug

The following code in Auth/Base.php is LDAP specific, and therefore fails when called by the Auth/Radius.php provider, as the RADIUS Class attribute format is incompatible with the LDAP format

https://github.com/opnsense/core/blob/a8bd3379b1a8c922ba181648626431c543b378b3/src/opnsense/mvc/app/library/OPNsense/Auth/Base.php#L163-L172

To Reproduce

Steps to reproduce the behavior:

1. Setup a RADIUS auth server that passes one or more groups using the Class attribute
2. Use the Tester to test a valid set of credentials
3. Notice the groups fail to come through

Expected behavior

Groups should be parsed. An alternative delimiter ; or , should also be supported, as I don't believe RADIUS will like the current \n delimiter.

Describe alternatives you considered

n/a

Screenshots

Relevant log files

n/a

Additional context

n/a

Environment

Software version used and hardware type if relevant, e.g.:

OPNsense 22.7.10

[AdSchellevis]

I've recently added https://github.com/opnsense/core/commit/ef0da3ea59ffaeaa70f7baef8245b347d3097589, if radius specifies a different delimiter, it should be changed here

https://github.com/opnsense/core/blob/ef0da3ea59ffaeaa70f7baef8245b347d3097589/src/opnsense/mvc/app/library/OPNsense/Auth/Radius.php#L462-L467

Some standard or RFC to refer to would be practical. The \n delimiter is just an internal convention, as implemented here for ldap

https://github.com/opnsense/core/blob/ef0da3ea59ffaeaa70f7baef8245b347d3097589/src/opnsense/mvc/app/library/OPNsense/Auth/LDAP.php#L510

[peter-dolkens]

Thanks @AdSchellevis - I actually got called away while I was filing this, but wanted to get it noted down.

I'm not focusing too much on delimiter just yet, as OneLogin's RADIUS server actually lets the user set a custom delimiter, just not sure if I can set it to \n yet.

More important though is the snippet highlighted is actually attempting to parse the LDAP format in the base class, so unless we convert the RADIUS Class attribute into LDAPs cn=<name> format, it's never going to produce any results.

https://github.com/opnsense/core/blob/a8bd3379b1a8c922ba181648626431c543b378b3/src/opnsense/mvc/app/library/OPNsense/Auth/Base.php#L163-L172

I'll likely open a PR for it once I've got something working.

[peter-dolkens]

Have confirmed things are working now with this hack in place

// collect all groups from the memberof attribute, store full object path for logging
// first cn= defines our local groupname
$ldap_groups = [];
foreach (explode("\n", $memberof) as $member) {
    if (stripos($member, "cn=") === 0) {
        $ldap_groups[strtolower(explode(",", substr($member, 3))[0])] = $member;
    }
}

// HACK: Fix for RADIUS
foreach (explode(";", $memberof) as $member) {
        $ldap_groups[strtolower($member)] = $member;
}

Reviewing the code, it looks like the $ldap_groups variable might be able to be simplified to a basic array too.

That would have made this a 1 liner (just need the explode) but I'll review further across the weekend - almost 5am here now.

[AdSchellevis]

@peter-dolkens I've tested https://github.com/opnsense/core/commit/ef0da3ea59ffaeaa70f7baef8245b347d3097589 using Freeradius + Active directory, but standards are hard to find in my experience. If there's a reason to support something else than the default "common name" format, we can always discuss, but reformatting in the radius authenticator might be a better option at the moment (could be a selectable option as well).

[peter-dolkens]

I'd argue that cn= is part of the LDAP/AD spec, unrelated to RADIUS or groups, and should probably be sanitized in the LDAP provider.

Sounds like your FreeRadius implementation is passing along things from your AD implementation. I'll try and see if FreeRadius has an option to sanitize the groups there, or if we need a way to sanitize ALL groups, regardless of provider.

I think the entire Auth setup could use some sanitization/encoding support - I can't log in using AzureAD for example because our usernames are email addresses, which are never going to validate. The question is, do we need to create system accounts for every user? I managed to "break" things just enough that the GUI happily let me sign up with email address, log in, and my account was visible in the admin, but there was no system account backing it.

Definitely a Pandora's box this one 🤣

[AdSchellevis]

I'd argue that cn= is part of the LDAP/AD spec, unrelated to RADIUS or groups, and should probably be sanitizer in the LDAP provider.

I'm not sure a generalised radius group actually exists, but a pointer to some rfc would probably help.

Sounds like your FreeRadius implementation is passing along things from your AD implementation. I'll try and see if FreeRadius has an option to sanitize the groups there, or if we need a way to sanitize ALL groups, regardless of provider.

Might be the case, but I guess it's either using a common name or a path in some way. In enterprise environments most users live in some sort of scope.

I think the entire Auth setup could use some sanitization/encoding support - I can't log in using AzureAD for example because our usernames are email addresses, which are never going to validate.

Our constraints are based on unix system users, in which case we can't use anything else indeed.

The question is, do we need to create system accounts for every user?

For most services we don't, but for a consistent user experience (allow system access for user X), we do.

Definitely a Pandora's box this one 🤣

The unix account constraint certainly complicates things, but currently I don't expect we are willing to change that.

[peter-dolkens]

Do you know if the Unix account needed for anything besides shell access?

Could potentially make it optional during account-creation, and only create the account when a shell is set (other than nologin)

Then we can move the validation to the user page.

Another option I was considering was some kind of "domain whitelist" - if a username is an email, then we could strip the domain part before account creation/login.

Might be the case, but I guess it's either using a common name or a path in some way. In enterprise environments most users live in some sort of scope.

Perhaps, though I'd suggest using Azure AD, along with one of the biggest Single Sign On vendors in the market would also be a pretty common use case in the enterprise 😉

[AdSchellevis]

Do you know if the Unix account needed for anything besides shell access?

At the top of my mind I don't expect it is needed, but as mentioned earlier we currently are not planning to drop this dependancy to avoid issues and false assumptions elsewhere. Experience learns this type of change will bite you in the tail later on, which we just are quite careful with (for quite some use-cases you don't need a local account anyway).

Could potentially make it optional during account-creation, and only create the account when a shell is set (other than nologin)

Maybe in the long run if we are at some point in time going to refactor the user/group management to our MVC framework. These projects usually take quite some time from the core team, it's just not at the top of our list at the moment.

Another option I was considering was some kind of "domain whitelist" - if a username is an email, then we could strip the domain part before account creation/login.

This may be possible within the current concept, as long as the authenticator handles the required settings and is responsible for the actual work.

Perhaps, though I'd suggest using Azure AD, along with one of the biggest Single Sign On vendors in the market would also be a pretty common use case in the enterprise 😉

Sure, but an rfc to how data is structured or some clear document describing the case avoids issues for the next one who wants to hook his/her provider using radius.

[OPNsense-bot]

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.