Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1

Hi Khushboo

Seems you have attached the wrong patch. Please send the updated patch.

On Wed, Jan 13, 2021 at 2:35 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch.

Thanks,

Khushboo

On Fri, Jan 1, 2021 at 1:07 PM Aditya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:

Hi Khushboo,

I've just done the code review. Apart from below, the patch looks good to me:

1) Move the auth source constants -ldap, kerberos out of app object. They don't belong there. You can create the constants somewhere else and import them.

+app.PGADMIN_LDAP_AUTH_SOURCE = 'ldap'

+app.PGADMIN_KERBEROS_AUTH_SOURCE = 'kerberos'

Done 

2) Are we going to make kerberos default for wsgi ?

--- a/web/pgAdmin4.wsgi

+++ b/web/pgAdmin4.wsgi

@@ -24,6 +24,10 @@ builtins.SERVER_MODE = True

 

 import config

 

+

+config.AUTHENTICATION_SOURCES = ['kerberos']

+config.KERBEROS_AUTO_CREATE_USER = True

+

Removed, it was only for testing. 

3) Remove the commented code.

+            # if self.form.data['email'] and self.form.data['password'] and \

+            #         source.get_source_name() ==\

+            #         current_app.PGADMIN_KERBEROS_AUTH_SOURCE:

+            #     continue

Removed the comment, it is actually the part of the code. 

4) KERBEROSAuthentication could be KerberosAuthentication

class KERBEROSAuthentication(BaseAuthentication):

Done. 

5) You can use the constants (ldap, kerberos) you had defined when creating a user.

+                    'auth_source': 'kerberos'

Done. 

6) The below URLs belong to the authenticate module. Currently they are in the browser module. I would also suggest rephrasing the URL from /kerberos_login to /login/kerberos. Same for logout.

Done the rephrasing as well as moved to the authentication module.

 

Also, even though the method GET works, we should use the POST method for login and DELETE for logout.

Kerberos_login just redirects the page to the actual login, so no need for the POST method.

I followed the same method for the Logout user we have used for the normal user.

 

+@blueprint.route("/kerberos_login",

+                 endpoint="kerberos_login", methods=["GET"])

+@blueprint.route("/kerberos_logout",

+                 endpoint="kerberos_logout", methods=["GET"])

 

On Tue, Dec 22, 2020 at 6:07 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Aditya

Can you please do the code review?

On Tue, Dec 22, 2020 at 3:44 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached patch to support Kerberos Authentication in pgAdmin RM 5457.

The patch introduces a new pluggable option for Kerberos authentication, using SPNEGO to forward kerberos tickets through a browser which will bypass the login page entirely if the Kerberos Authentication succeeds.

The complete setup of the Kerberos Server + pgAdmin Server + Client is documented in a separate file and attached.

This patch also includes the small fix related to logging #5829

Thanks,

Khushboo

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Thanks,

Aditya Toshniwal

pgAdmin hacker | Sr. Software Engineer | edbpostgres.com

"Don't Complain about Heat, Plant a TREE"

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246