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 1kzw9e-0007LU-SI for pgadmin-hackers@arkaria.postgresql.org; Thu, 14 Jan 2021 06:30:47 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kzw9d-0006qi-H8 for pgadmin-hackers@arkaria.postgresql.org; Thu, 14 Jan 2021 06:30:45 +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 1kzw9d-0006qb-2x for pgadmin-hackers@lists.postgresql.org; Thu, 14 Jan 2021 06:30:45 +0000 Received: from mail-io1-xd30.google.com ([2607:f8b0:4864:20::d30]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kzw9V-00021z-MW for pgadmin-hackers@postgresql.org; Thu, 14 Jan 2021 06:30:43 +0000 Received: by mail-io1-xd30.google.com with SMTP id d9so9152992iob.6 for ; Wed, 13 Jan 2021 22:30:37 -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=bV4WPgPWpd6Y3MnbcH+Noct0AmMZnZOZV6MvmnArqPI=; b=0VORz6cuqIRi5qqflihrTsPJYC6NlV/AcZIHm4f8ZTrygJRRRvqnXBw9uM1mdiAG1f UUD+1iesSBrusloyWvmBKZRZmNKFRx1uzFX6k0a4YrR0gu6py23DSEv9UHlOeWdSe9Is ZUNucbzzsyg/19piOyoXcMo9wzUay/qzOas3RK7rL3kAf/2yua65EyP1iCS2a78PoXq+ 0CpRkei0ByOwmeCo+TVoJ+zu/X3xCBLDXUIo5JTqtFlVKWRTV3Lqb0ewHj58ciW8hXPc 66XgyhzKNoGsPiNalOpurVknN9kl4RlA+o92qOsf3a+0KjLVN9JGjx1sfC6KRnKIQwQC 4WjQ== 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=bV4WPgPWpd6Y3MnbcH+Noct0AmMZnZOZV6MvmnArqPI=; b=epdrhkxcsptjUGsDdENIMPATQckhiAcZOc3DlG31LMHHB1P2BUWJ9vZFqSM6YlnEK0 CdC8F54qKAnk10gBF6GWSfHRtm+lXS3paO0PMKvjIYLqTlpaG0AqPBWnU/A/KhwxMtOH jle044Hkfns6+YKH97LZ8sPNEZ3hUzjt03G0Nvw+9bzez6fKXKLLsFkS2RcxPmMdt3ac bapqyY1qmRZpoa6Ktsn/KLrmSfxpHOriah12KIVmjsLI3pC9h6zuDf4Y6GEnQDDIJrXO V/5iJ915SlvYolie/QBQTHKx800xGVOjcXbDYP6Dt60OENAItGg0Se6oJR9HR+8PuUxm 4KKA== X-Gm-Message-State: AOAM533ytHovGAWQ8zfLYIRzri3Z9Uf5oJXxWiImIL/lHikF+v+MJNNF fBr/zIYRMhAiJVkr3uuzApmBHHfJtSUVo+XIzcbx3BBPMxg1bjNz8WYq0dxSMVw08O1XI1PuMgL 3rveptoj+ng4P4lbAAGhLFPrkikEs7hjnN70tYkS/TBMD/qHXWT799oMCWZQDcPSH9AbEp4/c0C ThLkQX12e5Oa3B7bF/tdNXUzD3edWRXwUkrm0nVLKIcRMjq44UY5TFUrrPNw== X-Google-Smtp-Source: ABdhPJzVKA5GwlnG8GuhCEBKxY+qI7NJ8oy1BIwFmj2w3X4M2dkZ/mVBqViRv+nL2U4CJXjw+1YnhIB6dKkh1FY7rrU= X-Received: by 2002:a92:da49:: with SMTP id p9mr5575442ilq.236.1610605836139; Wed, 13 Jan 2021 22:30:36 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Thu, 14 Jan 2021 12:00:25 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1 To: Khushboo Vashi Cc: Aditya Toshniwal , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000bc984f05b8d66431" 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 --000000000000bc984f05b8d66431 Content-Type: text/plain; charset="UTF-8" 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* --000000000000bc984f05b8d66431 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Khushboo

Seems you have attache= d 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.

<= /div>
Thanks,
Khushboo

On Fri, Jan 1, 2021 at 1:07 PM Ad= itya Toshniwal <aditya.toshniwal@enterprisedb.com> wrote:
Hi Khushboo,

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

<= div style=3D"font-family:verdana,sans-serif">1) Move the auth source consta= nts -ldap, kerberos out of app object. They don't belong there. You can= create the constants somewhere=C2=A0else and import them.

+app.PGADMIN_LDAP_AUTH_SOURCE =3D 'ldap'

+app.PGADMIN_KERBEROS_AUTH_SOURCE =3D 'kerberos'


Done=C2= =A0

2) A= re we going to make kerberos default for wsgi ?

--- a/web/pgAdmin4.wsgi

+++ b/web/pgAdmin4.wsgi

@@ -24,6 +24,10 @@<= /span> builtins.= SERVER_MODE =3D True

=C2=A0

=C2=A0import config

=C2=A0

+

+config.AUTHENTICATION_SOURCES =3D ['kerberos']

+config.KERBEROS_AUTO_C= REATE_USER =3D True

+


Removed, it was only for testing.=C2=A0

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['password'] 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.PGADMIN_KERBEROS_AUTH_SOURCE:

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # =C2=A0 =C2=A0 continue


=
Removed the comment, it is actually the part = of the code.=C2=A0
<= div dir=3D"ltr">

class KERBEROSAuthentication(BaseAuthentication):


Done.=C2=A0
=

5) You can 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'


Done.=C2=A0=

6) The below URL= s belong to the authenticate module. Currently they are in the browser modu= le. I would also suggest rephrasing the URL from /kerberos_login to /login/= kerberos. Same for logout.
Done the re= phrasing as well as moved to the authentication module.
=C2=A0
= 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 actua= l login, so no need for the POST method.
I followed the same meth= od for the Logout user we have used for the normal user.
=C2=A0

+@blueprint.route("/ke= rberos_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",

<= p style=3D"font-family:Menlo;margin:0px;font-variant-numeric:normal;font-va= riant-east-asian:normal;font-stretch:normal;font-size:16px;line-height:norm= al;color:rgb(57,192,38)">

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 endpoint=3D"kerberos_logout", methods=3D["= GET"])



=C2=A0
On Tue, = Dec 22, 2020 at 6:07 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrot= e:
Hi Aditya

Can you please do the code review?

On= Tue, Dec 22, 2020 at 3:44 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com<= /a>> wrote:
<= div dir=3D"ltr">Hi,

Please find the attached patch to su= pport Kerberos Authentication in pgAdmin RM 5457.

= The patch introduces a new pluggable option for Kerberos authentication, us= ing=C2=A0SPNEGO=C2=A0to forward kerberos tickets through a browser which wi= ll bypass the login page entirely if the Kerberos Authentication succeeds.<= /div>

The complete setup of the Kerberos Server=C2=A0+ p= gAdmin Server=C2=A0+ Client is documented in a separate file and attached.<= /div>

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

Thanks,
Khushboo


--


--
Thanks,
Aditya Toshniwal=
pgAdmin hacker=C2=A0| Sr. Software Engineer | edbpostgres.com
&quo= t;Don't Complain about Heat, Plant a TREE"


--
Thanks & Regards
Akshay Joshi
pgAdmi= n Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

--000000000000bc984f05b8d66431--