Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kvF0l-0007M2-Hh for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jan 2021 07:38:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kvF0j-00037n-4U for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jan 2021 07:38:09 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kvF0i-00037g-IJ for pgadmin-hackers@lists.postgresql.org; Fri, 01 Jan 2021 07:38:08 +0000 Received: from mail-lf1-x135.google.com ([2a00:1450:4864:20::135]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kvF0a-0007UW-Sk for pgadmin-hackers@postgresql.org; Fri, 01 Jan 2021 07:38:06 +0000 Received: by mail-lf1-x135.google.com with SMTP id o19so48027053lfo.1 for ; Thu, 31 Dec 2020 23:38:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=a2+/C/91s4fP0H2Uk1gEOWReW08IecU9aHrO/O5B/A4=; b=huQ/65yI16B2AE1VlBYSUB5K53y544BCu7eqUGV1btWWxIULIB2OztjFPFpqZX8zU/ JPsetIswn0Z+0fYCSmxlm5ZPKu0d7ngVUC/eG50OdLacZGbG2ZLiYw9cq4oJeXnwvFRD P4b1bYkYBM0E+Q56a3+sC/y10puCM2zRblXhnrLKZWI7GbBzJjTFtVQKweTovuzgXCfR UIcWWcaWhIUObW+RyvQ4SIOu2suyh2k5mYnzdYgj8domskcX7M7u/7X33ptMIy1DV+tf yj9PiOa8bKiT8nZ02svDVDDVpr2vCxDTVZUzZJaPVOXW5DGqkREs8n2LNQakO+VJBpaa 21iQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=a2+/C/91s4fP0H2Uk1gEOWReW08IecU9aHrO/O5B/A4=; b=EF9zOCPPhvYT6a03b98Z4unAw0gG748T0TgfvGy/pAj1549zob3tu6S4JP1jmtwy17 3Xu5t/+mBpeW1n2w3P2g0g5Ine7qjxwsvLC25fBsuzyoFWbKmdAidL10POE1xk9NKDl8 uVcMfx2CmlGKmbBqv6sXarBXi4oO8+FStQ1Km7u5iOcuiJt0onYtRCnpo+okxvNkjiy7 Uk+hsII3mbPEl7kDgAJe+LJemU6GPNRC7J+oyiOCa4JbmfJpbxnAjuglDKIHkzpWZ1Mi CGPQaOeVgRx0r8nxB+63Nw7T8u9BJxWDgBRHy50uR6xR3dn19E7qnMRgyvGgNWw+2hgC S1NQ== X-Gm-Message-State: AOAM533E1JWFaXyhTwpG+9QxJJmNBqs1NnLpsnbcE9iQke5gGzAoah55 il+rymBpZq+KTX9kLc5vu2hhjig+5dCz6cMrn6n5QbVFfry8GW6FuQvP8QO9DhV+WaIjB++B545 rl2qW4NHcujxN+WjxSn48kImf/a2I+wjgOj4lo4FunOwPiAKv3tPb6jHbQG3OvNQWAsR6AA0Ij9 rftz8BofpLcg0P+DY4gascKNStz/YRNED2tZ8gpi2fF5URTMkMoiFraxVvZNni71mX9pVj X-Google-Smtp-Source: ABdhPJx+QKQ5wP49WLpVvA1YosOsT+DXQ9ocL1T/JK1iGdByD5jXU1LstHOxzUVPkkf+x1k49aI6EL2PGphxNyzwn84= X-Received: by 2002:a05:6512:3089:: with SMTP id z9mr28385169lfd.433.1609486677646; Thu, 31 Dec 2020 23:37:57 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Aditya Toshniwal Date: Fri, 1 Jan 2021 13:07:21 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1 To: Khushboo Vashi Cc: Akshay Joshi , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000b1390a05b7d1d185" X-CLOUD-SEC-AV-Info: enterprisedb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000b1390a05b7d1d185 Content-Type: text/plain; charset="UTF-8" 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' 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 + 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 4) KERBEROSAuthentication could be KerberosAuthentication class KERBEROSAuthentication(BaseAuthentication): 5) You can use the constants (ldap, kerberos) you had defined when creating a user. + 'auth_source': 'kerberos' 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. Also, even though the method GET works, we should use the POST method for login and DELETE for logout. +@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 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" --000000000000b1390a05b7d1d185 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Khushboo,

I've just done the code review. Apart fro= m 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 cr= eate the constants somewhere=C2=A0else and import them.

+app.PGADMIN_LDAP_AUTH_SOUR= CE =3D 'ldap'

+app.PGADMIN_KERBEROS_AUTH_= SOURCE =3D 'kerberos'


2) Are we going to mak= e kerberos default for wsgi ?

--- a/web/pgAdmin4.wsgi

+++ b/web/pgAdmin4.wsgi

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

=C2=A0

=C2=A0impor= t config

=C2=A0

+

+config.AUTHENT= ICATION_SOURCES =3D ['kerberos']

+config.KERBEROS= _AUTO_CREATE_USER =3D True

+


3) Remove the commented code.

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = # if self.form.data['email'] and self.form.data['passwor= d'] and \

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # =C2=A0 =C2=A0 =C2=A0 =C2=A0 source.get_source_= name() =3D=3D\

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # =C2=A0 =C2=A0 =C2=A0 =C2=A0 current_app.PGADM= IN_KERBEROS_AUTH_SOURCE:

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # =C2=A0 =C2=A0 conti= nue


4) KERBEROSAuthentication could be Ker= berosAuthentication

class KERBEROSAu= thentication(BaseAuthentication):


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

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 'auth_source': 'kerberos'


6) The below URLs belong to the authenticat= e module. Currently they are in the browser module. I would also suggest re= phrasing the URL from /kerberos_login to /login/kerberos. Same for logout. = Also, even though the method GET works, we should use the POST method for l= ogin and DELETE for logout.

+@blueprint.route("/kerberos= _login",

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 endpoint=3D"kerberos_login", methods=3D["G= ET"])


+@blueprint.route("/kerberos_logout",

=

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 endpoint=3D"kerberos_logout", methods=3D["= 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&= gt; wrote:
Hi,

Please find the attached patch to suppor= t Kerberos Authentication in pgAdmin RM 5457.

The = patch introduces a new pluggable option for Kerberos authentication, using= =C2=A0SPNEGO=C2=A0to 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=C2=A0+ pgAd= min Server=C2=A0+ Client is documented in a separate file and attached.

This patch also includes the small fix related to log= ging #5829

Thanks,
Khushboo


--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Principal Softw= are Architect
EDB Po= stgres
Mobile: +91 976-788-8246



--
Thanks,
Aditya Toshniwal
pgAdmin hacker=C2=A0| Sr. Softwa= re Engineer | edbpostgres.com<= /font>
"Don't Complain about Heat, Plant a TREE&qu= ot;
--000000000000b1390a05b7d1d185--