nickw444 / flask-ldap3-login

LDAP3 Logins for Flask/Flask-Login
http://flask-ldap3-login.readthedocs.org/en/latest/
MIT License
73 stars 38 forks source link

Inappropriate setup of Tls context within init_app() for default server. #92

Open Yakuza-UA opened 3 years ago

Yakuza-UA commented 3 years ago

Hi @HeMan ! Just wanted to highlight the following observation. In current version of the app there's an option to add LDAP server during plugin initialization if LDAP_ADD_SERVER is set to true. However, I was unable to use this shortcut because there's no way to pass Tls context for this default server. The most weird bit is the fact that it init_app passes LDAP_USE_SSL value, which forces LDAP to use certificate, but it can't be provided :) So it's catch-22.

I have to populate Server pool manually AFTER I create a Tls context with certificate which I then pass during Server object creation. I believe this can be easily fixed by introducing additional config variable, such as LDAP_SSL_CERT which will point to the file of the certificate to use when both LDAP_ADD_SERVER and LDAP_USE_SSL are set to True

So instead of this

        if app.config["LDAP_ADD_SERVER"]:
            self.add_server(
                hostname=app.config["LDAP_HOST"],
                port=app.config["LDAP_PORT"],
                use_ssl=app.config["LDAP_USE_SSL"],
                app=app,
            )

Do this

        if app.config["LDAP_ADD_SERVER"]:
            ldap3_tls_cts = None

            if app.config["LDAP_USE_SSL"] and app.config["LDAP_SSL_CERT"]:
                ldap3_tls_ctx = Tls(
                    validate=CERT_REQUIRED,
                    version=PROTOCOL_TLSv1,
                    ca_certs_file=app.config.get('LDAP_SSL_CERT'),
                    valid_names=app.config.get('LDAP_VALID_NAMES')
                )

            self.add_server(
                hostname=app.config["LDAP_HOST"],
                port=app.config["LDAP_PORT"],
                use_ssl=app.config["LDAP_USE_SSL"],
                app=app,
                tls_ctx=ldap3_tls_ctx
            )

I also use LDAP_VALID_NAMES to define a list of server's names as defined in the certificate in case there's mismatch This one works like a charm for me, but of course I have to do this outside of your module.

If you agree with this, I can incorporate this into code as a pull request

HeMan commented 3 years ago

That looks interesting! Could you add LDAP_SSL_CONTEXT as a app.config-variable as well? I'll do some coding and testing as well, and see what I come up with.