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 1kzc65-0000MA-N9 for pgadmin-hackers@arkaria.postgresql.org; Wed, 13 Jan 2021 09:05:46 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kzc64-0008EC-M3 for pgadmin-hackers@arkaria.postgresql.org; Wed, 13 Jan 2021 09:05:44 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kzc64-0008E5-9W for pgadmin-hackers@lists.postgresql.org; Wed, 13 Jan 2021 09:05:44 +0000 Received: from mail-lf1-x131.google.com ([2a00:1450:4864:20::131]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kzc61-000540-Fi for pgadmin-hackers@postgresql.org; Wed, 13 Jan 2021 09:05:43 +0000 Received: by mail-lf1-x131.google.com with SMTP id o13so1670595lfr.3 for ; Wed, 13 Jan 2021 01:05:41 -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=g4BClgqnpAyGofbgvP24ChClXgysFon3KKbgMX/hHb8=; b=y5Y0O+6KXNgnu46LlejWB+rHLAI+whOWlF9KFaY/5SmQc+I2UrbvZ1eRrPU6DX3I57 d/uSIrp1U8jAEUDpoaQEp7zVO3CmABvC2Y+6Mhd92EmPaGnKLM2rVpVJzZxcryWlK/MI BVWz8uMhEvUB6jAcnyZpIUi7kuIigus7wzPdU0A5+a27zKzpngRu4NubSECt1p34zVXa PXDBdwmkl9qsxD9ptgD4pDRWN68FIKoT/U1NqoDiDFovILsH5siJbDjEVLRIoQK3mHJL lpPHsERYGG0D93w6sDx2pY6EQijAYk404qkgLmvpaMtM2N5tfuyPDVCg8wio7iWuW0g9 THww== 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=g4BClgqnpAyGofbgvP24ChClXgysFon3KKbgMX/hHb8=; b=ekpEOEi16oj2fSTrr13pBzJt8cHVxdlXHbLuRkz3ODFJbXugq37Oh+DerC3WgaLQLl 0CQD/khtgsCZ/yMUcYxhzBveM2b7AlJVzUl+euZ5p7+JXFPKJAAxOFV6Z/EN3KG6k+Gu hiqxsmyObBVuCNhOcbmM0mvn5BdL3sWOfFYMBR1wEyi/vOhn5Nstby7K7jeEhMUkeDvR pgaEHz7vuV7oAflRscePsrv17Zh406h0GcfSmPSEFi99uqQc6zcZnDKsroCQvsgtEuGn s1sMLSbMCFvoZlGxugIbVBOrgM55UQJxrC0HHGWQnjPYpySIDp/tCErKVpaPfclxbeZS Ec+g== X-Gm-Message-State: AOAM533kITiM0Y/3T6f6Ea1lczb8Ifx+2Ohs9ESCmzQOXAccTE/5XwE2 2uJTI+7ABFNTDWAECTU1Igxz7CRO5SZknBMNCbQvLKXMMN8jogFj8H6bQoz73GtHJoc6J3zl1Bu SxjzDzMbyJyvCRAlxUBSJLJd2ViyXyRjJsS4d57jtf6uSf8MIkPjb5NRjI1AoUaU3124rADf/qc nvDqS0AZfeoNps6B0ZwAiHMQsGvlQSTGGk/M10tNdq5fBVTTRwvZetsW8zhw== X-Google-Smtp-Source: ABdhPJyBaM0rPxglso3EG+0Q7MmtRQwWZKkKaTnFc3v76/Hid5WtqLl0Cej161SDukcS/sO8/VUtDMzuszdROeUfZ34= X-Received: by 2002:a19:23cf:: with SMTP id j198mr424800lfj.509.1610528740264; Wed, 13 Jan 2021 01:05:40 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Khushboo Vashi Date: Wed, 13 Jan 2021 14:35:38 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1 To: Aditya Toshniwal Cc: Akshay Joshi , pgadmin-hackers Content-Type: multipart/mixed; boundary="00000000000076fa5605b8c47188" 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 --00000000000076fa5605b8c47188 Content-Type: multipart/alternative; boundary="00000000000076fa5305b8c47186" --00000000000076fa5305b8c47186 Content-Type: text/plain; charset="UTF-8" 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" > --00000000000076fa5305b8c47186 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

Please find the at= tached 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 bel= ow, the patch looks good to me:

1) Move the= auth source constants -ldap, kerberos out of app object. They don't be= long there. You can create the constants somewhere=C2=A0else and import the= m.

+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"
--00000000000076fa5305b8c47186-- --00000000000076fa5605b8c47188 Content-Type: application/octet-stream; name="RM_5457_v1.patch" Content-Disposition: attachment; filename="RM_5457_v1.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kjv77crz0 ZGlmZiAtLWdpdCBhL3dlYi9wZ2FkbWluL3N0YXRpYy9qcy9zcWxlZGl0b3IvbWFjcm8uanMgYi93 ZWIvcGdhZG1pbi9zdGF0aWMvanMvc3FsZWRpdG9yL21hY3JvLmpzCmluZGV4IGM4ZjkyOTJmMy4u ZGE0ZjNiNGM3IDEwMDY0NAotLS0gYS93ZWIvcGdhZG1pbi9zdGF0aWMvanMvc3FsZWRpdG9yL21h Y3JvLmpzCisrKyBiL3dlYi9wZ2FkbWluL3N0YXRpYy9qcy9zcWxlZGl0b3IvbWFjcm8uanMKQEAg LTI3MiwxMSArMjcyLDEyIEBAIGxldCBNYWNyb0RpYWxvZyA9IHsKICAgICAgICAgICAgICAgICAg ICAgICAgICAgPGEgY2xhc3M9ImRyb3Bkb3duLWl0ZW0iIGlkPSJidG4tbWFuYWdlLW1hY3JvcyIg aHJlZj0iIyIgdGFiaW5kZXg9IjAiPgogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgPHNw YW4+ICR7Z2V0dGV4dCgnTWFuYWdlIE1hY3Jvcy4uLicpfSA8L3NwYW4+CiAgICAgICAgICAgICAg ICAgICAgICAgICAgIDwvYT4KLSAgICAgICAgICAgICAgICAgICAgICA8L2xpPgotICAgICAgICAg ICAgICAgICAgICAgIDxsaSBjbGFzcz0iZHJvcGRvd24tZGl2aWRlciI+PC9saT5gOworICAgICAg ICAgICAgICAgICAgICAgIDwvbGk+YDsKKworICAgICAgICAgICAgICAgICAgICBsZXQgbWFjcm9f bGlzdF90bXBsID0gJyc7CiAgICAgICAgICAgICAgICAgICAgIF8uZWFjaChtYWNyb3MsIGZ1bmN0 aW9uKG0pIHsKICAgICAgICAgICAgICAgICAgICAgICBpZiAobS5uYW1lKSB7Ci0gICAgICAgICAg ICAgICAgICAgICAgICBzdHIgKz0gYDxsaT4KKyAgICAgICAgICAgICAgICAgICAgICAgIG1hY3Jv X2xpc3RfdG1wbCArPSBgPGxpPgogICAgICAgICAgICAgICAgICAgICAgICAgPGEgY2xhc3M9ImRy b3Bkb3duLWl0ZW0gYnRuLW1hY3JvIiBkYXRhLW1hY3JvLWlkPSIke20uaWR9IiBocmVmPSIjIiB0 YWJpbmRleD0iMCI+CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgPHNwYW4+ICR7Xy5lc2Nh cGUobS5uYW1lKX0gPC9zcGFuPgogICAgICAgICAgICAgICAgICAgICAgICAgICAgIDxzcGFuPiAo JHttLmtleV9sYWJlbH0pIDwvc3Bhbj4KQEAgLTI4NSw2ICsyODYsNyBAQCBsZXQgTWFjcm9EaWFs b2cgPSB7CiAgICAgICAgICAgICAgICAgICAgICAgfQogICAgICAgICAgICAgICAgICAgICB9KTsK IAorICAgICAgICAgICAgICAgICAgICBpZiAobWFjcm9fbGlzdF90bXBsLmxlbmd0aCA+IDApIHN0 ciArPSAnPGxpIGNsYXNzPSJkcm9wZG93bi1kaXZpZGVyIj48L2xpPicgKyBtYWNyb19saXN0X3Rt cGw7CiAgICAgICAgICAgICAgICAgICAgICQoJC5maW5kKCdkaXYuYnRuLWdyb3VwLm1yLTEudXNl cl9tYWNyb3MgdWwuZHJvcGRvd24tbWVudScpKS5odG1sKCQoc3RyKSk7CiAKICAgICAgICAgICAg ICAgICAgICAgc2VsZi5jbG9zZSgpOyAvLyBDbG9zZSB0aGUgZGlhbG9nIG5vdwo= --00000000000076fa5605b8c47188--