Обсуждение: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1

Greetings,

* Khushboo Vashi (khushboo.vashi@enterprisedb.com) wrote:
> 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.

I've taken a (very short) look at this as it's certainly something that
I'm interested in and glad to see work is being done on it.

I notice that 'delegated_creds' is being set but it's unclear to me how
they're actually being used (if at all), which is a very important part
of Kerberos.

What's commonly done with mod_auth_kerb/mod_auth_gss is that the
delegated credentials are stored on the filesystem in a temporary
directory and then an environment variable is set to signal to libpq /
the Kerberos libraries that the delegated credentials can be found in
the temporary file.  I don't see any of that happening in this patch- is
that already handled in some way?  If not, what's the plan for making
that work?  Also important is to make sure that this approach will work
for constrainted delegation implementations.

Greetings Dave!

* Dave Page (dpage@pgadmin.org) wrote:
> On Sat, 2 Jan 2021 at 15:41, Stephen Frost <sfrost@snowman.net> wrote:
> > * Khushboo Vashi (khushboo.vashi@enterprisedb.com) wrote:
> > > 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.
> >
> > I've taken a (very short) look at this as it's certainly something that
> > I'm interested in and glad to see work is being done on it.
> >
> > I notice that 'delegated_creds' is being set but it's unclear to me how
> > they're actually being used (if at all), which is a very important part
> > of Kerberos.
> >
> > What's commonly done with mod_auth_kerb/mod_auth_gss is that the
> > delegated credentials are stored on the filesystem in a temporary
> > directory and then an environment variable is set to signal to libpq /
> > the Kerberos libraries that the delegated credentials can be found in
> > the temporary file.  I don't see any of that happening in this patch- is
> > that already handled in some way?  If not, what's the plan for making
> > that work?  Also important is to make sure that this approach will work
> > for constrainted delegation implementations.
>
> Phase 1 of this project (which this patch aims to implement) is to handle
> Kerberos logins to pgAdmin when running in server mode (as we’ve already
> done for LDAP, except KRB authenticated users don’t see a login page of
> course). Phase 2 will add support for logging into the PostgreSQL servers -
> I believe that is where we’ll need to handle delegated credentials, correct?

Yes, though I sure hope there isn't a plan to release just 'phase 1'
since that would imply that the user is still sending their password to
pgAdmin in some form that pgAdmin then turns around and impersonates the
user, basically completely against how Kerberos auth should be working
in this kind of a intermediate service arrangement.

In other words, if just 'phase 1' is released, it'd probably be CVE
worthy right out of the gate...

Greetings,

* Dave Page (dpage@pgadmin.org) wrote:
> On Sat, 2 Jan 2021 at 15:59, Stephen Frost <sfrost@snowman.net> wrote:
> > * Dave Page (dpage@pgadmin.org) wrote:
> > > On Sat, 2 Jan 2021 at 15:41, Stephen Frost <sfrost@snowman.net> wrote:
> > > > * Khushboo Vashi (khushboo.vashi@enterprisedb.com) wrote:
> > > > > 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.
> > > >
> > > > I've taken a (very short) look at this as it's certainly something that
> > > > I'm interested in and glad to see work is being done on it.
> > > >
> > > > I notice that 'delegated_creds' is being set but it's unclear to me how
> > > > they're actually being used (if at all), which is a very important part
> > > > of Kerberos.
> > > >
> > > > What's commonly done with mod_auth_kerb/mod_auth_gss is that the
> > > > delegated credentials are stored on the filesystem in a temporary
> > > > directory and then an environment variable is set to signal to libpq /
> > > > the Kerberos libraries that the delegated credentials can be found in
> > > > the temporary file.  I don't see any of that happening in this patch-
> > is
> > > > that already handled in some way?  If not, what's the plan for making
> > > > that work?  Also important is to make sure that this approach will work
> > > > for constrainted delegation implementations.
> > >
> > > Phase 1 of this project (which this patch aims to implement) is to handle
> > > Kerberos logins to pgAdmin when running in server mode (as we’ve already
> > > done for LDAP, except KRB authenticated users don’t see a login page of
> > > course). Phase 2 will add support for logging into the PostgreSQL
> > servers -
> > > I believe that is where we’ll need to handle delegated credentials,
> > correct?
> >
> > Yes, though I sure hope there isn't a plan to release just 'phase 1'
> > since that would imply that the user is still sending their password to
> > pgAdmin in some form that pgAdmin then turns around and impersonates the
> > user, basically completely against how Kerberos auth should be working
> > in this kind of a intermediate service arrangement.
> >
> > In other words, if just 'phase 1' is released, it'd probably be CVE
> > worthy right out of the gate...
>
> It wouldn’t impersonate the user at all, it would just work as it does now,
> requiring the user to supply a username/password for scram/md5/ldap etc, or
> a cert for SSL auth. Connection to a PostgreSQL server which required gss
> or sspi simply wouldn’t work (unless the service account under which the
> pgAdmin server is running has a valid ticket through other means).

That *is* impersonating the user..

Kerberized services really should *not* be accepting a cleartext
password to use to authenticate as the user against another service,
which is why I'd strongly recommend against releasing with just
'phase 1' done.. or at least heavily caveat'ing it that this isn't
actually real Kerberos support but is just an intermediate step that no
one should really deploy...

On Sun, Jan 3, 2021 at 6:31 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Dave Page (dpage@pgadmin.org) wrote:
> > On Sat, 2 Jan 2021 at 15:59, Stephen Frost <sfrost@snowman.net> wrote:
> > > * Dave Page (dpage@pgadmin.org) wrote:
> > > > On Sat, 2 Jan 2021 at 15:41, Stephen Frost <sfrost@snowman.net> wrote:
> > > > > * Khushboo Vashi (khushboo.vashi@enterprisedb.com) wrote:
> > > > > > 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.
> > > > >
> > > > > I've taken a (very short) look at this as it's certainly something that
> > > > > I'm interested in and glad to see work is being done on it.
> > > > >
> > > > > I notice that 'delegated_creds' is being set but it's unclear to me how
> > > > > they're actually being used (if at all), which is a very important part
> > > > > of Kerberos.
> > > > >
> > > > > What's commonly done with mod_auth_kerb/mod_auth_gss is that the
> > > > > delegated credentials are stored on the filesystem in a temporary
> > > > > directory and then an environment variable is set to signal to libpq /
> > > > > the Kerberos libraries that the delegated credentials can be found in
> > > > > the temporary file.  I don't see any of that happening in this patch-
> > > is
> > > > > that already handled in some way?  If not, what's the plan for making
> > > > > that work?  Also important is to make sure that this approach will work
> > > > > for constrainted delegation implementations.
> > > >
> > > > Phase 1 of this project (which this patch aims to implement) is to handle
> > > > Kerberos logins to pgAdmin when running in server mode (as we’ve already
> > > > done for LDAP, except KRB authenticated users don’t see a login page of
> > > > course). Phase 2 will add support for logging into the PostgreSQL
> > > servers -
> > > > I believe that is where we’ll need to handle delegated credentials,
> > > correct?
> > >
> > > Yes, though I sure hope there isn't a plan to release just 'phase 1'
> > > since that would imply that the user is still sending their password to
> > > pgAdmin in some form that pgAdmin then turns around and impersonates the
> > > user, basically completely against how Kerberos auth should be working
> > > in this kind of a intermediate service arrangement.
> > >
> > > In other words, if just 'phase 1' is released, it'd probably be CVE
> > > worthy right out of the gate...
> >
> > It wouldn’t impersonate the user at all, it would just work as it does now,
> > requiring the user to supply a username/password for scram/md5/ldap etc, or
> > a cert for SSL auth. Connection to a PostgreSQL server which required gss
> > or sspi simply wouldn’t work (unless the service account under which the
> > pgAdmin server is running has a valid ticket through other means).
>
> That *is* impersonating the user..
>
> Kerberized services really should *not* be accepting a cleartext
> password to use to authenticate as the user against another service,
> which is why I'd strongly recommend against releasing with just
> 'phase 1' done.. or at least heavily caveat'ing it that this isn't
> actually real Kerberos support but is just an intermediate step that no
> one should really deploy...

AIUI that's not what's being proposed.

Correct me if I'm wrong, but I think what's said is that this phase would:

1. Allow kerberos login *to pgadmin*.
2. Do exactly *nothing* to logins to the database server.

So per (2) logins to the db server would work exactly the same as it
does today, and bear no connection to the actual KRB login at all.

One question around that though -- when I click "save password" on a
database connection in pgadmin, it gets stored on the pgadmin server.
Isn't the key used to encrypt that derived from my password?  If I'm
logging into pgadmin without a password (using kerberos),what would
that key be derived from?

Greetings,

* Dave Page (dpage@pgadmin.org) wrote:
> On Mon, Jan 11, 2021 at 1:15 PM Magnus Hagander <magnus@hagander.net> wrote:
> > One question around that though -- when I click "save password" on a
> > database connection in pgadmin, it gets stored on the pgadmin server.
> > Isn't the key used to encrypt that derived from my password?  If I'm
> > logging into pgadmin without a password (using kerberos),what would
> > that key be derived from?
>
> Also correct - and right now, the plan is to disable password saving if
> logged in using Kerberos.

Disable password *saving*, or disable password *using*?

If you're saying that, when Kerberos is enabled, users will never be
prompted to provide a password because password-based auth has been
disabled, then perhaps that's reasonable.  I don't know how useful such
a pgadmin setup would be, but at least it wouldn't be violating one of
the core values that using Kerberos brings.

If you're saying that this is just disabling password *saving*, then
that implies that if someone actually wants to use pgadmin to, uh, log
into a PostgreSQL server which is configured for md5 or SCRAM auth or
LDAP based auth that the way that'll work is that pgadmin will prompt
the user for a password, which the user will provide and which will
then be sent from the client to the pgadmin system in the clear, and
which pgadmin will turn around and use to log into PG with, right?

It's the latter than I'm concerned with because it just wouldn't be
appropriate for a Kerberized service which is set up to use Kerberos to
then prompt the user for a password.

Accessing systems outside of the Kerberized environment is obviously a
different situation as you *can't* use the Kerberos credentials- and,
hopefully, everyone is using password managers and has a distinct and
different password for every service they do use outside of the
Kerberized environment.  When you're talking about a set of systems
which live inside of the Kerberized environment, however, it's simply
not sensible to ask the user to provide their *domain-level* credentials
which an attacker could use to log in as that user to the entire domain
and have complete access over their account and that's exactly what is
likely to end up being the case here because the only way to set this up
would be Kerberos for pgAdmin and LDAP for PG- at least until delegated
credentials are implemented.

> You basically seem to be saying that once a user logs into something using
> Kerberos, *everything* else they login to from there must also be done
> using Kerberos - which clearly will not be the case in the vast majority of
> deployments.

Everything else they login to from there in the same Kerberized
environment absolutely should be done using Kerberos delegated
credentials.  That's the point of Kerberos delegation.  Are you modeling
this approach based on some existing system which accepts Kerberos
logins but then *doesn't* allow use of delegated credentials to log into
other Kerberized systems from there?  Surely SSH works great with
delegated credentials, as does any website that uses mod_auth_kerb or
mod_auth_gss, or IIS..

I sure hope that the vast majority of deployments where pgAdmin is set
up with Kerberos will be using Kerberos for logging into PG with
delegated credentials, and further, that we will be *strongly*
encouraging that as otherwise you might as well use LDAP auth for all of
it and accept that any compromise of the web server or of PG will result
in complete compromise of any user's account who accesses the system.

I don't understand all this push-back.

The intent is to do the 'phase 2', right?  And it hopefully will happen
in relatively short order, no?  At least, I'd think it would make sense,
while people have developer environments set up and working with
Kerberos to go ahead and get that part done.  All I'm saying is that the
'phase 1' part really shouldn't be independently released, or if it is,
it should be *heavily* caveated that it is strongly discouraged for
people to run it in an environment where pgadmin and PG are in the same
Kerberized environment because it's not possible to set that up, with
just phase 1 done, in a manner which would avoid the pgadmin and PG
servers seeing the user's password.

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 <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"

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

Hi,

Please find the attached updated patch with the below changes:

- Dependencies are added into Linux packages in the RPM/DEBs.

- Dev packages are added in the setup scripts for Linux.

- The required packages are added in the Dockerfile.

- Conditional gssapi 1.6.2 dependency is added for Python 3.5 in requirements.txt.

- krb5 libs are not bundled with the Desktop packages, so added the gssapi dependency into the try/catch block. 

- .dockerignore is introduced to ignore unwanted files/folders like node_modules etc., which will make the docker build fast. (By Ashesh Vashi)

Thanks,

Khushboo

On Fri, Jan 15, 2021 at 3:48 PM Dave Page <dpage@pgadmin.org> wrote:

And another thought...

Some of the Jenkins QA jobs setup the virtual environment for running tests themselves. I believe these might actually be the cause of some of the failures we saw initially with the commit - I'll review those, and ensure they won't try to build the gssapi module from source on Windows.

On Thu, Jan 14, 2021 at 4:34 PM Dave Page <dpage@pgadmin.org> wrote:

FYI, I did a quick test (and browse of PyPI):

- On Windows, it seems there is a binary wheel available:

(gssapi) C:\Users\dpage>pip install gssapi
Collecting gssapi
  Downloading gssapi-1.6.12-cp39-cp39-win_amd64.whl (670 kB)
     |████████████████████████████████| 670 kB 3.3 MB/s
Collecting decorator
  Downloading decorator-4.4.2-py2.py3-none-any.whl (9.2 kB)
Installing collected packages: decorator, gssapi
Successfully installed decorator-4.4.2 gssapi-1.6.12

- On macOS, the wheel is built by pip, but it doesn't seem to have any additional binary dependencies.

This should simplify things a lot - we just need to ensure the build scripts use the binary package on Windows, and install the build deps on the Linux/Docker environments (and update the package builds with the additional dependencies of course).

On Thu, Jan 14, 2021 at 4:04 PM Dave Page <dpage@pgadmin.org> wrote:

Hi Khushboo,

As you know, this has been rolled back as the buildfarm blew up. I think there are a number of TODOs that need to be addressed, given that the gssapi Python module is dependent on MIT Kerberos:

In the patch:

- Linux packages will need the additional dependencies to be declared in the RPM/DEBs.

- The setup scripts for Linux will need to have the -dev packages added as appropriate.

- The various READMEs that describe how to build packages will need to be updated.

- The Dockerfile will need to be modified to add the required packages.

- The Windows build will need to be updated so the installer ships additional required DLLs.

- Are there any additional macOS dependencies? If so, they need to be handled.

In the buildfarm:

- All Linux build VMs need to be updated with the additional dependencies.

- On Windows, we need to figure out how to build/ship KfW. It's a pain to build, which we would typically do ourselves to ensure we're consistently using the same buildchain. If we do build it ourselves:

  - Will the Python package find it during it's build?

  - We'll need to create a Jenkins job to perform the build.

- Is any work required on macOS, or does it ship with everything that's needed? If not, we'll need to build it, and create the Jenkins job.

One final thought: on Windows/macOS, can we force a binary installation from PIP (pip install --only-binary=gssapi gssapi)? If so, will that include the required libraries, as psycopg2-binary does?

On Thu, Jan 14, 2021 at 8:18 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Thanks, patch applied.

On Thu, Jan 14, 2021 at 1:42 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi, 

Please ignore my previous patch, attached the updated one.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:17 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:00 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

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

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

Hi

On Mon, Jan 18, 2021 at 7:30 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch with the below changes:

- Dependencies are added into Linux packages in the RPM/DEBs.

- Dev packages are added in the setup scripts for Linux.

- The required packages are added in the Dockerfile.

- Conditional gssapi 1.6.2 dependency is added for Python 3.5 in requirements.txt.

1.6.9 is the last release that supports Python 3.4+. We should use that rather than older versions.

 

- krb5 libs are not bundled with the Desktop packages, so added the gssapi dependency into the try/catch block. 

- .dockerignore is introduced to ignore unwanted files/folders like node_modules etc., which will make the docker build fast. (By Ashesh Vashi)

Aside from that one comment above, eyeball review of the build changes looks good.

 

Thanks,

Khushboo

On Fri, Jan 15, 2021 at 3:48 PM Dave Page <dpage@pgadmin.org> wrote:

And another thought...

Some of the Jenkins QA jobs setup the virtual environment for running tests themselves. I believe these might actually be the cause of some of the failures we saw initially with the commit - I'll review those, and ensure they won't try to build the gssapi module from source on Windows.

On Thu, Jan 14, 2021 at 4:34 PM Dave Page <dpage@pgadmin.org> wrote:

FYI, I did a quick test (and browse of PyPI):

- On Windows, it seems there is a binary wheel available:

(gssapi) C:\Users\dpage>pip install gssapi
Collecting gssapi
  Downloading gssapi-1.6.12-cp39-cp39-win_amd64.whl (670 kB)
     |████████████████████████████████| 670 kB 3.3 MB/s
Collecting decorator
  Downloading decorator-4.4.2-py2.py3-none-any.whl (9.2 kB)
Installing collected packages: decorator, gssapi
Successfully installed decorator-4.4.2 gssapi-1.6.12

- On macOS, the wheel is built by pip, but it doesn't seem to have any additional binary dependencies.

This should simplify things a lot - we just need to ensure the build scripts use the binary package on Windows, and install the build deps on the Linux/Docker environments (and update the package builds with the additional dependencies of course).

On Thu, Jan 14, 2021 at 4:04 PM Dave Page <dpage@pgadmin.org> wrote:

Hi Khushboo,

As you know, this has been rolled back as the buildfarm blew up. I think there are a number of TODOs that need to be addressed, given that the gssapi Python module is dependent on MIT Kerberos:

In the patch:

- Linux packages will need the additional dependencies to be declared in the RPM/DEBs.

- The setup scripts for Linux will need to have the -dev packages added as appropriate.

- The various READMEs that describe how to build packages will need to be updated.

- The Dockerfile will need to be modified to add the required packages.

- The Windows build will need to be updated so the installer ships additional required DLLs.

- Are there any additional macOS dependencies? If so, they need to be handled.

In the buildfarm:

- All Linux build VMs need to be updated with the additional dependencies.

- On Windows, we need to figure out how to build/ship KfW. It's a pain to build, which we would typically do ourselves to ensure we're consistently using the same buildchain. If we do build it ourselves:

  - Will the Python package find it during it's build?

  - We'll need to create a Jenkins job to perform the build.

- Is any work required on macOS, or does it ship with everything that's needed? If not, we'll need to build it, and create the Jenkins job.

One final thought: on Windows/macOS, can we force a binary installation from PIP (pip install --only-binary=gssapi gssapi)? If so, will that include the required libraries, as psycopg2-binary does?

On Thu, Jan 14, 2021 at 8:18 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Thanks, patch applied.

On Thu, Jan 14, 2021 at 1:42 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi, 

Please ignore my previous patch, attached the updated one.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:17 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:00 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

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

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

On Mon, Jan 18, 2021 at 2:45 PM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Mon, Jan 18, 2021 at 7:30 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch with the below changes:

- Dependencies are added into Linux packages in the RPM/DEBs.

- Dev packages are added in the setup scripts for Linux.

- The required packages are added in the Dockerfile.

- Conditional gssapi 1.6.2 dependency is added for Python 3.5 in requirements.txt.

1.6.9 is the last release that supports Python 3.4+. We should use that rather than older versions.

As per the https://pypi.org/project/gssapi/1.6.9/, it says Requires: Python >=3.6.*

 

 

- krb5 libs are not bundled with the Desktop packages, so added the gssapi dependency into the try/catch block. 

- .dockerignore is introduced to ignore unwanted files/folders like node_modules etc., which will make the docker build fast. (By Ashesh Vashi)

Aside from that one comment above, eyeball review of the build changes looks good.

 

Thanks,

Khushboo

On Fri, Jan 15, 2021 at 3:48 PM Dave Page <dpage@pgadmin.org> wrote:

And another thought...

Some of the Jenkins QA jobs setup the virtual environment for running tests themselves. I believe these might actually be the cause of some of the failures we saw initially with the commit - I'll review those, and ensure they won't try to build the gssapi module from source on Windows.

On Thu, Jan 14, 2021 at 4:34 PM Dave Page <dpage@pgadmin.org> wrote:

FYI, I did a quick test (and browse of PyPI):

- On Windows, it seems there is a binary wheel available:

(gssapi) C:\Users\dpage>pip install gssapi
Collecting gssapi
  Downloading gssapi-1.6.12-cp39-cp39-win_amd64.whl (670 kB)
     |████████████████████████████████| 670 kB 3.3 MB/s
Collecting decorator
  Downloading decorator-4.4.2-py2.py3-none-any.whl (9.2 kB)
Installing collected packages: decorator, gssapi
Successfully installed decorator-4.4.2 gssapi-1.6.12

- On macOS, the wheel is built by pip, but it doesn't seem to have any additional binary dependencies.

This should simplify things a lot - we just need to ensure the build scripts use the binary package on Windows, and install the build deps on the Linux/Docker environments (and update the package builds with the additional dependencies of course).

On Thu, Jan 14, 2021 at 4:04 PM Dave Page <dpage@pgadmin.org> wrote:

Hi Khushboo,

As you know, this has been rolled back as the buildfarm blew up. I think there are a number of TODOs that need to be addressed, given that the gssapi Python module is dependent on MIT Kerberos:

In the patch:

- Linux packages will need the additional dependencies to be declared in the RPM/DEBs.

- The setup scripts for Linux will need to have the -dev packages added as appropriate.

- The various READMEs that describe how to build packages will need to be updated.

- The Dockerfile will need to be modified to add the required packages.

- The Windows build will need to be updated so the installer ships additional required DLLs.

- Are there any additional macOS dependencies? If so, they need to be handled.

In the buildfarm:

- All Linux build VMs need to be updated with the additional dependencies.

- On Windows, we need to figure out how to build/ship KfW. It's a pain to build, which we would typically do ourselves to ensure we're consistently using the same buildchain. If we do build it ourselves:

  - Will the Python package find it during it's build?

  - We'll need to create a Jenkins job to perform the build.

- Is any work required on macOS, or does it ship with everything that's needed? If not, we'll need to build it, and create the Jenkins job.

One final thought: on Windows/macOS, can we force a binary installation from PIP (pip install --only-binary=gssapi gssapi)? If so, will that include the required libraries, as psycopg2-binary does?

On Thu, Jan 14, 2021 at 8:18 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Thanks, patch applied.

On Thu, Jan 14, 2021 at 1:42 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi, 

Please ignore my previous patch, attached the updated one.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:17 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:00 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

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

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

On Mon, Jan 18, 2021 at 9:37 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

On Mon, Jan 18, 2021 at 2:45 PM Dave Page <dpage@pgadmin.org> wrote:

Hi

On Mon, Jan 18, 2021 at 7:30 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch with the below changes:

- Dependencies are added into Linux packages in the RPM/DEBs.

- Dev packages are added in the setup scripts for Linux.

- The required packages are added in the Dockerfile.

- Conditional gssapi 1.6.2 dependency is added for Python 3.5 in requirements.txt.

1.6.9 is the last release that supports Python 3.4+. We should use that rather than older versions.

As per the https://pypi.org/project/gssapi/1.6.9/, it says Requires: Python >=3.6.*

I think that's the metadata for the latest package version on the left. If you read the main text, it says:

Requirements

Basic

• A working implementation of GSSAPI (such as from MIT Kerberos) which includes header files
• a C compiler (such as GCC)
• either the enum34 Python package or Python 3.4+
• the six and decorator python package

 

For 1.6.10, that changed to:

Requirements

Basic

• A working implementation of GSSAPI (such as from MIT Kerberos) which supports delegation and includes header files
• a C compiler (such as GCC)
• Python 3.6+ (older releases support older versions, but are unsupported)
• the decorator python package

 

 

- krb5 libs are not bundled with the Desktop packages, so added the gssapi dependency into the try/catch block. 

- .dockerignore is introduced to ignore unwanted files/folders like node_modules etc., which will make the docker build fast. (By Ashesh Vashi)

Aside from that one comment above, eyeball review of the build changes looks good.

 

Thanks,

Khushboo

On Fri, Jan 15, 2021 at 3:48 PM Dave Page <dpage@pgadmin.org> wrote:

And another thought...

Some of the Jenkins QA jobs setup the virtual environment for running tests themselves. I believe these might actually be the cause of some of the failures we saw initially with the commit - I'll review those, and ensure they won't try to build the gssapi module from source on Windows.

On Thu, Jan 14, 2021 at 4:34 PM Dave Page <dpage@pgadmin.org> wrote:

FYI, I did a quick test (and browse of PyPI):

- On Windows, it seems there is a binary wheel available:

(gssapi) C:\Users\dpage>pip install gssapi
Collecting gssapi
  Downloading gssapi-1.6.12-cp39-cp39-win_amd64.whl (670 kB)
     |████████████████████████████████| 670 kB 3.3 MB/s
Collecting decorator
  Downloading decorator-4.4.2-py2.py3-none-any.whl (9.2 kB)
Installing collected packages: decorator, gssapi
Successfully installed decorator-4.4.2 gssapi-1.6.12

- On macOS, the wheel is built by pip, but it doesn't seem to have any additional binary dependencies.

This should simplify things a lot - we just need to ensure the build scripts use the binary package on Windows, and install the build deps on the Linux/Docker environments (and update the package builds with the additional dependencies of course).

On Thu, Jan 14, 2021 at 4:04 PM Dave Page <dpage@pgadmin.org> wrote:

Hi Khushboo,

As you know, this has been rolled back as the buildfarm blew up. I think there are a number of TODOs that need to be addressed, given that the gssapi Python module is dependent on MIT Kerberos:

In the patch:

- Linux packages will need the additional dependencies to be declared in the RPM/DEBs.

- The setup scripts for Linux will need to have the -dev packages added as appropriate.

- The various READMEs that describe how to build packages will need to be updated.

- The Dockerfile will need to be modified to add the required packages.

- The Windows build will need to be updated so the installer ships additional required DLLs.

- Are there any additional macOS dependencies? If so, they need to be handled.

In the buildfarm:

- All Linux build VMs need to be updated with the additional dependencies.

- On Windows, we need to figure out how to build/ship KfW. It's a pain to build, which we would typically do ourselves to ensure we're consistently using the same buildchain. If we do build it ourselves:

  - Will the Python package find it during it's build?

  - We'll need to create a Jenkins job to perform the build.

- Is any work required on macOS, or does it ship with everything that's needed? If not, we'll need to build it, and create the Jenkins job.

One final thought: on Windows/macOS, can we force a binary installation from PIP (pip install --only-binary=gssapi gssapi)? If so, will that include the required libraries, as psycopg2-binary does?

On Thu, Jan 14, 2021 at 8:18 AM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Thanks, patch applied.

On Thu, Jan 14, 2021 at 1:42 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi, 

Please ignore my previous patch, attached the updated one.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:17 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch.

Thanks,

Khushboo

On Thu, Jan 14, 2021 at 12:00 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

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

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com