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

LDAP server hostname in authsources.php #425

Open
maarbu opened this Issue Jul 27, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@maarbu
Copy link

maarbu commented Jul 27, 2016

When specifying a single LDAP server in the hostname directive, you don't need to put in ldap:// or ldaps://. 'hostname' => 'ldapserver.example.com', will work fine. However, if you add a failover server by separating with a space like this: 'hostname' => 'ldapserver1.example.com ldapserver2.example.com', this will cause a 500 error. This is remedied by putting in ldap:// or ldaps:// in front of each server name. I ran across this issue when upgrading. It used to work the first way.

I haven't sussed out exactly what upgrade caused the issue. I upgraded PHP and OpenLDAP at the same time, my guess is OpenLDAP.

Should there be code added to check for the ldaps:// or ldap:// prefix or is it assumed that people will put this in by default?

@tvdijen

This comment has been minimized.

Copy link
Member

tvdijen commented Jul 29, 2016

According to the PHP documentation, http://php.net/manual/en/function.ldap-connect.php:

This field supports using a hostname or, with OpenLDAP 2.x.x and later, a full LDAP URI of the form ldap://hostname:port or ldaps://hostname:port for SSL encryption.

You can also provide multiple LDAP-URIs separated by a space as one string
Note that hostname:port is not a supported LDAP URI as the schema is missing.

This describes exactly what you are experiencing, so I guess this works as designed.
So, although this is not a bug in any way, It would probably be nice to have SSP only allow LDAP URI's in the 'hostname' setting. That way we could test the format of the setting pretty easily and it would also provide one unmistakeable way for end-users to set it.
Also, this would take away the obscurity of setting a URI as a hostname and a non-standard port-number in the 'port'-setting (which does absolutely nothing when using URI's - see docs for this behaviour as well)
What are your thoughts on this, @jaimeperez ?

@jaimeperez

This comment has been minimized.

Copy link
Member

jaimeperez commented Jul 29, 2016

Hi @maarbu & @tvdijen!

My impression is that it doesn't make much sense to have multiple ways to end up with the same configuration. I'm not sure whether I prefer forcing URIs in the hostname(s) or not allowing them at all. I guess the fact that you can have multiple servers in the hostname directive while you can only specify one port, makes it a bit easier to decide, as that makes it impossible to have different servers listening on different ports.

Probably, I would suggest getting rid of the port option in all places, and having a servers configuration option allowing to specify an array of LDAP URIs. It is as flexible as it can get, offers just one canonical way to configure the servers, and it's pretty much in line with the idea to simplify the configuration.

    'servers' => array(
        'ldaps://example.com:636',
        'ldap://example.com:389',
    ),

Does that sound reasonable?

@maarbu

This comment has been minimized.

Copy link

maarbu commented Jul 29, 2016

I agree that this isn't a bug, but I wasn't sure how to go about getting the info to you.
So it must have been an openLDAP upgrade that got me.
I like the idea of an array of servers rather than a hostname string. Most other configuration directives that allow multiple values are using arrays already.

Thanks for your work on this project.

@tvdijen

This comment has been minimized.

Copy link
Member

tvdijen commented Jul 29, 2016

Yes @jaimeperez ! That is exactly what I was proposing. I'll see if I can come up with a PR for this.

tvdijen added a commit to tvdijen/simplesamlphp that referenced this issue Jul 29, 2016

tvdijen added a commit to tvdijen/simplesamlphp that referenced this issue Jul 29, 2016

tvdijen added a commit to tvdijen/simplesamlphp that referenced this issue Jul 29, 2016

tvdijen added a commit to tvdijen/simplesamlphp that referenced this issue Jul 29, 2016

tvdijen added a commit to tvdijen/simplesamlphp that referenced this issue Jul 29, 2016

@tvdijen

This comment has been minimized.

Copy link
Member

tvdijen commented Jul 30, 2016

Okay, I hope these commits above go away some time, because I threw away the branch I worked on..

Anyway... the more I think about this, the more interesting it gets.. The most common and senseful usecase is where you configure two LDAP stores that are replica's and require the exact same config.
But who knows, there may be use cases to configure completely different ldap stores with different settings (tls / timeout / refferals / basedn / username / pw)...

So I'm thinking of keeping those settings globally and allow to override them within the servers-array, like this:

    $basedn = 'OU=mydefaultOU,DC=example,DC=org';

    'servers' => array(
        array(
                'uri' => 'ldaps://example.com:636',
        ),
        array(
                'uri' => 'ldap://example.com:389',
                'enable_tls' => true,
                'basedn' = 'OU=myNOTsodefaultOU,DC=example_somethingelse,DC=org';
        ),
    ),

Any thoughts?

@maarbu

This comment has been minimized.

Copy link

maarbu commented Jul 31, 2016

@tvdijen I agree with your thoughts on usecase. The way the $hostname was being used was for redundancy / failover. I would think in a failover situation the directory servers would be the same. I think failover has different requirements than a multiple separate directory servers setup.

With regards to how it should be coded in this project, I'll yield to you.

@jaimeperez

This comment has been minimized.

Copy link
Member

jaimeperez commented Aug 1, 2016

I agree with your comments.

I'd say: even if we want to use identical servers for failover, it might make sense to allow different ports on them. I'm tempted to force the full URI in the `servers' option. However, on the other hand, if we set the servers as elements in an array, we would then need to parse each as an URI to make sure they are actually URIs. Once you reach that point, we could very well accept hostnames there too and fill the URI with other options available (meaning TLS and port). Besides, if we are specifying servers in an array, I think it's more comprehensive for users to specify hostname and port than a URI.

So, maybe a servers directive that allows overriding general options? Something like:

`BaseDN` => 'OU=mydefaultOU,DC=example,DC=org',
'port' => 636,
'TLS' => true,
'servers' => array(
    'ldap1.example.org',
    'ldap2.example.org' => array(
        'port' => 389,
        'TLS' => false,
        'BaseDN' => 'OU=anotherOU,DC=example,DC=org',
    ),
),

This would yield two servers, ldaps://ldap1.example.org:636 with base DN OU=mydefaultOU,DC=example,DC=org and ldap://ldap2.example.org:389 with base DN OU=anotherOU,DC=example,DC=org.

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