freman / caddy-reauth

Auth your Caddyserver requests against another server
MIT License
27 stars 15 forks source link

WIP: [ldap backend] add ldaps:// support #11

Closed petrus-v closed 6 years ago

petrus-v commented 6 years ago

I know ldap over ssl is deprecated but still quite used, add some support for it

I expect to add some unit-test that why I have prefixed with WIP.

Probably in an other PR I would support an other syntax style I don't feel online json format pretty readable, I mean support both syntax likes:

json format for compatibilty:

reauth {
    path /
    ldap {"host":"ldap.example.com","port":389....}
}

for convenience, homogeneity and readability:

reauth {
    path /
    ldap {
        host ldap.example.com
        port 389
        ...
    }
}

what do you think about ?

Finally, does it will be welcome if I create a third PR to launch unit test by travis ci ?

freman commented 6 years ago

Are you sure ldaps://user:pass@server:port didn't work?

jbq commented 6 years ago

Thanks for providing the reauth module.

@freman what do you mean with the afore-mentioned ldaps:// URL above? There is no option to specify an URL in the LDAP backend, there are only host, port and tls options. I confirm connecting to an LDAP server over SSL does not work with tls=true.

Here is the error log:

caddy_1  | 03/Jul/2018:13:12:20 +0000 [ERROR 500 /] connect to "ldap.jumpcloud.com:636": LDAP Result Code 200 "Network Error": dial tcp 52.29.98.95:636: i/o timeout

Is there any plan to fix this? How can I test @petrus-v changes? My build command is:

         docker build \
        --build-arg \
        plugins=reauth \
        github.com/abiosoft/caddy-docker.git

Thanks!

freman commented 6 years ago

Oh, I finally see what he was getting at.

tls is supported out of the box, but ldaps itself isn't, I can see how that'd be problematic. I'll sort something out.

freman commented 6 years ago

https://github.com/freman/caddy-reauth/commit/595cc04cdf67d7814f2cd82f76cf4d42ee362780 should fix it

no extra config should be required, if you pass port 636 it should prefer ldaps